Skip to content

Conversation

@csrwng
Copy link
Contributor

@csrwng csrwng commented May 28, 2020

Backport of #370

@openshift-ci-robot openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium 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 28, 2020
@openshift-ci-robot
Copy link
Contributor

@csrwng: This pull request references Bugzilla bug 1841239, which is invalid:

  • expected dependent Bugzilla bug 1838872 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.

Details

In response to this:

Bug 1841239: [release-4.4] Avoid pre-creating clusteroperators that should be excluded

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.

@wking wking changed the title Bug 1841239: [release-4.4] Avoid pre-creating clusteroperators that should be excluded Bug 1841239: Avoid pre-creating clusteroperators that should be excluded May 28, 2020
@wking
Copy link
Member

wking commented May 28, 2020

/lgtm

Looks like a clean backport to me. Was there a reason you didn't comment with /cherrypick release-4.4 in #370 and let the bot handle the backport bug and PR for you?

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 28, 2020
@wking
Copy link
Member

wking commented May 28, 2020

/hold

Unifying with #348 per this comment makes sense to me.

@openshift-ci-robot 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 28, 2020
@csrwng
Copy link
Contributor Author

csrwng commented May 28, 2020

Looks like a clean backport to me. Was there a reason you didn't comment with /cherrypick release-4.4 in #370 and let the bot handle the backport bug and PR for you?

Premature optimization :) I thought the patch wouldn't apply cleanly.

Unifying with #348 per this comment makes sense to me.

agree

@jottofar
Copy link
Contributor

Unifying with #348 per this comment makes sense to me as well.

@csrwng csrwng force-pushed the fix_exclude_clusteroperators_44 branch from aadb13e to e4e771c Compare May 28, 2020 19:59
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 28, 2020
@csrwng
Copy link
Contributor Author

csrwng commented May 28, 2020

@wking @jottofar now included the #348 commit in this one (have no idea how to change a cherry-pick PR)

@csrwng
Copy link
Contributor Author

csrwng commented May 28, 2020

/test e2e-aws

@wking
Copy link
Member

wking commented May 28, 2020

Hrm, #348 never had a backing bug. Which probably means we need to grow the QE verification scope on this series' 4.5 bug so they confirm that functionality is working as expected too. Or are we just going to assume "create ClusterOperators early" works and not cover it in QE for 4.5 or 4.4?

Also, I'm not sure why the update job failed. Is there a list of failing vs. flaky/optional failures in the build log that I'm just missing?

@csrwng
Copy link
Contributor Author

csrwng commented May 28, 2020

Or are we just going to assume "create ClusterOperators early" works and not cover it in QE for 4.5 or 4.4?

I think the right thing to do is to make the bug about introducing the "create clusteroperators early" functionality in 4.4. The other part is just a corollary fix so that it works properly for ROKS as well. Not sure how that works as far as depencies go though. There was no BZ to introduce the function in 4.5, but I think the 4.4 bug requires a 4.5 bug to exist.

@csrwng
Copy link
Contributor Author

csrwng commented May 28, 2020

[It] [Top Level] [Feature:Platform] ClusterOperators should define at least one namespace in their lists of related objects [Suite:openshift/conformance/parallel]
  /go/src/github.com/openshift/origin/_output/local/go/src/github.com/openshift/origin/test/extended/operators/clusteroperators.go:54
May 28 22:12:28.385: INFO: Running AfterSuite actions on all nodes
May 28 22:12:28.385: INFO: Running AfterSuite actions on node 1
fail [github.com/openshift/origin/test/extended/operators/clusteroperators.go:57]: ClusterOperator: machine-config

Looks like this is missing a backport of openshift/machine-config-operator#1566

@sdodson
Copy link
Member

sdodson commented May 29, 2020

Just posted a quick note to bug 1841239, QE still has to verify the 4.5 bug anyway and I'm sure once they do that they'll understand everything that's going on in this PR and the linked bug.

@sdodson
Copy link
Member

sdodson commented May 29, 2020

Looks like this is missing a backport of openshift/machine-config-operator#1566

Hmm, starting to think we should call in @deads2k to round up all the follow up work that should come along with backporting the CVO pre-fill? This is just docs but it's linked from the original PR too #363

@deads2k
Copy link
Contributor

deads2k commented Jun 17, 2020

looks faithful. I don't feel strongly about a readme

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2020
@deads2k
Copy link
Contributor

deads2k commented Jun 17, 2020

/hold

hold for openshift/machine-config-operator#1764

@openshift-ci-robot openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jun 22, 2020
@openshift-ci-robot
Copy link
Contributor

@sdodson: This pull request references Bugzilla bug 1841239, which is valid.

6 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.4.z) matches configured target release for branch (4.4.z)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
  • dependent bug Bugzilla bug 1838872 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA))
  • dependent Bugzilla bug 1838872 targets the "4.5.0" release, which is one of the valid target releases: 4.5.0, 4.5.z
  • bug has dependents
Details

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.

@sdodson
Copy link
Member

sdodson commented Jun 22, 2020

We still have no feature-request bug to hang this backport on though, right? I don't think we should pull this back to 4.4 hung on a bug about how the initial 4.5 feature (which landed without a bug) was fixed for ROKS. 4.4 isn't broken at all on ROKS without this PR, and will still not be broken once we land this PR. Can we grow a new series about pre-created cluster-operators, so QE can verify that that is working for 4.5, and then hang this PR on a 4.4 backport of that one?

We've provided QE notes in the bug that I believe explain the situation sufficiently. If they have questions I expect them to ask.

@sdodson
Copy link
Member

sdodson commented Jun 28, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@wking
Copy link
Member

wking commented Jun 28, 2020

/hold

Building github.com/openshift/cluster-version-operator (v1.0.0-204-gc1c8d84d-dirty)
# github.com/openshift/cluster-version-operator/pkg/cvo
pkg/cvo/sync_worker.go:621:8: undefined: contextIsCancelled
pkg/cvo/sync_worker.go:622:15: cr.CancelError undefined (type *consistentReporter has no field or method CancelError)

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2020
smarterclayton and others added 2 commits June 29, 2020 13:52
The must-gather and insights operator depend on cluster operators
and related objects in order to identify resources to create. Because
cluster operators are delegated to the operator install and upgrade
failures of new operators can fail to gather the requisite info if
the cluster degrades before those steps.

Add a new selective Precreating install mode and do a single pass
over all cluster operators in the payload without retries at the beginning
of an initializing or upgrading sync pass to attempt to create the
ClusterOperators if they don't exist. If we succeed at creating the
object, try exactly once to update status so that relatedObjects can
be set.
@csrwng csrwng force-pushed the fix_exclude_clusteroperators_44 branch from e4e771c to 44b7cb9 Compare June 29, 2020 17:55
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2020
@csrwng
Copy link
Contributor Author

csrwng commented Jun 29, 2020

rebased, fixed

@csrwng
Copy link
Contributor Author

csrwng commented Jul 14, 2020

/test images

@sdodson
Copy link
Member

sdodson commented Jul 15, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2020
@deads2k
Copy link
Contributor

deads2k commented Aug 25, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, deads2k, wking

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:

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

@eparis
Copy link
Member

eparis commented Sep 10, 2020

/bugzilla refresh

@openshift-ci-robot
Copy link
Contributor

@eparis: This pull request references Bugzilla bug 1841239, which is valid.

6 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.4.z) matches configured target release for branch (4.4.z)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
  • dependent bug Bugzilla bug 1838872 is in the state CLOSED (ERRATA), which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA))
  • dependent Bugzilla bug 1838872 targets the "4.5.0" release, which is one of the valid target releases: 4.5.0, 4.5.z
  • bug has dependents
Details

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.

@eparis eparis added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Sep 10, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 10, 2020

@csrwng: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp 44b7cb9 link /test e2e-gcp

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 9515b23 into openshift:release-4.4 Sep 12, 2020
@openshift-ci-robot
Copy link
Contributor

@csrwng: All pull requests linked via external trackers have merged:

Bugzilla bug 1841239 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1841239: Avoid pre-creating clusteroperators that should be excluded

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants