shutdown-manager issues

[skriss]

xref. #3192
xref. #4322
xref. #4812

We've encountered a number of interrelated issues with the graceful Envoy shutdown workflow that I want to capture in one place.

The first issue goes something like the following:

1. Something triggers Kubernetes to terminate/restart the shutdown-manager sidecar container in an Envoy pod. We've seen definite evidence of its liveness probes failing due to timeouts, which could be caused by the container not having any resource requests and getting CPU-throttled.
2. When the shutdown-manager container is terminated, its preStop hook is triggered, which runs the contour envoy shutdown command. This command tells the associated Envoy to start draining all Listeners, including the Listener that is the target of the Envoy container's readiness probe. So now, Envoy is draining HTTP/S connections as well as reporting unready.
3. The shutdown-manager's preStop hook eventually returns once enough connections have been closed, and the shutdown-manager container restarts successfully.
4. The Envoy container is stuck indefinitely in a "draining" state, failing its readiness probe, but never restarting because there is no liveness probe or anything else to trigger a restart.

A few thoughts about this issue:

• adding resource requests to the shutdown-manager (and really, all containers) may help minimize the occurrence of this issue since they should ensure the container(s) get the CPU they need to avoid being restarted
• adding a liveness probe to the Envoy container may help avoid it getting permanently stuck by restarting it when it's not healthy, as long as its implementation plays nicely with the whole graceful shutdown workflow
• a restart of the shutdown-manager sidecar should probably not be triggering a drain of Envoy listeners. That should only be triggered by the Envoy container itself terminating/restarting.
• GHSA-mjp8-x484-pm3r needs to be kept in mind as we potentially modify this workflow

Secondly, #4322 describes issues with draining nodes due to the emptyDir that is used to allow the shutdown-manager and envoy containers to communicate (via UDS for the envoy admin interface, and by file to communicate when the Listener drain is done). I won't repeat that discussion here, just linking it for reference.

[skriss]

A few more thoughts:

• it seems like the best solution here would be to build our own Envoy image, that includes the Contour binary, and then use the contour envoy shutdown command as a preStop hook for the Envoy container (that would trigger Envoy to drain connections and wait until they fall below the threshold). This would eliminate the sidecar, the emptyDir volume, and any need for communicating across containers.
• However, I'm not 100% sure we're ready to move to building and publishing our own Envoy image yet. In the near term, I think adding (a) resource requests for the shutdown-manager (and probably envoy) containers, and (b) adding a liveness probe to the envoy container to prevent it from getting stuck in an unhealthy state indefinitely, should largely mitigate at least the first issue. It does not, however, do anything to address the second issue re: using emptyDir local storage.

cc @sunjayBhatia, interested in your thoughts here when you get a chance.

[sunjayBhatia]

yeah I agree that idea is the best solution for functionality, I don't think its too hard on the face of it to implement, but has some implications for downstream users that mean we would need very good communication in our release notes and upgrade documentation

stepping through what would need to happen to do that:

• building the Envoy image
  • we could at first build our own Envoy image using the existing upstream one as a base and just add the contour binary
  • leaves the option in the future of building Envoy with some custom extensions etc. if we ever wanted to go that way
  • also gives us an opportunity to switch to distributing a distroless based image so our default offering is less susceptible to CVE scans (can do this anyways regardless tbh)
  • however any downstream consumer would now need to do this as well if they want to properly drain connections, which maybe is worth a major version change?
• changes to Envoy DaemonSet/Deployment YAML
  • the changes themselves are straightforward but would again need coordination with consumers of this YAML to ensure they can accomodate this change and that it needs to be paired with a specially built Envoy image
  • could do the image change and YAML changed in stepped fashion to make it easier for upgrades?
• we could eventually get rid of the shutdown-manager code and associated listener etc. in Envoy's xDS config to handle that flow, which would be nice to have

we could also probably think of a nice gradual release process to this final state that is minimally disruptive, but of course that will take time, though interested users could probably do a jump-upgrade change if needed

[sunjayBhatia]

Yeah it seems the only other option is to add a liveness probe to the envoy container.

One other quick idea i just had around getting a pod to "recover" after the shutdown-manager is restarted would be to get the shutdown-manager to ensure on startup that Envoy was in the right state. This won't work because you can't recover an Envoy from "draining," once it goes into that state it won't allow Listener updates/modifications (see Attention block here: https://www.envoyproxy.io/docs/envoy/latest/operations/admin#post--drain_listeners).

We could also have the shutdown-manager on startup check if Envoy was draining (using https://www.envoyproxy.io/docs/envoy/latest/api-v3/admin/v3/server_info.proto#envoy-v3-api-enum-admin-v3-serverinfo-state) and have it wait for the drain to finish and itself make Envoy exit (https://www.envoyproxy.io/docs/envoy/latest/operations/admin#post--quitquitquit). But again this seems worse than using proper pod lifecycle hooks for this and doesn't solve the emptyDir issue.

[sunjayBhatia]

But tbh I don't know if we've tried to move to this, but this endpoint could be useful: https://www.envoyproxy.io/docs/envoy/latest/operations/admin#post--drain_listeners

(rather than set the healthcheck to fail and wait for existing connections to finish)

[sunjayBhatia]

Opened this: #4852

[skriss]

Few more notes so I don't forget:

• Even if we do move to a single container that contains both envoy and contour binaries, we're still using an emptyDir volume for coordination between the init container that writes out a bootstrap config and the main envoy container, so using emptyDir in the Envoy daemonset/deployment / draining concerns / connection failures #4322 would not be solved
• Another failure mode for when the shutdown-manager container restarts is that, while running its preStop hook, it will write out an ok file to the emptyDir shared between shutdown-manager and envoy. Now, the next time the envoy container terminates, the /shutdown handler will see that this ok file exists and immediately return, allowing envoy to terminate immediately instead of waiting for connections to drain.
• If we add a liveness probe to the envoy container to avoid getting stuck, we need to make sure it won't restart the container in less than 300 seconds (i.e. the terminationGracePeriodSeconds for the pod), because this is the amount of time that the graceful drain can take when things go normally. So the liveness probe would be restarting envoy after >5min stuck in draining if the pod doesn't terminate first. It's a pretty slow recovery, but at least better than nothing. Also need to keep in mind that there may not be an ok file if the shutdown-manager has not actually run, so need to ensure we don't hang there on the preStop hook (xref Why Envoy container doesn't have a liveliness probe by default?  #4812)

[skriss]

main...skriss:contour:exp-sdm-requests-and-probes removes the liveness probe from the shutdown-manager container, adds a liveness probe to the envoy container, and adds resource requests to both containers. This should mitigate the issue by resulting in fewer shutdown-manager restarts, and enabling envoy to recover if/when it does get stuck in a "draining" state.

main...skriss:contour:exp-sdm-in-envoy gets rid of the shutdown-manager sidecar, creates a single Docker image with both contour and envoy binaries, and runs the contour envoy shutdown command as a preStop hook in the consolidated envoy container. This approach avoids most of the issues related to coordinating between multiple containers in the pod. Note that, because we're still using an emptyDir volume for the bootstrap config, this does not entirely solve #4322, but does get us a step closer.