Add option for minimal reboot period

[leonnicolas]

The flag --min-reboot-period can be used to define the minimal duration
between reboots of a node.

[ckotzbauer]

Thanks for your PR.

[leonnicolas]

Thanks @ckotzbauer for the super fast review! I pushed a commit according to your comments.

[leonnicolas]

Can I do anything about the failing e2e tests?

[jackfrancis]

I think it would be preferable to log the next time that a reboot will be allowed. So maybe we convert lastSuccessfulRebootWithinMinRebootPeriod() into a "getNextAllowedRebootTime()" func, and then do something like

nextRebootTime := getNextAllowedRebootTime(node)
if time.Now().Before(nextRebootTime) {
    log.Infof("Not allowed to reboot until %s", nextRebootTime)
}

[leonnicolas]

example confuses me. Why add minRebootPeriod to the value returned by getNextAllowedRebootTime?

Either way returning the next allowed reboot time (without adding the minRebootPeriod) or returning the last successful reboot time (and adding minRebootPeriod) sound good to me.

But what should we do if we don't know the last reboot time? Right now we just return "you should reboot" when there is no last successful reboot time. Would the getNextAllowedRebootTime just return &time.Time{} if we don't know about the last successful reboot?

[jackfrancis]

Sorry, copy/paste error in example (updated!).

In the instance where we aren't able to determine the last reboot time we can probably return time.Now(), and then update the statement to

if time.Now().Compare(nextRebootTime) < 0 {
    log.Infof("Not allowed to reboot until %s", nextRebootTime)
}

[leonnicolas]

are you ok with returning and error explicitly? Like

func nextRebootTime(node) (*time.Time, error)

So we don't have to return time.Now() when we don't know.

[leonnicolas]

for example we could do

if minRebootPeriod > 0 {
	if t, err := nextAllowedReboot(node); err != nil {
		log.Warnf("Failed to determine next allowed reboot time: %s", err.Error())
	} else if diff := t.Sub(time.Now()); diff > 0 {
		log.Infof("Reboot not allowed until %s (%s)", t.String(), diff.String())
	}
}

if we don't care about logging the duration (in how much time can we reboot), we should use time.Now().Before(t) instead, I guess. I am really bad with subtracting times in my head.

I see that it would work without returning an error. It would even take less code, but the users would never see the warning about a missing annotation or anything.

[jackfrancis]

I'm happy with an error, but in the case of the annotation not being found, that's not actually an error, as this is a new feature and plenty of users won't configure kured to use it. So silently ignoring that and returning time.Now() seems reasonable.

[leonnicolas]

We would only check the annotation if minRebootPeriod is configured to be larger than 0.

Right, if you suddenly turn on the feature you would get one "wrong" error log entry before the first reboot. After the first reboot with the --minRebootPeriod flag, a missing annotation would actually be a real error.

If we don't care so much about logging that error, I will totally agree to returning time.Now() if something unexpected happens.

[jackfrancis]

Right, if you suddenly turn on the feature you would get one "wrong" error log entry before the first reboot.

Could you clarify why this is so?

[leonnicolas]

You run cured without the --min-reboot-period flag. Kured will not annotate the node with the weave.works/kured-last-successful-reboot flag. Now you want to use the "new" feature, edit your manifest and roll out the new kured daemon set. Normally none of the pods will hold the lock from

kured/cmd/kured/main.go

Line 666 in d0bdc11

if holding(lock, &nodeMeta, concurrency > 1) {

. That means they won't add the weave.works/kured-last-successful-reboot annotation to its nodes. We wouldn't want that anyways because then restarting/rollilng out kured daemon set would falsify the last successful reboot time. Since the feature is enabled now, kured would check for the weave.works/kured-last-successful-reboot annotation, but fail since it is not there yet. So if we log an error when the annotation is missing, we would see one error log for each tick until the first reboot. That the annotation is missing is expected before the first reboot. However, after the first reboot the missing annotation would be an unexpected error.

Only after the first reboot with the --min-reboot-period flag, this feature would actually work.

[leonnicolas]

I am not sure what is right here. Should we never log an error on missing annotations to avoid confusion before the first reboot to the cost of silencing real errors after the first reboot? Or the other way around? (I added a commit that would log the errors, happy to change it though)

[ant31]

any blocker to have this merged?