-
Notifications
You must be signed in to change notification settings - Fork 270
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: consider dedicated cpus when scoring hosts #252
Conversation
5d13f44
to
d933ea4
Compare
d933ea4
to
2042186
Compare
2042186
to
98f642c
Compare
3e92c48
to
c713364
Compare
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.
- It's hard to follow on which policies you do count the
exclusive CPUs
and on which you don't. Can you comment on each policy somewhere to say it explicitly? countThreadsAsCores
, when needed should be taken into account.- I felt lose of track on variable names. Especially when it's CPU or cores.
...va/org/ovirt/engine/core/bll/scheduling/policyunits/EvenDistributionCPUWeightPolicyUnit.java
Outdated
Show resolved
Hide resolved
public void setSharedCpuCount(int sharedCpuCount) { | ||
this.sharedCpuCount = sharedCpuCount; | ||
} | ||
|
||
public int getVmsCoresCount() { | ||
return sharedCpuCount; | ||
} |
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.
is it shared CPU count or cores count?
- same question regarding the host countThreadsAsCores (as in core: Change the CPUPolicyUnit for dedicated cpu pinning #299)
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.
I've renamed it to the shared cpu count.
import org.junit.jupiter.api.extension.ExtendWith; | ||
import org.ovirt.engine.core.utils.MockConfigExtension; | ||
|
||
@ExtendWith(MockConfigExtension.class) | ||
public class CpuOverloadPolicyUnitTest { | ||
@Test | ||
public void testGetHighUtilizationForAllCores() { |
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.
don't we wish to have this test? seems this class is just empty without it.
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.
no, not really, the tested method was removed. I'll remove the whole class too.
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.
nice!
...d/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/PolicyUnitImpl.java
Outdated
Show resolved
Hide resolved
...d/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/PolicyUnitImpl.java
Outdated
Show resolved
Hide resolved
...n/java/org/ovirt/engine/core/bll/scheduling/policyunits/CpuAndMemoryBalancingPolicyUnit.java
Show resolved
Hide resolved
...va/org/ovirt/engine/core/bll/scheduling/policyunits/EvenDistributionCPUWeightPolicyUnit.java
Outdated
Show resolved
Hide resolved
...va/org/ovirt/engine/core/bll/scheduling/policyunits/EvenDistributionCPUWeightPolicyUnit.java
Outdated
Show resolved
Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java
Outdated
Show resolved
Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java
Show resolved
Hide resolved
...les/common/src/main/java/org/ovirt/engine/core/common/businessentities/CpuPinningPolicy.java
Outdated
Show resolved
Hide resolved
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java
Outdated
Show resolved
Hide resolved
5d2754a
to
6993809
Compare
/ost |
When the host is scored, its total CPU usage is divided by the number of cores. This works well when all CPUs are shared because we expect the underlying systems to distribute the load equally. However, with dedicated CPUs, the load is not equal. We do not really care about the load on the dedicated CPUs as we cannot run any other VM there. We care about the remaining CPUs to see if there is enough resources left for the VMs already running on the host. This patch changes the calculation of the host cpu load so that the load is calcualted only for the shared CPUs.
6993809
to
f33e95d
Compare
/ost |
A general question: |
@liranr23 can you please rephrase the question? |
When the host is scored, its total CPU usage is divided by the number of cores. This works well when all CPUs
are shared because we expect the underlying systems to distribute the load equally.
However, with dedicated CPUs, the load is not equal. We do not really care about the load on the dedicated CPUs as we cannot run any other VM there. We care about the remaining CPUs to see if there is enough resources left for the VMs already running on the host.