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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 10 additions & 3 deletions pkg/kubelet/nodelease/controller.go
View file
Open in desktop
Expand Up @@ -108,7 +108,7 @@ func (c *controller) sync() {
lease, created := c.backoffEnsureLease()
c.latestLease = lease
// we don't need to update the lease if we just created it
if !created {
if !created && lease != nil {
if err := c.retryUpdateLease(lease); err != nil {
klog.Errorf("%v, will retry after %v", err, c.renewInterval)
}
Expand Down Expand Up @@ -144,8 +144,15 @@ func (c *controller) backoffEnsureLease() (*coordinationv1.Lease, bool) {
func (c *controller) ensureLease() (*coordinationv1.Lease, bool, error) {
lease, err := c.leaseClient.Get(c.holderIdentity, metav1.GetOptions{})
if apierrors.IsNotFound(err) {
// lease does not exist, create it
lease, err := c.leaseClient.Create(c.newLease(nil))
// 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

// Thus, given that we weren't able to set it correctly, we simply
// not create it this time - we will retry in the next iteration.
return nil, false, nil
}
lease, err := c.leaseClient.Create(leaseToCreate)
if err != nil {
return nil, false, err
}
Expand Down
48 changes: 39 additions & 9 deletions pkg/kubelet/nodelease/controller_test.go
View file
Open in desktop
Expand Up @@ -308,14 +308,17 @@ func TestUpdateUsingLatestLease(t *testing.T) {

cases := []struct {
desc string
existingObjs []runtime.Object
latestLease *coordinationv1.Lease
updateReactor func(action clienttesting.Action) (bool, runtime.Object, error)
getReactor func(action clienttesting.Action) (bool, runtime.Object, error)
createReactor func(action clienttesting.Action) (bool, runtime.Object, error)
expectLatestLease bool
expectLeaseResourceVersion string
}{
{
desc: "latestLease is nil and need to create",
existingObjs: []runtime.Object{node},
latestLease: nil,
updateReactor: nil,
getReactor: func(action clienttesting.Action) (bool, runtime.Object, error) {
Expand All @@ -324,30 +327,50 @@ func TestUpdateUsingLatestLease(t *testing.T) {
createReactor: func(action clienttesting.Action) (bool, runtime.Object, error) {
return true, makeLease(nodeName, "1"), nil
},
expectLatestLease: true,
expectLeaseResourceVersion: "1",
},
{
desc: "latestLease is nil and need to update",
latestLease: nil,
desc: "latestLease is nil and need to create, node doesn't exist",
existingObjs: nil,
latestLease: nil,
updateReactor: nil,
getReactor: func(action clienttesting.Action) (bool, runtime.Object, error) {
return true, nil, notFoundErr
},
createReactor: func(action clienttesting.Action) (bool, runtime.Object, error) {
return true, makeLease(nodeName, "1"), nil
},
expectLatestLease: false,
expectLeaseResourceVersion: "1",
},
{
desc: "latestLease is nil and need to update",
existingObjs: []runtime.Object{node},
latestLease: nil,
updateReactor: func(action clienttesting.Action) (bool, runtime.Object, error) {
return true, makeLease(nodeName, "2"), nil
},
getReactor: func(action clienttesting.Action) (bool, runtime.Object, error) {
return true, makeLease(nodeName, "1"), nil
},
expectLatestLease: true,
expectLeaseResourceVersion: "2",
},
{
desc: "latestLease exist and need to update",
latestLease: makeLease(nodeName, "1"),
desc: "latestLease exist and need to update",
existingObjs: []runtime.Object{node},
latestLease: makeLease(nodeName, "1"),
updateReactor: func(action clienttesting.Action) (bool, runtime.Object, error) {
return true, makeLease(nodeName, "2"), nil
},
expectLatestLease: true,
expectLeaseResourceVersion: "2",
},
{
desc: "update with latest lease failed",
latestLease: makeLease(nodeName, "1"),
desc: "update with latest lease failed",
existingObjs: []runtime.Object{node},
latestLease: makeLease(nodeName, "1"),
updateReactor: func() func(action clienttesting.Action) (bool, runtime.Object, error) {
i := 0
return func(action clienttesting.Action) (bool, runtime.Object, error) {
Expand All @@ -366,12 +389,13 @@ func TestUpdateUsingLatestLease(t *testing.T) {
getReactor: func(action clienttesting.Action) (bool, runtime.Object, error) {
return true, makeLease(nodeName, "2"), nil
},
expectLatestLease: true,
expectLeaseResourceVersion: "3",
},
}
for _, tc := range cases {
t.Run(tc.desc, func(t *testing.T) {
cl := fake.NewSimpleClientset(node)
cl := fake.NewSimpleClientset(tc.existingObjs...)
if tc.updateReactor != nil {
cl.PrependReactor("update", "leases", tc.updateReactor)
}
Expand All @@ -392,8 +416,14 @@ func TestUpdateUsingLatestLease(t *testing.T) {

c.sync()

if tc.expectLeaseResourceVersion != c.latestLease.ResourceVersion {
t.Fatalf("latestLease RV got %v, expected %v", c.latestLease.ResourceVersion, tc.expectLeaseResourceVersion)
if tc.expectLatestLease {
if tc.expectLeaseResourceVersion != c.latestLease.ResourceVersion {
t.Fatalf("latestLease RV got %v, expected %v", c.latestLease.ResourceVersion, tc.expectLeaseResourceVersion)
}
} else {
if c.latestLease != nil {
t.Fatalf("unexpected latestLease: %v", c.latestLease)
}
}
})
}
Expand Down
24 changes: 24 additions & 0 deletions test/e2e/common/node_lease.go
View file
Open in desktop
Expand Up @@ -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.

leaseClient := f.ClientSet.CoordinationV1().Leases(v1.NamespaceNodeLease)
var (
err error
leaseList *coordinationv1.LeaseList
)
gomega.Eventually(func() error {
leaseList, err = leaseClient.List(metav1.ListOptions{})
if err != nil {
return err
}
return nil
}, 5*time.Minute, 5*time.Second).Should(gomega.BeNil())
// All the leases should have OwnerReferences set to their corresponding
// Node object.
for i := range leaseList.Items {
lease := &leaseList.Items[i]
ownerRefs := lease.ObjectMeta.OwnerReferences
framework.ExpectEqual(len(ownerRefs), 1)
framework.ExpectEqual(ownerRefs[0].Kind, v1.SchemeGroupVersion.WithKind("Node").Kind)
framework.ExpectEqual(ownerRefs[0].APIVersion, v1.SchemeGroupVersion.WithKind("Node").Version)
}
})

ginkgo.It("the kubelet should report node status infrequently", func() {
ginkgo.By("wait until node is ready")
e2enode.WaitForNodeToBeReady(f.ClientSet, nodeName, 5*time.Minute)
Expand Down