Parallelize info intra vm #3465
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3465 +/- ##
==========================================
- Coverage 88.87% 88.82% -0.06%
==========================================
Files 253 254 +1
Lines 14104 14115 +11
==========================================
+ Hits 12535 12537 +2
- Misses 1569 1578 +9 ☔ View full report in Codecov by Sentry. |
townsend2010
dismissed
their stale review
I put my review comments in the wrong PR...
|
Hey @ricab, Could you please rebase and fix the conflicts here at your earliest convenience? I'm ready to functionally test this and get it in! Thanks! |
Use a single, composite, and sequential command to obtain all the runtime info we need from the VM over SSH. Structure the code such that it can be reused for a parallel command.
Use a parallel single composite command to obtain runtime information from VMs that have more than one CPU. Continue using the sequential version for VMs with a single CPU.
Add a placeholder helper class meant to house the code to populate runtime instance info, with corresponding header and compilation unit, to avoid polluting the unnamed namespace in the daemon compilation unit any further.
Move utility to verify the validity of IPv4 addresses to the generally accessible Utils singleton, in preparation for its use in the RuntimeInstanceInfoHelper class.
Shorten the name of implementation-detail classes whose scope is now contained.
ricab
force-pushed
the
parallelize-info-intra-vm
branch
from
10de80a to
ebb3b0a
Compare
|
Hey @townsend2010, just to let you know I've rebased. |
There was a problem hiding this comment.
Hey @ricab!
Nice, great work here! Works well in practice and the code is code. 👍
| instance_info->set_cpu_times(vm.ssh_exec("head -n1 /proc/stat")); | ||
| instance_info->set_uptime(vm.ssh_exec("uptime -p | tail -c+4")); |
There was a problem hiding this comment.
Oh no, I just realized these were not carried into populate_runtime_info()!!!!! And now the CPU usage meter is missing in the GUI!!!!
There was a problem hiding this comment.
Bah, I must have missed it during a rebase.
Introduce the parallelization of the SSH commands that are used for
infowithin VMs with multiple cores.With the exception of the command to fetch IPs (which follows a different scheme that is used also for
list), all the necessary commands are combined into a single one and executed in one go on the instane over SSH. Two versions of this single combined command are derived: one sequential and one parallel. The parallel version is used when the VM has multiple cores. On single-core VMs, the sequential version is used.For organization, and to avoid polluting the unnamed namespace in
daemon.cppany further, the code is now in a new helper class:RuntimeInstanceInfoHelper.Credit goes to @andrei-toterman for the idea implemented here. Thanks!