Restart console container when config changes

[spadgett]

Opening this to get feedback. We have a problem where there is no good way to rollout the console after editing the console config in its config map. Right now we have to tell users to delete the console pods, which is error prone and not friendly. The console only reads config at startup and doesn't watch for changes.

This adds a liveness probe that detects if the config has changed on the filesystem using an md5 hash. If the config changes, the liveness probe fails, and the container restarts. It'a similar to what @aweiteka has done for prometheus config changes. It's a bit of a hack, but it works.

@sdodson This would simplify the install because we'd no longer need to force a console rollout on config changes from the metrics and logging playbooks.

Any objections to this approach?

/assign @jwforres
/cc @smarterclayton @derekwaynecarr @deads2k
/hold

Holding for feedback :)

@jupierce fyi

[deads2k]

@derekwaynecarr @sjenning If there was a way to directly restart a pod, we could write a controller that restarted pods that opt-in via an annotation. Restarting them based on whether the content of a secret or configmap they mounted changed. The logic is pretty simple.

The same API endpoint could be used to drive restarts for static pods. What are my chances of getting such an API?

[deads2k]

You probably want some kind of a jitter or randomness to avoid killing all your pods at once, right?

[spadgett]

Just a quick note that this is really meant to be a stop-gap solution until we have something better.

Also it looks like the md5sum command is not expensive at all.

sh-4.2$ time md5sum /var/webconsole-config/webconsole-config.yaml
0c2f2cc813a6c755515dcf46b2d8e722  /var/webconsole-config/webconsole-config.yaml

real    0m0.001s
user    0m0.000s
sys     0m0.000s

[spadgett]

You probably want some kind of a jitter or randomness to avoid killing all your pods at once, right?

Yeah. I thought there might be some jitter built-in since the liveness probes and config map updates won't happen at the same time for all pods, but maybe that's not the case.

[sdodson]

You probably want some kind of a jitter or randomness to avoid killing all your pods at once, right?

Don't liveness probes already have a jitter?

[sjenning]

Why is a rollout not desirable?

That is the official way to restart the pods in a deploymentconfig and respects all the policies we have in place about how many pods can be down at once. I agree that it is overkill, but if the application is not going to inotify watch the configmap and reload on its own, then this is other official way to pick up the change.

There really is no API to the kubelet to request a pod restart. The kubelet itself has internal mechanism for doing this, such as when liveness/readiness probes fail, but there is no way for external controllers to request this. Said another way, there is no property of the pod spec that would indicate that intention, like setting the deletionTimestamp indicates the kubelet should kill the pod.

Failing the liveness probe when the configmap changes is a clever (ab)use of the mechanism, I must say :P

[spadgett]

Why is a rollout not desirable?

I wish we could rollout. The problem is this is an k8s deployment, not a deployment config. There's no command to rollout a deployment again if the pod spec hasn't changed. So it'e either:

1. Delete the pods, or
2. Add some annotation to the deployment pod spec to trigger a rollout

It doesn't feel great to ask users to do either of those things after editing console config.

Failing the liveness probe when the configmap changes is a clever (ab)use of the mechanism, I must say :P

Credit to @aweiteka :)

[sjenning]

I wish we could rollout. The problem is this is an k8s deployment, not a deployment config.

Any reason not to use a DeploymentConfig instead of a Deployment?

[spadgett]

Any reason not to use a DeploymentConfig instead of a Deployment?

To avoid needing to migrate to a Deployment later on.

[spadgett]

@smarterclayton Any concerns with this change?

We'd like to go ahead with it unless anyone objects.

[smarterclayton]

Do it

[aweiteka]

Similar pattern here: #18391

[spadgett]

/hold cancel

@jwforres PTAL

[jwforres]

/lgtm

but I dont think i have approver rights here

[jwforres]

@deads2k or @smarterclayton would you mind approving

[deads2k]

/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, jwforres, spadgett

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• OWNERS [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[spadgett]

/retest

[spadgett]

/retest

[sdodson]

/test gcp

[spadgett]

/retest

[spadgett]

flake #18136

/test extended_conformance_install

[openshift-merge-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[openshift-merge-robot]

Automatic merge from submit-queue.