Skip to content

Conversation

hakman
Copy link
Member

@hakman hakman commented Aug 18, 2025

Iterating on updated Karpenter setup.
Includes:

  • storing a copy of the startup script that can be used later by kops-controller to generate the EC2NodeClass
  • disabling LaunchTemplate generation, which can no longer be used by Karpenter.

/cc @rifelpet @justinsb @ameukam

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/addons area/api area/provider/aws Issues or PRs related to aws provider labels Aug 18, 2025
@hakman
Copy link
Member Author

hakman commented Aug 18, 2025

/override pull-kops-e2e-aws-upgrade-k129-ko129-to-k130-kolatest-karpenter

@k8s-ci-robot
Copy link
Contributor

@hakman: Overrode contexts on behalf of hakman: pull-kops-e2e-aws-upgrade-k129-ko129-to-k130-kolatest-karpenter

In response to this:

/override pull-kops-e2e-aws-upgrade-k129-ko129-to-k130-kolatest-karpenter

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@rifelpet
Copy link
Member

FYI I started working on a model that translates an InstanceGroup to EC2NodeClass and NodePool resources:

master...rifelpet:kops:karpenter-model

I was imagining that we'd add a new karpenter controller to kops-controller that periodically fetches the instance groups (and this new userdata object) from VFS, generates the model, and reconciles the resulting resources. It should be straight forward to add models for other cloud providers too.

@hakman
Copy link
Member Author

hakman commented Aug 19, 2025

FYI I started working on a model that translates an InstanceGroup to EC2NodeClass and NodePool resources:

master...rifelpet:kops:karpenter-model

I was imagining that we'd add a new karpenter controller to kops-controller that periodically fetches the instance groups (and this new userdata object) from VFS, generates the model, and reconciles the resulting resources. It should be straight forward to add models for other cloud providers too.

Very nice @rifelpet, I think that is exactly the missing piece. This PR adds generates a file with the user data, which is not that easy to generate separately. This way, everything needed for generating the EC2NodeClass is in the state store.

@hakman
Copy link
Member Author

hakman commented Aug 22, 2025

@rifelpet @justinsb could you take a look at this one, I think it should be ready.

@hakman hakman force-pushed the karpenter-1.6.2 branch 3 times, most recently from c8e16df to 5132f34 Compare August 22, 2025 17:13
@hakman
Copy link
Member Author

hakman commented Aug 22, 2025

/test pull-kops-verify-terraform

@kubernetes kubernetes deleted a comment from k8s-ci-robot Aug 23, 2025
@hakman
Copy link
Member Author

hakman commented Aug 23, 2025

/override pull-kops-e2e-aws-karpenter

@k8s-ci-robot
Copy link
Contributor

@hakman: Overrode contexts on behalf of hakman: pull-kops-e2e-aws-karpenter

In response to this:

/override pull-kops-e2e-aws-karpenter

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@hakman
Copy link
Member Author

hakman commented Aug 23, 2025

/test pull-kops-scenario-aws-karpenter

@hakman
Copy link
Member Author

hakman commented Aug 23, 2025

/test pull-kops-scenario-aws-karpenter

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2025
@hakman
Copy link
Member Author

hakman commented Aug 23, 2025

/test pull-kops-scenario-aws-karpenter

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2025
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2025
@ameukam
Copy link
Member

ameukam commented Aug 29, 2025

/retest

@hakman
Copy link
Member Author

hakman commented Aug 30, 2025

I would have expected that peter's get merged in this PR before we merge it.

@ameukam I think we decided to merge and wait for Peter's PR another 3 weeks.

Copy link
Member

@rifelpet rifelpet left a comment

Choose a reason for hiding this comment

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

we mention in the docs that upgrading a cluster from the old version isn't supported. This is probably worthy of a release note, especially if someone has been able to have a usable kops 1.33 cluster with karpenter 0.31.3.

nth := clusterSpec.CloudProvider.AWS.NodeTerminationHandler
if nth.Enabled == nil {
if clusterSpec.Karpenter != nil && clusterSpec.Karpenter.Enabled {
nth.Enabled = fi.PtrTo(false)
Copy link
Member

Choose a reason for hiding this comment

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

should we check this in api validation? to ensure spec.karpenter.enabled != spec.cloudprovider.aws.nodeTerminationHandler.enabled ?

app.kubernetes.io/managed-by: Helm
spec:
replicas: {{ ControlPlaneControllerReplicas false }}
Copy link
Member

Choose a reason for hiding this comment

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

should we preserve this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, but added back for now.

Comment on lines 1530 to 1535
{{ if not IsIPv6Only }}
dnsPolicy: Default
{{ else }}
# Must use ClusterFirst on IPv6 clusters in order to get DNS64
dnsPolicy: ClusterFirst
{{ end }}
Copy link
Member

Choose a reason for hiding this comment

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

and preserve this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like a good idea, added back.

Signed-off-by: Ciprian Hacman <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2025
@hakman
Copy link
Member Author

hakman commented Sep 6, 2025

we mention in the docs that upgrading a cluster from the old version isn't supported. This is probably worthy of a release note, especially if someone has been able to have a usable kops 1.33 cluster with karpenter 0.31.3.

Thanks for the review. Added the release notes piece along with the dns and replicas, so should be ready to go now.

@hakman hakman requested a review from rifelpet September 6, 2025 05:16
@hakman
Copy link
Member Author

hakman commented Sep 6, 2025

/test pull-kops-scenario-aws-karpenter

@hakman
Copy link
Member Author

hakman commented Sep 6, 2025

/test pull-kops-scenario-aws-karpenter

@hakman
Copy link
Member Author

hakman commented Sep 6, 2025

/override pull-kops-kubernetes-e2e-ubuntu-gce-build
/override pull-kops-e2e-aws-upgrade-k133-ko133-to-klatest-kolatest-many-addons

@k8s-ci-robot
Copy link
Contributor

@hakman: Overrode contexts on behalf of hakman: pull-kops-e2e-aws-upgrade-k133-ko133-to-klatest-kolatest-many-addons, pull-kops-kubernetes-e2e-ubuntu-gce-build

In response to this:

/override pull-kops-kubernetes-e2e-ubuntu-gce-build
/override pull-kops-e2e-aws-upgrade-k133-ko133-to-klatest-kolatest-many-addons

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@hakman
Copy link
Member Author

hakman commented Sep 6, 2025

/test pull-kops-scenario-aws-karpenter
/override pull-kops-kubernetes-e2e-ubuntu-gce-build
/override pull-kops-e2e-aws-upgrade-k133-ko133-to-klatest-kolatest-many-addons

@k8s-ci-robot
Copy link
Contributor

@hakman: Overrode contexts on behalf of hakman: pull-kops-e2e-aws-upgrade-k133-ko133-to-klatest-kolatest-many-addons, pull-kops-kubernetes-e2e-ubuntu-gce-build

In response to this:

/test pull-kops-scenario-aws-karpenter
/override pull-kops-kubernetes-e2e-ubuntu-gce-build
/override pull-kops-e2e-aws-upgrade-k133-ko133-to-klatest-kolatest-many-addons

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 6, 2025
@hakman
Copy link
Member Author

hakman commented Sep 6, 2025

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 6, 2025
@k8s-ci-robot k8s-ci-robot merged commit 06737ef into kubernetes:master Sep 6, 2025
38 checks passed
@hakman hakman deleted the karpenter-1.6.2 branch September 6, 2025 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/addons area/api area/documentation area/nodeup area/provider/aws Issues or PRs related to aws provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants