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

feat: Set default values on the APIVersion and Kind on the nodeClassRef #1093

Closed

Conversation

engedaam
Copy link
Contributor

@engedaam engedaam commented Mar 12, 2024

Fixes #N/A

Description

  • Add GetNodeClassGVK to the cloudprovider interface
  • The cloudprovider will give the APIVersion and Kind of the NodeClass that is supported by the provider. Karpenter will default all NodePools to the APIVersion and Kind, given by the provider, inside of each nodeClassRef.
  • To avoid a breaking change, we will use the hash controller to set the APIVersion and Kind at runtime.
  • The team is considering on making all fields on the nodeClassRef required at v1, which will allow us to drop the defaulting behavior.
  • Related to issue: How to handle unset apiVersion and kind in nodeClassRef #909

How was this change tested?

  • make presubmit

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

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 12, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: engedaam
Once this PR has been reviewed and has the lgtm label, please assign bwagner5 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 requested review from jmdeal and tallaxes March 12, 2024 00:39
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 12, 2024
kwok/cloudprovider/cloudprovider.go Outdated Show resolved Hide resolved
pkg/apis/v1beta1/nodepool.go Show resolved Hide resolved
@engedaam engedaam force-pushed the add-nodeclassref-defaults branch 2 times, most recently from e2cc36c to c12bdf4 Compare March 12, 2024 07:54
@coveralls
Copy link

coveralls commented Mar 12, 2024

Pull Request Test Coverage Report for Build 8472762287

Details

  • 22 of 26 (84.62%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 79.016%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controllers/controllers.go 0 1 0.0%
kwok/cloudprovider/cloudprovider.go 0 3 0.0%
Totals Coverage Status
Change from base Build 8456314668: 0.08%
Covered Lines: 8288
Relevant Lines: 10489

💛 - Coveralls

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 14, 2024
@engedaam engedaam force-pushed the add-nodeclassref-defaults branch 4 times, most recently from 4e6aa07 to 06665e6 Compare March 17, 2024 20:50
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 17, 2024
@engedaam engedaam force-pushed the add-nodeclassref-defaults branch from 06665e6 to 6bc40db Compare March 17, 2024 20:55
@engedaam engedaam marked this pull request as ready for review March 17, 2024 20:57
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2024
@engedaam
Copy link
Contributor Author

/hold

We need to make sure that this PR is merged in after drift hash versioning have been released. Issue: #957

@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 Mar 17, 2024
@engedaam
Copy link
Contributor Author

/test all

@k8s-ci-robot
Copy link
Contributor

@engedaam: No jobs can be run with /test all.
The following commands are available to trigger optional jobs:

  • /test pull-karpenter-test-1-26
  • /test pull-karpenter-test-1-27
  • /test pull-karpenter-test-1-28
  • /test pull-karpenter-test-1-29

In response to this:

/test all

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/test-infra repository.

@engedaam engedaam force-pushed the add-nodeclassref-defaults branch from 6bc40db to 4462c2f Compare March 18, 2024 19:03
@engedaam engedaam force-pushed the add-nodeclassref-defaults branch 2 times, most recently from 99852cd to 067b22d Compare March 18, 2024 19:04
kwok/cloudprovider/cloudprovider.go Outdated Show resolved Hide resolved
kwok/cloudprovider/cloudprovider.go Outdated Show resolved Hide resolved
pkg/apis/v1beta1/nodepool.go Show resolved Hide resolved
pkg/apis/v1beta1/nodepool.go Show resolved Hide resolved
pkg/cloudprovider/fake/cloudprovider.go Outdated Show resolved Hide resolved
kwok/cloudprovider/cloudprovider.go Outdated Show resolved Hide resolved
pkg/controllers/nodepool/hash/controller.go Outdated Show resolved Hide resolved
@engedaam engedaam changed the title feat: Set default values on the APIVersion and Kind on the nodeClassRef feat: Set default values on the APIVersion and Kind on the nodeClassRef (DO NOT MERGE) Mar 18, 2024
@engedaam engedaam force-pushed the add-nodeclassref-defaults branch from 067b22d to 5dcddba Compare March 18, 2024 20:16
@engedaam engedaam changed the title feat: Set default values on the APIVersion and Kind on the nodeClassRef (DO NOT MERGE) feat: Set default values on the APIVersion and Kind on the nodeClassRef Mar 21, 2024
@engedaam
Copy link
Contributor Author

/remove-hold

@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 Mar 21, 2024
@engedaam engedaam force-pushed the add-nodeclassref-defaults branch from 5dcddba to 21039c6 Compare March 27, 2024 22:08
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 27, 2024
@engedaam engedaam force-pushed the add-nodeclassref-defaults branch from 21039c6 to 0ee2838 Compare March 27, 2024 22:10
kwok/cloudprovider/cloudprovider.go Show resolved Hide resolved
kwok/cloudprovider/cloudprovider.go Outdated Show resolved Hide resolved
Comment on lines +56 to +59
// To avoid a breaking change on the NodePool API, we will be setting a default APIVersion and Kind
// defined by the cloudprovider to each nodeClassRef on every NodePool. This will be removed once
// the NodePool API requires APIVersion and Kind to be set at NodePool creation.
// TODO: remove at v1 when APIVersion and Kind are required fields on NodePool
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking here, should we make it required, or should we make it optional and just require that cloud providers add the defaulting logic to include the GVK into NodePools? If we require it, we can't default it. Is there a point in allowing users to be wrong here?

pkg/controllers/nodepool/hash/suite_test.go Outdated Show resolved Hide resolved
@engedaam engedaam force-pushed the add-nodeclassref-defaults branch from 0ee2838 to 8a5ff14 Compare March 28, 2024 17:16
@engedaam engedaam marked this pull request as draft March 28, 2024 17:33
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2024
@engedaam engedaam force-pushed the add-nodeclassref-defaults branch from 8a5ff14 to 51a4027 Compare March 28, 2024 19:36
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 28, 2024
@engedaam engedaam marked this pull request as ready for review March 28, 2024 19:40
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 28, 2024
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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/test-infra repository.

@jonathan-innis
Copy link
Member

jonathan-innis commented Apr 7, 2024

@engedaam Given that we are punting on setting the default GVK and we have already merged in the CloudProvider changes to allow for shorter reconciles, do we need to keep this PR open? I think if we were to make this change down-the-line, it would most likely take the form of something in our conversion webhooks

@engedaam
Copy link
Contributor Author

engedaam commented Apr 9, 2024

Closing for now. We plan on handling defaults for the APIVersion and Kind as part of the conversion webhooks at v1.

@engedaam engedaam closed this Apr 9, 2024
@engedaam engedaam deleted the add-nodeclassref-defaults branch April 14, 2024 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants