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

Ensure that Node lease has OwnerReference set #84998

Merged
merged 1 commit into from Nov 13, 2019

Conversation

@wojtek-t
Copy link
Member

wojtek-t commented Nov 8, 2019

Ref #84981

Fix a bug that a node Lease object may have been created without OwnerReference.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 8, 2019

[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

@@ -84,6 +84,30 @@ var _ = framework.KubeDescribe("NodeLease", func() {
time.Duration(*lease.Spec.LeaseDurationSeconds/4)*time.Second)
})

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

This comment has been minimized.

Copy link
@shyamjvs

shyamjvs Nov 8, 2019

Member

Should this be a conformance test?

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Nov 12, 2019

Author Member

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 force-pushed the wojtek-t:fix_owner_refs branch from c85407b to 07200a0 Nov 12, 2019
@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
@wojtek-t wojtek-t added this to the v1.17 milestone Nov 12, 2019
@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented Nov 12, 2019

Added unit tests - this is now ready for review

// 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.

This comment has been minimized.

Copy link
@wangzhen127

wangzhen127 Nov 12, 2019

Member

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?

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Nov 12, 2019

Author Member

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.

This comment has been minimized.

Copy link
@wangzhen127
@wangzhen127

This comment has been minimized.

Copy link
Member

wangzhen127 commented Nov 12, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 12, 2019
@k8s-ci-robot k8s-ci-robot merged commit 43ccfce into kubernetes:master Nov 13, 2019
16 checks passed
16 checks passed
cla/linuxfoundation wojtek-t authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-kind-ipv6 Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.