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

[Michal Privoznik]

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 at redhat.com>
>>>> ---
>>>>  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.

Michal