daemon: Remove rollback deployment #2220

cgwalters · 2020-11-12T16:20:00Z

Came out of concerns from compliance about having bootloader
entries that may not have expected parameters.

JAORMX · 2020-11-12T16:31:21Z

pkg/daemon/update.go

+/// takes up space, and we don't generally expect administrators to
+/// use this versus e.g. removing broken configuration.  We only
+/// remove the rollback once the MCD pod has landed on a node, so
+/// we know kubelet is working.


oops, gotta replace /// with //

Context switching between Rust and Go is hard 😄

sinnykumari · 2020-11-12T16:40:22Z

I am not sure how relevant it is, but I find it useful to have previous deployment. When things goes wrong, it is quick to look at rpm-ostree status output from MCD log which is very easy to read the difference between booted and rollback deployment. Since this PR deletes the rollback deployment, I believe we won't see it now.

sinnykumari · 2020-11-12T16:42:18Z

pkg/daemon/update.go

+/// remove the rollback once the MCD pod has landed on a node, so
+/// we know kubelet is working.
+func (dn *Daemon) removeRollback() error {
+	_, err := runGetOut("rpm-ostree", "cleanup", "-r")


haven't checked myself, does rpm-ostree return success if there is no rollback deployment available to delete?

Yep, it's a no-op if not present.

Came out of concerns from compliance about having bootloader entries that may not have expected parameters.

cgwalters · 2020-11-12T16:53:54Z

I am not sure how relevant it is, but I find it useful to have previous deployment. When things goes wrong, it is quick to look at rpm-ostree status output from MCD log which is very easy to read the difference between booted and rollback deployment. Since this PR deletes the rollback deployment, I believe we won't see it now.

Agree with that concern! I think it's addressed though because today we call dn.LogSystemData() very early on which will add the previous deployment information into the pod logs before we prune the rollback.

cgwalters · 2020-11-12T16:56:35Z

Another reason to do this is that until we implement #1190 rolling back will make the MCD go degraded and require the force flag to fix, so it's rarely going to be useful except as a last ditch recovery on bare metal if kubelet fails to come up.

sinnykumari · 2020-11-12T17:00:55Z

Agree with that concern! I think it's addressed though because today we call dn.LogSystemData() very early on which will add the previous deployment information into the pod logs before we prune the rollback.

just checked the code, you are right!

sinnykumari · 2020-11-12T17:14:02Z

Another reason to do this is that until we implement #1190 rolling back will make the MCD go degraded and require the force flag to fix, so it's rarely going to be useful except as a last ditch recovery on bare metal if kubelet fails to come up.

agree.

Before adding lgtm, will give some time to see if anyone in the team has came across case where user have used rpm-ostree rollback feature to recover a cluster node.

sinnykumari · 2020-11-16T11:19:57Z

/lgtm

openshift-ci-robot · 2020-11-16T11:20:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, JAORMX, sinnykumari

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,sinnykumari]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-11-16T12:58:21Z

/retest