On 05/03/2018 05:26 AM, Nikolay Shirokovskiy wrote:
>
>
> On 01.05.2018 01:03, John Ferlan wrote:
>>
>>
>> On 04/18/2018 10:44 AM, Nikolay Shirokovskiy wrote:
>>> Clearing beingDestroyed right after acquiring job condition is racy.
>>> It is not known when EOF will be delivired. Let's keep this flag
>>
>> delivered
>>
>>> set. This makes possible to make a clear distinction between monitor
>>> errors/eofs and domain being destroyed in qemuDomainObjWait.
>>>
>>> Also let's move setting destroyed flag out of qemuProcessBeginStopJob
>>> as the function is called from eof handler too.
>>>
>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
>>> ---
>>> src/qemu/qemu_domain.c | 4 ++--
>>> src/qemu/qemu_domain.h | 2 +-
>>> src/qemu/qemu_driver.c | 8 +++++++-
>>> src/qemu/qemu_process.c | 24 ++++++++++++------------
>>> 4 files changed, 22 insertions(+), 16 deletions(-)
>>>
>>
>> This one didn't git am -3 apply as well, but I see you're changing
>> DomainObjWait so that makes sense as to why.
>>
>> I do recall looking at this code at one time, but I cannot recall my
>> exact conclusion given how qemuDomainObjBeginJobInternal allows the
>> QEMU_JOB_DESTROY to be run, but then waits for whatever is taking place
>> now to finish.
>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index 1f40ff1..431901c 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -11913,9 +11913,9 @@ qemuDomainObjWait(virDomainObjPtr vm, unsigned long long until)
>>> return -1;
>>> }
>>>
>>> - if (!virDomainObjIsActive(vm)) {
>>> + if (priv->destroyed) {
>>> virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>>> - _("domain is not running"));
>>> + _("domain is destroyed"));
>>> return -1;
>>> }
>>>
>>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>>> index 494ed35..69112ea 100644
>>> --- a/src/qemu/qemu_domain.h
>>> +++ b/src/qemu/qemu_domain.h
>>> @@ -269,7 +269,7 @@ struct _qemuDomainObjPrivate {
>>> bool agentError;
>>>
>>> bool gotShutdown;
>>> - bool beingDestroyed;
>>> + bool destroyed;
>>> char *pidfile;
>>>
>>> virDomainPCIAddressSetPtr pciaddrs;
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 03969d8..4356c0d 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -2227,7 +2227,13 @@ qemuDomainDestroyFlags(virDomainPtr dom,
>>> state = virDomainObjGetState(vm, &reason);
>>> starting = (state == VIR_DOMAIN_PAUSED &&
>>> reason == VIR_DOMAIN_PAUSED_STARTING_UP &&
>>> - !priv->beingDestroyed);
>>> + !priv->destroyed);
>>> +
>>> + /* We need to prevent monitor EOF callback from doing our work (and
>>> + * sending misleading events) while the vm is unlocked inside
>>> + * BeginJob/ProcessKill API
>>> + */
>>> + priv->destroyed = true;
>>
>> would this be the right place anyway? especially since you don't clear
>> it in error paths after setting it. Once the job starts and we either
>> goto cleanup or endjob on failure - how does a future call distinguish?
>
> We send SIGTERM/SIGKILL right away after this flag is set so even if
> this API fails eventually keeping destroyed flag set looks good because we
> send signal to qemu and good chances are qemu will die because of that.
>
> I guess it will be nicer then to move setting the flag to qemuProcessBeginStopJob
> function to keep setting the flag and killing domain together.
>
> Anyway we can clear the flag on failure too.
>
>>
>> Not sure this works conceptually. At least with the current model if
>> DestroyFlags finally gets the job, it checks the domain active state,
>> and goes to endjob after printing a message. So if while waiting, EOF
>> occurred, there's no qemuProcessStop
>
> Not true. After sending signal to qemu to terminate EOF will occur but
> handler will return right away because of beingDestroyed/destroyed flag
> check and then after destroyFlags gets the job it will call qemuProcessStop.
>
> This is not the place I'm addressing with the patch. It is rather if destroyFlags
> is called when we are migrating for example then the interrupted
> API call can report 'domain is not active'/'some monitor error' rather
> then much nicer 'domain is destroyed' without this patch. See the
> scenario below.
>
>>
>> Perhaps the only thing I recall wondering is whether we clear
>> beingDestroyed too soon... If we're successful, we're still destroying
>> but not until destroy is successful should the flag then be cleared
>> while still holding the job.
>
> It does not matter if we clear the flag at the begin or the end of time
> we holding the job because during the job nobody else who needs the job
> too can progress.
>
> I propose to prolong setting the flag after destroy job is finished (and
> thus rename flag too). Consider next scenario:
>
> - migrate is called and wait for migrationg complete event from qemu
> - we call destroy and as destroy can run concurrently with migration
> destroy gets the job and executes domain stop
> - migrate awaiks on monitor EOF, beingDestroyed is cleared back at this
> point. We can check for domain is active and give 'domain is not active'
> error but 'domain is destroyed' is nicer thus we need 'destroyed' flag
>
> Nikolay
>
Honestly - hoping things will be clearer for the round of this.
John
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
