[RFR] wrongly updated method during wrapanapi 3.0 conversion #351
Conversation
| @@ -368,7 +368,7 @@ def mark_as_template(self, template_name=None, cluster=None, delete=True, delete | |||
| def get_hardware_configuration(self): | |||
| self.refresh() | |||
| return { | |||
| 'ram': self.raw.memory, | |||
| 'ram': self.raw.memory / 1024 / 1024, | |||
There was a problem hiding this comment.
This was intentionally removed, I believe any calculations done on the memory value reported by rhevm should be done by the caller. If we decide to manipulate the values here, it needs to be very obviously documented - otherwise I believe callers will expect the units (assumed and not stated in our return) that are reported by rhevm's API.
There was a problem hiding this comment.
@mshriver, I don't mind but value should be the same for all providers. All providers except rhevm return value in MB.
This unexpected change broke one of sprout task chains.
There was a problem hiding this comment.
Seems we're scratching at a fundamental wrapanapi question then, one which we need to apply uniformly across the modules:
Should wrapanapi manipulate scalar values that have a unit, such that all returns are in the same unit (Mhz/Ghz, MB/GB, etc)?
If we have some agreement among the maintainers that in fact it should report consistent scalar units across providers, than we need to be consistent across all the wrapanapi methods (do the same for storage, memory, network methods)
There was a problem hiding this comment.
I'm going to approve PR with this change as the impact to integration_tests looks small. I would like to hear from @psav, @bsquizz, @mkoura, @miha-plesko, and others on this topic.
Ultimately I agree with you Ievgen, wrapanapi should standardize the scalar units reported by the various providers, but we need thorough documentation of the intended unit conversions.
| @@ -223,7 +223,7 @@ def enable_virtual_services(self): | |||
|
|
|||
| def get_hardware_configuration(self): | |||
| self.refresh(read_from_hyperv=True) | |||
| data = {'mem': self.raw['CPUCount'], 'cpu': self.raw['Memory']} | |||
| data = {'mem': self.raw['Memory'], 'cpu': self.raw['CPUCount']} | |||
|
MIQ source code (in ruby) uses numbers bundled with units; for example I kind of like it because it's nicely readable and you can use whatever unit you find appropriate and it gets converted correctly. Perhaps we could opt-in for something similar here? Googling gives me this library https://pypi.org/project/numericalunits, but I haven't actually tried it yet. Wdyt, should we go that direction? |
Some sprout tasks fail during vm hardware configuration update because get_vm_hardware methods return memory in bytes instead of MB