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

set default values for Instance Type for AWS CP and Worker nodes #485

Open
supershal opened this issue Apr 6, 2024 · 2 comments
Open

set default values for Instance Type for AWS CP and Worker nodes #485

supershal opened this issue Apr 6, 2024 · 2 comments
Assignees

Comments

@supershal
Copy link
Contributor

InstanceType is a mandatory field in AWSMachineTemplate. Currently we set respective defaults CP: "m5.xlarge" and workers: "m5.2xlarge" in the default cluster class templates.

We should move it to the CAREN API so that the default values are discoverable to the CAREN Clients.

@supershal supershal self-assigned this Apr 6, 2024
@supershal
Copy link
Contributor Author

Please checkout d2iq-labs/cluster-api-runtime-extensions-nutanix@main...shalin/default_instance_type for an implementation that we discussed.

Currently AWS nodespec variable is shared between CP and worker node, that makes it difficult to set a single default in Json variable schema
This implementation sets default at the time of Discovery so that the CAREN client will have a default populated for respective CP and worker AWS node.

However the way the current API type is defined and JsonSchema is built it requires code changes in multiple places or need helper functions. I personally prefer some other solution than implemented in this PR.

Alternatives:

  • Keep the defaults in the Default cluster class template (same as now) and add defaults to the variable description
  • Create defaultInstanceType patches for AWS and Workers where it always replace value of instanceType with defaults if the AWSMachineTemnplate has value set to PLACEHOLDER. Leave current instanceType patch as it is. Document the defaults in the variable description
  • Refactor API type to not use Generic types. i.e. use AWSClusterConfigSpec, AWSNodeSpec, instead of ClusterConfigSpec{"AWS": ...} and NodeSpec{"AWS": ...}
  • mutating webhook
  • any other ideas???
    cc: @dkoshkin , @faiq , @dlipovetsky , @jimmidyson

@supershal
Copy link
Contributor Author

We had a discussion today. We decided to implement this with a mutating webhook which will add defaults to the cluster variables.

jimmidyson pushed a commit that referenced this issue Apr 15, 2024
Moved
d2iq-labs#51
Addressed the pending review comments
**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 #
#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant