Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFR] wrongly updated method during wrapanapi 3.0 conversion #351

Merged
merged 1 commit into from
Jan 4, 2019
Merged

[RFR] wrongly updated method during wrapanapi 3.0 conversion #351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion wrapanapi/systems/rhevm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@izapolsk izapolsk Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

'cpu': self.raw.cpu.topology.cores * self.raw.cpu.topology.sockets
}

Expand Down
2 changes: 1 addition & 1 deletion wrapanapi/systems/scvmm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']}
Copy link
Collaborator

Choose a reason for hiding this comment

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

hardcore oops.

return {
key: str(val) if isinstance(val, six.string_types) else val
for key, val in data.items()
Expand Down