Block hot plug CPU with exclusive pinning

[liranr23]

This patch prevent the user from hotplugging CPU when the VM is set with
exclusive pinning as we don't support it. Currently it will add a new
CPU without any pinning, which breaks the VM configuration.
With this patch, it won't be allowed and the user will be noticed.

Change-Id: Ie8d32b9e85879e6764bab63399c3d5762ba806db
Signed-off-by: Liran Rotenberg lrotenbe@redhat.com

[liranr23]

/ost

[ahadas]

and then let's set the topology accordingly (with no extra sockets for future hot-plugs)?

[liranr23]

and then let's set the topology accordingly (with no extra sockets for future hot-plugs)?

We already do.
And actually, the user won't notice anything because the validation is already being done in HotSetNumberOfCpusCommand:

if (getVm().getCpuPinningPolicy().isExclusive()) {
  valid = failValidation(EngineMessage.HOT_PLUG_CPU_IS_NOT_SUPPORTED_DEDICATED,
          String.format("$policy %1$s", getVm().getCpuPinningPolicy().name()));
}

We can either change it to the update command, to have it more visible or leave it as is. The user can see the error in the log:

2022-06-16 10:37:59,196+03 WARN  [org.ovirt.engine.core.bll.HotSetNumberOfCpusCommand] (default task-7) [11bdbc32] Validation of action 'HotSetNumberOfCpus' failed for user admin@internal-authz. Reasons: VAR__ACTION__HOT_SET_CPUS,VAR__TYPE__VM,$clusterVersion 4.7,$architecture x86_64,HOT_PLUG_CPU_IS_NOT_SUPPORTED_DEDICATED,$policy DEDICATED
2022-06-16 10:37:59,214+03 ERROR [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector] (default task-7) [11bdbc32] EVENT_ID: FAILED_HOT_SET_NUMBER_OF_CPUS(2,034), Failed to hot set number of CPUS to VM el8. Underlying error message: Hot plugging a CPU is not supported with the CPU pinning policy DEDICATED.

That being said, the update vm command will pass, without any changes to the VM or pending changes (next-run).

[ljelinkova]

This should be probably changed to check only the number of shared CPUs of the host otherwise we will end up with running VM that has more cores than shared CPUs on the host and I am not sure how that affects calculations in the scheduler.

[liranr23]

so how about:
vmStaticData.getNumOfCpus(false) > getVds().getCpuThreads() - VdsCpuUnitPinningHelper.countUnavailableCpus(.., true) ?
the true is because getCpuThreads() is the total amount of threads.

[ljelinkova]

shouldn't we also take into account countThreadsAsCore?

[liranr23]

I missed this comment.
The only reason to not consider it is because the comment: In alignment with the CPUPolicyUnit, VM's hyperthreading is not considered.
I just tried on 2:2:2 host, having a vm with 2:2:2 and changed it to 3:2:2 (from 8 CPUs to 12 CPUs) and it worked - which logically shouldn't, right?
So either we stay with this comment above and keep it that way, or we will need to change it to always true? i am not sure if we should consider the cluster configuration in that case.

[ljelinkova]

I am sorry, I was not clear - I meant if we should consider countThreadsAsCores when counting available CPUs on the host.

int availableThreads = getVds().getCpuThreads() - vdsCpuUnitPinningHelper.countUnavailableCpus(
                getVdsManager(getVdsId()).getCpuTopology(), true);

Looking into the CpuPolicyUnit it should be something like this:

int availableCpus = SlaValidator.getEffectiveCpuCores(host, getCluster().getCountThreadsAsCores()) - vdsCpuUnitPinningHelper.countUnavailableCpus(
                getVdsManager(getVdsId()).getCpuTopology(),  getCluster().getCountThreadsAsCores()));

I know it was not done that way before but this part is not connected to scheduler in any way that it might just have been a mistake that was never found because the VM was able to run. After the refactoring due to the exclusive pinning I think we might end up in much worse state.

I have not tried the scenario, but I can imagine that if the cluster is set with countThreadsAsCores = false and we run and hot plug a shared VM with so many CPUs as is host's threads and it will propagate to the host's statistics, that we will not be able to schedule any other shared VM because of the calculation in CpuPolicyUnit:

hostCpuCount - exclusiveCpus - maxSharedCpuCount < 0

E.g. 2 (host cores) - 0 - 4 (host threads and the number of VM's CPU after the hotplugging) < 0 (always)

[ljelinkova]

I am also thinking how to connect this code with CpuPolicyUnit so that they share the same logic. I found that the policies are instantiated in SchedulerManager like this:

InternalPolicyUnits.instantiate(CPUPolicyUnit.class, getPendingResourceManager());

Maybe we can instantiate the CPUPolicyUnit here and use it to calculate if the VM can run? The calculation would always be identicat and it would include the pending resources too. WDYT? Worth a shot or is it an overkill?

[ljelinkova]

ok, and we will need to change also vmStaticData.getNumOfCpus(false) to consider countThreadsAsCores

No, not really, CpuPolicyUnit counts VM's CPUs as vmStaticData.getNumOfCpus(false) too.

[liranr23]

I am also thinking how to connect this code with CpuPolicyUnit so that they share the same logic. I found that the policies are instantiated in SchedulerManager like this:

InternalPolicyUnits.instantiate(CPUPolicyUnit.class, getPendingResourceManager());

Maybe we can instantiate the CPUPolicyUnit here and use it to calculate if the VM can run? The calculation would always be identicat and it would include the pending resources too. WDYT? Worth a shot or is it an overkill?

I think it's overkill to run the whole policy, although it will be the most correct way (the most precise).
The question if it worth it, the big benefit in my opinion is the pending resource. You really need to hit the timing of initiating new VMs and hotplugging.
And if we are looking about CPUPolicyUnit, why not running CpuOverloadPolicyUnit policy as well? which even make it more overkill.

ok, and we will need to change also vmStaticData.getNumOfCpus(false) to consider countThreadsAsCores

No, not really, CpuPolicyUnit counts VM's CPUs as vmStaticData.getNumOfCpus(false) too.

OK, so better to align with that. Done.

[ahadas]

I am also thinking how to connect this code with CpuPolicyUnit so that they share the same logic. I found that the policies are instantiated in SchedulerManager like this:

InternalPolicyUnits.instantiate(CPUPolicyUnit.class, getPendingResourceManager());

Maybe we can instantiate the CPUPolicyUnit here and use it to calculate if the VM can run? The calculation would always be identicat and it would include the pending resources too. WDYT? Worth a shot or is it an overkill?

I think it's overkill to run the whole policy, although it will be the most correct way (the most precise). The question if it worth it, the big benefit in my opinion is the pending resource. You really need to hit the timing of initiating new VMs and hotplugging. And if we are looking about CPUPolicyUnit, why not running CpuOverloadPolicyUnit policy as well? which even make it more overkill.

it sounds not easy but I don't think it's an overkill - yes, the likelihood of that to happen is really low but unless I'm missing something, if that happens we may fail to run any vm with dedicated cpu afterwards
but yeah, that's something that can be done separately, no need to fix everything in this PR

[ljelinkova]

@ahadas To be clear, It is not related to dedicated, it affects all VMs.
@liranr23 The main difference between CPUPolicyUnit and CpuOverloadPolicyUnit is that the cpu overload would lead to some overloading issues and would be resolved by ballancer. On the other hand, if we calculate it wrong, we might not be able to run any VM.

[ahadas]

and then let's set the topology accordingly (with no extra sockets for future hot-plugs)?

We already do. And actually, the user won't notice anything because the validation is already being done in HotSetNumberOfCpusCommand:

if (getVm().getCpuPinningPolicy().isExclusive()) {
  valid = failValidation(EngineMessage.HOT_PLUG_CPU_IS_NOT_SUPPORTED_DEDICATED,
          String.format("$policy %1$s", getVm().getCpuPinningPolicy().name()));
}

so if we don't change the topology to support hot-plugs, why do we need this validation? wouldn't we get another failure saying that hot-plug cannot be executed since we've reached the max number of CPUs?

also need to see that it applies to resize-and-pin btw

[liranr23]

and then let's set the topology accordingly (with no extra sockets for future hot-plugs)?

We already do. And actually, the user won't notice anything because the validation is already being done in HotSetNumberOfCpusCommand:

if (getVm().getCpuPinningPolicy().isExclusive()) {
  valid = failValidation(EngineMessage.HOT_PLUG_CPU_IS_NOT_SUPPORTED_DEDICATED,
          String.format("$policy %1$s", getVm().getCpuPinningPolicy().name()));
}

so if we don't change the topology to support hot-plugs, why do we need this validation? wouldn't we get another failure saying that hot-plug cannot be executed since we've reached the max number of CPUs?

also need to see that it applies to resize-and-pin btw

Nope, we will send it to VDSM and there we will get an error (at least for the exclusive pinning that it required to have the new cpuSet - VDSM does support hotplugging, if the engine will provide new pinning). I guess without this validation, we will just fail via VDSM/libvirt.

resize-and-pin has a validation on the hot plug command, which should be enough. in the UI it's blocked so it may happen only in API, which then will probably (from my check), would get error in NUMA validation.

[liranr23]

/ost

[ahadas]

We already do.

ok, good! I forgot you already did that, so CPU hot plug is already not possible

Nope, we will send it to VDSM and there we will get an error (at least for the exclusive pinning that it required to have the new cpuSet - VDSM does support hotplugging, if the engine will provide new pinning). I guess without this validation, we will just fail via VDSM/libvirt.

if that's true then something basic is missing, imagine that we add X spare slots for hot plugs and we have plugged X CPUs, we should fail the next attempt to plug a CPU on the engine side... that way we won't need a validation per type as you wrote above for resize-and-pin, we can have a single one saying "CPU hot plug is not enabled for the VM with its current settings or the maximum number of CPUs has been reached", without various messages to translate and update every time a new case is added..

[ahadas]

if that's true then something basic is missing, imagine that we add X spare slots for hot plugs and we have plugged X CPUs, we should fail the next attempt to plug a CPU on the engine side... that way we won't need a validation per type as you wrote above for resize-and-pin, we can have a single one saying "CPU hot plug is not enabled for the VM with its current settings or the maximum number of CPUs has been reached", without various messages to translate and update every time a new case is added..

on a second thought, that would be too difficult to implement because we don't save the "spare slots" anywhere but as part of the domain xml - so let's keep that part as is now

[ahadas]

I see three things here:

1. moving the validation from the inner command to update-vm
2. "blocking" the hot-plug operation from the UI (which is what bz 2097717 that this PR is associated with is about)
3. fixing the validation of hot plug with dedicated CPUs
   and we're focusing on (3)
   how about separating that part out and do what we need to do in order to make the webadmin not to suggest cpu hot plugs for vms with dedicated cpus?

[ahadas]

ah no, the thread I read is on a different PR :)
so just to see whether it makes sense to add the relevant changes to the UI here or to de-correlate this PR from the aforementioned bug

[ahadas]

I changed the bug, so we can go with what we have here and handle the bug in a separate PR

[liranr23]

I see three things here:

1. moving the validation from the inner command to update-vm
2. "blocking" the hot-plug operation from the UI (which is what bz 2097717 that this PR is associated with is about)
3. fixing the validation of hot plug with dedicated CPUs
   and we're focusing on (3)
   how about separating that part out and do what we need to do in order to make the webadmin not to suggest cpu hot plugs for vms with dedicated cpus?

i think all of them are in a good shape now.

[ljelinkova]

I think the calculation is now correct (except for the pending resources which would be difficult to implement).

[liranr23]

fixed unit tests.

[liranr23]

/ost

[liranr23]

what's holding it? :)