v1alpha4 API Proposal #676

ellistarn · 2021-09-14T16:41:13Z

1. Issue, if available:
#661

2. Description of changes:

3. Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: link to issue
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

netlify · 2021-09-14T16:41:19Z

✔️ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: e308989

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/6143783b71f8f2000771f6db

JacobGabrielson · 2021-09-14T18:13:41Z

designs/v1alpha4-api.md

+    launchTemplateName: "foo"
+```
+
+The Cloud Provider *may* continue to support corresponding well known labels at the pod level (e.g. `node.k8s.aws/subnet-name`).


It is not clear whether this will hold true for the alternatives listed in later sections.

In my mind, we'd be resetting most of these and just adding them at the provisioner level (for now).

JacobGabrielson · 2021-09-14T18:14:20Z

designs/v1alpha4-api.md

+*Note: Not all `spec.provider` fields must have a corresponding well known label.*
+
+### Alternative: Inline vs Nested Parameters
+It's possible to leverage inline json capabilities to collapse these parameters to the top level. For example


It is not clear what 'inline json capabilities' means - capabilities of what/where?

https://play.golang.org/p/NnwTh9KI5r

JacobGabrielson · 2021-09-14T18:16:23Z

designs/v1alpha4-api.md

+
+We introduce a new `provider` field that enables strongly typed vendor specific parameters without violating vendor neutrality principles in the Karpenter codebase. We leverage Kubernetes `runtime.RawExtensions` to encapsulate these fields as raw bytes, which are then unmarshaled in vendor specific code. Vendors may implement arbitrary validation, defaulting, and provisioning behavior over the entire structure of these extensions. These structures are versioned separately from the Provisioner GVK to enable Cloud Providers to make backwards incompatible changes to provider specific configuration without requiring a version bump to the Provisioner CRD. If versioning is not specified by the user, it will be inferred and defaulted.
+
+```bash


why are these bash rather than yaml?

JacobGabrielson · 2021-09-14T18:18:48Z

designs/v1alpha4-api.md

+  limits: {} # Vendor neutral, recognized by Karpenter generic controllers
+  labels: {} # Vendor neutral, recognized by Karpenter generic controllers
+  taints: [] # Vendor netural, recognized by Karpenter generic controllers
+  subnets: [] # Vendor specific, only recognized by AWS Cloud Provider code


Aren't you back to the name collision (potential) problem here?

Structural vs nominal types. Discuss. 😛

Unless we want to generalise over abstract providers for some reason, I recommend avoiding this and sticking to composition of nominal types (the RawExtension recommendation above).

JacobGabrielson · 2021-09-14T18:20:12Z

designs/v1alpha4-api.md

+2. (+) Vendors may change versions decoupled from each other.
+3. (+) Parameters that are natural siblings (e.g. `zones`, `securityGroups`) will be no longer be separated by `provider`.
+4. (-) Cloud Provider must do more than simply implementing an interface (they must define an CR, etc).
+


I think there's a con here that customers can get confused about whether a field is vendor-specific or not. And also that when "porting" someone else's yaml code from one vendor to another, they may not even realize certain fields need to change (e.g., the subnets example above).

JacobGabrielson · 2021-09-14T18:21:27Z

designs/v1alpha4-api.md

+3. (+) Parameters that are natural siblings (e.g. `zones`, `securityGroups`) will be no longer be separated by `provider`.
+4. (-) Cloud Provider must do more than simply implementing an interface (they must define an CR, etc).
+
+This approach is quite attractive, but requires significant effort and changes to the project.


I'd probably re-word this a bit, as "quite attractive" is somewhat difficult to quantify. I'd suggest more focusing on how much effort it would be (perhaps quantify that a bit) as a reason not to do it. (I'm also not 100% sure all of the potential downsides were listed?)

JacobGabrielson · 2021-09-14T18:24:38Z

designs/v1alpha4-api.md

+These changes recommend:
+1. Cloud Provider specific extensions under `spec.provider`.
+2. Removal of `spec.cluster`.
+3. Pluralization of `spec.architecture` and `spec.operatingSystem`.


This was pretty confusing, I thought you meant you were going to change the names to (eg) operatingSystems.

designs/v1alpha4-api.md

bwagner5 · 2021-09-15T19:33:53Z

designs/v1alpha4-api.md

+apiVersion: karpenter.sh/v1alpha4
+kind: Provisioner
+spec:
+  provider:


++ provider :)

+1, this looks like a great improvement. from our discussion on Sep 16, would this be an appropriate place for a provider to inject a secret location for kubeconfigs that it might need?

geoffcline · 2021-09-15T20:07:18Z

designs/v1alpha4-api.md

+- `spec.labels['node.k8s.aws/subnet-name']`
+
+One benefit of this approach is that pods may use corresponding `spec.nodeSelector[...]` to request additional constraints on provisioned nodes. This information must be communicated through Kubernetes label values, which is awkward for the following use cases:
+1. Parameters are a list (e.g. `subnets: ["subneta", "subnetb", "subnetc"]`)


I'd change to "may be a list" unless all parameters are a list.

This is in reference to the previous "in the following use cases" -> "parameters are a list", "parameters are a struct"

geoffcline · 2021-09-15T20:09:04Z

designs/v1alpha4-api.md

+    node.k8s.aws/subnet-name: "123" # Vendor Specific Field
+    kubernetes.io/instance-type: "m5.large" # Vendor Neutral Field


oh so it's vendor neutral based on the key and not the value?

Correct -- the value is just a string

anguslees · 2021-09-16T00:50:33Z

designs/v1alpha4-api.md

+```yaml
+spec:
+  limits:
+    ready: 500 # Flat or Percentage. Karpenter will not launch additional capacity if ready nodes exceeds this value


What does "ready=90%" mean? (percentage of what total?)

This is a good point. It slipped my mind when I was thinking "IntOrString all the things" and "unready should have parity, so let's make ready". I suppose it could mean, this provisioner can deploy 50% of the nodes in this cluster. Though I'd much prefer that the provisioners are their own universes and are not aware of other provisioners.

I keep going back and forth on a node count as a limit (as opposed to resource limits below). I'm leaning simply leaving this out entirely until I have a real use case for it.

anguslees · 2021-09-16T00:58:26Z

designs/v1alpha4-api.md

+  limits: {} # Vendor neutral, recognized by Karpenter generic controllers
+  labels: {} # Vendor neutral, recognized by Karpenter generic controllers
+  taints: [] # Vendor netural, recognized by Karpenter generic controllers
+  subnets: [] # Vendor specific, only recognized by AWS Cloud Provider code


Structural vs nominal types. Discuss. 😛

Unless we want to generalise over abstract providers for some reason, I recommend avoiding this and sticking to composition of nominal types (the RawExtension recommendation above).

bwagner5

nice doc!

bwagner5 · 2021-09-16T16:32:41Z

designs/v1alpha4-api.md

+We expand the constraints of a Provisioner from a scalar to a vector in all cases. This will apply to both `operatingSystem` and `architecture` enabling the operator to specify greater flexibility. This supports use cases such as heterogenous architectures within a single provisioner, selected dynamically at runtime by the cloud provider. More importantly, this change creates a consistent and predictable semantic for all vendor neutral constraints. Similar to other constraints, the semantic of this change allows cloud providers to choose any value in the constraint slice, e.g. prioritizing arm64 for cost reasons.
+### Example
+```yaml
+spec:


How would these be expanded? We'd need to inspect the container manifest's support for the architecture and operating system before we could expand the constraints which is possible.

Architecture could be considered optimistic where Karpenter assumes that a container for both amd64 and arm64 exist. But that same strategy couldn't be used for OS, I don't think (at least from a practical use-case).

bwagner5

lgtm, nice doc

JacobGabrielson reviewed Sep 14, 2021

View reviewed changes

ellistarn force-pushed the api branch 2 times, most recently from 891b15b to bf7a0e9 Compare September 15, 2021 19:23

ellistarn changed the title ~~[WIP] v1alpha4 API Proposal~~ v1alpha4 API Proposal Sep 15, 2021

ellistarn force-pushed the api branch from bf7a0e9 to d1d0b2f Compare September 15, 2021 19:29

bwagner5 reviewed Sep 15, 2021

View reviewed changes

JacobGabrielson previously approved these changes Sep 15, 2021

View reviewed changes

geoffcline reviewed Sep 15, 2021

View reviewed changes

ellistarn mentioned this pull request Sep 15, 2021

Implemented a CloudProvider extension mechanism using runtime.RawExtensions #648

Closed

anguslees reviewed Sep 16, 2021

View reviewed changes

bwagner5 previously approved these changes Sep 16, 2021

View reviewed changes

bwagner5 reviewed Sep 16, 2021

View reviewed changes

v1alpha4 API Proposal

e308989

ellistarn dismissed stale reviews from bwagner5 and JacobGabrielson via e308989 September 16, 2021 17:00

ellistarn force-pushed the api branch from d1d0b2f to e308989 Compare September 16, 2021 17:00

bwagner5 approved these changes Sep 16, 2021

View reviewed changes

ellistarn merged commit a8a5ab4 into aws:main Sep 16, 2021

ellistarn deleted the api branch September 16, 2021 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1alpha4 API Proposal #676

v1alpha4 API Proposal #676

ellistarn commented Sep 14, 2021 •

edited

netlify bot commented Sep 14, 2021 •

edited

JacobGabrielson Sep 14, 2021

ellistarn Sep 14, 2021

JacobGabrielson Sep 14, 2021

ellistarn Sep 14, 2021

JacobGabrielson Sep 14, 2021

ellistarn Sep 14, 2021

JacobGabrielson Sep 14, 2021

ellistarn Sep 14, 2021

anguslees Sep 16, 2021

JacobGabrielson Sep 14, 2021

JacobGabrielson Sep 14, 2021

JacobGabrielson Sep 14, 2021

ellistarn Sep 15, 2021

bwagner5 Sep 15, 2021

elmiko Sep 16, 2021

ellistarn Sep 16, 2021

geoffcline Sep 15, 2021

ellistarn Sep 15, 2021

geoffcline Sep 15, 2021

ellistarn Sep 16, 2021

anguslees Sep 16, 2021

ellistarn Sep 16, 2021

anguslees Sep 16, 2021

bwagner5 left a comment

bwagner5 Sep 16, 2021

bwagner5 left a comment


		We introduce a new `provider` field that enables strongly typed vendor specific parameters without violating vendor neutrality principles in the Karpenter codebase. We leverage Kubernetes `runtime.RawExtensions` to encapsulate these fields as raw bytes, which are then unmarshaled in vendor specific code. Vendors may implement arbitrary validation, defaulting, and provisioning behavior over the entire structure of these extensions. These structures are versioned separately from the Provisioner GVK to enable Cloud Providers to make backwards incompatible changes to provider specific configuration without requiring a version bump to the Provisioner CRD. If versioning is not specified by the user, it will be inferred and defaulted.

		```bash

		node.k8s.aws/subnet-name: "123" # Vendor Specific Field
		kubernetes.io/instance-type: "m5.large" # Vendor Neutral Field

v1alpha4 API Proposal #676

v1alpha4 API Proposal #676

Conversation

ellistarn commented Sep 14, 2021 • edited

netlify bot commented Sep 14, 2021 • edited

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

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

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

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

bwagner5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwagner5 left a comment

Choose a reason for hiding this comment

ellistarn commented Sep 14, 2021 •

edited

netlify bot commented Sep 14, 2021 •

edited