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: Add Versioned for EC2NodeClass Hash to Prevent Drift on EC2NodeClass CRD Upgrade #5770

Merged
merged 3 commits into from
Mar 12, 2024

Conversation

engedaam
Copy link
Contributor

@engedaam engedaam commented Mar 3, 2024

Fixes #N/A

Description
Add versioned NodePool Hash to prevent Drift from occurring under these scenario:

  1. A EC2NodeClass adds default values to existing fields that are included in the hash calculations
  2. A EC2NodeClass field is added to the hash calculation with an already-set value
  3. A EC2NodeClass field is removed from the hash calculations

RFC: kubernetes-sigs/karpenter#1015
Related Issue: kubernetes-sigs/karpenter#957

How was this change tested?

  • make presubmit

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

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

@engedaam engedaam requested a review from a team as a code owner March 3, 2024 07:16
@engedaam engedaam requested a review from ellistarn March 3, 2024 07:16
Copy link

netlify bot commented Mar 3, 2024

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 473e723
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/65f0912c7eaa530008cce2d5

@engedaam engedaam force-pushed the versioned-hash branch 2 times, most recently from 8595ac8 to fcc97cf Compare March 3, 2024 07:22
@coveralls
Copy link

coveralls commented Mar 3, 2024

Pull Request Test Coverage Report for Build 8253246984

Details

  • 44 of 50 (88.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 82.387%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controllers/nodeclass/controller.go 31 37 83.78%
Totals Coverage Status
Change from base Build 8253107821: 0.03%
Covered Lines: 5342
Relevant Lines: 6484

💛 - Coveralls

@engedaam engedaam force-pushed the versioned-hash branch 4 times, most recently from 63c81ef to 8a4a9f5 Compare March 4, 2024 18:24
@engedaam engedaam force-pushed the versioned-hash branch 4 times, most recently from db723d1 to 1b7b223 Compare March 7, 2024 00:06
pkg/apis/v1beta1/ec2nodeclass.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/suite_test.go Outdated Show resolved Hide resolved
test/suites/drift/suite_test.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/suite_test.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/suite_test.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/suite_test.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/suite_test.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
@engedaam engedaam force-pushed the versioned-hash branch 6 times, most recently from 65c5050 to 0fca6bd Compare March 8, 2024 00:45
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/controller.go Show resolved Hide resolved
pkg/controllers/nodeclass/suite_test.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/suite_test.go Outdated Show resolved Hide resolved
@engedaam engedaam force-pushed the versioned-hash branch 7 times, most recently from 0d13e86 to edc833e Compare March 9, 2024 18:29
@engedaam engedaam force-pushed the versioned-hash branch 7 times, most recently from c55145b to 5f87834 Compare March 11, 2024 06:44
@engedaam engedaam force-pushed the versioned-hash branch 2 times, most recently from 3d7d154 to 4d374e4 Compare March 12, 2024 17:25
Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@engedaam engedaam merged commit bc62192 into aws:main Mar 12, 2024
17 checks passed
@engedaam engedaam deleted the versioned-hash branch March 13, 2024 01:48
engedaam added a commit to engedaam/karpenter that referenced this pull request Mar 25, 2024
engedaam added a commit to engedaam/karpenter that referenced this pull request Mar 25, 2024
engedaam added a commit to engedaam/karpenter that referenced this pull request Mar 25, 2024
engedaam added a commit to engedaam/karpenter that referenced this pull request Mar 25, 2024
engedaam added a commit that referenced this pull request Mar 26, 2024
engedaam added a commit that referenced this pull request Mar 26, 2024
engedaam added a commit that referenced this pull request Mar 26, 2024
engedaam added a commit that referenced this pull request Mar 26, 2024
johngmyers pushed a commit to johngmyers/karpenter that referenced this pull request May 31, 2024
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.

5 participants