Re: [libvirt] [PATCH] Block info query: Add flag to allow failure if not active
On Wed, Dec 18, 2013 at 09:49:46AM -0500, John Ferlan wrote:
On 12/18/2013 09:35 AM, Daniel P. Berrange wrote:
> On Wed, Dec 18, 2013 at 06:58:10AM -0500, John Ferlan wrote:
>
> I'm not convinced this flag is desirable. Can't apps just
> check whether the guest is running themselves, or indeed
> something like RHEV surely knows what its own VM is doing
> already.
I do agree with you and that's been my argument in the referenced BZ;
however, as I understand it - they have a thread that continually polls
for blockInfo information and a separate thread that handles the
migration. The blockInfo thread doesn't have the state information.
But it must have a virDomainPtr instance, so it can use
virDomainGetState() or virDomainIsActive() if it cares
about this.
I contend it's just as simple to add a check about the domain
state and
to get/check the reason value as well. That is - I think the blockInfo
thread should be more aware of state. Of course, the return argument is
that libvirt shouldn't return different answers on consecutive fetches
where the first fetch is when the guest is "active" and the second is
when it's not.
Sure, I agree that libvirt isn't ideal here - our hands are tied by
the fact that QEMU doesn't make this data available to us offline.
If we changed anything on the libvirt side, then I'd want to see us
do a proper fix to get the data but that'd require qemu help;
To be honest this feels a bit hackish and it requires a deliberate
change to their code to not only provide the flag, but handle the error.
Since they'd already be changing their code to handle this, then why not
change to check state. There is a downside to their checking state
though and that's a timing condition where their check could indicate
the guest was active, but the "next clock tick" it becomes inactive.
Thus, their code would be in the same position of getting what is felt
is the wrong answer.
If they check the state after querying the data, then yes they could
get bogus data. If they check the state before querying the info,
then the race condition scenario does not lead to bad behaviour - they
would be simply be discarding the "allocation" value unncessarily
which is exactly the same situation they'd be in if the libvirt API
itself checked this too, and the guest shutoff after libvirt checked
its state. So I don't see a need for this libvirt change here.
I figure opening it up to community discussion will help
"decide" one
way or another the best approach.
I'm all for adjusting the documentation and leaving the code the way it
is now as well.
Updating docs would be my recommendation.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|