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?

[github-actions]

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

[ant31]

How can I help?

[ckotzbauer]

@jackfrancis Can you maybe have a look again please?

[evrardjp]

I read the comments here above and the code.
Here are my $0.02:

• If not allowed to reboot because the last reboot was too recent, there is no reason to go into the rebootAsRequired function. The refactoring with the blockers is made for that. The minimal reboot should therefore be a blocker instead. (You should have a blocker right away tell you that it doesn't work, before going further).
• Assuming the deployers picks to annotate the nodes, I fail to see why anyone would consider your new annotation (of the last reboot) irrelevant. Hence, I believe that information could be exposed, in all cases. Yes, it has a large impact on large clusters, but I don't see why it couldn't be covered in a release note. On top of that, it could be exposed for operational needs (prom monitoring).
• If we consider that the annotation will always be present, it's okay to seed it on the run of kured. I wouldn't do it every tick though. But the function can be used at first run if the annotation does not exist yet, and the value of the annotation gets rewritten when you see fit (to be clarified for me, I don't know if this needs to recorded post reboot, or pre-reboot ). To seed it, you have two choices. You could check and parse the content of /proc/uptime (permissions get in the way here) or set it to a "reasonable" value (timestamp of the "now" for example). The later case is not representing "real" uptime ("fake" uptime) until the first successful reboot campaign, while the first approach represents real uptime. Amongst the two, I prefer the "fake" uptime: the code will be easier to maintain (comments would go a long way here) and less complex.
• With that in mind, IMO a log at the maximum level of "warning" (info is fine) should be thrown on the first run of the daemonset ("Migrating to new kured version, adding annotation xxx"), and that's it. No errors should be thrown.
• Functionally, a "fake" uptime in the first cycle is not really hurting in the long run.

There are 4 cases I see (tell me if I am wrong):
A) min-reboot-period small and you want your first campaign of reboots happening soon
B) min-reboot-periiod large and you want your first campaign of reboots happening soon
C) min-reboot-period small but you don't want your first campaign of reboots happening soon (you rebooted recently)
D) min-reboot-period large and you don't want your first campaign of reboots happening soon

Case A: Everything is good, setting a fake value on the first kured boot is fine (value is small).
Case B: This is easily fixed by overriding the node annotation's value manually once.
Case C: Clever setting of the annotation value would work too. Equals to "Don't start before"
Case D: Everything is good.

Did I miss something?

[github-actions]

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).