Bug 1791022: Ensure removal of 4.4-only resources on downgrade to 4.3 #93

marun · 2020-01-14T17:13:40Z

This PR is in support of a 4.4 PR that unifies all service ca controllers into a single deployment to simplify maintenance and administration: #89

openshift-ci-robot · 2020-01-14T17:13:49Z

@marun: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

Ensure removal of 4.4-only resources on downgrade to 4.3

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-01-14T17:19:44Z

@marun: This pull request references Bugzilla bug 1791022, which is invalid:

expected the bug to target the "4.3.0" release, but it targets "---" instead
expected Bugzilla bug 1791022 to depend on a bug in one of the following states: MODIFIED, ON_QA, VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

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.

In response to this:

Bug 1791022: Ensure removal of 4.4-only resources on downgrade to 4.3

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.

marun · 2020-01-14T17:19:56Z

/bugzilla refresh

openshift-ci-robot · 2020-01-14T17:20:08Z

@marun: This pull request references Bugzilla bug 1791022, which is invalid:

expected the bug to target the "4.3.0" release, but it targets "---" instead
expected Bugzilla bug 1791022 to depend on a bug in one of the following states: MODIFIED, ON_QA, VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

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.

In response to this:

/bugzilla refresh

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.

marun · 2020-01-15T08:02:48Z

/bugzilla refresh

openshift-ci-robot · 2020-01-15T08:02:51Z

@marun: This pull request references Bugzilla bug 1791022, which is invalid:

expected the bug to target the "4.3.0" release, but it targets "4.3.z" instead
expected dependent Bugzilla bug 1791188 to be in one of the following states: MODIFIED, ON_QA, VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is POST 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.

In response to this:

/bugzilla refresh

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.

marun · 2020-01-15T08:07:08Z

/hold pending #89

marun · 2020-01-21T17:38:20Z

/bugzilla refresh

openshift-ci-robot · 2020-01-21T17:38:23Z

@marun: This pull request references Bugzilla bug 1791022, which is invalid:

expected the bug to target the "4.3.0" release, but it targets "4.3.z" 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.

In response to this:

/bugzilla refresh

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.

marun · 2020-01-27T22:55:44Z

/hold cancel
/bugzilla refresh

openshift-ci-robot · 2020-01-27T22:55:48Z

@marun: This pull request references Bugzilla bug 1791022, which is invalid:

expected dependent Bugzilla bug 1791188 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is ON_QA 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.

In response to this:

/hold cancel
/bugzilla refresh

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.

marun · 2020-02-17T17:02:05Z

/bugzilla refresh

openshift-ci-robot · 2020-02-17T17:02:10Z

@marun: This pull request references Bugzilla bug 1791022, which is valid.

In response to this:

/bugzilla refresh

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.

marun · 2020-02-17T17:02:19Z

/test all

marun · 2020-02-17T18:53:59Z

/retest

marun · 2020-02-19T04:58:46Z

@sttts @stlaz This PR would ideally merge in time for the next 4.3.z release.

sttts · 2020-02-19T12:33:54Z

pkg/operator/starter.go

+		if err != nil {
+			klog.Warningf("Failed to retrieve 4.4 deployment: %v", err)
+		}
+		err = deployClient.Delete(deployName, &metav1.DeleteOptions{})


why not delete directly, why a GET?

Shouldn't we stop the loop as soon as it succeeds?

I think the original code is still in place, did you add your changes to your latest commit?

Get is still there. But I don't care too much.

But I would prefer to see the loop stop when the deployment is gone.

I reverted the changes as per @deads2k. iirc his assertion is that continual reconciliation is required to avoid confusing users when behavior is inconsistent (removes a deployment on startup not subsequently).

As to why get, I thought it would be preferable to see 'get' in the log vs 'delete' if continually reconciling but maybe that's specious logic?

ok, fine with me.

Am still confused why the deployment is different from the other resources in cleanupUnifiedDeployment.

That's what I asked in #93 (comment). Those other resources don't have an operational impact for the operator, so deleting them isn't so critical, but the principle of consistent behavior suggests continual reconciliation for them too.

I guess I've answered my own question. Maybe reconcile on a longer timeline - 20m?

Fair enough. 20m is also fine, so is 1m.

sttts · 2020-02-20T08:52:33Z

pkg/operator/sync.go

+	//
+	// The 4.4 deployment does have an operational impact, and is continually monitored
+	// for removal via a goroutine started in RunOperator.
+	if err := once.Do(func() error { return cleanupUnifiedDeployment(c) }); err != nil {


why is there no loop around these like it is in the other case?

The enclosing function is the operator sync loop.

The combination with sync.Once confused me. Anyway, seems to be good enough.

marun · 2020-02-20T18:54:01Z

/hold

Need to revert the deployment cleanup to be continuous rather than once as per @deads2k

marun · 2020-02-21T05:46:32Z

/hold cancel

Updated to be in keeping with openshift/cluster-openshift-apiserver-operator#313, PTAL.

marun · 2020-02-21T05:50:31Z

pkg/operator/sync.go

@@ -14,6 +36,16 @@ func syncControllers(c serviceCAOperator, operatorConfig *operatorv1.ServiceCA)
 		return err
 	}

+	// Remove resources related to the 4.4 controller deployment at most once. These


@deads2k Is it acceptable to remove the resources related to the deployment at most once rather than continually, since the resources in question have no operational impact without the deployment (which is continually removed by the starter)?

I don't care too much. It's good enough to do it once.

marun · 2020-02-23T22:25:46Z

/retest

marun · 2020-02-25T04:01:04Z

/hold

marun · 2020-02-26T01:50:00Z

/hold cancel

Updated to attempt removal of 4.4 resources every 20 minutes.

sttts · 2020-02-26T09:03:34Z

/approve
/lgtm

openshift-ci-robot · 2020-02-26T09:04:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marun, sttts

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~~ [sttts]

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

shawn-hurley · 2020-03-06T13:25:05Z

/retest

openshift-ci-robot · 2020-03-06T15:12:30Z

@marun: All pull requests linked via external trackers have merged. Bugzilla bug 1791022 has been moved to the MODIFIED state.

In response to this:

Bug 1791022: Ensure removal of 4.4-only resources on downgrade to 4.3

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 added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 14, 2020

openshift-ci-robot requested review from deads2k and stlaz January 14, 2020 17:16

marun changed the title ~~Ensure removal of 4.4-only resources on downgrade to 4.3~~ Bug 1791022: Ensure removal of 4.4-only resources on downgrade to 4.3 Jan 14, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jan 14, 2020

marun force-pushed the cleanup-4.4-resources branch from 32939ed to f7323cb Compare January 15, 2020 07:18

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

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

marun force-pushed the cleanup-4.4-resources branch from f7323cb to 192ce3e Compare January 27, 2020 23:02

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 17, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 17, 2020

sttts reviewed Feb 19, 2020

View reviewed changes

marun force-pushed the cleanup-4.4-resources branch 2 times, most recently from ca636ac to 4a632a4 Compare February 20, 2020 06:34

marun force-pushed the cleanup-4.4-resources branch from 4a632a4 to b91b80d Compare February 20, 2020 06:38

sttts reviewed Feb 20, 2020

View reviewed changes

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

marun force-pushed the cleanup-4.4-resources branch from b91b80d to b2e457c Compare February 21, 2020 05:45

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 21, 2020

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

marun force-pushed the cleanup-4.4-resources branch from b2e457c to 5cdc9d2 Compare February 21, 2020 05:48

marun commented Feb 21, 2020

View reviewed changes

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

marun force-pushed the cleanup-4.4-resources branch from 5cdc9d2 to 766a703 Compare February 26, 2020 01:48

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 26, 2020

Ensure removal of 4.4-only resources on downgrade to 4.3

8afc576

marun force-pushed the cleanup-4.4-resources branch from 766a703 to 8afc576 Compare February 26, 2020 01:49

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

openshift-ci-robot assigned sttts Feb 26, 2020

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

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

shawn-hurley added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 6, 2020

openshift-merge-robot merged commit dd04bc4 into openshift:release-4.3 Mar 6, 2020

Bug 1791022: Ensure removal of 4.4-only resources on downgrade to 4.3 #93

Bug 1791022: Ensure removal of 4.4-only resources on downgrade to 4.3 #93

Conversation

marun commented Jan 14, 2020

openshift-ci-robot commented Jan 14, 2020

openshift-ci-robot commented Jan 14, 2020

marun commented Jan 14, 2020

openshift-ci-robot commented Jan 14, 2020

marun commented Jan 15, 2020

openshift-ci-robot commented Jan 15, 2020

marun commented Jan 15, 2020

marun commented Jan 21, 2020

openshift-ci-robot commented Jan 21, 2020

marun commented Jan 27, 2020

openshift-ci-robot commented Jan 27, 2020

marun commented Feb 17, 2020

openshift-ci-robot commented Feb 17, 2020

marun commented Feb 17, 2020

marun commented Feb 17, 2020

marun commented Feb 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun Feb 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun commented Feb 20, 2020

marun commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun commented Feb 23, 2020

marun commented Feb 25, 2020

marun commented Feb 26, 2020

sttts commented Feb 26, 2020

openshift-ci-robot commented Feb 26, 2020

shawn-hurley commented Mar 6, 2020

openshift-ci-robot commented Mar 6, 2020

marun Feb 20, 2020 •

edited