-
Notifications
You must be signed in to change notification settings - Fork 261
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
Reset NUMA configuration with resize and pin VM #504
Conversation
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Outdated
Show resolved
Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Outdated
Show resolved
Hide resolved
...manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java
Outdated
Show resolved
Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsEventListener.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch looks good and will make some flows easier. However, it might still be missing some parts if we want to keep (resize and pin -> no numa nodes defined by the user as invariant).
For example, we should set an empty list also when creating a new VM with resize and pin numa (for REST API), for the import and we could drop some validations in VmHandler.validateCpuPinningPolicy().
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Outdated
Show resolved
Hide resolved
b374935
to
244cdda
Compare
AFAIK, for REST-API you need to execute other actions, not AddVm. |
.../common/src/main/java/org/ovirt/engine/core/common/action/VmNumaNodeOperationParameters.java
Outdated
Show resolved
Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Outdated
Show resolved
Hide resolved
...odules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/monitoring/VmsMonitoring.java
Outdated
Show resolved
Hide resolved
244cdda
to
3171014
Compare
Could you please update to the latest master before we continue the review? Changes introduced in 2d8e98f will effect your PR. |
3171014
to
1b6f5c2
Compare
Any specific flow I should test with it? The annoying part now is about running VM, changing something that relates to NUMA (selecting host to run on, changing to high performance), indicates that NUMA configuration changed but not really. |
1b6f5c2
to
a1b7dca
Compare
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Outdated
Show resolved
Hide resolved
...r/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/monitoring/VmAnalyzer.java
Outdated
Show resolved
Hide resolved
a1b7dca
to
092ec61
Compare
/ost |
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java
Outdated
Show resolved
Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Outdated
Show resolved
Hide resolved
092ec61
to
1e6e09d
Compare
/ost |
1e6e09d
to
fffd518
Compare
do we still need this part or is everything sorted out with the last change that skipped that validation? |
well, now we reset the vNUMA count to 0 on update/shutdown. so it's still correct to my eyes. |
/ost |
I meant whether we need it in the code itself - if we clear the topology when the vm reaches down state and in update-vm we skip the problematic validation, is there another justification for setting the nodes to empty list on update-vm? |
I think that is is a reasonable default and it is more understandable to the user, compared to having some nodes when the VM is down and different during the runtime. And when the VM is stopped back to the static nodes... It is confusing. |
i agree. although i think another change is required to the UI (in resize and pin don't show the popup field regarding vNUMA change), but without this part in backend it will be even more confusing, it will looks as there is next-run changes. |
/ost |
I'm not sure we talk about the same thing then because I'm talking about something that should not be visible for users but to developers that see the code - from user's perspective it would be the same - the vm won't have numa nodes as long as the vm is down |
fffd518
to
5f911f9
Compare
the current state hopefully answers it. |
5f911f9
to
839709d
Compare
/ost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks better now, minor comment inside
I'm ok with dropping the validation of numa nodes and ignore it instead, if that simplifies the client side
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Outdated
Show resolved
Hide resolved
When running a `Resize and Pin NUMA` VM, we create the virtual NUMA nodes including the pinning to the physical NUMA nodes of the host. This is a static configuration that being set on the VM. Until now, this configuration harmed many flows to update the VM configuration and a manual workaround needed to overcome it. Now the NUMA configuration will be deleted once the VM shutdown and when updating the VM, it will see the current configuration as there is no NUMA to that VM. Change-Id: I7d6e8200e7830dc6903a76ac219225fa359d2c53 Bug-Url: https://bugzilla.redhat.com/2074525 Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>
839709d
to
13a6922
Compare
/ost |
When running a
Resize and Pin NUMA
VM, we create the virtual NUMAnodes including the pinning to the physical NUMA nodes of the host.
This is a static configuration that being set on the VM.
Until now, this configuration harmed many flows to update the VM
configuration and a manual workaround needed to overcome it. Now the
NUMA configuration will be deleted once the VM shutdown and when
updating the VM, it will see the current configuration as there is no
NUMA to that VM.
Change-Id: I7d6e8200e7830dc6903a76ac219225fa359d2c53
Bug-Url: https://bugzilla.redhat.com/2074525
Signed-off-by: Liran Rotenberg lrotenbe@redhat.com