Introduce virtualMachineOptions to hco cr #2356

fossedihelm · 2023-05-30T09:55:24Z

What this PR does / why we need it:
VirtualMachineOptions holds the cluster level information regarding the virtual machine. This defines the default behavior of some features related to the virtual machines.

DisableFreePageReporting With freePageReporting the guest OS informs the hypervisor about pages which are not in use by the guest anymore. The hypervisor can use this information for freeing these pages.

freePageReporting is an attribute that can be defined at Memory balloon device
in libvirt. freePageReporting will NOT be enabled for the vmis which does not have the Memballoon driver, OR which are requesting any high performance components. A vmi is considered as high performance if one of the following is true:
- the vmi requests a dedicated cpu.
- the realtime flag is enabled.
- the vmi requests hugepages.
With DisableFreePageReporting freePageReporting will never be enabled in any vmi.
DisableFreePageReporting is a boolean and freePageReporting is disabled by default.

Reviewer Checklist

Reviewers are supposed to review the PR for every aspect below one by one. To check an item means the PR is either "OK" or "Not Applicable" in terms of that item. All items are supposed to be checked before merging a PR.

Jira Ticket:

https://issues.redhat.com/browse/CNV-29040

Release note:

Introduce cluster level virtualMachineOptions field.

kubevirt-bot · 2023-05-30T09:55:26Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2023-05-30T09:55:26Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

fossedihelm · 2023-07-03T09:41:45Z

/test pull-hyperconverged-cluster-operator-e2e-k8s-1.27

openshift-ci · 2023-07-03T09:41:48Z

@fossedihelm: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test ci-index-hco-bundle
/test ci-index-hco-upgrade-bundle
/test ci-index-hco-upgrade-prev-bundle
/test hco-e2e-kv-smoke-azure
/test hco-e2e-kv-smoke-gcp
/test hco-e2e-operator-sdk-aws
/test hco-e2e-operator-sdk-azure
/test hco-e2e-operator-sdk-gcp
/test hco-e2e-upgrade-index-aws
/test hco-e2e-upgrade-index-azure
/test hco-e2e-upgrade-prev-index-aws
/test hco-e2e-upgrade-prev-index-azure
/test images
/test okd-ci-index-hco-bundle
/test okd-ci-index-hco-upgrade-bundle
/test okd-ci-index-hco-upgrade-prev-bundle
/test okd-hco-e2e-operator-sdk-aws
/test okd-hco-e2e-operator-sdk-gcp
/test okd-hco-e2e-upgrade-index-aws
/test okd-hco-e2e-upgrade-index-gcp
/test okd-images

The following commands are available to trigger optional jobs:

/test hco-e2e-operator-sdk-sno-aws
/test hco-e2e-operator-sdk-sno-azure
/test hco-e2e-upgrade-index-sno-aws
/test hco-e2e-upgrade-index-sno-azure
/test hco-e2e-upgrade-prev-index-sno-aws
/test hco-e2e-upgrade-prev-index-sno-azure

Use /test all to run all jobs.

In response to this:

/test pull-hyperconverged-cluster-operator-e2e-k8s-1.27

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fossedihelm · 2023-07-03T09:45:58Z

/unhold

coveralls · 2023-07-03T09:45:58Z

Pull Request Test Coverage Report for Build 5464448074

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 86.173%

Totals
Change from base Build 5457040060:	0.02%
Covered Lines:	5042
Relevant Lines:	5851

💛 - Coveralls

fossedihelm · 2023-07-03T12:10:38Z

/retest

openshift-ci · 2023-07-05T07:00:17Z

@fossedihelm: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test ci-index-hco-bundle
/test ci-index-hco-upgrade-bundle
/test ci-index-hco-upgrade-prev-bundle
/test hco-e2e-kv-smoke-azure
/test hco-e2e-kv-smoke-gcp
/test hco-e2e-operator-sdk-aws
/test hco-e2e-operator-sdk-azure
/test hco-e2e-operator-sdk-gcp
/test hco-e2e-upgrade-index-aws
/test hco-e2e-upgrade-index-azure
/test hco-e2e-upgrade-prev-index-aws
/test hco-e2e-upgrade-prev-index-azure
/test images
/test okd-ci-index-hco-bundle
/test okd-ci-index-hco-upgrade-bundle
/test okd-ci-index-hco-upgrade-prev-bundle
/test okd-hco-e2e-operator-sdk-aws
/test okd-hco-e2e-operator-sdk-gcp
/test okd-hco-e2e-upgrade-index-aws
/test okd-hco-e2e-upgrade-index-gcp
/test okd-images

The following commands are available to trigger optional jobs:

/test hco-e2e-operator-sdk-sno-aws
/test hco-e2e-operator-sdk-sno-azure
/test hco-e2e-upgrade-index-sno-aws
/test hco-e2e-upgrade-index-sno-azure
/test hco-e2e-upgrade-prev-index-sno-aws
/test hco-e2e-upgrade-prev-index-sno-azure

Use /test all to run all jobs.

In response to this:

/test pull-hyperconverged-cluster-operator-e2e-k8s-1.27

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nunnatsa

Thanks for the PR, @fossedihelm.

Added some inline comments.

api/v1beta1/hyperconverged_types.go

controllers/operands/kubevirt_test.go

docs/cluster-configuration.md

nunnatsa · 2023-07-05T07:20:08Z

tests/func-tests/defaults_test.go

+				g.Expect(hc.Spec.VirtualMachineOptions).Should(Equal(defaultVirtualMachineOptions), "virtualMachineOptions should be equal to default")
+			}).WithTimeout(2 * time.Second).WithPolling(100 * time.Millisecond).Should(Succeed())
+		},
+			Entry("when removing /spec/virtualMachineOptions", "/spec/virtualMachineOptions"),


maybe also add Entry("when removing /spec/virtualMachineOptions", "/spec/virtualMachineOptions/disableFreePageReporting") ?

nunnatsa · 2023-07-05T07:21:32Z

controllers/operands/kubevirt.go

+	if hc.Spec.VirtualMachineOptions != nil && hc.Spec.VirtualMachineOptions.DisableFreePageReporting {
+		config.VirtualMachineOptions = &kubevirtcorev1.VirtualMachineOptions{DisableFreePageReporting: &kubevirtcorev1.DisableFreePageReporting{}}
+	}
+


We got a code smell here. The getKVConfig is too complex. Please consider to somehow refactor this function.

Not Blocking!!

I have some ideas, do you agree on addressing this with a followup PR?

yes. Either that or we'll take it.

deploy/crds/hco00.crd.yaml

api/v1beta1/hyperconverged_types.go

VirtualMachineOptions holds the cluster level information regarding the virtual machine. This defines the default behavior of some features related to the virtual machines. - `DisableFreePageReporting` With freePageReporting the guest OS informs the hypervisor about pages which are not in use by the guest anymore. The hypervisor can use this information for freeing these pages. freePageReporting is an attribute that can be defined at [Memory balloon device](https://libvirt.org/formatdomain.html#memory-balloon-device) in libvirt. freePageReporting will NOT be enabled for the vmis which does not have the Memballoon driver, OR which are requesting any high performance components. A vmi is considered as high performance if one of the following is true: - the vmi requests a dedicated cpu. - the realtime flag is enabled. - the vmi requests hugepages. With `DisableFreePageReporting` freePageReporting will never be enabled in any vmi. `DisableFreePageReporting` is an empty struct and freePageReporting is disabled by default. Signed-off-by: fossedihelm <ffossemo@redhat.com>

sonarcloud · 2023-07-05T12:51:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

fossedihelm · 2023-07-06T06:17:34Z

/retest

fossedihelm · 2023-07-06T09:08:33Z

/unhold

openshift-ci · 2023-07-06T09:32:54Z

@fossedihelm: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/hco-e2e-upgrade-prev-index-sno-aws	5e80f0ddba4b753d8e51fdba3224ba09452d4432	link	false	`/test hco-e2e-upgrade-prev-index-sno-aws`
ci/prow/hco-e2e-upgrade-index-sno-aws	5e80f0ddba4b753d8e51fdba3224ba09452d4432	link	false	`/test hco-e2e-upgrade-index-sno-aws`
ci/prow/hco-e2e-upgrade-prev-index-aws	5e80f0ddba4b753d8e51fdba3224ba09452d4432	link	true	`/test hco-e2e-upgrade-prev-index-aws`
ci/prow/hco-e2e-upgrade-index-aws	5e80f0ddba4b753d8e51fdba3224ba09452d4432	link	true	`/test hco-e2e-upgrade-index-aws`
ci/prow/hco-e2e-upgrade-index-azure	5e80f0ddba4b753d8e51fdba3224ba09452d4432	link	true	`/test hco-e2e-upgrade-index-azure`
ci/prow/hco-e2e-upgrade-prev-index-azure	5e80f0ddba4b753d8e51fdba3224ba09452d4432	link	true	`/test hco-e2e-upgrade-prev-index-azure`
ci/prow/hco-e2e-upgrade-index-sno-azure	5e80f0ddba4b753d8e51fdba3224ba09452d4432	link	false	`/test hco-e2e-upgrade-index-sno-azure`
ci/prow/hco-e2e-upgrade-prev-index-sno-azure	5e80f0ddba4b753d8e51fdba3224ba09452d4432	link	false	`/test hco-e2e-upgrade-prev-index-sno-azure`
ci/prow/okd-hco-e2e-upgrade-index-aws	`083a214`	link	true	`/test okd-hco-e2e-upgrade-index-aws`
ci/prow/hco-e2e-kv-smoke-azure	`083a214`	link	true	`/test hco-e2e-kv-smoke-azure`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fossedihelm · 2023-07-06T09:35:59Z

/hold cancel

tiraboschi · 2023-07-07T09:40:41Z

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

kubevirt-bot · 2023-07-07T09:40:45Z

@tiraboschi: Overrode contexts on behalf of tiraboschi: ci/prow/hco-e2e-kv-smoke-azure

In response to this:

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tiraboschi · 2023-07-07T09:42:35Z

/approve

kubevirt-bot · 2023-07-07T09:43:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tiraboschi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tiraboschi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tiraboschi · 2023-07-07T10:06:27Z

/retest

kubevirt-bot requested review from sradco and tiraboschi May 30, 2023 09:55

kubevirt-bot added the size/L label May 30, 2023

fossedihelm force-pushed the disable_freepagereporting branch from 8d5e11f to 770f5df Compare May 30, 2023 09:57

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels May 30, 2023

fossedihelm force-pushed the disable_freepagereporting branch from 770f5df to 71ef97c Compare May 31, 2023 06:14

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2023

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 7, 2023

fossedihelm force-pushed the disable_freepagereporting branch from 71ef97c to 3c0b17c Compare June 7, 2023 09:24

kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 7, 2023

fossedihelm force-pushed the disable_freepagereporting branch from 3c0b17c to 77de595 Compare July 3, 2023 09:40

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2023

fossedihelm marked this pull request as ready for review July 3, 2023 09:45

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2023

kubevirt-bot requested a review from machadovilaca July 3, 2023 09:45

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2023

fossedihelm marked this pull request as ready for review July 5, 2023 07:07

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2023

kubevirt-bot requested a review from nunnatsa July 5, 2023 07:07

nunnatsa requested changes Jul 5, 2023

View reviewed changes

kubevirt-bot assigned nunnatsa Jul 5, 2023

fossedihelm force-pushed the disable_freepagereporting branch from 5e80f0d to 81a546a Compare July 5, 2023 09:44

tiraboschi reviewed Jul 5, 2023

View reviewed changes

deploy/crds/hco00.crd.yaml Outdated Show resolved Hide resolved

fossedihelm force-pushed the disable_freepagereporting branch 2 times, most recently from 31816fe to 4a4fea3 Compare July 5, 2023 10:26

nunnatsa reviewed Jul 5, 2023

View reviewed changes

api/v1beta1/hyperconverged_types.go Show resolved Hide resolved

fossedihelm force-pushed the disable_freepagereporting branch from 4a4fea3 to 083a214 Compare July 5, 2023 12:48

nunnatsa approved these changes Jul 5, 2023

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2023

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2023

tiraboschi approved these changes Jul 7, 2023

View reviewed changes

kubevirt-bot assigned tiraboschi Jul 7, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2023

kubevirt-bot merged commit a3414f3 into kubevirt:main Jul 7, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce virtualMachineOptions to hco cr #2356

Introduce virtualMachineOptions to hco cr #2356

fossedihelm commented May 30, 2023 •

edited

Loading

kubevirt-bot commented May 30, 2023

openshift-ci bot commented May 30, 2023

fossedihelm commented Jul 3, 2023

openshift-ci bot commented Jul 3, 2023

fossedihelm commented Jul 3, 2023

coveralls commented Jul 3, 2023 •

edited

Loading

fossedihelm commented Jul 3, 2023

openshift-ci bot commented Jul 5, 2023

nunnatsa left a comment

nunnatsa Jul 5, 2023

nunnatsa Jul 5, 2023

fossedihelm Jul 5, 2023

nunnatsa Jul 5, 2023

sonarcloud bot commented Jul 5, 2023

fossedihelm commented Jul 6, 2023

fossedihelm commented Jul 6, 2023

openshift-ci bot commented Jul 6, 2023

fossedihelm commented Jul 6, 2023

tiraboschi commented Jul 7, 2023

kubevirt-bot commented Jul 7, 2023

tiraboschi commented Jul 7, 2023

kubevirt-bot commented Jul 7, 2023

tiraboschi commented Jul 7, 2023

Introduce virtualMachineOptions to hco cr #2356

Introduce virtualMachineOptions to hco cr #2356

Conversation

fossedihelm commented May 30, 2023 • edited Loading

kubevirt-bot commented May 30, 2023

openshift-ci bot commented May 30, 2023

fossedihelm commented Jul 3, 2023

openshift-ci bot commented Jul 3, 2023

fossedihelm commented Jul 3, 2023

coveralls commented Jul 3, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5464448074

💛 - Coveralls

fossedihelm commented Jul 3, 2023

openshift-ci bot commented Jul 5, 2023

nunnatsa left a comment

Choose a reason for hiding this comment

nunnatsa Jul 5, 2023

Choose a reason for hiding this comment

nunnatsa Jul 5, 2023

Choose a reason for hiding this comment

fossedihelm Jul 5, 2023

Choose a reason for hiding this comment

nunnatsa Jul 5, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Jul 5, 2023

fossedihelm commented Jul 6, 2023

fossedihelm commented Jul 6, 2023

openshift-ci bot commented Jul 6, 2023

fossedihelm commented Jul 6, 2023

tiraboschi commented Jul 7, 2023

kubevirt-bot commented Jul 7, 2023

tiraboschi commented Jul 7, 2023

kubevirt-bot commented Jul 7, 2023

tiraboschi commented Jul 7, 2023

fossedihelm commented May 30, 2023 •

edited

Loading

coveralls commented Jul 3, 2023 •

edited

Loading