3:03 a.m.
On Mon, Mar 30, 2015 at 13:26:40 +0530, Nehal J Wani wrote:
On Mon, Mar 30, 2015 at 1:16 PM, Peter Krempa
<pkrempa(a)redhat.com> wrote:
> On Mon, Mar 30, 2015 at 03:29:40 +0530, Nehal J Wani wrote:
>> This is an attempt to fix: https://bugzilla.redhat.com/show_bug.cgi?id=812911
>>
>> Calculate vm uptime by subtracting process starttime from system uptime:
>> Almost equivalent to:
>> echo $(($(($(awk '{print $1}' /proc/uptime)*1e9 - $(($(cut -d "
" -f22 /proc/$PID/stat)*1e7))))/(1.0 * 1e9)))
>>
>> ---
>> include/libvirt/libvirt-domain.h | 1 +
>> src/qemu/qemu_driver.c | 32 +++++++++++++++++++++++++-------
>> src/remote/remote_protocol.x | 1 +
>> src/remote_protocol-structs | 1 +
>> tools/virsh-domain-monitor.c | 8 ++++++++
>> 5 files changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/libvirt/libvirt-domain.h
b/include/libvirt/libvirt-domain.h
>> index 7be4219..2df0241 100644
>> --- a/include/libvirt/libvirt-domain.h
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -275,6 +275,7 @@ struct _virDomainInfo {
>> unsigned long memory; /* the memory in KBytes used by the domain */
>> unsigned short nrVirtCpu; /* the number of virtual CPUs for the domain
*/
>> unsigned long long cpuTime; /* the CPU time used in nanoseconds */
>> + unsigned long long upTime; /* the total uptime in nanoseconds */
>> };
>>
>
> The public structs can't be changed once they are released. The next
> best place will be the bulk stats API that is extensible.
>
>> /**
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index f07e4fb..0b5098f 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -1309,11 +1309,12 @@ static char *qemuConnectGetCapabilities(virConnectPtr
conn) {
>>
>> static int
>> qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
>> - pid_t pid, int tid)
>> + pid_t pid, int tid, unsigned long long *upTime)
>> {
>> char *proc;
>> FILE *pidinfo;
>> - unsigned long long usertime = 0, systime = 0;
>> + unsigned long long usertime = 0, systime = 0, starttime = 0;
>> + double _uptime;
>> long rss = 0;
>> int cpu = 0;
>> int ret;
>> @@ -1337,13 +1338,29 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int
*lastCpu, long *vm_rss,
>> /* pid -> stime */
>> "%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu
%llu"
>> /* cutime -> endcode */
>> - "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
>> + "%*d %*d %*d %*d %*d %*d %llu %*u %ld %*u %*u %*u"
>> /* startstack -> processor */
>> "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
>> - &usertime, &systime, &rss, &cpu) != 4) {
>> + &usertime, &systime, &starttime, &rss, &cpu)
!= 5) {
>> VIR_WARN("cannot parse process status data");
>> }
>>
>> + ret = virAsprintf(&proc, "/proc/uptime");
>
> This copies a static string using virAsprintf?
>
>> + if (ret < 0)
>> + return -1;
>> +
>> + pidinfo = fopen(proc, "r");
>
> And uses it in a place where you can use static strings? That doesn't
> make sense.
>
>> + VIR_FREE(proc);
>> +
>> + if (!pidinfo || fscanf(pidinfo, "%lf %*f", &_uptime) != 1) {
>> + VIR_WARN("cannot parse machine uptime data");
>> + }
>> +
>> + if (upTime)
>> + *upTime = 1000ull * 1000ull * 1000ull * _uptime -
>> + (1000ull * 1000ull * 1000ull * starttime)
>> + / (unsigned long long)sysconf(_SC_CLK_TCK);
>
> This certainly does not calculate uptime of the guest, merely just the
> time since the process started. This will not work at least in these
> cases:
>
> 1) When the VM is migrated to a different host
> 2) When the VM is saved
> 3) The uptime will not be accurate when the guest was paused
4) If the guest OS reboots, it's uptime restarts
Is storing the timestamp at which the VM was started and then taking a
diff with the current timestamp right way to go?
How is uptime defined in case of (2) and (3) ?
At first, I'd not call it uptime at all. If you want to return the
uptime of the guest you need to use the guest agent connection as you
need to ask the actual guest for it's uptime. Guessing it is always
wrong.
For the calculation you are doing you should invent a different name as
that definitely is not uptime and will be confused with guest uptime.
Additionally I don't think that the stat would be useful.
Peter