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.

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

🌱 Add flag to reserve memory #3130

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,7 @@ generate-e2e-templates-main: $(KUSTOMIZE) ## Generate test templates for the mai
"$(KUSTOMIZE)" --load-restrictor LoadRestrictionsNone build "$(E2E_GOVMOMI_TEMPLATE_DIR)/main/ownerrefs-finalizers" > "$(E2E_GOVMOMI_TEMPLATE_DIR)/main/cluster-template-ownerrefs-finalizers.yaml"
# for IPAM tests
"$(KUSTOMIZE)" --load-restrictor LoadRestrictionsNone build "$(E2E_GOVMOMI_TEMPLATE_DIR)/main/ipam" > "$(E2E_GOVMOMI_TEMPLATE_DIR)/main/cluster-template-ipam.yaml"
"$(KUSTOMIZE)" --load-restrictor LoadRestrictionsNone build "$(E2E_GOVMOMI_TEMPLATE_DIR)/main/memory-reservation-locked" > "$(E2E_GOVMOMI_TEMPLATE_DIR)/main/cluster-template-memory-reservation-locked.yaml"
# generate supervisor templates
cp "$(RELEASE_DIR)/main/cluster-template-supervisor.yaml" "$(E2E_SUPERVISOR_TEMPLATE_DIR)/main/base/cluster-template-supervisor.yaml"
"$(KUSTOMIZE)" --load-restrictor LoadRestrictionsNone build "$(E2E_SUPERVISOR_TEMPLATE_DIR)/main/base" > "$(E2E_SUPERVISOR_TEMPLATE_DIR)/main/cluster-template-supervisor.yaml"
Expand Down
7 changes: 7 additions & 0 deletions apis/v1beta1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

please update it everywhere.

// memory resource reservation for this virtual machine will always be
// equal to the virtual machine's memory size.
// Defaults to the eponymous property value in the template from which the
// virtual machine is cloned.
// +optional
MemoryReservationLockedToMax *bool `json:"memoryReservationLockedToMax,omitempty"`
// DiskGiB is the size of a virtual machine's disk, in GiB.
// Defaults to the eponymous property value in the template from which the
// virtual machine is cloned.
Expand Down
5 changes: 5 additions & 0 deletions apis/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

extra space

network:
description: Network is the network configuration for this machine's
VM.
Expand Down Expand Up @@ -1042,6 +1042,14 @@ spec:
virtual machine is cloned.
format: int64
type: integer
memoryReservationLockedToMax:
description: |-
MemoryReservationLockedToMax is a flag that indicates
whether or not the memory resource reservation for this virtual
machine will always be equal to the virtual machine's memory size.
Defaults to the eponymous property value in the template from which
the virtual machine is cloned.
type: boolean
network:
description: Network is the network configuration for this machine's
VM.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,14 @@ spec:
virtual machine is cloned.
format: int64
type: integer
memoryReservationLockedToMax:
description: |-
MemoryReservationLockedToMax is a flag that indicates
whether or not the memory resource reservation for this
virtual machine will always be equal to the virtual machine's
memory size. Defaults to the eponymous property value in
the template from which the virtual machine is cloned.
type: boolean
network:
description: Network is the network configuration for this
machine's VM.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1130,6 +1130,14 @@ spec:
virtual machine is cloned.
format: int64
type: integer
memoryReservationLockedToMax:
description: |-
MemoryReservationLockedToMax is a flag that indicates
whether or not the memory resource reservation for this virtual
machine will always be equal to the virtual machine's memory size.
Defaults to the eponymous property value in the template from which
the virtual machine is cloned.
type: boolean
network:
description: Network is the network configuration for this machine's
VM.
Expand Down
17 changes: 9 additions & 8 deletions pkg/services/govmomi/vcenter/clone.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,14 +174,15 @@ func Clone(ctx context.Context, vmCtx *capvcontext.VMContext, bootstrapData []by
// Assign the clone's InstanceUUID the value of the Kubernetes Machine
// object's UID. This allows lookup of the cloned VM prior to knowing
// the VM's UUID.
InstanceUuid: string(vmCtx.VSphereVM.UID),
Flags: newVMFlagInfo(),
DeviceChange: deviceSpecs,
ExtraConfig: extraConfig,
NumCPUs: numCPUs,
NumCoresPerSocket: numCoresPerSocket,
MemoryMB: memMiB,
VAppConfigRemoved: &vappConfigRemoved,
InstanceUuid: string(vmCtx.VSphereVM.UID),
Flags: newVMFlagInfo(),
DeviceChange: deviceSpecs,
ExtraConfig: extraConfig,
NumCPUs: numCPUs,
NumCoresPerSocket: numCoresPerSocket,
MemoryMB: memMiB,
MemoryReservationLockedToMax: vmCtx.VSphereVM.Spec.MemoryReservationLockedToMax,
VAppConfigRemoved: &vappConfigRemoved,
},
Location: types.VirtualMachineRelocateSpec{
DiskMoveType: string(diskMoveType),
Expand Down
1 change: 1 addition & 0 deletions test/e2e/config/vsphere.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ providers:
- sourcePath: "../../../test/e2e/data/infrastructure-vsphere-govmomi/main/cluster-template.yaml"
- sourcePath: "../../../test/e2e/data/infrastructure-vsphere-govmomi/main/clusterclass-quick-start.yaml"
- sourcePath: "../../../test/e2e/data/infrastructure-vsphere-govmomi/main/clusterclass-quick-start-runtimesdk.yaml"
- sourcePath: "../../../test/e2e/data/infrastructure-vsphere-govmomi/main/cluster-template-memory-reservation-locked.yaml"
- sourcePath: "../../../test/e2e/data/infrastructure-vsphere-supervisor/main/cluster-template-topology-supervisor.yaml"
- sourcePath: "../../../test/e2e/data/infrastructure-vsphere-supervisor/main/cluster-template-topology-runtimesdk-supervisor.yaml"
- sourcePath: "../../../test/e2e/data/infrastructure-vsphere-supervisor/main/cluster-template-supervisor.yaml"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- ../base
patches:
- target:
kind: VSphereMachineTemplate
patch: |-
- op: add
path: /spec/template/spec/memoryReservationLockedToMax
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@hrbasic hrbasic Aug 8, 2024

Choose a reason for hiding this comment

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

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),

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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"?

value: true

115 changes: 115 additions & 0 deletions test/e2e/memory_reservation_locked_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/*
Copyright 2022 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Copyright 2022 The Kubernetes Authors.
Copyright 2024 The Kubernetes Authors.

new file


Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package e2e

import (
"context"
"fmt"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/vmware/govmomi/object"
"github.com/vmware/govmomi/vim25/mo"
corev1 "k8s.io/api/core/v1"
"sigs.k8s.io/cluster-api/test/framework"
"sigs.k8s.io/cluster-api/test/framework/clusterctl"
. "sigs.k8s.io/cluster-api/test/framework/ginkgoextensions"
"sigs.k8s.io/cluster-api/util"
)

type MemoryReservationLockedSpecInput struct {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type MemoryReservationLockedSpecInput struct {
type memoryReservationLockedSpecInput struct {

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

InfraClients
Global GlobalInput
SpecName string
Namespace *corev1.Namespace
// Allows to inject a function to be run after test namespace is created.
// If not specified, this is a no-op.
PostNamespaceCreated func(managementClusterProxy framework.ClusterProxy, workloadClusterNamespace string)
}

var _ = Describe("Set memory reservation locked to max on VMs", func() {
const specName = "memory-reservation-locked"
Setup(specName, func(testSpecificSettingsGetter func() testSettings) {
var (
namespace *corev1.Namespace
)

BeforeEach(func() {
Expect(bootstrapClusterProxy).NotTo(BeNil(), "BootstrapClusterProxy can't be nil")
namespace = setupSpecNamespace(specName, testSpecificSettingsGetter().PostNamespaceCreatedFunc)
})

AfterEach(func() {
cleanupSpecNamespace(namespace)
})

It("Creates a workload cluster whose VMs have memory reservation locked set to true", func() {
VerfiyMemoryReservationLockToMax(ctx, MemoryReservationLockedSpecInput{
SpecName: specName,
Namespace: namespace,
InfraClients: InfraClients{
Client: vsphereClient,
RestClient: restClient,
Finder: vsphereFinder,
},
Global: GlobalInput{
BootstrapClusterProxy: bootstrapClusterProxy,
ClusterctlConfigPath: testSpecificSettingsGetter().ClusterctlConfigPath,
E2EConfig: e2eConfig,
ArtifactFolder: artifactFolder,
},
})
})
})
})

func VerfiyMemoryReservationLockToMax(ctx context.Context, input MemoryReservationLockedSpecInput) {
var (
specName = input.SpecName
namespace = input.Namespace
clusterResources = new(clusterctl.ApplyClusterTemplateAndWaitResult)
)

clusterName := fmt.Sprintf("%s-%s", specName, util.RandomString(6))
By("Creating a workload cluster")
configCluster := defaultConfigCluster(clusterName, namespace.Name, specName, 1, 1, input.Global)

clusterctl.ApplyClusterTemplateAndWait(ctx, clusterctl.ApplyClusterTemplateAndWaitInput{
ClusterProxy: input.Global.BootstrapClusterProxy,
ConfigCluster: configCluster,
WaitForClusterIntervals: input.Global.E2EConfig.GetIntervals(specName, "wait-cluster"),
WaitForControlPlaneIntervals: input.Global.E2EConfig.GetIntervals(specName, "wait-control-plane"),
WaitForMachineDeployments: input.Global.E2EConfig.GetIntervals(specName, "wait-worker-nodes"),
}, clusterResources)

Byf("Fetching the VSphereVM objects for the cluster %s", clusterName)
vms := getVSphereVMsForCluster(clusterName, namespace.Name)

By("Verifying memory reservation locked to max is set to true")
for _, vm := range vms.Items {
vmObj, err := input.Finder.VirtualMachine(ctx, vm.Name)
Expect(err).ToNot(HaveOccurred(), "expected to find VM %s", vm.Name)
Expect(getMemoryReservationLockedToMaxFromObj(vmObj)).To(Equal(true), "expected memory reservation locked to max to be set to true")

Check failure on line 106 in test/e2e/memory_reservation_locked_test.go

View workflow job for this annotation

GitHub Actions / lint (test)

ginkgo-linter: multiple issues: wrong boolean assertion; comparing a pointer to a value will always fail. Consider using `Expect(getMemoryReservationLockedToMaxFromObj(vmObj)).To(HaveValue(BeTrue()), "expected memory reservation locked to max to be set to true")` instead (ginkgolinter)
}
}

func getMemoryReservationLockedToMaxFromObj(vmObj *object.VirtualMachine) *bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return virtualMachine.Config.MemoryReservationLockedToMax
return *virtualMachine.Config.MemoryReservationLockedToMax

}
Loading