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

fix: ensure node leases aren't leaked #1807

Merged

Conversation

jmdeal
Copy link
Member

@jmdeal jmdeal commented Nov 12, 2024

Fixes #N/A

Description

This PR makes two changes to the NodeClaim lifecycle controller to ensure node leases aren't leaked:

  • The NodeClaim must be registered before being initialized. This ensures NodeClaims aren't candidates for voluntary disruption mechanisms if the Node hasn't had the termination finalizer added by the registration sub-reconciler.
  • When terminating a NodeClaim, only delete the associated Nodes if the NodeClaim is registered. Similar to the first change, this ensures we don't delete Nodes which don't have Karpenter's termination finalizer. These Nodes should be removed out-of-band by CCM once the underlying instance has terminated.

How was this change tested?
make test

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 12, 2024
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 12, 2024
Copy link
Contributor

@njtran njtran left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 12, 2024
@njtran
Copy link
Contributor

njtran commented Nov 12, 2024

/hold

@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 Nov 12, 2024
@coveralls
Copy link

coveralls commented Nov 12, 2024

Pull Request Test Coverage Report for Build 11802960194

Details

  • 13 of 18 (72.22%) changed or added relevant lines in 2 files are covered.
  • 6 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.005%) to 80.832%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controllers/nodeclaim/lifecycle/controller.go 10 15 66.67%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/nodeclaim/lifecycle/controller.go 1 71.07%
pkg/utils/nodeclaim/nodeclaim.go 2 37.07%
pkg/controllers/nodeclaim/lifecycle/initialization.go 3 86.11%
Totals Coverage Status
Change from base Build 11787793721: -0.005%
Covered Lines: 8476
Relevant Lines: 10486

💛 - Coveralls

@jmdeal jmdeal force-pushed the fix/only-initialize-registered-node branch from 7d3d97c to 289d642 Compare November 12, 2024 01:30
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 12, 2024
@jmdeal jmdeal changed the title fix: only initialize registered nodes fix: ensure node leases aren't leaked Nov 12, 2024
@jmdeal jmdeal force-pushed the fix/only-initialize-registered-node branch from 9d10b3f to 99e2a3b Compare November 12, 2024 17:54
@jmdeal jmdeal force-pushed the fix/only-initialize-registered-node branch from 99e2a3b to 501c259 Compare November 12, 2024 17:56
Copy link
Member

@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
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmdeal, jonathan-innis, njtran

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:
  • OWNERS [jonathan-innis,njtran]

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

@jonathan-innis
Copy link
Member

/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 Nov 12, 2024
@k8s-ci-robot k8s-ci-robot merged commit d0d5d8b into kubernetes-sigs:main Nov 12, 2024
12 checks passed
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. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants