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

docs: RFC v1beta1 #2964

Closed
wants to merge 1 commit into from
Closed

docs: RFC v1beta1 #2964

wants to merge 1 commit into from

Conversation

ellistarn
Copy link
Contributor

@ellistarn ellistarn commented Dec 1, 2022

Fixes kubernetes/kubernetes#1327

Description

v1beta1 proposal.

Release Note


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

@ellistarn ellistarn requested a review from a team as a code owner December 1, 2022 18:31
@ellistarn ellistarn requested a review from engedaam December 1, 2022 18:31
@netlify
Copy link

netlify bot commented Dec 1, 2022

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit d6fa45c
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/63c7166fec8099000815319e

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I recommend some clarifications, and I've added other opinions.


Customers will be able to migrate from v1alpha5 to v1alpha6 in a single cluster using a single Karpenter version.

Kubernetes custom resources have built in support for API version compatibility. CRDs with multiple versions must define a “storage version”, which controls the data stored in etcd. Other versions are views onto this data and converted using [conversion webhooks](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#webhook-conversion). However, there is a fundamental limitation that all versions must be safely [round-tripable through each other](https://book.kubebuilder.io/multiversion-tutorial/api-changes.html). This means that it must be possible to define a function that converts a v1alpha5 Provisioner into a v1alpha6 Provisioner and vise versa.
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
Kubernetes custom resources have built in support for API version compatibility. CRDs with multiple versions must define a “storage version”, which controls the data stored in etcd. Other versions are views onto this data and converted using [conversion webhooks](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#webhook-conversion). However, there is a fundamental limitation that all versions must be safely [round-tripable through each other](https://book.kubebuilder.io/multiversion-tutorial/api-changes.html). This means that it must be possible to define a function that converts a v1alpha5 Provisioner into a v1alpha6 Provisioner and vise versa.
Kubernetes custom resources have built in support for API version compatibility. CRDs with multiple versions must define a “storage version”, which controls the data stored in etcd. Other versions are views onto this data and converted using [conversion webhooks](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#webhook-conversion). However, there is a fundamental limitation that all versions must be safely [round-tripable through each other](https://book.kubebuilder.io/multiversion-tutorial/api-changes.html). This means that it must be possible to define a function that converts a v1alpha5 Provisioner into a v1alpha6 Provisioner and vice versa.

2. [Recommended] Require that users delete the existing v1alpha5 Provisioner CRD and then install the v1alpha6 Provisioner CRD. This will result in all capacity being shut down, and cannot be done in place if the customer has already launched nodes.
3. Keep the legacy fields in our API forever

Option 2 is untenable and easily discarded. We must provide a migration path for existing customers. Option 3 minimizes immediate impact to customers, but results in long term customer pain. There are other benefits to renaming Provisioner, so while it does cause some churn, it results in long term value.
Copy link
Contributor

Choose a reason for hiding this comment

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

How is Option 2 both untenable and recommended? This is unclear.


Opinion

I do like Option 2, BTW - it's the right approach for an alpha API that needs to change, even if people are using it. However, I can see the cost that it imposes on people who already adopted the alpha API.

If you mean that Option 2 is what the Kubernetes project recommends in general, it's OK to mention that detail.

Comment on lines 78 to 80
### Deprecate: provisioner.spec.provider

We’ve recommended that customers leverage spec.providerRef in favor of spec.provider since Q2 2022. Documentation for this feature has been removed since Q3 2022. We will take the opportunity to remove the feature entirely to minimize code bugs/complexity and user confusion.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is a removal or a deprecation; the text is ambiguous.

Copy link
Contributor

@sftim sftim Jan 3, 2023

Choose a reason for hiding this comment

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

If deprecating (ie, retaining, but asking cluster operators not to use the field), then optionally: emit a Warning: if that field is set on a NodeProvisioner (via an admission webhook).

See https://kubernetes.io/blog/2020/09/03/warnings/ for how these work.

This Warning: behavior could also arrive via a Karpenter release that comes out ahead of the switch to v1alpha6.


1. Install Karpenter version 0.x.0, this version supports both v1alpha5.Provisioner and v1alpha6.NodeProvisioner
1. We may choose to support both APIs in more than one Karpenter version at the cost of developer velocity
2. Create a v1alpha6.NodeProvisioner, and add a taint to the old v1alpha5.Provisioner
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the new Karpenter version automate that transition? I think it's technically possible.

If not, we could provide a container image that runs a one-time migration job (either in-cluster with appropriate access, or to run locally).

Instead, we should separate out properties of the node in the same way that pod properties are separated from their parent controllers. This has a nice effect of simultaneously creating a place for users to define node metadata. This design mirrors that of kubernetes objects that contain podtemplatespecs, like Deployment, StatefulSet, Job, and CronJob.
```
spec:
ttlSecondsUntilExpired: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Opinion

Consider moving this into a containing field such as nodeLifecycle. We could do that for v1beta1 though as the API change would survive round trips.

Comment on lines 82 to 86
### Deprecate: awsnodetemplate.spec.launchTemplate

Direct launch template support is problematic for many reasons, outlined in the [design](https://github.com/aws/karpenter/blob/main/designs/aws/aws-launch-templates-v2.md). Customers continue to run into issues when directly using launch templates. From many conversations with customers, our AWSNodeTemplate design has achieved feature parity with launch templates. The only gap is for users who maintain external workflows for launch template management. This requirement is in direct conflict with users who run into this sharp edge.

This change simply removes legacy support for launch templates in favor of the AWSNodeTemplate design.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is a removal or a deprecation; the text is ambiguous.

Following option #1, customers would upgrade as follows:

1. Install Karpenter version 0.x.0, this version supports both v1alpha5.Provisioner and v1alpha6.NodeProvisioner
1. We may choose to support both APIs in more than one Karpenter version at the cost of developer velocity
Copy link
Contributor

Choose a reason for hiding this comment

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

Opinion

We could support both APIs for several minor releases that are essentially the same underlying code but with different release-time behaviors around API support. The way I picture it, those changes mostly cover three things: updates to Helm charts, default parameters, and docs.

Doing that lets people pick the release that gets the behavior they like. Eg:

  • v0.23.0 for Provisioner favored, NodeProvisioner available for early migration
    • this is a big release because it adds the v1alpha6 API group version and any related features
  • v0.24.0 for NodeProvisioner favored, Provisioner supported, no warnings emitted
  • v0.25.0 for NodeProvisioner favored, Provisioner supported, warnings on creating a Provisioner
  • v0.25.0 for NodeProvisioner favored, Provisioner supported, creating a new Provisioner prohibited by default
  • v0.26.0 for NodeProvisioner available, Provisioner API no longer present
    • this is an important release because it drops the v1alpha5 API group version

The way I imagine this, those releases could come out 1 calendar week apart. (I don't know how automated the release process is). Or even more frequently, if folks want to. Once made, those releases stay published and people who want to stick with v1alpha5 have a few options about that.

PS. https://kubernetes.io/blog/2020/09/03/warnings/ has some details about Warning: headers; an admission webhook can set these whilst still admitting an object. It's a nice way to tell ends users that they're not doing something the best way.

@ellistarn ellistarn changed the title docs: RFC v1alpha6 docs: RFC v1beta1 Jan 17, 2023
@ellistarn ellistarn force-pushed the rfc branch 2 times, most recently from 5fe6d7b to 21ceefb Compare January 17, 2023 21:40
@sftim
Copy link
Contributor

sftim commented Jan 18, 2023

Related: kubernetes/website#45074

As far as I can tell, we don't hav e acomprehensive doc which covers the expected lifecycle of nodes in Kubernetes.

Specifically, we have lots of intersecting, async things which involve nodes. For example:

  • Many environments have VMs "behind" Nodes. Those VMs can be deleted without telling k8s.
  • Many environments have subsystems which cross-reference things which need to coordinate with node lifecycle. E.g. the service controller puts VMs into LBs.
  • Some components manage nodes (e.g. Cluster Autoscaler, Karpenter).

For an example of things that I think are "weird" for lack of docs, look at kubernetes/autoscaler#5201 (comment) . ClusterAutoscaler defines a taint which it uses to prevent work from landing on "draining" nodes (even though we have the unschedulable field already). The service LB controller currently uses that taint to manage LBs. Cluster autoscaler removes the VM from the cloud, and leaves the Node object around for someone else to clean up.

The discussion is about the MEANING of the taint, when it happens, and how to be more graceful. What we want is a clear signal that "this node is going away" and a way for 3rd parties to indicate they have work to do when that happens. It strikes me that we HAVE such a mechanism - delete and finalizers. But CA doesn't do that. I don't know why, but I suspect there are reasons. Should it evolve?

@@ -0,0 +1,93 @@
# v1beta11
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
# v1beta11
# v1beta1

@jonathan-innis
Copy link
Contributor

This RFC is going to go under a re-write before it's re-proposed as we're still trying to capture more items from kubernetes/kubernetes#1327. Closing this for now since the discussion has gone a bit stale since the initial RFC was opened. I'll re-open with the revised version when the proposed items for v1beta1 are finalized.

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

Successfully merging this pull request may close these issues.

4 participants