Use the correct XML attribute to indicate volume status

[epienbroek]

No description provided.

[abonas]

@jhernand was this indeed fixed in 3.4.1 that it can be no longer reproduced?
@epienbroek if I understood you correctly, this particular patch is no longer necessary?

[jhernand]

The status of any object (not just volumes) is potentially something like this:

<status>
  <state>ok</state>
  <detail>A detailed description of the state</detail>
</status>

For most objects we don't currently populate the detail part. But any code dealing with these responses from the server should be prepared to handle it, as it may be added at any point in time.

The current code in rbovirt, if I understand it correctly, is assuming that status is a text only node, and is getting the text content directly. This probably results in concatenating all the inner text nodes (not 100% sure, I'm not familiar with the Ruby XML library) so in the above example you would get the string ok A detailed description of the state. Also, as the XML document is pretty-printed, white space is also added to split lines and to indent. This white space also be part of the result.

I'd suggest to modify all places where status is handled so that the code explicitly gets the value of the first status/state.

@abonas I'm afraid I didn't explain correctly. When I say that there are potentially multiple values I mean that there are multiple (up to two) elements inside status, currently state and detail. Also, in the future, new elements may be added, so unconditionally getting all the text of the outer node isn't good idea.

[abonas]

@jhernand, very detailed info, thanks, indeed I misunderstood when you wrote before regarding a "list of pairs containg state and description".
Looking at the volume.rb code again (lines 40-41), it seems that there is some sort of awareness of the state field (also in network.rb):
@status = ((xml/'status').first.text rescue nil)
@status ||= ((xml/'status/state').first.text rescue nil)

what doesn't make sense to me is that it's mentioned above it stopped reproducing.

[epienbroek]

@abonas Back when the issue was first discovered (one month ago) we were already using RHEV 3.4.1 (no software updates have happened on our environment recently). I don't know what has changed in our setup that could have 'resolved' the issue..

Anyway, I guess that the statement @status = ((xml/'status').first.text rescue nil) shouldn't be used anyway and only the status/state field should be used. If desired I can update this PR for that

[abonas]

@jhernand your opinion? should this patch be altered to take care of it, or do it separately for all relevant entities/how many entities are handled incorrectly?

[jhernand]

I agree with the latest suggestion from @epienbroek, keep only the following line:

@status = ((xml/'status/state').first.text rescue nil)

I just tested this, with latest 3.6, and it works correctly. Without that, as I explained before, the whitespace used for pretty printing is included in the result.

There wasn't any change in this area in oVirt as far as I know, this worked like this for a long time. @epienbroek, maybe the behaviour of the Ruby .text method changed? Or maybe you added a call to .strip somwhere else in your code?

@abonas I'd suggest to use this PR to fix this known issue with volumes, and others if the issue exists in other places.

[jhernand]

I see the same issue in the following source files:

lib/ovirt/datacenter.rb:      @status = (xml/'status').first.text.strip
lib/ovirt/host.rb:      @status = (xml/'status').first.text
lib/ovirt/template.rb:      @status = ((xml/'status').first.text rescue 'unknown')
lib/ovirt/vm.rb:      @status = ((xml/'status').first.text rescue 'unknown')

In network.rb the value is extracted correctly:

lib/ovirt/network.rb:      @status = ((xml/'status/state').first.text rescue nil)

[epienbroek]

Thanks for the research, I'll update this PR with an updated commit soon

[abonas]

@jhernand , @epienbroek the plan sounds good to me :)
@jhernand , a general thought - perhaps the other entities also have the status field? that way we could take care of it once, for instance in base_object or something similar. currently there's a heavy "copy pasting" here, would be nice to avoid it if possible.

[jhernand]

Potentially all entities have a status element, as it is defined as part of the base class in oVirt server side (but not all entities actually populate it). I agree is a good candiate for BaseObject.

[epienbroek]

@abonas I just checked, but the REST API at https://RHEV_SERVER/api/clusters (which should be used by rbovirt's lib/ovirt/cluster.rb) doesn't contain a status element so I'm not sure that catching the status element from rbovirt's baseobject will work for all situations

[abonas]

@epienbroek, @jhernand is the oVirt REST maintainer, so I'd like to hear his opinion how to handle it, he knows all the entities really well :)

[jhernand]

In the oVirt server all objects, including clusters, may have a status element. Should it be present it will have always the same format, as described before. The cluster just happens to be one of those objects that doesn't populate it. So, all in all, it is safe to add a status attribute to BaseObject and parse it assuming the previously described structure, as long as you make sure that it isn't mandatory. You can do it in a generic way, inside BaseObject as suggested by @abonas or fix the multiple places where it is currently parsed. I really don't have an strong preference.

[abonas]

I think the right way to do it is to handle it generically in base obj. Having said that, it doesn't have to be part of this patch. What should be part of this patch is correct handling of the status in volume - for which this patch was originally opened for.
So... @epienbroek please adjust this patch to handle correctly the status AND rename the subject because this is no longer a whitespacing issue - so it will reflect what it actually fixes.
In the meantime, I will create a separate open issue to be considered later on for a better generic status handling in base obj.

[epienbroek]

This PR has just been updated with a new commit.
A testcase for this change was added in #43

[jhernand]

Looks good to me, I'd merge it if I had permissions.

[abonas]

@abenari can you add @jhernand as collaborator?

[abenari]

@jhernand you are now collaborator!
welcome on board.

[jhernand]

Thanks @abenari, I'm merging this pull request then.

[epienbroek]

Thanks for the approval!
Could you please also take a look at #43 ?

[lzap]

Guys I am running rbovirt integration tests against oVirt 3.4 and 3.5 on RHEL6 and RHEL7. In case it fails, I will comment on particular PRs. I cannot do beforehand tests tho, only after it's merged. I am not there yet...

[lzap]

I mean - it runs automatically every night.