Will adapt to #1571

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yuqi-zhang

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

The pull request process is described here

/retest e2e-aws

/retest

Rebased onto Antonio's PR to bump drain to using upstream libraries.

So after prodding a bit more I've updated my assessment to resolving #1443 as follows:

1. Do nothing (close this fix)

Pros: we seem to rarely be in a situation where the MCD deadlocks as it stands. In the case of applying a machineconfig causing a failure to update files/units as described in #1443 , we could fix by:

1. manually deleting the bad machineconfig
2. oc edit node/node-in-scheduling-disabled
3. edit desiredconfig back to currentconfig
4. oc adm uncordon node/node-in-scheduling-disabled

Cons: I'd consider this a bug since for other issues (e.g. a bad ignition section) we are able to unstick the node by deleting a bad machineconfig and have it auto recover. The manual workaround, although easy to apply, is not immediately intuitive to users. And not fixing a bug feels bad.

2. Merge this fix as is (I would say this is more of a patch than a long term solution, see 4.)

Pros: Since we uncordon upon failure, all new MC failures pre-reboots would be consistent (the node would not stay schedulingdisabled). This allows the node annotations to still be written and if the bad MC is deleted, we would "auto recover".

Cons: we will now flip between Ready and Ready,SchedulingDisabled, which means we will drain the node, mark it unschedulable, fail, mark it schedulable again, every ~30 seconds or so on a default cluster. This seems operation heavy and could cause a lot of short lived pods

3. Rework controller logic to allow writing desired config to unavailable nodes, instead of nothing at all: https://github.com/openshift/machine-config-operator/blob/master/pkg/controller/node/node_controller.go#L796

Pros: would achieve the same result as 2, without the flipping back and forth between cordoned and uncordoned.

Cons: This could potentially open other problems. One example: let's say the node fails after the reboot and not during file/unit updates before the reboot, as highlighted above. We should retain what config the node attempted to update to. If something else gets applied in the meantime, the node controller would update the node annotation, but the daemon running on the node would never see this request and thereby cause a skew. Thus it will not attempt to fix itself, but also potentially confuse a user to what happened and which config failed.

4. Fundamentally rework the update process, either by making it transactional (update the files in an rpm-ostree prepared root instead of the current running one) or otherwise reworking how we progress updates.

Pros: cleaner operation in general

Cons: probably going to take awhile to implement

1. Do nothing (close this fix)

Pros: we seem to rarely be in a situation where the MCD deadlocks as it stands.

Is this the case? What do we define as rare? Could we get some data on this to help us decide?

In the case of applying a machineconfig causing a failure to update files/units as described in #1443 , we could fix by:

1. manually deleting the bad machineconfig

Deleting MCs can cause an issue if any node somewhere has it set as current/pending, including in the nodeannotations on disk for bootstrapping. I’ll need to think through the exact situations. It might be mostly ok because the current/pending MC are saved on disk in 4.2+ and the MCD uses that. There are still some edge cases though where those got corrupted or don’t match. I’ll need to think through the exact situations more.

2. oc edit node/node-in-scheduling-disabled

3. edit desiredconfig back to currentconfig

4. oc adm uncordon node/node-in-scheduling-disabled

Cons: I'd consider this a bug since for other issues (e.g. a bad ignition section) we are able to unstick the node by deleting a bad machineconfig and have it auto recover. The manual workaround, although easy to apply, is not immediately intuitive to users. And not fixing a bug feels bad.



2. Merge this fix as is (I would say this is more of a patch than a long term solution, see 4.)



Pros: Since we uncordon upon failure, all new MC failures pre-reboots would be consistent (the node would not stay schedulingdisabled). This allows the node annotations to still be written and if the bad MC is deleted, we would "auto recover".

I’m worried about kubelet/crio/api-server skew issues from partial updates.

Cons: we will now flip between Ready and Ready,SchedulingDisabled, which means we will drain the node, mark it unschedulable, fail, mark it schedulable again, every ~30 seconds or so on a default cluster. This seems operation heavy and could cause a lot of short lived pods

Yeah I think this wouldn’t fly. Would at the least cause a lot of noise for people debugging their clusters.

3. Rework controller logic to allow writing desired config to unavailable nodes, instead of nothing at all: https://github.com/openshift/machine-config-operator/blob/master/pkg/controller/node/node_controller.go#L796

This one makes sense to me.

Pros: would achieve the same result as 2, without the flipping back and forth between cordoned and uncordoned.

Cons: This could potentially open other problems. One example: let's say the node fails after the reboot and not during file/unit updates before the reboot, as highlighted above. We should retain what config the node attempted to update to.

What about if we keep the pending annotation for that information?

If something else gets applied in the meantime, the node controller would update the node annotation, but the daemon running on the node would never see this request and thereby cause a skew.

I’m missing something. Why would the daemon never see the request?

Thus it will not attempt to fix itself, but also potentially confuse a user to what happened and which config failed.

4. Fundamentally rework the update process, either by making it transactional (update the files in an rpm-ostree prepared root instead of the current running one) or otherwise reworking how we progress updates.

Pros: cleaner operation in general

Cons: probably going to take awhile to implement

I’m in favor of this one as much as possible. We’re never going to get around all the edge cases with the current approach. Doing it right will save us time in the long run.

@cgwalters does it seem like the right time to tackle this?

Is this the case? What do we define as rare? Could we get some data on this to help us decide?

I don't recall any issues like this personally. The only deadlock'ed situation that was similar was reported by Crawford since he did some manual file editing. And that wasn't reproduced so we closed it.

Deleting MCs can cause an issue if any node somewhere has it set as current/pending, including in the nodeannotations on disk for bootstrapping. I’ll need to think through the exact situations. It might be mostly ok because the current/pending MC are saved on disk in 4.2+ and the MCD uses that. There are still some edge cases though where those got corrupted or don’t match. I’ll need to think through the exact situations more.

to clarify, I don't mean deleting a rendered-mc-xxx, just a regular MC, which we support anyways. I don't think I've seen a scenario where we drop into some weird state but I can see it happening if we somehow trigger a race between an annotation write and a node going schedulingdisabled? Anyhow, out of the context of this PR.

I’m worried about kubelet/crio/api-server skew issues from partial updates.

We shouldn't have any unless we fail the rollback in which case we are in deep trouble anyways. This PR doesn't affect that path since its LIFO deferred and runs last.

Yeah I think this wouldn’t fly. Would at the least cause a lot of noise for people debugging their clusters.

Agreed

I’m missing something. Why would the daemon never see the request?

Hm now thinking about it I think it wouldn't really be a huge issue. What I was thinking was something like:

So if we change controller behaviour, we are basically saying, if a node is schedulingdisabled, you can still write the desiredConfig. Lets say you have nodes on rendered-1. If you apply a MC that is correct, but caused a failure during reboot (say, in the initramfs), it generates a new renderedconfig rendered-2, targets maxUnavailable nodes, and applies the changes and reboots. Lets say during this time you realized something went wrong, did an oc delete bad-mc and applied a new mc that doesn't have the problem, which generates rendered-3. The controller sees that there is 1 unavailable node, and targets that one's desiredConfig to rendered-3, since we allow that now, whereas before it would still be on 2

You then realize that since the node died in the initramfs the cluster is still broken, so you open a BZ with a must-gather. Since we lost the node entirely we no longer have the MCD and logs on it, so we can see in the MCP/node annotation the desiredConfig is rendered-3, which is not what borked the cluster. That said we can probably see in the MCP what was the original operation that failed it?

@yuqi-zhang: The following tests failed, say /retest to rerun all failed tests:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Closing this for now as it is not a high priority fix. Will revisit later with better approaches