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.

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

webadmin: Refactor NUMA for VM #488

Merged
merged 2 commits into from Jul 5, 2022
Merged

Conversation

ljelinkova
Copy link
Collaborator

With recent changes in the VM NUMA area the behavior of the NUMA count and the NUMA support button was not consistent and sometimes the changes were not saved on the backend at all.

This patch refactors the source code so that:

  • The numaEnabled field has been removed from the UnitVmModel because after the recent changes only the NUMA support button was affected by its value. Instead, we enable and disable directly the NUMA support action.
  • the NUMA support was renamed to NUMA pinning (in the code, not UI message) as we now enable NUMA by setting the numaCount
  • the updateNuma parameter sent to the backend is now calculated in the behaviors. For the new VM, it is sent if the numa count > 0. For existing VMs, there is a series of checks to determine if the NUMA should be updated on the backend. As a result, the initialVmNumaNodes has been moved from UnitVmModel to the ExistingVmModelBehavior because that is the only place where it is used.

Bug-Url: https://bugzilla.redhat.com/2099225

@ljelinkova
Copy link
Collaborator Author

Ready for the review - next run configuration for NUMA will be fixed in a separate PR.

With recent changes in the VM NUMA area the behavior
of the NUMA count and the NUMA support button was not consistent
and sometimes the changes were not saved on the backend at all.

This patch refactors the source code so that:
- The numaEnabled field has been removed from the UnitVmModel because
after the recent changes only the NUMA support button was affected by
its value. Instead, we enable and disable directly the NUMA support action.
- the NUMA support was renamed to NUMA pinning (in the code, not UI message) as
we now enable NUMA by setting the numaCount
- the updateNuma parameter sent to the backend is now calculated in the behaviors.
For the new VM, it is sent if the numa count > 0. For existing VMs, there is a series
of checks to determine if the NUMA should be updated on the backend. As a result,
the initialVmNumaNodes has been moved from UnitVmModel to the ExistingVmModelBehavior
because that is the only place where it is used.
- added support from configuring NUMA when creating a new VM from a template page

Bug-Url: https://bugzilla.redhat.com/2099225
@ljelinkova
Copy link
Collaborator Author

/ost

Copy link
Member

@ahadas ahadas left a comment

Choose a reason for hiding this comment

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

looks good, looked briefly at some of the UI code though

When updating a VM, it was required to send updateNuma
parameter that determined if the numa node changes will be saved.

The UI had to handle the logic which started to be quite complicated
- the numa nodes changes with memory, cpu, hugepages, numa count,
numa pinning, numa tune mode change.

This patch adds the possibility to leave out that parameter and
in that case the new numa node list is compared to the one
in the database. If they differ, the new numa node list will
be saved.
@ljelinkova
Copy link
Collaborator Author

/ost

Copy link
Member

@ahadas ahadas left a comment

Choose a reason for hiding this comment

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

let's get it in so we'll be able to get early feedback on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants