Re: [libvirt] [PATCH v4 05/14] qemu: Store pr runtime data in status XML

On 04/17/2018 02:00 PM, John Ferlan wrote: > > > On 04/16/2018 10:56 AM, Michal Privoznik wrote: >> On 04/13/2018 10:57 PM, John Ferlan wrote: >>> >>> >>> On 04/10/2018 10:58 AM, Michal Privoznik wrote: >>>> Now that we generate pr-manager alias and socket path store them >>>> in status XML so that they are preserved across daemon restarts. >>>> >>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >>>> --- >>>> src/qemu/qemu_domain.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 64 insertions(+) >>>> >>> >>> So if we were to save this information (and at this point I don't think >>> we need to), then this is where we should be allocating and filling in >>> the private data (e.g. not in the previous patch). >> >> How come? What would be left from the previous patch if private runtime >> struct would be introduced only here? Or are you just suggesting >> swapping these two patches? >> > > I hope I provided enough feedback in the prior response to answer this. > >>> >>> Again as I noted previously, save the alias when printing the domain >>> active information and I think you're good. >> >> No, I don't want to expose info on PR helper more than is necessary. The >> fact that it's a separate process should not be visible to users because >> it is an implementation detail of QEMU. Other hypervisors might do this >> differently. And even though it might not be visible from the patches, >> using unmanaged mode is discouraged. In fact, unmanaged mode is on the >> edge. If pr-helper is viewed as internal implementation the unmanaged >> mode has no place in libvirt. However, qemu devels are experimenting >> with systemd socket activation and for socket path must be configurable >> through libvirt. So the only reason for using unmanaged PRs is systemd >> socket activation. > > We "expose" aliases a lot in the active domain XML. Someone that's going > to add a <reservations enabled='yes' managed='yes'/> to their <disk...> > definition I cannot believe would be surprised to see an alias printed. We already don't expose some aliases. For instance, if a domain is configured to use hugepages and we use memory-backend-file we don't report generated aliases anywhere. Why? Because the fact we are using memory-backend-file to tell qemu to use hugepages is internal implementation. And users should not be concerned with that. It is the same story with pr-manager and its alias. It is internal implementation deatail and as such we should not expose it.

> > How would they know from the alias that it's a separate process? The > only way to correlate the two would be to read the code and know what > QEMU did to make libvirt do a little dance in order to support. You probably misunderstood what I meant. My idea is to expose as little info back to user as possible in this case. I don't see any compelling reason for user to learn the pr-manager's alias. > > As for systemd, oh great another area to fall flat on our faces... > Wasn't another reason to shorten the path w/ domain name because there > was some sort of bad systemd interaction? Don't recall. It's not relevant. > >> >> Side note, we are not even exposing qemu's PID anywhere because not >> every hypervisor we support has VMs as separate processes. >> > > The PID though could be an unexposed domain private data, couldn't it? Why should we track PID of pr-helper? What do we need it for? As Peter pointed out in review to my previous patches, PIDs change therefore if we start pr-helper process with PID X, later when shutting down domain we could find a different process under the same PID. Because pr-helper might have died, released the PID and another process could have been started with the same PID.