🌱 Add flag to reserve memory #3130

hrbasic · 2024-08-01T07:52:41Z

What this PR does / why we need it:
Introduces a new flag that allows users to set a memoryReservationLockedToMax at VSphereMachineTemplate level.

Which issue(s) this PR fixes:
Fixes #2468

…eploymentzone 🌱 Fix flake where VSphereFailureDomain still exists from previous test

k8s-ci-robot · 2024-08-01T07:52:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign killianmuldoon for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2024-08-01T07:52:50Z

Welcome @hrbasic!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-vsphere 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-vsphere has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-08-01T07:52:51Z

Hi @hrbasic. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

sbueringer · 2024-08-07T08:53:48Z

cc @neolit123 @lubronzhan

neolit123 · 2024-08-07T09:34:07Z

config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml

@@ -119,7 +119,7 @@ spec:
                  Defaults to the eponymous property value in the template from which the
                  virtual machine is cloned.
                format: int64
-                type: integer
+                type: integer          


extra space

neolit123 · 2024-08-07T09:34:42Z

apis/v1beta1/types.go

@@ -172,6 +172,13 @@ type VirtualMachineCloneSpec struct {
 	// virtual machine is cloned.
 	// +optional
 	MemoryMiB int64 `json:"memoryMiB,omitempty"`
+	// MemoryReservationLockedToMax is a flag that indicates whether or not the


Suggested change

// MemoryReservationLockedToMax is a flag that indicates whether or not the

// MemoryReservationLockedToMax indicates whether or not the

please update it everywhere.

neolit123 · 2024-08-07T09:35:10Z

test/e2e/memory_reservation_locked_test.go

@@ -0,0 +1,115 @@
+/*
+Copyright 2022 The Kubernetes Authors.


Suggested change

Copyright 2022 The Kubernetes Authors.

Copyright 2024 The Kubernetes Authors.

new file

neolit123 · 2024-08-07T09:36:08Z

test/e2e/memory_reservation_locked_test.go

+	"sigs.k8s.io/cluster-api/util"
+)
+
+type MemoryReservationLockedSpecInput struct {


Suggested change

type MemoryReservationLockedSpecInput struct {

type memoryReservationLockedSpecInput struct {

I've been following the naming conventions from other tests, such as hardware_upgrade_test.go. Since there is no need to export anything from this test, would it be okay if I use lowercase for the function VerifyMemoryReservationLockToMax()? I would also like to rename the function to verifyMemoryReservationLockedToMax to better follow the convention.

I've been following the naming conventions from other tests, such as hardware_upgrade_test.go. Since there is no need to export anything from this test, would it be okay if I use lowercase for the function VerifyMemoryReservationLockToMax()? I would also like to rename the function to verifyMemoryReservationLockedToMax to better follow the convention.

yes

chrischdi · 2024-08-07T12:36:03Z

/hold we need to validate this with tests "manually" before mreging

/ok-to-test

chrischdi · 2024-08-07T12:37:48Z

test/e2e/data/infrastructure-vsphere-govmomi/main/memory-reservation-locked/kustomization.yaml

+      kind: VSphereMachineTemplate
+    patch: |-
+      - op: add
+        path: /spec/template/spec/memoryReservationLockedToMax


may be enough to add this to an existing test to not increase the runtime

Would it then make sense to add default variable in env package:

var ( DefaultMemoryReservationLockedToMax = ptr.To(true) )

or directly set value in generators:

MemoryReservationLockedToMax: ptr.To(true),

let's just add it to one test, not to all :-)

I reviewed the code, and it seems that if I want to integrate this with an existing test, like the hw-upgrade flavor, I can directly add the memory-locked-to-max check in the hardware_upgrade_test.go file. This would allow us to remove the memory_reservation_locked_test.go file entirely. However, I'm unsure if this is what you meant by "add this to an existing test to not increase the runtime"?

k8s-ci-robot · 2024-08-07T12:45:19Z

@hrbasic: 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
pull-cluster-api-provider-vsphere-verify-main	`9e48ed0`	link	true	`/test pull-cluster-api-provider-vsphere-verify-main`
pull-cluster-api-provider-vsphere-test-main	`9e48ed0`	link	true	`/test pull-cluster-api-provider-vsphere-test-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

lubronzhan · 2024-08-08T01:01:39Z

test/e2e/memory_reservation_locked_test.go

+	}
+}
+
+func getMemoryReservationLockedToMaxFromObj(vmObj *object.VirtualMachine) *bool {


Suggested change

func getMemoryReservationLockedToMaxFromObj(vmObj *object.VirtualMachine) *bool {

func getMemoryReservationLockedToMaxFromObj(vmObj *object.VirtualMachine) bool {

lubronzhan · 2024-08-08T01:01:52Z

test/e2e/memory_reservation_locked_test.go

+	var virtualMachine mo.VirtualMachine
+	Expect(vmObj.Properties(ctx, vmObj.Reference(), []string{"config.memoryReservationLockedToMax"}, &virtualMachine)).To(Succeed())
+	Expect(virtualMachine.Config.MemoryReservationLockedToMax).ToNot(BeEmpty())
+	return virtualMachine.Config.MemoryReservationLockedToMax


Suggested change

return virtualMachine.Config.MemoryReservationLockedToMax

return *virtualMachine.Config.MemoryReservationLockedToMax

hrbasic added 2 commits August 1, 2024 09:07

Merge pull request kubernetes-sigs#3123 from chrischdi/pr-fix-flake-d…

a4fa551

…eploymentzone 🌱 Fix flake where VSphereFailureDomain still exists from previous test

Add flag to configure memory reservation

9e48ed0

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 1, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 1, 2024

k8s-ci-robot requested review from fabriziopandini and killianmuldoon August 1, 2024 07:53

neolit123 reviewed Aug 7, 2024

View reviewed changes

chrischdi reviewed Aug 7, 2024

View reviewed changes

lubronzhan reviewed Aug 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add flag to reserve memory #3130

🌱 Add flag to reserve memory #3130

hrbasic commented Aug 1, 2024

k8s-ci-robot commented Aug 1, 2024

k8s-ci-robot commented Aug 1, 2024

k8s-ci-robot commented Aug 1, 2024

sbueringer commented Aug 7, 2024

neolit123 Aug 7, 2024

neolit123 Aug 7, 2024

neolit123 Aug 7, 2024

neolit123 Aug 7, 2024

hrbasic Aug 12, 2024

neolit123 Aug 12, 2024

chrischdi commented Aug 7, 2024 •

edited

Loading

chrischdi Aug 7, 2024

hrbasic Aug 8, 2024 •

edited

Loading

chrischdi Aug 9, 2024

hrbasic Aug 12, 2024

k8s-ci-robot commented Aug 7, 2024

lubronzhan Aug 8, 2024

lubronzhan Aug 8, 2024

	// MemoryReservationLockedToMax is a flag that indicates whether or not the
	// MemoryReservationLockedToMax indicates whether or not the

	Copyright 2022 The Kubernetes Authors.
	Copyright 2024 The Kubernetes Authors.

	type MemoryReservationLockedSpecInput struct {
	type memoryReservationLockedSpecInput struct {

	func getMemoryReservationLockedToMaxFromObj(vmObj object.VirtualMachine) bool {
	func getMemoryReservationLockedToMaxFromObj(vmObj *object.VirtualMachine) bool {

	return virtualMachine.Config.MemoryReservationLockedToMax
	return *virtualMachine.Config.MemoryReservationLockedToMax

🌱 Add flag to reserve memory #3130

Are you sure you want to change the base?

🌱 Add flag to reserve memory #3130

Conversation

hrbasic commented Aug 1, 2024

k8s-ci-robot commented Aug 1, 2024

k8s-ci-robot commented Aug 1, 2024

k8s-ci-robot commented Aug 1, 2024

sbueringer commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrischdi commented Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

hrbasic Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrischdi commented Aug 7, 2024 •

edited

Loading

hrbasic Aug 8, 2024 •

edited

Loading