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

Add hugepages support #67

Merged
merged 1 commit into from
Oct 1, 2020
Merged

Add hugepages support #67

merged 1 commit into from
Oct 1, 2020

Conversation

mfranczy
Copy link
Contributor

Signed-off-by: Marcin Franczyk [email protected]

How to categorize this PR?

/area performance
/kind enhancement
/priority normal
/platform kubevirt

What this PR does / why we need it:
We need this to improve VMs performance in terms of memory consumption by applications running inside.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

improvement_user

@mfranczy mfranczy requested a review from a team as a code owner September 30, 2020 12:47
@gardener-robot gardener-robot added area/performance Performance (across all domains, such as control plane, networking, storage, etc.) related kind/enhancement Enhancement, improvement, extension platform/kubevirt Container Native Virtualization (CNV) KubeVirt platform/infrastructure priority/normal labels Sep 30, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 30, 2020
@mfranczy
Copy link
Contributor Author

PR should not be merged yet, I am going to test this together with mcm-provider-kubevirt. I've added possiblity to specify Guest memory together with hugepages, altough it's not documented yet due to not supporting resource limits as mentioned here.

@mfranczy mfranczy added reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies and removed reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies labels Sep 30, 2020
@mfranczy
Copy link
Contributor Author

mfranczy commented Oct 1, 2020

extension adds appropriate fields in a machine class object, I think we can merge this.

stoyanr
stoyanr reviewed Oct 1, 2020
View reviewed changes
Copy link
Contributor

@stoyanr stoyanr left a comment

Choose a reason for hiding this comment

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

Very nice PR, I have just a small question regarding naming, see below.

charts/internal/machine-class/templates/machine-class.yaml Show resolved Hide resolved
@stoyanr stoyanr self-requested a review October 1, 2020 12:35
@stoyanr
Copy link
Contributor

stoyanr commented Oct 1, 2020

/lgtm, conflicts should be resolved before merging

@mfranczy
add hugepages support
167b29e 
Signed-off-by: Marcin Franczyk <[email protected]>
@mfranczy mfranczy force-pushed the add-huge-page-support branch from bcd0385 to 167b29e Compare October 1, 2020 13:30
@mfranczy
Copy link
Contributor Author

mfranczy commented Oct 1, 2020

conflicts resolved

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 1, 2020
@mfranczy mfranczy merged commit 7825ccf into gardener-attic:master Oct 1, 2020
rootfs
rootfs reviewed Oct 1, 2020
View reviewed changes
pkg/apis/kubevirt/v1alpha1/types_worker.go
// For hugepages take a look at:
// k8s - https://kubernetes.io/docs/tasks/manage-hugepages/scheduling-hugepages/
// okd - https://docs.okd.io/3.9/scaling_performance/managing_hugepages.html#huge-pages-prerequisites
MemoryFeatures *kubevirtv1.Memory `json:"memoryFeatures,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

You may also need to set memory request here too, though the doc doesn't say what is the consequence of empty memory request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You may also need to set memory request here too

thanks for your input @rootfs, we do set memory request but in a bit different place to align with gardener API. It's passed from a machineType object https://github.com/gardener/gardener-extension-provider-kubevirt/blob/master/pkg/controller/worker/machines.go#L149.

though the doc doesn't say what is the consequence of empty memory request.

KubeVirt validates if your hugepages size is bigger than requested memory, so in case it's empty KubeVirt would return an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rootfs rootfs rootfs left review comments

@stoyanr stoyanr stoyanr approved these changes

Assignees
No one assigned
Labels
area/performance Performance (across all domains, such as control plane, networking, storage, etc.) related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/kubevirt Container Native Virtualization (CNV) KubeVirt platform/infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mfranczy @stoyanr @rootfs @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot