checkpointer: make grace period a flag.

[diegs]

This allows users to configure the checkpoint removal grace period
parameter as a flag. This may be useful for tests, among other things.

[diegs]

coreosbot run e2e checkpointer

[squat]

LGTM

[xiang90]

among other things.

what are other things?

I do not feel a flag should be added solely for testing. it is better to create a unit test if we want to get rid of the dependencies of timing.

[diegs]

@xiang90 it's a tunable parameter. I don't know what the optimal value is. Adding a flag helps for experimentation.

[xiang90]

ithat -> that

[xiang90]

it's a tunable parameter. I don't know what the optimal value is. Adding a flag helps for experimentation.

To be honest, I would rather have less tunable parameters, and good default values. Also it is not clear to me that why do we have this gracePeriod at the first place by reading the code. Maybe it is for dealing with a delete then create case, so that the deleted pod will still run for a while until the created one comes up?

[diegs]

@xiang90 #755 is the PR where it is added. You can see the linked issues for why. This was required for us to ship 1.8. Self-hosted did not work without the change.

[xiang90]

If I understand it correctly setting graceperiod depends on how fast the kubelet will start the replaced pod after the API server changes the state of the pod. What would be the side effect to set it long enough for 99% of the cases, say 5 minutes instead of 1 minutes for real world cases? And what is the plan to do the experiments you mentioned? What I want to avoid is to add a flag, and either someone misuses it or no one uses it.

[aaronlevy]

In the future I might expect there to be more of these types of configurables (like don't start checkpoint if it's older than X - to protect against stale nodes rejoining with really old workloads), or even control these types of things per-pod via annotations.

So to move this forward, @xiang90 would opening an issue to discuss experimenting with the default grace period, and maybe adding a note on the flag that it's alpha / could be removed satisfy your concerns?

[xiang90]

So to move this forward, @xiang90 would opening an issue to discuss experimenting with the default grace period, and maybe adding a note on the flag that it's alpha / could be removed satisfy your concerns?

will do.

[xiang90]

and maybe adding a note on the flag that it's alpha / could be removed satisfy your concerns?

that would be great.

[aaronlevy]

Thinking about this a bit more, changing a flag from --alpha-foo to --foo is just as much friction to us and users as eventually deprecating foo if we move to something like annotations (or decide that there is one true grace period).

So for now, let's just merge this as is - I'm not concerned about this as a configurable for now.

Feel free to add some info into the flag description if you like, but won't block on it

[aaronlevy]

lgtm

[xiang90]

@aaronlevy Shall we get this merged? I will go ahead and create an issue for evaluating the default timeout and potentially remove the flag once we have high confidence after this PR gets merged.

Thanks.