search

kubernetes-sigs / karpenter

Karpenter is a Kubernetes Node Autoscaler built for flexibility, performance, and simplicity.
Apache License 2.0
567 stars 188 forks source link

v1 Laundry List #758

Closed jonathan-innis closed 1 month ago

jonathan-innis commented 11 months ago

Description

This issue contains a list of breaking API changes that we want to make for v1.

Linked Cloud Provider v1 Laundry Lists

ellistarn commented 11 months ago

Drop support for kubectl delete node, now that we have kubectl delete nodeclaim instead. While this is nice for Karpenter users, it teaches users to take an action that's dangerous and disruptive in non-karpenter environments.

mbevc1 commented 11 months ago

I quite like the convenience of having finalizers on delete node, maybe I'm just being lazy :) But it also offers a safety net I reckon :thinking:

ellistarn commented 11 months ago

I'm open minded to it, and would love to hear more feedback on it. Minimally, we need to stop deleting the node and allow the kubelet to deregister as part of our standard termination workflow, as it's currently causing daemons to leak.

jonathan-innis commented 11 months ago

Align the karpenter_interruption_actions_performed values with the karpenter_disruption_actions_performed values. Currently the values are different and (since these components have similar names) we should make them similar

njtran commented 10 months ago

Drop support for do-not-consolidate and do-not-evict annotations from v1alpha5.

garvinp-stripe commented 10 months ago

I think having finalizer in nodeclaim make sense that way as operators we can force the node to terminate outside of the finalizer when necessary. https://github.com/aws/karpenter-core/issues/796 / https://github.com/aws/karpenter-core/issues/743

garvinp-stripe commented 10 months ago

Can we move the Karpenter docs from aws provider github to the k8s sigs Karpenter?

Bryce-Soghigian commented 10 months ago

@garvinp-stripe there is an ongoing discussion on this https://github.com/kubernetes-sigs/karpenter/issues/832

jonathan-innis commented 9 months ago

We should consider requiring apiVersion and kind in the nodeClassRef so that we can dynamically look-up the NodeClass from the NodePool in the neutral code. Some use-cases for this are called out here: https://github.com/kubernetes-sigs/karpenter/issues/337

jonathan-innis commented 9 months ago

Consider changing the apiVersion field in the nodeClassRef to group to match what is done in Gateway API. Referencing references by apiVersion is also not useful here since the reference is only going to be reasoned about in terms of a single version anyways. We really only need the group parameter here.

sftim commented 9 months ago

We should check that the v1 API will work with InPlacePodVerticalScaling if / when that becomes stable https://github.com/kubernetes-sigs/karpenter/issues/829

sftim commented 9 months ago

Drop support for kubectl delete node, now that we have kubectl delete nodeclaim instead. While this is nice for Karpenter users, it teaches users to take an action that's dangerous and disruptive in non-karpenter environments.

We could retain support but inject a Warning: when we see it. I think we can do that just using a ValidatingAdmissionPolicy, too.

sftim commented 9 months ago

NodeClaim's status field has imageID hard coded, which I think refers to AMIs (maybe also AKIs, the legacy thing before AMIs). Do we want to provide something more generic?

Some cloud providers might not have an equivalent field; for example, there might be a JSON document that defines bootstrap for a new machine but no single field within that document that uniquely specifies the initial OS.

My ask for the v1 API is to provide a place to put arbitrary values that are relevant to the node class for this NodeClaim. For AWS, it could be a structure containing the ARN of the AMI and nothing else. For Azure, it could be the imageReference.

Maybe one day for AWS the status of a NodeClaim includes the launch template ARN as well as the image ARN, which could be helpful - and, with this model, doesn't require a change to the v1 API of NodeClaim.

jonathan-innis commented 9 months ago

I'd like to remove references to knative for v1, including the tasklist that's included here (#332). That includes leveraging the controller runtime logger (#922) rather than using our own knative-based custom-built logger as well as relying on our own status conditions library rather than the knative-based one.

jonathan-innis commented 8 months ago

We absolutely need to define #493 ahead of releasing the v1 APIs. Given that, I think we need to do #909 so that we can default these values for pre-v1 and then require them post-v1

jonathan-innis commented 8 months ago

For Azure, it could be the imageReference.

One thing I'm curious about with this is hearing from the Azure folks what they think about the current implementation of the imageID field that sits on the NodeClaim. This was added because it was assumed that every node has an imageID; however, I'm curious if there are other fields on the NodeClaim that y'all would like to make first-class citizens.

cc: @Bryce-Soghigian @tallaxes @jackfrancis

njtran commented 8 months ago

I'd like to see the aws specific labels for resource based selection move to a cloud provider agnostic api group and label:

Currently in AWS, these are karpenter.k8s.aws/instance-cpu and karpenter.k8s.aws/instance-memory. Ideally, since this is a common concept, we can use karpenter.sh/instance-cpu and karpenter.sh/instance-memory.

sftim commented 8 months ago

I'd like to see the aws specific labels for resource based selection move to a cloud provider agnostic api group and label:

Currently in AWS, these are karpenter.k8s.aws/instance-cpu and karpenter.k8s.aws/instance-memory. Ideally, since this is a common concept, we can use karpenter.sh/instance-cpu and karpenter.sh/instance-memory.

Proposal sounds fine. Aside: if we wanted kubernetes.io/node-cpu and ~kubernetes.io/node-cpu~ kubernetes.io/node-memory, we can probably have 'em.

Bryce-Soghigian commented 8 months ago

I'd like to see the aws specific labels for resource based selection move to a cloud provider agnostic api group and label: we can probably also share the karpenter.sh gpu labels, instance family etc

Just FYI On the AKS Providers selectors https://learn.microsoft.com/en-gb/azure/aks/node-autoprovision?tabs=azure-cli#sku-selectors-with-well-known-labels

jonathan-innis commented 8 months ago

Aside: if we wanted kubernetes.io/node-cpu and kubernetes.io/node-cpu, we can probably have 'em.

@sftim Should it be in the top-level K8s namespace or would this get scoped into node.kubernetes.io?

jonathan-innis commented 8 months ago

We should settle on what we want our stable JSON printer columns to be for NodePool and NodeClaims.

sftim commented 8 months ago

Should it be in the top-level K8s namespace or would this get scoped into node.kubernetes.io?

Either is fine with me. Whatever we pick, it should be obvious that CPU is a quantity for the node overall, and that values such as "AMD64" or "Graviton 42" or "Ryzen Pro" would be very wrong.

node.kubernetes.io/resource-cpu or node.kubernetes.io/total-cpu could help signal that.

jonathan-innis commented 8 months ago

My ask for the v1 API is to provide a place to put arbitrary values that are relevant to the node class for this NodeClaim

I'm not sure that we have enough examples of this point from different cloud providers of trying to set a bunch of arbitrary fields. I could see the utility for it, but I'd rather let cloud providers add this data to labels and annotations until we get a critical mass of "things" where it's really easy to conceptualize what should go in that bucket.

The other thing here is that it would be nice if we just didn't have an arbitrary bucket of things to begin with and we could just type everything. I get this probably isn't realistic since there are enough differences between CloudProviders, but it's probably sufficient to take these cases one-by-one so we can evaluate whether they belong in the common schema or not.

wmgroot commented 7 months ago

I'd like to recommend that https://github.com/kubernetes-sigs/karpenter/issues/651 be addressed before v1 is released.

This can waste a lot of time causing confusion for cluster admins and their users tracking down why Karpenter can evict a pod with the "do-not-disrupt" annotation added.

jonathan-innis commented 7 months ago

I'd like to recommend that https://github.com/kubernetes-sigs/karpenter/issues/651 be addressed before v1 is released

Agreed. This is definitely on the list of things that we want to knock-out by v1.

jonathan-innis commented 7 months ago

I'd like to propose that Karpenter start tainting nodes with karpenter.sh/unregistered on startup through their startup script and then signal that the node has been seen and operated on by Karpenter by removing the taint: https://github.com/kubernetes-sigs/karpenter/issues/1049

jonathan-innis commented 7 months ago

I'd also like to propose that Karpenter re-consider its entire monitoring story before going v1. We should think about our metrics, logging, eventing, status conditions, and other status fields that we currently surface back to users: https://github.com/kubernetes-sigs/karpenter/issues/1051

jonathan-innis commented 7 months ago

I'd like to consider shifting the kubelet into the CloudProvider-specific configuration for v1. The more we think about it, the more it seems like this is a property of how the node is brought-up and configured, and less about the selection behavior of the node. This also allows CloudProviders to pick and choose what they want to allow users to directly specify through their API. Right now, there isn't any deep coupling with the kubelet in the neutral code. AWS is only using these values right now through the GetInstanceTypes() call to affect the instance type allocatable.

wmgroot commented 7 months ago

This also allows CloudProviders to pick and choose what they want to allow users to directly specify through their API.

Kubelet is cloud provider agnostic, maybe EKS should allow users to configure feature gates and other k8s configuration options instead? :slightly_smiling_face:

This is the primary reason that CAPI has both XMachineTemplate and KubeadmConfigTemplate CRDs.

ROunofF commented 7 months ago

I opened a small PR that bring a bit of life-improvements to kubectl get nodepools.karpenter.sh: #1055, I believe the current change isn't API-Breaking but if we want to take this further, it may require an api change (I believe this is the laundry list for breaking change)?

So proposing to look at the output from the different CRDs and adjust accordingly. My changes allow to understand at a glance the utilization of the different nodepools in your cluster.

sftim commented 6 months ago

We should ensure that the README for https://artifacthub.io/packages/helm/karpenter/karpenter very, very clearly tells people it's out of date.

We could consider publishing a new release of that legacy chart that is almost the same code, but you get a Warning: every time you create or update a Provisioner.

garvinp-stripe commented 6 months ago

I'd like to consider shifting the kubelet into the CloudProvider-specific configuration for v1.

Not sure where the issue lives but we should clearly define what is the responsibility of each CRD karpenter creates. I think its fine to move it but I want to make sure each move isn't just throwing things from one bucket to another

jonathan-innis commented 6 months ago

Kubelet is cloud provider agnostic, maybe EKS should allow users to configure feature gates and other k8s configuration options instead

I wish this was the case. But from the Cloud Providers that we've heard from at this point, it strikes me that each CloudProvider is rather opinionated about what can and can't be set.

Not sure where the issue lives but we should clearly define what is the responsibility of each CRD karpenter creates

100% agree. I think we are really talking about high-level node scheduling and selection in the NodePool vs. Node-level configuration on the NodeClass side. I agree that an addition to the docs on this could be super valuable if we had this. Maybe a preamble in the https://karpenter.sh/docs/concepts/ that talks about the purpose of each of these APIs.

sftim commented 6 months ago

Drop support for kubectl delete node, now that we have kubectl delete nodeclaim instead. While this is nice for Karpenter users, it teaches users to take an action that's dangerous and disruptive in non-karpenter environments.

— https://github.com/kubernetes-sigs/karpenter/issues/758#issuecomment-1790931672

We could support it but emit a Warning: (via a separate ValidatingAdmissionPolicy)

k8s-triage-robot commented 3 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

GnatorX commented 3 months ago

/remove-lifecycle stale

GnatorX commented 3 months ago

Is there any appetite to change all references to cloudprovider to just plain provider, https://github.com/kubernetes-sigs/karpenter/tree/main/pkg/cloudprovider? Since not all provider are cloud

jonathan-innis commented 1 month ago

Since not all provider are cloud

Oh that's interesting. Are you thinking about kwok specifically? In a lot of ways this is just a "fake" cloudprovider. I'm not sure how much value we would get here by changing the naming

jonathan-innis commented 1 month ago

Closing this one out since we closed-out on the v1 items that we were taking in with #1222

jonathan-innis commented 1 month ago

/close

k8s-ci-robot commented 1 month ago

@jonathan-innis: Closing this issue.

In response to [this](https://github.com/kubernetes-sigs/karpenter/issues/758#issuecomment-2285065001): >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.