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

[Michal Privoznik]

On 04/17/2018 05:07 PM, John Ferlan wrote:
> 
> 
> On 04/17/2018 10:19 AM, Michal Privoznik wrote:
>> 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 
>> ---
>>  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  to their 
>>> 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.
>>
> 
> Does that code save the alias in some private structure? The correlation
> being it's the libvirt <-> qemu interaction and saving it has
> implications related to what any future change may need to handle.

No it doesn't. Because that devices can't be manipulated. So the alias
is constructed just once, added to the command line and the forgot. But
with pr-manger we need to be able to manipulate 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.
>>
> 
> True...  Of course death of pr-helper is another problem not entirely
> managed by any of this IIRC - the assumption being that if we started
> one once, then it'll run forever, but if it does die then we won't
> because we assume that once started is always started. Still, as you
> pointed out elsewhere some sort of future event should help us to
> perform a restart. Leaving libvirt to manage the qemu problem.
> 
> I guess I see pr-helper as a domain private thing ready to start/stop
> when some other disk source code comes along and says I have this thing
> and need to use the managed 

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

[John Ferlan]

On 04/17/2018 10:19 AM, Michal Privoznik wrote:
> 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 
> ---
>  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  to their 
>> 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.
> 

Does that code save the alias in some private structure? The correlation
being it's the libvirt <-> qemu interaction and saving it has
implications related to what any future change may need to handle.

>>
>> 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.
> 

True...  Of course death of pr-helper is another problem not entirely
managed by any of this IIRC - the assumption being that if we started
one once, then it'll run forever, but if it does die then we won't
because we assume that once started is always started. Still, as you
pointed out elsewhere some sort of future event should help us to
perform a restart. Leaving libvirt to manage the qemu problem.

I guess I see pr-helper as a domain private thing ready to start/stop
when some other disk source code comes along and says I have this thing
and need to use the managed domain pr-helper - please add me. The domain
pr-helper code then could say, well this is a first - something needs to
be started too. Using return values you could know failure=-1, new=0,
just-add=1. Keeping track of the number of disks using it in the domain
private structure and when hotunplug causes that count to go to zero, we
can remove the pr-helper. 

Re: [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 
 ---
  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  to their 
> 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

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

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

[John Ferlan]

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 
>>> ---
>>>  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  to their 
definition I cannot believe would be surprised to see an alias printed.

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.

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?

> 
> 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?


John

>>
>> AFAICT the only thing printed now (@relPath) is something generated via
>> qemu_driver calls (I didn't dig deep); whereas, this data is easily
>> regeneratable from existing domain definition data.
> 
> Yes it is. Currently. But just look at the history of channelTargetDir,
> e.g. a89f05ba8df095875f. We have to have qemuDomainSetPrivatePathsOld(),
> worse we have to keep it around for the rest of libvirt's life time.
> Only because nobody thought of storing channelTargerDir in runtime XML.
> 
> Michal
> 

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

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

[Michal Privoznik]

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 
>> ---
>>  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?

> 
> 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.

Side note, we are not even exposing qemu's PID anywhere because not
every hypervisor we support has VMs as separate processes.

> 
> AFAICT the only thing printed now (@relPath) is something generated via
> qemu_driver calls (I didn't dig deep); whereas, this data is easily
> regeneratable from existing domain definition data.

Yes it is. Currently. But just look at the history of channelTargetDir,
e.g. a89f05ba8df095875f. We have to have qemuDomainSetPrivatePathsOld(),
worse we have to keep it around for the rest of libvirt's life time.
Only because nobody thought of storing channelTargerDir in runtime XML.

Michal

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

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

[John Ferlan]

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 
> ---
>  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).

Again as I noted previously, save the alias when printing the domain
active information and I think you're good.

AFAICT the only thing printed now (@relPath) is something generated via
qemu_driver calls (I didn't dig deep); whereas, this data is easily
regeneratable from existing domain definition data.

John


> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 361a80be84..0856f04406 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1961,13 +1961,74 @@ qemuDomainObjPrivateFree(void *data)
>  }
>  
>  
> +static int
> +qemuStorageSourcePrivateDataParsePR(xmlXPathContextPtr ctxt,
> +virStorageSourcePtr src)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv = 
> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +qemuDomainDiskPRDPtr prd = NULL;
> +int rc;
> +int ret = -1;
> +
> +if ((rc = virXPathBoolean("boolean(./prd)", ctxt)) == 0) {
> +return 0;
> +} else if (rc < 0) {
> +return ret;
> +}
> +
> +if (VIR_ALLOC(prd) < 0)
> +return ret;
> +
> +if (!(prd->alias = virXPathString("string(./prd/alias)", ctxt)) ||
> +!(prd->path = virXPathString("string(./prd/path)", ctxt))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("malformed "));
> +goto cleanup;
> +}
> +
> +VIR_STEAL_PTR(srcPriv->prd, prd);
> +ret = 0;
> + cleanup:
> +qemuDomainDiskPRDFree(prd);
> +return ret;
> +}
> +
> +
> +static int
> +qemuStorageSourcePrivateDataFormatPR(virStorageSourcePtr src,
> + virBufferPtr buf)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv = 
> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +qemuDomainDiskPRDPtr prd;
> +
> +if (!srcPriv || !srcPriv->prd)
> +return 0;
> +
> +prd = srcPriv->prd;
> +
> +virBufferAddLit(buf, "\n");
> +virBufferAdjustIndent(buf, 2);
> +virBufferAsprintf(buf, "%s\n", prd->alias);
> +virBufferEscapeString(buf, "%s\n", prd->path);
> +virBufferAdjustIndent(buf, -2);
> +virBufferAddLit(buf, "\n");
> +return 0;
> +}
> +
> +
>  static int
>  qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
>virStorageSourcePtr src)
>  {
> +if (!(src->privateData = qemuDomainStorageSourcePrivateNew()))
> +return -1;
> +
>  if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0)
>  return -1;
>  
> +if (qemuStorageSourcePrivateDataParsePR(ctxt, src) < 0)
> +return -1;
> +
>  return 0;
>  }
>  
> @@ -1979,6 +2040,9 @@ qemuStorageSourcePrivateDataFormat(virStorageSourcePtr 
> src,
>  if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0)
>  return -1;
>  
> +if (qemuStorageSourcePrivateDataFormatPR(src, buf) < 0)
> +return -1;
> +
>  return 0;
>  }
>  
> 

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

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

[Michal Privoznik]

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 
---
 src/qemu/qemu_domain.c | 64 ++
 1 file changed, 64 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 361a80be84..0856f04406 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1961,13 +1961,74 @@ qemuDomainObjPrivateFree(void *data)
 }
 
 
+static int
+qemuStorageSourcePrivateDataParsePR(xmlXPathContextPtr ctxt,
+virStorageSourcePtr src)
+{
+qemuDomainStorageSourcePrivatePtr srcPriv = 
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
+qemuDomainDiskPRDPtr prd = NULL;
+int rc;
+int ret = -1;
+
+if ((rc = virXPathBoolean("boolean(./prd)", ctxt)) == 0) {
+return 0;
+} else if (rc < 0) {
+return ret;
+}
+
+if (VIR_ALLOC(prd) < 0)
+return ret;
+
+if (!(prd->alias = virXPathString("string(./prd/alias)", ctxt)) ||
+!(prd->path = virXPathString("string(./prd/path)", ctxt))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("malformed "));
+goto cleanup;
+}
+
+VIR_STEAL_PTR(srcPriv->prd, prd);
+ret = 0;
+ cleanup:
+qemuDomainDiskPRDFree(prd);
+return ret;
+}
+
+
+static int
+qemuStorageSourcePrivateDataFormatPR(virStorageSourcePtr src,
+ virBufferPtr buf)
+{
+qemuDomainStorageSourcePrivatePtr srcPriv = 
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
+qemuDomainDiskPRDPtr prd;
+
+if (!srcPriv || !srcPriv->prd)
+return 0;
+
+prd = srcPriv->prd;
+
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+virBufferAsprintf(buf, "%s\n", prd->alias);
+virBufferEscapeString(buf, "%s\n", prd->path);
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+return 0;
+}
+
+
 static int
 qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
   virStorageSourcePtr src)
 {
+if (!(src->privateData = qemuDomainStorageSourcePrivateNew()))
+return -1;
+
 if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0)
 return -1;
 
+if (qemuStorageSourcePrivateDataParsePR(ctxt, src) < 0)
+return -1;
+
 return 0;
 }
 
@@ -1979,6 +2040,9 @@ qemuStorageSourcePrivateDataFormat(virStorageSourcePtr 
src,
 if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0)
 return -1;
 
+if (qemuStorageSourcePrivateDataFormatPR(src, buf) < 0)
+return -1;
+
 return 0;
 }
 
-- 
2.16.1

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