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.

Sign up for GitHub

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

Jump to bottom

Ensure that Node lease has OwnerReference set #84998

Merged
merged 1 commit into from Nov 13, 2019
Merged

Ensure that Node lease has OwnerReference set #84998

merged 1 commit into from Nov 13, 2019

Conversation

wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Nov 8, 2019 •
edited

Ref #84981

Fix a bug that a node Lease object may have been created without OwnerReference.

@wojtek-t wojtek-t self-assigned this Nov 8, 2019
@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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 8, 2019
@wojtek-t wojtek-t mentioned this pull request Nov 8, 2019
Closed
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 8, 2019
@wojtek-t wojtek-t added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 8, 2019
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 8, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wojtek-t

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 release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 8, 2019
shyamjvs
shyamjvs reviewed Nov 8, 2019
View reviewed changes
test/e2e/common/node_lease.go
@@ -84,6 +84,30 @@ var _ = framework.KubeDescribe("NodeLease", func() {
time.Duration(*lease.Spec.LeaseDurationSeconds/4)*time.Second)
})

ginkgo.It("should have OwnerReferences set", func() {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a conformance test?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't create conformance test from scratch.
At some point it probably should be, but we need to create it as is and after proving its stability promote it to conformance.

@wojtek-t wojtek-t removed their assignment Nov 12, 2019
@wojtek-t wojtek-t changed the title [WIP] Ensure that Node lease has OwnerReference set Ensure that Node lease has OwnerReference set Nov 12, 2019
@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 Nov 12, 2019
@wojtek-t wojtek-t added this to the v1.17 milestone Nov 12, 2019
@wojtek-t
Copy link
Member Author

Added unit tests - this is now ready for review

wangzhen127
wangzhen127 reviewed Nov 12, 2019
View reviewed changes
pkg/kubelet/nodelease/controller.go
// lease does not exist, create it.
leaseToCreate := c.newLease(nil)
if len(leaseToCreate.OwnerReferences) == 0 {
// We want to ensure that a lease will always have OwnerReferences set.
Copy link
Member

Choose a reason for hiding this comment

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

If we want to ensure a new lease always have OwnerReferences, should we do it in newLease() and return "missing owner ref" error in such case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about that, but that would be much more invasive change (despite the first glance, the flow is non-trivial). I wanted to avoid making significant rewrite on the last moment before code-freeze.
Also note that if we create the Lease correctly, we will be fine (assuming noone will remove it, but that could cause issues anyway, because the node may be deleted in the meantime, so protecting against it is not the goal).

So I actually think the current approach is the best tradeoff between simplicity of code-changes and the goal to achieve.

Copy link
Member

Choose a reason for hiding this comment

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

ACK

@wangzhen127
Copy link
Member

/lgtm

@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, 2019
@k8s-ci-robot k8s-ci-robot merged commit 43ccfce into kubernetes:master Nov 13, 2019
@wojtek-t wojtek-t deleted the fix_owner_refs branch December 18, 2019 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wangzhen127 wangzhen127 wangzhen127 left review comments

@shyamjvs shyamjvs shyamjvs left review comments

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@sjenning sjenning Awaiting requested review from sjenning

Assignees

@wangzhen127 wangzhen127

@shyamjvs shyamjvs

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wojtek-t @k8s-ci-robot @wangzhen127 @shyamjvs