On 05/05/2017 01:17 AM, Przemysław Sztoch wrote:

...

On 4 May 2017, at 17:03, Michal Privoznik <mprivozn@redhat.com> wrote:

...

2. Please add new columns to virsh list (for —details): a) CPUs b) MaxMem c) Flags: p-persistent, t-transient, a-autostart, m-managed save, s-with snapshot(s) and total sum for column vCPUs and Mem.

In my opinion it is very popular use case. It will help you control vCPU and memory overcommiting in very simple and fast way. Put flags on list will prevents many additional commands from being executed. You will get all important information in one place.

Frankly, this one looks at the edge of the scope of virsh list. I mean, I view virsh as a simple CLI utility that you can build your own scripts on the top of. Also, you want 5 flags. Cool. But tomorrow somebody else wants another 5 (title, description, has given device, etc.). The possibilities are endless. I suggest writing your own management application on the top of libvirt.

In my opinion you are wrong. It is very natural place for more information about your domains. Of course, everybody can build his own scripts, but bash script will be too much slow and too much complicated for typical tech stuff.

Why they would be slow?

My patch is very simple: virsh list —details

Please, audit attached diff, because I have not any skill about C:

index 0ca53e4..c98160d 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1769,6 +1769,14 @@ static const vshCmdOptDef opts_list[] = { .type = VSH_OT_BOOL, .help = N_("show domain title") }, + {.name = "description", + .type = VSH_OT_BOOL, + .help = N_("show domain description") + }, + {.name = "details", + .type = VSH_OT_BOOL, + .help = N_("show domain details") + }, {.name = NULL} };

@@ -1780,6 +1788,8 @@ cmdList(vshControl *ctl, const vshCmd *cmd) { bool managed = vshCommandOptBool(cmd, "managed-save"); bool optTitle = vshCommandOptBool(cmd, "title"); + bool optDescription = vshCommandOptBool(cmd, "description"); + bool optDetails = vshCommandOptBool(cmd, "details"); bool optTable = vshCommandOptBool(cmd, "table"); bool optUUID = vshCommandOptBool(cmd, "uuid"); bool optName = vshCommandOptBool(cmd, "name"); @@ -1822,6 +1832,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd)

VSH_EXCLUSIVE_OPTIONS("table", "name"); VSH_EXCLUSIVE_OPTIONS("table", "uuid"); + VSH_EXCLUSIVE_OPTIONS("title", "description");

if (!optUUID && !optName) optTable = true; @@ -1836,6 +1847,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd) _("Id"), _("Name"), _("State"), _("Title"), "-----------------------------------------" "-----------------------------------------"); + else if (optDescription) + vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n", + _("Id"), _("Name"), _("State"), _("Description"), + "-----------------------------------------" + "-----------------------------------------"); + else if (optDetails) + vshPrintExtra(ctl, " %-5s %-30s %-10s %5s %6s %9s %-5s %-20s\n%s\n", + _("Id"), _("Name"), _("State"), _("vCPU"), + _("Memory"), _("Snapshots"), _("Flags"), _("Time"), + "-----------------------------------------" + "-----------------------------------------" + "----------------------");

What if I'd run 'list --description --details'? Only description header will be printed out.

else vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n", _("Id"), _("Name"), _("State"), @@ -1862,8 +1885,8 @@ cmdList(vshControl *ctl, const vshCmd *cmd) virDomainHasManagedSaveImage(dom, 0) > 0) state = -2;

- if (optTitle) { - if (!(title = virshGetDomainDescription(ctl, dom, true, 0))) + if (optTitle || optDescription) { + if (!(title = virshGetDomainDescription(ctl, dom, optTitle, 0))) goto cleanup;

vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf, @@ -1873,6 +1896,62 @@ cmdList(vshControl *ctl, const vshCmd *cmd) title);

VIR_FREE(title); + } else if (optDetails) { + int vcpu = -1; + int memory = -1; + int persistent; + int autostart; + int nsnap; + int mansave; + long long seconds = 0; + unsigned int nseconds = 0; + + virDomainInfo info; + + if (virDomainGetInfo(dom, &info) == 0) { + vcpu = info.nrVirtCpu; + memory = info.memory / 1024; + }

I think that instead of silently ignoring errors, we should report the error message and fail. This applies here and in the rest of your patch.

+ + persistent = virDomainIsPersistent(dom); + if (virDomainGetAutostart(dom, &autostart) < 0) { + autostart = -1; + } + nsnap = virDomainSnapshotNum(dom, 0); + mansave = virDomainHasManagedSaveImage(dom, 0); + + char timestr[100]; + if (state == VIR_DOMAIN_RUNNING) { + if (virDomainGetTime(dom, &seconds, &nseconds, false) < 0) {

false? The last argument is 'unsigned int flags'. s/false/0/

+ strcpy(timestr, "??"); + } else { + time_t cur_time = seconds; + struct tm time_info; + + if (!gmtime_r(&cur_time, &time_info)) { + vshError(ctl, _("Unable to format time")); + goto cleanup; + } + strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S", &time_info); + } + } else { + strcpy(timestr, ""); + } + + vshPrint(ctl, " %-5s %-30s %-10s %5d %6d %9d %s%s%s%s%s %s\n", id_buf, + virDomainGetName(dom), + state == -2 ? _("saved") + : virshDomainStateToString(state), + vcpu, memory, + nsnap, + persistent == 1 ? "p" : persistent == 0 ? "t" : "?", + autostart == 1 ? "a" : autostart == 0 ? " " : "?", + mansave == 1 ? "m" : mansave == 0 ? " " : "?", + nsnap > 0 ? "s" : nsnap == 0 ? " " : "?", + " ", + timestr + ); + } else { vshPrint(ctl, " %-5s %-30s %s\n", id_buf, virDomainGetName(dom),

Michal

On Mon, May 08, 2017 at 09:16:26AM +0200, Przemysław Sztoch wrote:

Dear Michal and Pavel,

We cover about 100 clients who have their own and simple CentOS KVM installations. Their technical skills are far from writing python scripts. They expect simple solutions.

Libvirt is an library that provides unified API to manage different hypervisors. Virsh is a simple command line tool to manage one host, it's not an advanced management application.

Talking to our helpdesk, I found that 70% of libvirt and virtualization problems are: A) lack of autostart activation on critical guests; then occasional failures and reboots affect lack of automatic startup of key services, B) frequent overcommiting of allocated virtual processors and memory due to the lack of basic planning and addition operations of local admin stuff :-(, C) misconfiguration of qemu-agent, which affects many problems with safe restart, snapshot, backup, etc. (the "Time" column is a perfect diagnostic here) D) leaving unnecessary snapshots that lie unused after many months, E) live migration attempts that fail to put domain in a transient mode leave the guests disappearing in unexplained circumstances after kvm host restart :-)

Virtually all the above problems of everyday life, our helpdesk is now able to diagnose by command: virsh list --details --managed-save By the way, they can easily update the documentation with one compact list.

I do not understand your dislike for the proposed changes. All the members of our team and teams of our partners have been very enthusiastic about the new functionality. You govern, so you have to decide. ;-)

My dislike is that virsh isn't a management application, it's more like a shell for virtualization. All the listed problems are things, that should be handled by management application build on top of libvirt. This is a common misuse of libvirt. For simple workstation usage there is virt-manager. For advanced management tasks there are different type of applications, see <http://libvirt.org/apps.html>. To address the patch itself, as it was already stated by Peter, instead of adding --details there is command line tool "virt-top" and adding --description isn't a good idea, because it will break the simple table printed by "virsh list". Abusing "virsh list" to provide statistics and detailed information about domains isn't a way to go. Pavel

Przemyslaw Sztoch

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0ca53e4..1c3ec37 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1769,6 +1769,14 @@ static const vshCmdOptDef opts_list[] = { .type = VSH_OT_BOOL, .help = N_("show domain title") }, + {.name = "description", + .type = VSH_OT_BOOL, + .help = N_("show domain description") + }, + {.name = "details", + .type = VSH_OT_BOOL, + .help = N_("show domain details") + }, {.name = NULL} };

@@ -1780,6 +1788,8 @@ cmdList(vshControl *ctl, const vshCmd *cmd) { bool managed = vshCommandOptBool(cmd, "managed-save"); bool optTitle = vshCommandOptBool(cmd, "title"); + bool optDescription = vshCommandOptBool(cmd, "description"); + bool optDetails = vshCommandOptBool(cmd, "details"); bool optTable = vshCommandOptBool(cmd, "table"); bool optUUID = vshCommandOptBool(cmd, "uuid"); bool optName = vshCommandOptBool(cmd, "name"); @@ -1822,6 +1832,9 @@ cmdList(vshControl *ctl, const vshCmd *cmd)

VSH_EXCLUSIVE_OPTIONS("table", "name"); VSH_EXCLUSIVE_OPTIONS("table", "uuid"); + VSH_EXCLUSIVE_OPTIONS("description", "title"); + VSH_EXCLUSIVE_OPTIONS("details", "title"); + VSH_EXCLUSIVE_OPTIONS("details", "description");

if (!optUUID && !optName) optTable = true; @@ -1831,9 +1844,19 @@ cmdList(vshControl *ctl, const vshCmd *cmd)

/* print table header in legacy mode */ if (optTable) { - if (optTitle) + if (optTitle || optDescription) vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n", - _("Id"), _("Name"), _("State"), _("Title"), + _("Id"), _("Name"), _("State"), + optTitle ? _("Title") : _("Description"), + "-----------------------------------------" + "-----------------------------------------"); + else if (optDetails) + vshPrintExtra(ctl, " %-5s %-30s %-10s %-13s %-13s %5s %9s %9s %s\n%s\n", + _("Id"), _("Name"), _("State"), + _("Autostart"), _("Persistent"), + _("vCPU"), _("Memory"), _("Snapshots"), + _("Time"), + "-----------------------------------------" "-----------------------------------------" "-----------------------------------------"); else @@ -1862,8 +1885,8 @@ cmdList(vshControl *ctl, const vshCmd *cmd) virDomainHasManagedSaveImage(dom, 0) > 0) state = -2;

- if (optTitle) { - if (!(title = virshGetDomainDescription(ctl, dom, true, 0))) + if (optTitle || optDescription) { + if (!(title = virshGetDomainDescription(ctl, dom, optTitle, 0))) goto cleanup;

vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf, @@ -1873,6 +1896,60 @@ cmdList(vshControl *ctl, const vshCmd *cmd) title);

VIR_FREE(title); + } else if (optDetails) { + int autostart; + int persistent; + int vcpu = -1; + int memory = -1; + int nsnap; + long long seconds = 0; + unsigned int nseconds = 0; + + virDomainInfo info; + + if (virDomainGetAutostart(dom, &autostart) < 0) { + autostart = -1; + } + persistent = virDomainIsPersistent(dom); + + if (virDomainGetInfo(dom, &info) == 0) { + vcpu = info.nrVirtCpu; + memory = info.memory / 1024; + } + + nsnap = virDomainSnapshotNum(dom, 0); + + char timestr[100]; + if (state == VIR_DOMAIN_RUNNING) { + if (virDomainGetTime(dom, &seconds, &nseconds, 0) < 0) { + strcpy(timestr, _("unknown")); + } else { + time_t cur_time = seconds; + struct tm time_info; + + if (!gmtime_r(&cur_time, &time_info)) { + vshError(ctl, _("Unable to format time")); + goto cleanup; + } + strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S", &time_info); + } + } else { + strcpy(timestr, ""); + } + + vshPrint(ctl, " %-5s %-30s %-10s %-13s %-13s %5d %9d %9d %s\n", id_buf, + virDomainGetName(dom), + state == -2 ? _("saved") + : virshDomainStateToString(state), + autostart == 1 ? _("yes") : + autostart == 0 ? _("no") : _("unknown"), + persistent == 1 ? _("yes") : + persistent == 0 ? _("no") : _("unknown"), + vcpu, memory, + nsnap, + timestr + ); + } else { vshPrint(ctl, " %-5s %-30s %s\n", id_buf, virDomainGetName(dom),

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list