Skip to content

Override memory method to convert KubeVirt format to bytes#186

Merged
evgeni merged 1 commit intotheforeman:masterfrom
archanaserver:fix-memory-display
Feb 18, 2026
Merged

Override memory method to convert KubeVirt format to bytes#186
evgeni merged 1 commit intotheforeman:masterfrom
archanaserver:fix-memory-display

Conversation

@archanaserver
Copy link
Contributor

Fixes the host edit form displayed 'NaN MB' for KubeVirt VM memory

@archanaserver
Copy link
Contributor Author

Those CI tests are failing seems like unrelated to this change.

@evgeni
Copy link
Member

evgeni commented Feb 12, 2026

Ci failing is unrelated, yes.

This makes the edit of a host work for me, but now the "VMs" list on the CR throws

2026-02-12T03:06:42 [W|app|6881f772] no implicit conversion of Integer into String
2026-02-12T03:06:42 [I|app|6881f772] Backtrace for 'no implicit conversion of Integer into String' error (ActionView::Template::Error): no implicit conversion of Integer into String
 6881f772 | /usr/share/gems/gems/fog-kubevirt-1.6.0/lib/fog/kubevirt/compute/utils/unit_converter.rb:42:in `match'
 6881f772 | /usr/share/gems/gems/fog-kubevirt-1.6.0/lib/fog/kubevirt/compute/utils/unit_converter.rb:42:in `validate'
 6881f772 | /usr/share/gems/gems/fog-kubevirt-1.6.0/lib/fog/kubevirt/compute/utils/unit_converter.rb:22:in `convert'
 6881f772 | /usr/share/gems/gems/foreman_kubevirt-0.5.1/app/models/foreman_kubevirt/kubevirt.rb:542:in `convert_memory'
 6881f772 | /usr/share/gems/gems/foreman_kubevirt-0.5.1/app/models/foreman_kubevirt/kubevirt.rb:313:in `convert_memory_to_bytes'
 6881f772 | /usr/share/gems/gems/foreman_kubevirt-0.5.1/app/views/compute_resources_vms/index/_kubevirt.html.erb:15:in `block in __usr_share_gems_gems_foreman_kubevirt_______app_views_compute_resources_vms_index__kubevirt_html_erb___567930508050518905_935980'
 6881f772 | /usr/share/gems/gems/fog-core-2.6.0/lib/fog/core/collection.rb:19:in `each'
 6881f772 | /usr/share/gems/gems/fog-core-2.6.0/lib/fog/core/collection.rb:19:in `each'
 6881f772 | /usr/share/gems/gems/foreman_kubevirt-0.5.1/app/views/compute_resources_vms/index/_kubevirt.html.erb:11:in `__usr_share_gems_gems_foreman_kubevirt_______app_views_compute_resources_vms_index__kubevirt_html_erb___567930508050518905_935980'
 6881f772 | /usr/share/gems/gems/actionview-7.0.10/lib/action_view/base.rb:244:in `public_send'

@arvind4501
Copy link

arvind4501 commented Feb 12, 2026

Yes IT breaks on compute resource -> vms as here memory method is overriden and https://github.com/theforeman/foreman_kubevirt/blob/master/app/views/compute_resources_vms/show/_kubevirt.html.erb#L23 here with the current changes we pass the converted memory not the original one(which comes from fog). so technically converting it twice vm.memory does it once and then convert_memory_to_bytes call does it again

@stejskalleos stejskalleos self-assigned this Feb 12, 2026
@archanaserver
Copy link
Contributor Author

yeah so, convert_memory_to_bytes method trying to convert values that were already converted to bytes, and didn't handle empty strings. and with this it seems to be working.

# @param memory - The memory of the VM to convert
#
def convert_memory_to_bytes(memory)
return nil if memory.nil? || memory == ''

Choose a reason for hiding this comment

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

I think if we are completely skipping convert_memory method call here than isn't it better to remove convert_memory_to_bytes method from https://github.com/theforeman/foreman_kubevirt/blob/master/app/views/compute_resources_vms/show/_kubevirt.html.erb#L23 and let @vm.memory handle conversion itself?

@@ -310,6 +310,13 @@ def max_memory
# @param memory - The memory of the VM to convert
#
def convert_memory_to_bytes(memory)
Copy link
Member

Choose a reason for hiding this comment

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

I think no users of convert_memory_to_bytes are left, so we can drop that method alltogether.

alias_method :fog_memory, :memory

define_method(:memory) do
raw_memory = fog_memory
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to create a copy of the value here?
You're not modifying it and Server.memory should already be just accessing an attribute, so there seems to be no performance benefit to store it locally before accessing it?

Not blocking, just curious :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point but no strong reason. I initially kept raw_memory for readability while working through the logic and to make debugging thing easier while validating the fallback behavior. but either way it's fine for me, so I simplified it. Thanks for the catch!

@evgeni
Copy link
Member

evgeni commented Feb 16, 2026

Tested using rubygem-foreman_kubevirt-0.5.1-1.20260212112949781331.pr186.2.g4051aff.el9.noarch.rpm, works fine on the host edit screen and in the CR VM list.

Two comments, but neither is blocking.

Fixes the host edit form displayed 'NaN MB' for KubeVirt VM memory
Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

Final round with rubygem-foreman_kubevirt-0.5.1-1.20260218045458190635.pr186.2.g7b7375f.el9.noarch.rpm, still works fine.

Thanks!

@evgeni evgeni merged commit a447833 into theforeman:master Feb 18, 2026
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants