feat: set default instance type for aws cp and worker nodes by supershal · Pull Request #51 · d2iq-labs/cluster-api-runtime-extensions-nutanix

supershal · 2024-04-10T20:05:43Z

What problem does this PR solve?:

sets defaults for aws nodes CP: m5.xlarge and Workers: m5.2xlarge
sets PLACEHOLDER value as defaults in AWSMachineTemplates because instanceType field is required by CAPA.
sets instance type as required field in the schema since CAREN will be using the default if not provided by the user.
update existing unit tests to use default schema

NOTE
Once this PR is finalized, I will create follow up PR to set instance profile and some other changes in the API types that would ensure that it creates variable schema from the object instead of creating new one.

Which issue(s) this PR fixes:
Fixes # nutanix-cloud-native#485

How Has This Been Tested?:

Tried to test locally using make e2e-test E2E_LABEL='provider:AWS && cni:Cilium' but local environment timed outs during initializations. Looking into fixing local environment (colima +docker +kind on mac)

Tested manually be creating AWS cluster.

aws-quick-start cluster class has following

 instanceType:
                      default: m5.xlarge
                      description: The AWS instance type to use for the cluster Machines
                      type: string
                  required:
                  - instanceType

and workers

                instanceType:
                  default: m5.2xlarge
                  description: The AWS instance type to use for the cluster Machines
                  type: string
              required:
              - instanceType

Created AWS cluster without adding instanceType variable, CAREN patched AWSMachineTemplate with default instanceType.

Special notes for your reviewer:

The approach taken in this PR will be used as guide to set default for other variables.

Missed when reorganising example kustomizations in previous PR.

- Defines a cluster-level variable for defining one or more users - Patches bootstrap templates for control plane and worker node pools with user configuration

Co-authored-by: Faiq <faiq.raza@nutanix.com> Signed-off-by: Deepak Muley <deepak.muley@nutanix.com>

Address review comments

Expand comments

Without this, defaults declared in the JSON schema are not included in validation steps, which can lead to invalid failures, while also not allowing for tests that target defaults.

Fix typo in lockPassword logic, and add unit test

feat: Add examples for Nutanix provider

Add unit test for empty hashed password

Change Sudo field from pointer to value The zero value (empty string) is not valid, so the field does not need to be a pointer.

Make username required

Explain why we do not validate hashed password input

Explain why we do not validate sudo input

Update type comments

Remove errant comment

Add users to the docs site

Manually wrap lines in doc

Also deploy infra provider versions that match the API.

CPI is a term unique to the vSphere CCM. Renaming to the more generic "CCM".

* refactor: combine PC host and port into a single url var This makes it simpler for clients to provide a single input field and not have to do any parsing to split the hostname and port. It also allows us to use API validation for bad input. * fixup! refactor: combine PC host and port into a single url var * fixup! refactor: combine PC host and port into a single url var * fixup! refactor: combine PC host and port into a single url var

The existing code created a ClusterResourceSet with the user provided Secret. However, that won't work unless that Secret has an embedded Secret in it.

* ci: adds tooling to create configmap * feat: use a configmap to get helmchart info * fix: precommit issues * fix: typo in cilium * fix: remove workspace files * build: template name for configmap * refactor: names for helm chart info getter * refactor: use nutanix-storage name instead of nutnaix-csi * refactor: move to globaloptions * fix: adds snapshot to helm config * fix: comments after review * fix: adds a warning and removes ebs csi * fix: typo * fix: adds missing script file * fix: precommit

* feat: Add patch to configure containerd metrics

This makes the deployment consistent as well as avoiding issues when providers are in the middle of a release.

* build: remove nutanix CCM from examples * build: add scripts to sync Nutanix CCM manifests * build: add CCM addon var to Nutanix examples * feat: deploy Nutanix CCM addon Aligns the method of deploying the CCM with all other addons. * fixup! build: add scripts to sync Nutanix CCM manifests * fixup! feat: deploy Nutanix CCM addon * build: remove unused CRS tooling for Nutanix CCM The CRS strategy is not supported for the Nutanix CCM, removing it until we actually need it.

dkoshkin

Looks great!

faiq

elegant solution.

jimmidyson · 2024-04-11T15:27:17Z

+	instanceType := InstanceType("")
+	if a.InstanceType != nil {
+		instanceType = *a.InstanceType
+	}


Suggested change

instanceType := InstanceType("")

if a.InstanceType != nil {

instanceType = *a.InstanceType

}

instanceType := ptr.Deref(a.InstanceType, "")

jimmidyson · 2024-04-11T15:28:11Z

+		OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+			Type:        "string",
+			Description: "The AWS instance type to use for the cluster Machines",
+			Default:     variables.MustMarshal(string(i)),


Suggested change

Default: variables.MustMarshal(string(i)),

Default: variables.MustMarshal(i),

jimmidyson and others added 30 commits March 21, 2024 12:30

build: Remove unused example kustomizations

6c53bb1

Missed when reorganising example kustomizations in previous PR.

Merge pull request #1 from d2iq-labs/jimmi/remove-unused-kustomizations

ae3e809

feat: Add user configuration for all providers

9f56b56

- Defines a cluster-level variable for defining one or more users - Patches bootstrap templates for control plane and worker node pools with user configuration

[WIP] added nutanix support

08457eb

added nutanix examples for clusterclass and cluster

e00083a

removed extra $ sign from all the variables

1d13297

Update hack/examples/sync.sh

e20cf32

Co-authored-by: Faiq <faiq.raza@nutanix.com> Signed-off-by: Deepak Muley <deepak.muley@nutanix.com>

added new line to fix precommit lint

601c6a8

fix: removed trailing whitespaces

db87f18

reverted devbox.lock changes

7e2b6e6

fix: updated patches to use existing variable

c1695ce

test(e2e): Rename config for CAREN rather than CRE

d6f54fb

Merge pull request #8 from d2iq-labs/jimmi/more-caren-renames

0cd416f

fixup! feat: Add user configuration for all providers

650c3cf

Address review comments

fixup! feat: Add user configuration for all providers

27072c6

Expand comments

test: Ensure defaults from JSON schema are respected

5656042

Without this, defaults declared in the JSON schema are not included in validation steps, which can lead to invalid failures, while also not allowing for tests that target defaults.

Merge pull request #9 from d2iq-labs/jimmi/variable-defaulting-in-tests

b17718a

fixup! feat: Add user configuration for all providers

9748994

Fix typo in lockPassword logic, and add unit test

Merge pull request #5 from d2iq-labs/nutanix-examples

554dacd

feat: Add examples for Nutanix provider

fixup! feat: Add user configuration for all providers

1205a59

Add unit test for empty hashed password

fixup! feat: Add user configuration for all providers

a0cdd4b

Change Sudo field from pointer to value The zero value (empty string) is not valid, so the field does not need to be a pointer.

fixup! feat: Add user configuration for all providers

64b7afc

Make username required

fixup! feat: Add user configuration for all providers

37ec7c9

Explain why we do not validate hashed password input

fixup! feat: Add user configuration for all providers

0968ea6

Explain why we do not validate sudo input

fixup! feat: Add user configuration for all providers

f09a28f

Update type comments

fixup! feat: Add user configuration for all providers

5a73b71

Remove errant comment

fixup! feat: Add user configuration for all providers

b8bcb0e

Add users to the docs site

fixup! fixup! feat: Add user configuration for all providers

dfd5cbe

Manually wrap lines in doc

build: add Nutanix infra provider (#12)

b0761a7

Also deploy infra provider versions that match the API.

fix: rename cpi to ccm (#11)

47e578a

CPI is a term unique to the vSphere CCM. Renaming to the more generic "CCM".

dkoshkin and others added 14 commits April 9, 2024 08:35

fix: the handlers should be changing user provided input (#35)

0e91d4d

ci: Always use latest go for govulncheck (#41)

92184b0

ci: Disable fail-fast for matrix jobs (#43)

5a384e3

fix: Nutanix CSI credentials Secret creation (#34)

21f4055

The existing code created a ClusterResourceSet with the user provided Secret. However, that won't work unless that Secret has an embedded Secret in it.

feat: Add patch to configure containerd metrics (#44)

c59f46a

* feat: Add patch to configure containerd metrics

fix: Nutanix CSI HelmChartProxy name and namespace (#45)

a137bb5

build: Specify CAPI provider versions from go module versions (#47)

bf3a644

This makes the deployment consistent as well as avoiding issues when providers are in the middle of a release.

ci: Switch to jetify-com/devbox-install-action (#48)

3a83bd3

ci: Update govulncheck task to use same version of go as project (#49)

1137f86

fix: set 'PLACEHOLDER' as default for instance type

cdf1979

feat: set default instance type for aws cp and workers

3f673f9

github-actions Bot added the feature label Apr 10, 2024

supershal force-pushed the shalin/default_instance_type branch from 481a55a to ffdff16 Compare April 10, 2024 20:17

supershal self-assigned this Apr 10, 2024

supershal requested review from dkoshkin, dlipovetsky, faiq and jimmidyson April 10, 2024 20:27

dkoshkin approved these changes Apr 10, 2024

View reviewed changes

Comment thread api/v1alpha1/aws_node_types.go Outdated

fix: set defaults for AWS CP and Worker instanceType

c5067a4

supershal force-pushed the shalin/default_instance_type branch from ffdff16 to c5067a4 Compare April 10, 2024 21:52

supershal mentioned this pull request Apr 11, 2024

feat: set default instance profile for AWS CP and worker nodes #54

Open

jimmidyson force-pushed the main branch from 05b8c1a to 3774cad Compare April 11, 2024 08:17

faiq approved these changes Apr 11, 2024

View reviewed changes

jimmidyson reviewed Apr 11, 2024

View reviewed changes

supershal mentioned this pull request Apr 11, 2024

fix: set defaults for AWS CP and Worker instanceType nutanix-cloud-native/cluster-api-runtime-extensions-nutanix#504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: set default instance type for aws cp and worker nodes#51

feat: set default instance type for aws cp and worker nodes#51
supershal wants to merge 96 commits into
mainfrom
shalin/default_instance_type

supershal commented Apr 10, 2024

Uh oh!

dkoshkin left a comment

Uh oh!

Uh oh!

faiq left a comment

Uh oh!

jimmidyson Apr 11, 2024

Uh oh!

jimmidyson Apr 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	Default: variables.MustMarshal(string(i)),
	Default: variables.MustMarshal(i),

Uh oh!

Conversation

supershal commented Apr 10, 2024

Uh oh!

dkoshkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

faiq left a comment

Choose a reason for hiding this comment

Uh oh!

jimmidyson Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

jimmidyson Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants