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.

Sign up for GitHub

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

Jump to bottom

Dedicated CPUs #65

Merged
merged 3 commits into from Mar 2, 2022
Merged

Dedicated CPUs #65

merged 3 commits into from Mar 2, 2022

Conversation

liranr23
Copy link
Member

@liranr23 liranr23 commented Feb 9, 2022

This PR is the implementation of https://bugzilla.redhat.com/1782077 on the engine side.

ljelinkova
ljelinkova requested changes Feb 10, 2022
View reviewed changes
...r/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsCpuUnit.java Outdated Show resolved Hide resolved
...r/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsCpuUnit.java Outdated Show resolved Hide resolved
...modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsProperties.java Outdated Show resolved Hide resolved
...ker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilderTest.java Outdated Show resolved Hide resolved
...odules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/UnitVmModel.java Outdated Show resolved Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.java Outdated Show resolved Hide resolved
...anager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/CpuPinningHelper.java Show resolved Hide resolved
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.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
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java Outdated Show resolved Hide resolved
ljelinkova
ljelinkova reviewed Feb 14, 2022
View reviewed changes
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.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
...odules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CPUPolicyUnit.java Outdated Show resolved Hide resolved
...odules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CPUPolicyUnit.java Outdated Show resolved Hide resolved
...odules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CPUPolicyUnit.java Outdated Show resolved Hide resolved
...bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CpuPinningPolicyUnit.java Show resolved Hide resolved
...bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CpuPinningPolicyUnit.java Show resolved Hide resolved
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java Outdated Show resolved Hide resolved
.../bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/DedicatedCpuPinningHelper.java Outdated Show resolved Hide resolved
.../bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/DedicatedCpuPinningHelper.java Outdated Show resolved Hide resolved
@ahadas ahadas added the virt label Feb 14, 2022
ljelinkova
ljelinkova reviewed Feb 15, 2022
View reviewed changes
...r/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsCpuUnit.java Outdated Show resolved Hide resolved
...anager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/CpuPinningHelper.java Outdated Show resolved Hide resolved
...anager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VmCpuCountHelper.java Show resolved Hide resolved
...broker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/LibvirtVmXmlBuilder.java Outdated Show resolved Hide resolved
...modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/InClusterUpgradeValidator.java Show resolved Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java Outdated Show resolved Hide resolved
ahadas
ahadas reviewed Feb 15, 2022
View reviewed changes
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.

don't get scared by the amount of comments, it's just a big PR

backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.java Outdated Show resolved Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.java Outdated Show resolved Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java Outdated Show resolved Hide resolved
...odules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingCpuPinning.java Show resolved Hide resolved
...odules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CPUPolicyUnit.java Outdated Show resolved Hide resolved
...odules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CPUPolicyUnit.java Outdated Show resolved Hide resolved
...bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CpuPinningPolicyUnit.java Outdated Show resolved Hide resolved
...modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/InClusterUpgradeValidator.java Show resolved Hide resolved
...odules/common/src/main/java/org/ovirt/engine/core/common/action/ProcessDownVmParameters.java Outdated Show resolved Hide resolved
...broker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/LibvirtVmXmlBuilder.java Show resolved Hide resolved
ljelinkova
ljelinkova reviewed Feb 24, 2022
View reviewed changes
Copy link
Collaborator

@ljelinkova ljelinkova left a 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, I have some comments but they can be addressed in follow up patches if needed.

...r/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsCpuUnit.java Show resolved Hide resolved
...badmin/modules/frontend/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties Outdated Show resolved Hide resolved
packaging/dbscripts/upgrade/04_05_0160_add_cpu_topology_to_vds_dynamic.sql Outdated Show resolved Hide resolved
...bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CpuPinningPolicyUnit.java Outdated Show resolved Hide resolved
.../bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/DedicatedCpuPinningHelper.java Outdated Show resolved Hide resolved
.../bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/DedicatedCpuPinningHelper.java Outdated Show resolved Hide resolved
.../bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/DedicatedCpuPinningHelper.java Outdated Show resolved Hide resolved
.../bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/DedicatedCpuPinningHelper.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
...odules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CPUPolicyUnit.java Outdated Show resolved Hide resolved
@liranr23 liranr23 force-pushed the dedicated_cpus2 branch 2 times, most recently from 5af48ac to 1543954 Compare February 24, 2022 17:31
ahadas
ahadas reviewed Feb 26, 2022
View reviewed changes
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.

partially reviewed

backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.java Outdated Show resolved Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java Outdated Show resolved Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java Outdated Show resolved Hide resolved
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java Outdated Show resolved Hide resolved
...anager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/CpuPinningHelper.java Outdated Show resolved Hide resolved
...badmin/modules/frontend/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties Outdated Show resolved Hide resolved
...badmin/modules/frontend/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties Outdated Show resolved Hide resolved
ahadas
ahadas reviewed Feb 27, 2022
View reviewed changes
...anager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/SchedulingManager.java Outdated Show resolved Hide resolved
...odules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CPUPolicyUnit.java Outdated Show resolved Hide resolved
...odules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CPUPolicyUnit.java Outdated Show resolved Hide resolved
...odules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CPUPolicyUnit.java Outdated Show resolved Hide resolved
...bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CpuPinningPolicyUnit.java Show resolved Hide resolved
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.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
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.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
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java Outdated Show resolved Hide resolved
ljelinkova
ljelinkova reviewed Feb 28, 2022
View reviewed changes
Copy link
Collaborator

@ljelinkova ljelinkova left a comment •
edited

Choose a reason for hiding this comment

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

Few comments (yeah, Github did not allow me to post my code comments without "Few comments" comment)

@liranr23
Copy link
Member Author

Something not good happened and I think I somehow mixed 2 commits into one...

ahadas
ahadas reviewed Feb 28, 2022
View reviewed changes
...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 Outdated Show resolved Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java Show resolved Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java Show resolved Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java Outdated Show resolved Hide resolved
...r/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsCpuUnit.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
ljelinkova
ljelinkova reviewed Mar 1, 2022
View reviewed changes
Copy link
Collaborator

@ljelinkova ljelinkova left a comment

Choose a reason for hiding this comment

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

More comments

...manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java Show resolved Hide resolved
...broker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/LibvirtVmXmlBuilder.java Show resolved Hide resolved
...r/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsCpuUnit.java Outdated Show resolved Hide resolved
@liranr23 liranr23 force-pushed the dedicated_cpus2 branch 3 times, most recently from 0d4fb9b to 36757ba Compare March 1, 2022 15:32
@liranr23
Copy link
Member Author

liranr23 commented Mar 1, 2022

/ost

@liranr23
Copy link
Member Author

liranr23 commented Mar 1, 2022

/ost

1 similar comment
@liranr23
Copy link
Member Author

liranr23 commented Mar 1, 2022

/ost

ahadas
ahadas reviewed Mar 2, 2022
View reviewed changes
...odules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/CPUPolicyUnit.java Outdated Show resolved Hide resolved
...es/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/utils/VdsCpuUnitPinningHelper.java Show resolved Hide resolved
...r/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsCpuUnit.java Show resolved Hide resolved
...r/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsCpuUnit.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
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.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
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java Outdated Show resolved Hide resolved
@saifabusaleh @liranr23
engine: add dedicated cpu filter and maintain
c4735f6 
This patch will hold the CpuTopology in VdsManager. Each VdsCpuUnit can
be pinned. Automatically, we pin the CPU that VDSM runs on.
The dedicated CPUs will be represented in a new PendingResource.

Once the host is initialized, the engine will look on the current
running VMs to set the CpuTopology pinned CPUs correctly. Upon shutdown,
we will clear the pinned CPUs.

A logic was added to the CpuPolicyUnit filtering, we will drop the
current pinned CPUs in order to let new VMs to run on that host, having
the right number of CPUs they can use. Also, when scheduling a dedicated
CPU VM, we check if the existing VMs would be able to schedule on that
host. If one of the condition is false, we will filter that host.

As for the CpuPinningPolicyUnit, we will check if we have the topology
data and if so, if we actually can pin the VM into the host. We wish to
have it with usage within the same socket (when the number of CPUs can
be within one socket), and the same for cores.

Change-Id: Ie258d30ce0696c79ad5432c270f4d94e6ef834ba
Bug-Url: https://bugzilla.redhat.com/1782077
Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>
Signed-off-by: Saif Abu Saleh <sabusale@redhat.com>
ljelinkova
ljelinkova approved these changes Mar 2, 2022
View reviewed changes
Copy link
Collaborator

@ljelinkova ljelinkova left a comment

Choose a reason for hiding this comment

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

LGTM

@liranr23
Copy link
Member Author

liranr23 commented Mar 2, 2022

/ost

@liranr23
core: pass dedicated to VDSM
909cbf4 
This patch will take the VM's dedicate policy and pass it to the domxml
to let VDSM to consume it.

A metadata of cpuPolicy added to every policy. Once the CPU policy is
dedicated we will set it to the dynamic VM data and add it to the
domxml.

Change-Id: Ifa9376dc80e661d0a85435d2d7c72c5be838b06d
Bug-Url: https://bugzilla.redhat.com/1782077
Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>
@liranr23 liranr23 added verified and removed verified labels Mar 2, 2022
@liranr23 liranr23 requested a review from ahadas March 2, 2022 15:09
@liranr23
core: fix clearing on shutdown and recovery
3c833a7 
Change-Id: Ic93761f565728d18e84b2c84d07e2ba600aaeba3
Bug-Url: https://bugzilla.redhat.com/1782077
Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>
@liranr23
Copy link
Member Author

liranr23 commented Mar 2, 2022

/ost

@ahadas ahadas merged commit 47eec5a into oVirt:master Mar 2, 2022
@liranr23 liranr23 deleted the dedicated_cpus2 branch March 15, 2022 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahadas ahadas ahadas approved these changes

@ljelinkova ljelinkova ljelinkova approved these changes

@almusil almusil Awaiting requested review from almusil

@bennyz bennyz Awaiting requested review from bennyz bennyz is a code owner

@didib didib Awaiting requested review from didib didib is a code owner

@emesika emesika Awaiting requested review from emesika emesika is a code owner

@michalskrivanek michalskrivanek Awaiting requested review from michalskrivanek michalskrivanek is a code owner

@mwperina mwperina Awaiting requested review from mwperina mwperina is a code owner

@oliel oliel Awaiting requested review from oliel oliel is a code owner

@sandrobonazzola sandrobonazzola Awaiting requested review from sandrobonazzola

@sgratch sgratch Awaiting requested review from sgratch sgratch is a code owner

Assignees
No one assigned
Labels
verified virt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@liranr23 @ljelinkova @ahadas @saifabusaleh