Validate Memory Usage for given resource pool for Vsphere provider #6680

rahulbabu95 · 2023-09-14T01:00:10Z

Issue #, if available:

Description of changes:
Add a preflight validation that checks for requested memory is available in the resource pool for the respective machine configs before proceeding with the create or upgrade operation.

Testing (if applicable):

Documentation added/planned (if applicable):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

eks-distro-bot · 2023-09-14T01:00:14Z

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

codecov · 2023-09-14T01:08:46Z

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (801186a) 75.61% compared to head (a30e86a) 75.65%.
Report is 38 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6680      +/-   ##
==========================================
+ Coverage   75.61%   75.65%   +0.03%     
==========================================
  Files         474      474              
  Lines       38268    38403     +135     
==========================================
+ Hits        28937    29054     +117     
- Misses       7724     7736      +12     
- Partials     1607     1613       +6

Files	Coverage Δ
pkg/providers/vsphere/spec.go	`100.00% <100.00%> (ø)`
pkg/executables/govc.go	`70.75% <94.11%> (+1.49%)`	⬆️
pkg/providers/vsphere/vsphere.go	`69.87% <81.01%> (+1.09%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rahulbabu95 · 2023-09-18T06:40:01Z

/retest eks-anywhere-presubmit

eks-distro-bot · 2023-09-18T06:40:06Z

@rahulbabu95: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test eks-anywhere-cli-attribution-presubmit
/test eks-anywhere-cluster-controller-tooling-presubmit
/test eks-anywhere-docs-presubmit
/test eks-anywhere-e2e-presubmit
/test eks-anywhere-e2e-validate-tinkerbell-hardware-presubmit
/test eks-anywhere-generate-files-presubmits
/test eks-anywhere-presubmit
/test eks-anywhere-release-tooling-presubmit
/test eks-anywhere-release-tooling-test-presubmit

Use /test all to run the following jobs that were automatically triggered:

eks-anywhere-cli-attribution-presubmit
eks-anywhere-e2e-presubmit
eks-anywhere-generate-files-presubmits
eks-anywhere-presubmit

In response to this:

/retest eks-anywhere-presubmit

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.

rahulbabu95 · 2023-09-18T06:40:48Z

/retest-required

…lity for vsphere machine configs for create/upgrade as a preflight check Signed-off-by: Rahul Ganesh <[email protected]>

Signed-off-by: Rahul Ganesh <[email protected]>

chrisdoherty4 · 2023-09-19T17:04:21Z

pkg/providers/vsphere/vsphere.go

+	needMemoryMiB      int
+}
+
+func (p *vsphereProvider) getPrevMachineConfigMemoryUsage(ctx context.Context, mc *v1alpha1.VSphereMachineConfig, cluster *types.Cluster, count int) (memoryMiB int, err error) {


count is quite an ambiguous parameter here, what is this a count of?

renaming it to machineConifgCount

chrisdoherty4 · 2023-09-19T17:05:27Z

pkg/providers/vsphere/vsphere.go

+	return poolInfo[MemoryAvailable], needMemoryMiB, nil
+}
+
+func (p *vsphereProvider) calculateResourcePoolMemoryUsage(ctx context.Context, dc string, mc *v1alpha1.VSphereMachineConfig, cluster *types.Cluster, mu map[string]*memoryUsage, prevCount, newCount int) error {


The keys for mu are really unclear. You can either create a dedicated data structure and give it an appropriate description or clearly document the param on this method. Same for counts.

Mind expanding dc as well given its of type string, its not clear what that means.

will expand the parameter names

pkg/providers/vsphere/vsphere.go

chrisdoherty4 · 2023-09-19T17:10:54Z

pkg/providers/vsphere/vsphere.go

+	memoryUsage := make(map[string]*memoryUsage)
+	datacenter := clusterSpec.VSphereDatacenter.Spec.Datacenter
+	cpMachineConfig := clusterSpec.controlPlaneMachineConfig()
+	controlPlaneAvailableMiB, controlPlaneNeedMiB, err := p.getMachineConfigMemoryRequirements(ctx, datacenter, cpMachineConfig, clusterSpec.Cluster.Spec.ControlPlaneConfiguration.Count)


A method called getMachineConfigMemoryRequirements that returns 'available memory' is a little confusing. This may just be a naming thing.

i have added an comment and changed the naming a bit

… helper function in validation to simplify the algorithm Signed-off-by: Rahul Ganesh <[email protected]>

chrisdoherty4 · 2023-10-05T21:12:34Z

/approve
/lgtm

eks-distro-bot · 2023-10-05T21:12:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisdoherty4

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~~ [chrisdoherty4]

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

eks-distro-bot added the do-not-merge/work-in-progress label Sep 14, 2023

eks-distro-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 14, 2023

rahulbabu95 force-pushed the validate-resource-availability branch from c2be09f to b2026b4 Compare September 16, 2023 00:07

rahulbabu95 marked this pull request as ready for review September 18, 2023 04:46

eks-distro-bot removed the do-not-merge/work-in-progress label Sep 18, 2023

rahulbabu95 force-pushed the validate-resource-availability branch from ed317ee to 2bbb953 Compare September 18, 2023 05:40

eks-distro-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 18, 2023

rahulbabu95 force-pushed the validate-resource-availability branch from 2bbb953 to 3f8bfd0 Compare September 18, 2023 06:04

eks-distro-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 18, 2023

rahulbabu95 force-pushed the validate-resource-availability branch from 3f8bfd0 to 2d7bbff Compare September 18, 2023 23:36

Rahul Ganesh added 2 commits September 18, 2023 16:57

Retrieve resource pool memory usage and validate the memory availaibi…

6d38e3b

…lity for vsphere machine configs for create/upgrade as a preflight check Signed-off-by: Rahul Ganesh <[email protected]>

remove commented lines and add functional comments

01e748d

Signed-off-by: Rahul Ganesh <[email protected]>

rahulbabu95 force-pushed the validate-resource-availability branch from 2d7bbff to 01e748d Compare September 19, 2023 00:01

chrisdoherty4 reviewed Sep 19, 2023

View reviewed changes

Add a helper function that returns Machine Config with count. Use the…

a30e86a

… helper function in validation to simplify the algorithm Signed-off-by: Rahul Ganesh <[email protected]>

chrisdoherty4 approved these changes Oct 5, 2023

View reviewed changes

eks-distro-bot assigned chrisdoherty4 Oct 5, 2023

eks-distro-bot added the lgtm label Oct 5, 2023

eks-distro-bot added the approved label Oct 5, 2023

eks-distro-bot merged commit a5cc615 into aws:main Oct 5, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Memory Usage for given resource pool for Vsphere provider #6680

Validate Memory Usage for given resource pool for Vsphere provider #6680

rahulbabu95 commented Sep 14, 2023 •

edited

Loading

eks-distro-bot commented Sep 14, 2023

codecov bot commented Sep 14, 2023 •

edited

Loading

rahulbabu95 commented Sep 18, 2023

eks-distro-bot commented Sep 18, 2023

rahulbabu95 commented Sep 18, 2023

chrisdoherty4 Sep 19, 2023

rahulbabu95 Sep 20, 2023

chrisdoherty4 Sep 19, 2023

rahulbabu95 Sep 20, 2023

chrisdoherty4 Sep 19, 2023

rahulbabu95 Sep 20, 2023

chrisdoherty4 commented Oct 5, 2023

eks-distro-bot commented Oct 5, 2023

Validate Memory Usage for given resource pool for Vsphere provider #6680

Validate Memory Usage for given resource pool for Vsphere provider #6680

Conversation

rahulbabu95 commented Sep 14, 2023 • edited Loading

eks-distro-bot commented Sep 14, 2023

codecov bot commented Sep 14, 2023 • edited Loading

Codecov Report

rahulbabu95 commented Sep 18, 2023

eks-distro-bot commented Sep 18, 2023

rahulbabu95 commented Sep 18, 2023

chrisdoherty4 Sep 19, 2023

Choose a reason for hiding this comment

rahulbabu95 Sep 20, 2023

Choose a reason for hiding this comment

chrisdoherty4 Sep 19, 2023

Choose a reason for hiding this comment

rahulbabu95 Sep 20, 2023

Choose a reason for hiding this comment

chrisdoherty4 Sep 19, 2023

Choose a reason for hiding this comment

rahulbabu95 Sep 20, 2023

Choose a reason for hiding this comment

chrisdoherty4 commented Oct 5, 2023

eks-distro-bot commented Oct 5, 2023

rahulbabu95 commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading