-
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
core: update cpuTopology on status changes #525
Conversation
if (cachedVds.getDynamicData().getStatus() == VDSStatus.Maintenance) { | ||
cpuTopology = new ArrayList<>(); |
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.
This probably should part of InitVdsOnUpCommand
, where the host switch from maintenance to UP without clearing it out, just recalling initAvailableCpus()
.
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.
Since it is just update of VdsManager fields, I would rather put it into VdsManager.setStatus()
.
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.
This probably should part of
InitVdsOnUpCommand
, where the VM switch from maintenance to UP without clearing it out, just recallinginitAvailableCpus()
.
I would prefer to keep the logic inside the VdsManager
, The cpuTopology
field is internal to VdsManager
, it makes sense that it is initialized and handled here - there is no need for the outside world to know about it.
The field is cleared to make sure that it is not accidentally overwritten - e.g. when the host is non responsive but still runs the VMs and then it is set to UP state again.
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.
Since it is just update of VdsManager fields, I would rather put it into
VdsManager.setStatus()
.
If we updated only when setting the status, the cachedVds might not have the cpuTopology initialized yet. So I added it also to the refresh() method that updates the cachedVds.
ccb2007
to
fe6e367
Compare
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java
Show resolved
Hide resolved
@@ -294,6 +294,7 @@ public void refreshImpl() { | |||
setIsSetNonOperationalExecuted(false); | |||
synchronized (this) { | |||
refreshCachedVds(); | |||
updateCpuTopology(); |
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.
we call here the cpuTopology update and within we use cachedVds
, but based on 2 lines below it may be null.
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.
+1
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.
you're right, I've updated the patch
Previously, once the CPU topology was initialized in the VdsManager, it was not possible to change it without the restart of the engine. There are 2 use cases: 1. When adding a new host, the manager is created, but no CPU topology has been reported yet. As a result, the host would act as if it does not support exclusive pinning until the engine is restarted. 2. Changing the CPU topology of the host (not a common scenario, but happens during development when using hosts in a virtual machine). This patch updates the CPU topology in the VmManager if the state of the host changes and the CPU topology has not been initialized yet. Bug-Url: https://bugzilla.redhat.com/2104858
fe6e367
to
216132c
Compare
/ost |
Previously, once the CPU topology was initialized in the VdsManager, it was not possible to change it without
restarting the engine. There are 2 use cases:
would act as if it does not support exclusive pinning until the engine is restarted.
This patch updates the CPU topology in the VmManager if the state of the host changes and the CPU topology has not been initialized yet.
Bug-Url: https://bugzilla.redhat.com/2104858