Adding --annotation-ttl for automatic unlock #119

michalschott · 2020-03-09T21:53:55Z

I have observed this behavior when running kured together with cluster autoscaler, reason why this idea was born.

CA adds nodes (CoreOS in my case)
Nodes are starting to reboot themselves after fetching updates, ie. annotation has been added (lock owned by node 123.123.123.123)
CA decides to downscale ASG (not all nodes had a chance to reboot yet), and it decides to cut off node 123.123.123.123
Need to manually take off annotation

This PR introduces --annotationTTL parameter, by default set to 0 (means do not use this feature), which allows other kured pods to take off the lock if it has expired.

Comparing kured to coreos-update-operator latter one is adding lock annotation onto node object which makes such feature as lockTTL completely not required.

michalschott · 2020-03-28T00:37:07Z

@dholbach any feedback on this one please?

bboreham

Thanks for the PR.
I think it needs a few more safeguards as noted.
Also, how about unit tests?

README.md

bboreham · 2020-05-05T09:09:17Z

pkg/daemonsetlock/daemonsetlock.go

@@ -79,6 +86,11 @@ func (dsl *DaemonSetLock) Test(metadata interface{}) (holding bool, err error) {
 		if err := json.Unmarshal([]byte(valueString), &value); err != nil {
 			return false, err
 		}
+
+		if ttlExpired(value.Created, value.TTL) {


What happens if you have a running kured install, and you add the TTL flag; won't it see the previous annotation as expired immediately?
Similarly if someone takes the lock manually

If you take lock manually as described, then set TTL flag it will invalidate it instantly as there will be no created and ttl values set in annotation:

if ttl > 0 && time.Since(created) >= ttl

Do you think this is acceptable?

So double checked this behaviour.

Launch params:

kured-pzs8b kured time="2020-05-05T18:27:40Z" level=info msg="Kubernetes Reboot Daemon: -64ebf53" kured-pzs8b kured time="2020-05-05T18:27:40Z" level=info msg="Node ID: XXX.compute.internal" kured-pzs8b kured time="2020-05-05T18:27:40Z" level=info msg="Lock Annotation: kube-system/kured:weave.works/kured-node-lock" kured-pzs8b kured time="2020-05-05T18:27:40Z" level=info msg="Reboot Sentinel: /var/run/update_engine_autoupdate_completed every 1m0s" kured-pzs8b kured time="2020-05-05T18:27:40Z" level=info msg="Blocking Pod Selectors: []" kured-pzs8b kured time="2020-05-05T18:27:40Z" level=info msg="Reboot on: SunMonTueWedThuFriSat between 00:00 and 23:59 UTC" kured-pzs8b kured time="2020-05-05T18:27:40Z" level=info msg="Force annotation cleanup after: 2m0s"

Manually disabled reboots:

➜ ~ kubectl -n kube-system annotate ds kured weave.works/kured-node-lock='{"nodeID":"manual"}' daemonset.apps/kured annotated

Output:

kured-pzs8b kured time="2020-05-05T18:29:29Z" level=info msg="Reboot required" kured-pzs8b kured time="2020-05-05T18:29:29Z" level=warning msg="Lock already held: manual" kured-pzs8b kured time="2020-05-05T18:30:29Z" level=info msg="Reboot required" kured-pzs8b kured time="2020-05-05T18:30:29Z" level=warning msg="Lock already held: manual" kured-pzs8b kured time="2020-05-05T18:31:29Z" level=info msg="Reboot required" kured-pzs8b kured time="2020-05-05T18:31:29Z" level=warning msg="Lock already held: manual" kured-pzs8b kured time="2020-05-05T18:32:29Z" level=info msg="Reboot required" kured-pzs8b kured time="2020-05-05T18:32:29Z" level=warning msg="Lock already held: manual" kured-pzs8b kured time="2020-05-05T18:33:29Z" level=info msg="Reboot required" kured-pzs8b kured time="2020-05-05T18:33:29Z" level=warning msg="Lock already held: manual"

Removing lock manually:

➜ ~ kubectl -n kube-system annotate ds kured weave.works/kured-node-lock- daemonset.apps/kured annotated

Output:

kured-pzs8b kured time="2020-05-05T18:35:29Z" level=info msg="Reboot required" kured-pzs8b kured time="2020-05-05T18:35:29Z" level=info msg="Acquired reboot lock" kured-pzs8b kured time="2020-05-05T18:35:29Z" level=info msg="Draining node XXX.compute.internal" ...

So if lock has been taken manually, enabling this won't take over as I though initially.

To answer your question if this is acceptable - turns out an ability to manually disable reboot is not gone despite enabling annotation TTL functionality, so all is ok.

Of course this might not fit in every environments, but ideally I think the better solution would be to annotate node objects instead ds which would not require any cleanup mechanism being implemented.

Thanks - I had missed that if ttl > 0 is checking the ttl set by the locker (if any), not Kured's command-line setting.

pkg/daemonsetlock/daemonsetlock.go

cmd/kured/main.go

bboreham · 2020-05-05T09:15:45Z

pkg/daemonsetlock/daemonsetlock.go

@@ -79,6 +86,11 @@ func (dsl *DaemonSetLock) Test(metadata interface{}) (holding bool, err error) {
 		if err := json.Unmarshal([]byte(valueString), &value); err != nil {
 			return false, err
 		}
+
+		if ttlExpired(value.Created, value.TTL) {
+			return true, nil


I think I would print a log message in this case, for when it does something unexpected.

I can not see any other log references inside this pkg, shall this be changed then?

That's valid; could add another return value and print at the higher level?

I do not think I could find place for err inside ttlExpired function as this is simple true/false check function.

If I'd return an err value here, it would be errored as fatal - https://github.com/weaveworks/kured/blob/f2a0f8e20dbe6b0cc4a5d03cbe06889333adee98/cmd/kured/main.go#L196-L205

Buuut, I might be blind...

OK I'll fix it in post.

michalschott · 2020-05-05T12:57:37Z

@bboreham I'd love to extend unit tests if there are any ;)

Addressed your concerns, can you have 2nd look please?

README.md

bboreham · 2020-05-05T17:00:54Z

@bboreham I'd love to extend unit tests if there are any ;)

Yes there are some:
https://github.com/weaveworks/kured/blob/master/pkg/timewindow/days_test.go
https://github.com/weaveworks/kured/blob/master/pkg/timewindow/timewindow_test.go

michalschott · 2020-05-05T20:39:30Z

@bboreham I'd love to extend unit tests if there are any ;)

Yes there are some:
https://github.com/weaveworks/kured/blob/master/pkg/timewindow/days_test.go
https://github.com/weaveworks/kured/blob/master/pkg/timewindow/timewindow_test.go

Added tests for func ttlExpired.

bboreham

Thanks! LGTM.

bboreham · 2020-05-20T10:27:46Z

pkg/daemonsetlock/daemonsetlock.go

@@ -79,6 +86,11 @@ func (dsl *DaemonSetLock) Test(metadata interface{}) (holding bool, err error) {
 		if err := json.Unmarshal([]byte(valueString), &value); err != nil {
 			return false, err
 		}
+
+		if ttlExpired(value.Created, value.TTL) {


Thanks - I had missed that if ttl > 0 is checking the ttl set by the locker (if any), not Kured's command-line setting.

bboreham · 2020-05-20T10:28:26Z

pkg/daemonsetlock/daemonsetlock.go

@@ -79,6 +86,11 @@ func (dsl *DaemonSetLock) Test(metadata interface{}) (holding bool, err error) {
 		if err := json.Unmarshal([]byte(valueString), &value); err != nil {
 			return false, err
 		}
+
+		if ttlExpired(value.Created, value.TTL) {
+			return true, nil


OK I'll fix it in post.

michalschott changed the title ~~Adding annotationTTL.~~ [WIP] Adding annotationTTL. Mar 16, 2020

michalschott changed the title ~~[WIP] Adding annotationTTL.~~ Adding annotationTTL. Mar 16, 2020

michalschott changed the title ~~Adding annotationTTL.~~ Adding annotationTTL for automatic unlock Mar 28, 2020

bboreham reviewed May 5, 2020

View reviewed changes

michalschott added 5 commits May 5, 2020 14:10

Adding annotationTTL.

7fb16fe

Removing spurious change.

1fc2522

Be clean when this feature is disabled.

1257d97

Calculate time difference easier.

615e3d4

Typo in logic.

64ebf53

ckotzbauer reviewed May 5, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

michalschott added 2 commits May 5, 2020 20:52

Renaming flag as suggested.

59a6700

Adding unit tests for ttlExpired.

cf03bc5

michalschott changed the title ~~Adding annotationTTL for automatic unlock~~ Adding --annotation-ttl for automatic unlock May 6, 2020

bboreham approved these changes May 20, 2020

View reviewed changes

bboreham merged commit ec75533 into kubereboot:master May 20, 2020

lunarfs mentioned this pull request Jun 22, 2020

Added expiration capabilities for the lock #54

Closed

dholbach mentioned this pull request Jun 29, 2020

Create 1.4.1 release #154

Closed

6 tasks

mvisonneau mentioned this pull request Oct 22, 2020

Replaced --annotationTTL with --lockTTL and made it work correctly #213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding --annotation-ttl for automatic unlock #119

Adding --annotation-ttl for automatic unlock #119

michalschott commented Mar 9, 2020 •

edited

michalschott commented Mar 28, 2020

bboreham left a comment

bboreham May 5, 2020

michalschott May 5, 2020

bboreham May 5, 2020

michalschott May 5, 2020

bboreham May 20, 2020

bboreham May 5, 2020

michalschott May 5, 2020

bboreham May 5, 2020

michalschott May 5, 2020

bboreham May 20, 2020

michalschott commented May 5, 2020

bboreham commented May 5, 2020

michalschott commented May 5, 2020

bboreham left a comment

bboreham May 20, 2020

bboreham May 20, 2020

Adding --annotation-ttl for automatic unlock #119

Adding --annotation-ttl for automatic unlock #119

Conversation

michalschott commented Mar 9, 2020 • edited

michalschott commented Mar 28, 2020

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalschott commented May 5, 2020

bboreham commented May 5, 2020

michalschott commented May 5, 2020

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalschott commented Mar 9, 2020 •

edited