v1alpha2 API changes #477

ellistarn · 2021-06-25T16:22:15Z

Issue #, if available:

Description of changes:

Introduced a concept of node Expiration (not yet implemented)
Utilization functionality is now disabled by default and must be specified
v1alpha1 is now gone, customers will be forced to upgrade if they upgrade karpenter.
Simplified controller folder structure

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

rothgar · 2021-06-28T17:03:16Z

charts/karpenter/templates/provisioning.karpenter.sh_provisioners.yaml

@@ -52,6 +52,18 @@ spec:
                - endpoint
                - name
                type: object
+              expiration:


Will there be a way for a user to pause this or opt out specific instances? A use case I could think of is during a big event/launch it's nice to temporarily pause these automatic termination events to prevent potential outages. It would also be nice to have a label that can be added (or removed) to skip a specific instance from expiring if you need to do troubleshooting or debugging on it. If karpenter adds a label it would be nice for a company to know which nodes will be automatically recycled based on a common label.

Some sort of locking mechanism at the cluster level would also be helpful. Something that understands the health of the cluster and a percentage of healthy instances to consider before blindly terminating instances. I've been in real world situations where automation was terminating healthy instances while new, unhealthy instances were being created which was quickly degrading the health of the system.

Totally agree. I think we should respect a node.karpenter.sh/do-not-expire label on nodes. I see this as more of a manual intervention, so I'm wary of plumbing this through the API. I'll include this with the implementation.

Something that understands the health of the cluster and a percentage of healthy instances to consider before blindly terminating instances.

See @njtran's doc on node termination budgets, which handles exactly this.

Further, if you remove the expiration concept from the provisioner spec, it will never expire nodes (even if the nodes were created before this change).

charts/karpenter/templates/provisioning.karpenter.sh_provisioners.yaml

cmd/controller/main.go

docs/aws/examples/provisioner.yaml

pkg/apis/provisioning/v1alpha2/provisioner.go

njtran

I think there might be some inconsistencies with the api comments and the functionality of the underutilization controller. Did you add in the no-op when underutilization isn't defined?

njtran · 2021-06-28T23:20:39Z

Makefile

@@ -63,6 +63,11 @@ codegen: ## Generate code. Must be run if changes are made to ./pkg/apis/...
 	# CRDs don't currently jive with VolatileTime, which has an Any type.
 	perl -pi -e 's/Any/string/g' charts/karpenter/templates/provisioning.karpenter.sh_provisioners.yaml
 	hack/boilerplate.sh
+	gen-crd-api-reference-docs \
+		-api-dir ./pkg/apis/provisioning/v1alpha2 \


There's a lot of files, so it's hard to grok the new file structure here. Does removing the version names from pkg/controllers directory have any effect on this?

Is there a point to keeping a previous apis version if the pkg/controllers folder doesn't distinguish pkg?

Yes. My expectation is that we will only ever have one version of the controller version, but we may at some point support multiple CRD API versions. K8s uses a concept of "storage version" for CRs, and then all CRs are translated (via a map function) into that storage version. Therefore we should only ever need to maintain one version of the controller code, but we may have multiple versions of the APIs. In this case, we're not keeping v1alpha1 around, since we can backwards incompat it.

njtran · 2021-06-28T23:27:15Z

docs/aws/README.md

@@ -107,6 +107,8 @@ spec:
    name: ${CLUSTER_NAME}
    caBundle: $(aws eks describe-cluster --name ${CLUSTER_NAME} --query "cluster.certificateAuthority.data" --output json)
    endpoint: $(aws eks describe-cluster --name ${CLUSTER_NAME} --query "cluster.endpoint" --output json)
+  utilization:
+    ttlSeconds: 0


I think we want to suggest the ttlSecondsUtilization here to be 300, right? Setting it to 0 would cause a lot of thrashing.

This should also be a top level parameter now, right?

From @rothgar's PR, making this 30.

docs/aws/examples/provisioner.yaml

pkg/apis/provisioning/v1alpha2/provisioner_validation.go

njtran · 2021-06-28T23:31:05Z

pkg/cloudprovider/aws/ami.go

+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+


You added this in a previous PR, right? Do you need a rebase?

Yeah I built on a previous PR -- now that it's merged, this will disappear.

njtran · 2021-06-28T23:39:47Z

pkg/cloudprovider/aws/utils/predicates/strings.go

+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+


njtran · 2021-06-28T23:39:58Z

pkg/cloudprovider/aws/utils/predicates/tags.go

+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+


re: rebase?

njtran · 2021-06-28T23:41:48Z

pkg/controllers/controller.go

@@ -46,16 +46,12 @@ func (c *GenericController) Reconcile(ctx context.Context, req reconcile.Request
 	}
 	// 2. Copy object for merge patch base
 	persisted := resource.DeepCopyObject()
-	// 3. Set defaults to enforce invariants on object being reconciled
-	if _, ok := resource.(apis.Defaultable); ok {


So we added this to enforce invariants in case the webhooks were down. Are you removing this because there are no fields that need defaulting right now? In this case, shouldn't we keep this in the case where some more apis need defaulting functionality?

I'm actually a little wary of the defaulting logic. I decided that we should use something like ptr.Int64Value() wherever possible. This is why I implemented the knative PR.

pkg/controllers/reallocation/utilization.go

njtran · 2021-06-28T23:43:38Z

pkg/utils/conditions/conditions.go

+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+


re: re: re: rebase

Makefile

njtran

LGTM

bwagner5

lgtm

This reverts commit 8aab1e5.

This reverts commit d48734f.

* Revert "Revert "v1alpha2 API changes (#477)" (#490)" This reverts commit d48734f. * Verified changes

ellistarn force-pushed the ttl branch 4 times, most recently from 4099a87 to 532b426 Compare June 25, 2021 17:06

ellistarn changed the title ~~[WIP] v1alpha2 API changes~~ v1alpha2 API changes Jun 25, 2021

JacobGabrielson self-requested a review June 28, 2021 16:49

rothgar reviewed Jun 28, 2021

View reviewed changes

JacobGabrielson reviewed Jun 28, 2021

View reviewed changes

ellistarn force-pushed the ttl branch 3 times, most recently from ecb9057 to 36276c4 Compare June 28, 2021 21:09

njtran requested changes Jun 28, 2021

View reviewed changes

njtran reviewed Jun 28, 2021

View reviewed changes

Makefile Show resolved Hide resolved

ellistarn force-pushed the ttl branch from 36276c4 to 8fca034 Compare June 29, 2021 17:21

v1alpha2 API changes

e3b4bc5

ellistarn force-pushed the ttl branch from 8fca034 to e3b4bc5 Compare June 29, 2021 17:59

JacobGabrielson approved these changes Jun 29, 2021

View reviewed changes

njtran approved these changes Jun 29, 2021

View reviewed changes

ellistarn merged commit 8aab1e5 into aws:main Jun 29, 2021

ellistarn deleted the ttl branch June 29, 2021 23:27

bwagner5 reviewed Jun 29, 2021

View reviewed changes

ellistarn added a commit to ellistarn/karpenter-provider-aws that referenced this pull request Jun 29, 2021

Revert "v1alpha2 API changes (aws#477)"

d64c229

This reverts commit 8aab1e5.

ellistarn added a commit that referenced this pull request Jun 30, 2021

Revert "v1alpha2 API changes (#477)" (#490)

d48734f

This reverts commit 8aab1e5.

ellistarn added a commit to ellistarn/karpenter-provider-aws that referenced this pull request Jun 30, 2021

Revert "Revert "v1alpha2 API changes (aws#477)" (aws#490)"

faec484

This reverts commit d48734f.

ellistarn added a commit that referenced this pull request Jun 30, 2021

Revert "Revert "v1alpha2 API changes (#477)" (#490)" (#491)

3cb806a

* Revert "Revert "v1alpha2 API changes (#477)" (#490)" This reverts commit d48734f. * Verified changes

gfcroft pushed a commit to gfcroft/karpenter-provider-aws that referenced this pull request Nov 25, 2023

chore: Enable v1beta1/NodePool provisioner hash controller (aws#477)

850dc85

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1alpha2 API changes #477

v1alpha2 API changes #477

ellistarn commented Jun 25, 2021 •

edited

Loading

rothgar Jun 28, 2021

rothgar Jun 28, 2021

ellistarn Jun 28, 2021

ellistarn Jun 28, 2021 •

edited

Loading

ellistarn Jun 28, 2021 •

edited

Loading

njtran left a comment

njtran Jun 28, 2021

ellistarn Jun 29, 2021

njtran Jun 28, 2021

ellistarn Jun 29, 2021

njtran Jun 28, 2021

ellistarn Jun 29, 2021

njtran Jun 28, 2021

njtran Jun 28, 2021

njtran Jun 28, 2021

ellistarn Jun 29, 2021

njtran Jun 28, 2021

njtran left a comment

bwagner5 left a comment

v1alpha2 API changes #477

v1alpha2 API changes #477

Conversation

ellistarn commented Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

ellistarn Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

njtran left a 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

njtran left a comment

Choose a reason for hiding this comment

bwagner5 left a comment

Choose a reason for hiding this comment

ellistarn commented Jun 25, 2021 •

edited

Loading

ellistarn Jun 28, 2021 •

edited

Loading

ellistarn Jun 28, 2021 •

edited

Loading