Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

Implement KeyAnyValue serialization. Fix integer/long serialization. #36

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

tacho
Copy link

@tacho tacho commented Mar 31, 2014

Add support for serializing KeyAnyValue objects. Also fix integer/long serialization/deserialization.

This allows operations with Host Profiles to succeed. Without this fix, operations like editing a host profile, or applying it - fail due to the object containing KeyAnyValue items, which rbvmomi doesn't know how to serialize to XML at all. Another issue was that the deserializer casts every integer to a long, which for the most part works, but the host profile API does not like that. The fixes for int/long were only done in the new deserializer.

Rake tests pass, and I've tested a lot of other vSphere operations to verify that this doesn't break anything, to the best of my knowledge.

This allows operations with Host Profiles to succeed.
@@ -59,6 +59,8 @@ def deserialize node, type=nil
when :boolean
node.content == '1' || node.content == 'true'
when :int
BasicTypes::Int.new(node.content)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this change? I am worried that existing client code may take offense to seeing this type rather than a regular integer. And I am not sure why it is required. Is it because of serializer changes? If so, I find it quite likely that this breaks something.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. There were originally 2 issues with using RbVmomi for host profile manipulation. The first one was missing support for KeyAnyValue serialization, the second was casting all integers to long - the deserializer strips the exact type (xsd:int, xsd:long) from the incoming data. This normally isn't an issue, but with host profiles it becomes one.

Let me give an example: to update an existing host profile, you need a HostProfileCompleteConfigSpec, which contains the field applyProfile. You don't fill this by hand, but get it from the existing profile managed object's config.applyProfile field (HostProfile.ConfigInfo has a field applyProfile of type HostApplyProfile). You change what's needed and pass it as parameters to UpdateHostProfile. Since that isn't a newly defined object but comes from the deserializer, any original type info for integers has been lost, and by default the serializer casts every int as xsd:long, which usually isn't an issue, but with this particular case it is.

So the fix is to preserve the original type in the deserializer, so the serializer knows what to emit. I'm of course open to a better approach - you have way more experience with that code, so it is possible that I'm going in the completely wrong direction.

But so far my tests don't indicate any breakage on existing code - and I've tested a lot (admittedly haven't covered the whole API) - template cloning, all sorts of VM, host, distributed/standard vSwitch, etc. operations without failures. I've also verified that the emitted XML matches the ones emitted by the vSphere client and by PyVmomi. So I can be pretty certain that this shouldn't break anything that uses the API correctly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an additional comment - existing client code that generates new objects won't hit that. Existing client code that updates existing objects also shouldn't have issues, since for all intents and purposes the BasicTypes::Int mimics Fixnum (assignment, comparison, etc.).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I get your explanation. But existing code that makes an API call, gets an integer and expects to be able to use it like a ruby int will fail, no? Like "foo.bar + 1" will no longer work if bar is of type int. Isn't that what your change would do?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are of course correct. Would inheriting from Integer solve that? I know it isn't very clean since Integer isn't a normal object in Ruby, but it's doable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've amended the pull request to make BasicTypes::Int behave like a Ruby Integer in the aforementioned operations.

@hartsock hartsock modified the milestone: v1.8.3 Sep 23, 2014
@hartsock
Copy link
Contributor

I'm going to admit to not being able to understand this from a first glance so that's why I'm leaving it on the table for later.

@tacho
Copy link
Author

tacho commented Jul 27, 2016

If you have questions - I'm available. It has been used in production for quite a while now within our group and haven't encountered any side effects so far.

@tacho
Copy link
Author

tacho commented Jul 27, 2016

Also, it should be labeled as a bug fix, not an enhancement. Without it, you'll be unable to edit a HostProfile.

@hartsock
Copy link
Contributor

@tacho please do :-)

@hartsock hartsock added bug and removed enhancement labels Jul 27, 2016
@hartsock hartsock modified the milestones: v1.8.2, v1.8.3 Jul 27, 2016
@hartsock hartsock self-assigned this Jul 27, 2016
@hartsock
Copy link
Contributor

I'll try and put this in the 1.8.2 stack and move it back out again if I can't make sense of it.

@hartsock hartsock modified the milestones: v1.8.2, v1.8.3 Jul 29, 2016
@jrgarcia
Copy link
Contributor

jrgarcia commented Aug 1, 2016

@tacho @hartsock I'm looking at this right now, but I'm not too familiar with this section of the code. I'm going to circle back around to this soon and give it a better look.

@jrgarcia jrgarcia removed this from the v1.8.3 milestone May 8, 2019
@jrgarcia jrgarcia removed their assignment Apr 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants