Bug 1764001: pkg/daemon: rollback dropins and units #1715

runcom · 2020-05-06T09:02:35Z

Restarted from #1203 and that's why we have the dropins rename as well (which is something we should do anyway as we ship those).

Backup and restore (when needed) units and dropins

Also, we need #1716

Signed-off-by: Antonio Murdaca [email protected]

openshift-ci-robot · 2020-05-06T09:02:49Z

@runcom: This pull request references Bugzilla bug 1764116, which is invalid:

expected the bug to be open, but it isn't
expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is CLOSED (DUPLICATE) instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1764116: pkg/daemon: rollback dropins and units

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.

openshift-ci-robot · 2020-05-06T09:03:41Z

@runcom: This pull request references Bugzilla bug 1764001, 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.5.0) matches configured target release for branch (4.5.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1764001: pkg/daemon: rollback dropins and units

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.

runcom · 2020-05-06T09:04:02Z

cc @yuqi-zhang

openshift-ci-robot · 2020-05-06T09:20:38Z

@runcom: This pull request references Bugzilla bug 1764001, which is valid.

3 validation(s) were run on this bug

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

Details

In response to this:

Bug 1764001: pkg/daemon: rollback dropins and units

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.

templates/common/_base/units/etc-systemd-system-crio.service.d-10-default-env.conf.yaml

runcom · 2020-05-06T12:18:26Z

something funny is happening, dropins files seem like they don't get written when empty - causing the cluster to fail. I'm looking into this.

/hold

runcom · 2020-05-06T14:11:20Z

/skip

runcom · 2020-05-06T14:18:22Z

something funny is happening, dropins files seem like they don't get written when empty - causing the cluster to fail. I'm looking into this.

this looks like the case, really funny.

runcom · 2020-05-06T15:09:31Z

Tested the proxy functionality with this (since this is touching that area) and I can enable proxy day2 just fine 👍

yuqi-zhang

There is an awkward case that arises because we didn't do backups before, so its similar to the files case where we were backing up wrongly.

pkg/daemon/update.go

runcom · 2020-05-21T15:32:07Z

/skip

This will make sure we don't ship files that are dropins. If we do that (like we're doing), someone could ship a dropin for the same service/same droping name and the validation will fail as the "file" is validate first. See https://bugzilla.redhat.com/show_bug.cgi?id=1764001 Signed-off-by: Antonio Murdaca <[email protected]>

Backup and restore (when needed) units and dropins Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2020-05-24T15:57:28Z

/refresh

runcom · 2020-05-24T15:57:40Z

/retest

runcom · 2020-05-24T17:37:26Z

/retest

runcom · 2020-05-25T12:46:12Z

/retest

yuqi-zhang

Ok, I think we've covered the cases we care about, thanks for fixing it up!

Now one thing to note is that if a systemd unit was overwritten before this patch, we will not restore that file even with this patch in place. This is not a regressive behaviour but is something we can fix if we really wanted to (I don't mind either way). If you are ok with the way it is I can give it the lgtm.

runcom · 2020-05-26T08:29:00Z

Now one thing to note is that if a systemd unit was overwritten before this patch, we will not restore that file even with this patch in place. This is not a regressive behaviour but is something we can fix if we really wanted to (I don't mind either way). If you are ok with the way it is I can give it the lgtm.

@yuqi-zhang right, let's follow up on that as it's not a regression unless this patch makes it harder to fix that in a follow up even between releases

ashcrow · 2020-05-26T13:31:54Z

pkg/daemon/update.go

 		for j := range u.Dropins {
 			path := filepath.Join(pathSystemd, u.Name+".d", u.Dropins[j].Name)
 			if _, ok := newDropinSet[path]; !ok {
+				if _, err := os.Stat(noOrigFileStampName(path)); err == nil {


nit: Done more than once. Consider making a function in the future.

yuqi-zhang · 2020-05-26T17:11:01Z

let's follow up on that as it's not a regression unless this patch makes it harder to fix that in a follow up even between releases

Yes, I think this is fine.

/lgtm

openshift-ci-robot · 2020-05-26T17:11:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, runcom, yuqi-zhang

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ashcrow,runcom,yuqi-zhang]

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

ashcrow · 2020-05-26T17:51:11Z

/test e2e-gcp-upgrade

openshift-ci-robot · 2020-05-26T19:48:11Z

@runcom: All pull requests linked via external trackers have merged: openshift/machine-config-operator#1715. Bugzilla bug 1764001 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1764001: pkg/daemon: rollback dropins and units

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.

runcom requested a review from yuqi-zhang May 6, 2020 09:02

openshift-ci-robot added bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels May 6, 2020

runcom mentioned this pull request May 6, 2020

Bug 1764116: templates: rename our dropins to include the mco string #1203

Closed

runcom changed the title ~~Bug 1764116: pkg/daemon: rollback dropins and units~~ Bug 1764001: pkg/daemon: rollback dropins and units May 6, 2020

openshift-ci-robot requested review from ashcrow and ericavonb May 6, 2020 09:04

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2020

runcom force-pushed the backuprestore-systemd branch 2 times, most recently from 49d9ec8 to 32eaf39 Compare May 6, 2020 09:23

runcom mentioned this pull request May 6, 2020

Bug 1764001: templates: move proxy dropins to dropins #1716

Closed

runcom commented May 6, 2020

View reviewed changes

templates/common/_base/units/etc-systemd-system-crio.service.d-10-default-env.conf.yaml Outdated Show resolved Hide resolved

runcom force-pushed the backuprestore-systemd branch 4 times, most recently from e6c85dc to 7816742 Compare May 6, 2020 10:47

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2020

runcom force-pushed the backuprestore-systemd branch from 03f30e6 to 0f9f0f5 Compare May 6, 2020 14:39

runcom removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2020

yuqi-zhang requested changes May 6, 2020

View reviewed changes

pkg/daemon/update.go Outdated Show resolved Hide resolved

runcom force-pushed the backuprestore-systemd branch from 0f9f0f5 to ce70da3 Compare May 21, 2020 08:22

yuqi-zhang reviewed May 21, 2020

View reviewed changes

pkg/daemon/update.go Outdated Show resolved Hide resolved

runcom force-pushed the backuprestore-systemd branch from ce70da3 to d2dc8ea Compare May 24, 2020 14:02

runcom added 2 commits May 24, 2020 16:11

pkg/daemon: rollback dropins and units

a0f0a4a

Backup and restore (when needed) units and dropins Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the backuprestore-systemd branch from d2dc8ea to a0f0a4a Compare May 24, 2020 14:12

yuqi-zhang approved these changes May 25, 2020

View reviewed changes

ashcrow reviewed May 26, 2020

View reviewed changes

ashcrow approved these changes May 26, 2020

View reviewed changes

openshift-ci-robot assigned yuqi-zhang May 26, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 26, 2020

openshift-merge-robot merged commit 4bbc400 into openshift:master May 26, 2020

runcom deleted the backuprestore-systemd branch May 26, 2020 20:33

Bug 1764001: pkg/daemon: rollback dropins and units #1715

Bug 1764001: pkg/daemon: rollback dropins and units #1715

Uh oh!

Conversation

runcom commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 6, 2020

Uh oh!

openshift-ci-robot commented May 6, 2020

Uh oh!

runcom commented May 6, 2020

Uh oh!

openshift-ci-robot commented May 6, 2020

Uh oh!

Uh oh!

runcom commented May 6, 2020

Uh oh!

runcom commented May 6, 2020

Uh oh!

runcom commented May 6, 2020

Uh oh!

runcom commented May 6, 2020

Uh oh!

yuqi-zhang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

runcom commented May 21, 2020

Uh oh!

runcom commented May 24, 2020

Uh oh!

runcom commented May 24, 2020

Uh oh!

runcom commented May 24, 2020

Uh oh!

runcom commented May 25, 2020

Uh oh!

yuqi-zhang left a comment

Choose a reason for hiding this comment

Uh oh!

runcom commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashcrow May 26, 2020

Choose a reason for hiding this comment

Uh oh!

yuqi-zhang commented May 26, 2020

Uh oh!

openshift-ci-robot commented May 26, 2020

Uh oh!

ashcrow commented May 26, 2020

Uh oh!

openshift-ci-robot commented May 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

runcom commented May 6, 2020 •

edited

Loading

runcom commented May 26, 2020 •

edited

Loading