Bug 1895360: pkg/daemon: don't delete a file if its replaced with a dropin by vrutkovs · Pull Request #2196 · openshift/machine-config-operator

vrutkovs · 2020-11-03T11:30:57Z

Some systemd service settings may be defined in .Storage.Files and in .Systemd.Units.Dropins. Dropins is preferable, so users may want to migrate to the way.

However if a file representing a dropin was defined in .Storage.Files and converted into .Systemd.Units.Dropin in new version of MC, it would be placed by Ignition's dropin implementation and then garbage-collected by MCD's deleteStaleData.

This change ensures we're checking Systemd.Units.Dropins before attempting to remove this file. This is crucial for OKD 4.5 -> 4.6 upgrades - kubelet MCO config was defined as a file in 4.5 and converted into a dropin in 4.6

kikisdeliveryservice · 2020-11-03T18:12:22Z

hey @vrutkovs this new func seems like it should have a test, can you add one?

vrutkovs · 2020-11-03T19:19:58Z

Sure, will add a unit test (verifying that it fixes OKD upgrade first). Not sure how to approach unittesting here - do you know any test which checks deleteStaleFiles?

vrutkovs · 2020-11-05T15:23:52Z

/retest

vrutkovs · 2020-11-06T10:36:32Z

/cc @kikisdeliveryservice

Added a unit test, verified that OKD 4.5 can be upgraded using MCO with this patch

vrutkovs · 2020-11-06T12:54:51Z

Flakes

/retest

openshift-ci-robot · 2020-11-06T13:23:52Z

@vrutkovs: This pull request references Bugzilla bug 1895360, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1895360: pkg/daemon: don't delete a file if its replaced with a dropin

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vrutkovs · 2020-11-06T13:24:08Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2020-11-06T13:24:09Z

@vrutkovs: once the present PR merges, I will cherry-pick it on top of release-4.6 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vrutkovs · 2020-11-08T13:06:04Z

/skip

e2e-gcp-op is permafailing

vrutkovs · 2020-11-08T13:09:51Z

/skip

yuqi-zhang

I think this patch should fix the bug in question, however this doesn't seem very bulletproof. Consider the following scenarios:

systemd units in the unit section would have this problem as well right? if a systemd unit that was originally being written as a file gets replaced by a unit section, we should also correctly handle that.
the opposite case is ironically not handled as well, where a dropin that was replaced by a file would also not work (why would you ever do this though). I guess in general anything cross-dependency between unit and file sections is not handled well.
if there was a dropin originally on the system (call it version A). which the MCD made a backup of when you wrote the file via a MC (call it version B), and then you try to replace with a dropin (call it version C), it looks like the logic will end up writing version A to the file instead of version C like we want, since we perform a backup and then a no-op. A rare edge case but maybe we should account for it anyways for correctness.

Those 3 scenarios are just some that came to mind (my assessment might not be entirely correct here). I am not opposed to merging this fix as is but we should consider the interactions between files and unit sections a bit deeper if that's the route we take. I'd be ok to merge this as is as well (maybe fix scenario 3?) provided we don't regress any behaviour

yuqi-zhang · 2020-11-09T19:52:52Z

the diff looks somewhat confusing, but this line should be the essence of the change right?

Right. I had to rework the whole function as gosec was complaining about nested ifs in e79a186 (#2196)

the way github is showing diff is indeed confusing

openshift-bot · 2020-11-13T14:33:42Z

/retest