Skip to content

Conversation

@deads2k
Copy link
Contributor

@deads2k deads2k commented Nov 22, 2021

rebase and push of #546

It still needs an auto-shutdown and probably (haven't yet looked) a few tests.

Implements openshift/enhancements#957

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2021
@openshift-ci openshift-ci bot requested review from jottofar and wking November 22, 2021 20:17
@deads2k deads2k changed the title [wip] Exclude featuregate.release.openshift/tech-preview=true manifests Exclude featuregate.release.openshift/tech-preview=true manifests Nov 23, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2021
@JoelSpeed
Copy link
Contributor

Have reviewed, have nothing to add, LGTM. Won't add the label as there are some linter errors that may want to be addressed

@alexander-demicev
Copy link

thanks!

@LalatenduMohanty
Copy link
Member

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2021
This allows consistency between invocations so that the value never
hanges after the operator instance is created.  Since the binary will
shutdown on featuregate value change, this simplifies the flow.
Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Good stuff; thanks :)

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2021
changes

This value changes infrequently and can never be disabled. This simple
mechanism restarts the CVO so that when the CVO starts again it will
start creating using techpreview manifests
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2021
@LalatenduMohanty
Copy link
Member

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 3, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, LalatenduMohanty, 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:
  • OWNERS [LalatenduMohanty,wking]

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

@wking
Copy link
Member

wking commented Dec 3, 2021

sandbox error is orthogonal:

/override ci/prow/e2e-agnostic-upgrade

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 3, 2021

@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic-upgrade

Details

In response to this:

sandbox error is orthogonal:

/override ci/prow/e2e-agnostic-upgrade

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
Copy link
Member

wking commented Dec 3, 2021

Looks like #703 snuck in in parallel, and now Tide is retesting the world against the new target commit, but I want to close out this epic and move on to other things, so skip the slow stuff:

/override ci/prow/e2e-agnostic
/override ci/prow/e2e-agnostic-operator

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 3, 2021

@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic, ci/prow/e2e-agnostic-operator

Details

In response to this:

Looks like #703 snuck in in parallel, and now Tide is retesting the world against the new target commit, but I want to close out this epic and move on to other things, so skip the slow stuff:

/override ci/prow/e2e-agnostic
/override ci/prow/e2e-agnostic-operator

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-merge-robot openshift-merge-robot merged commit de6f4b1 into openshift:master Dec 3, 2021
wking added a commit to wking/cluster-version-operator that referenced this pull request Dec 7, 2021
From the enhancement [1]:

  During bootstrapping, the CVO will assume no feature sets are
  enabled until it can successfully retrieve
  `featuregates.config.openshift.io` from the Kubernetes API server.

So this softens the error from 18dd189 (simplify includeTechPreview
flag to be a static bool, 2021-11-22, openshift#694) to be log-and-continue.
If it turns out that we actually were TechPreviewNoUpgrade, the
TechPreviewChangeStopper controller will eventually succeed in pulling
the feature-gate, notice, and ask the CVO to shut down.  In the
meantime, the CVO will have been able to get a head start on
reconciling the vast majority of manifests which are not tech-preview.

[1]: https://github.com/openshift/enhancements/blame/f74ffe7776f40dbd096b9ca10c27ee7a0a579e58/enhancements/update/cvo-techpreview-manifests.md#L70-L71
wking added a commit to wking/cluster-version-operator that referenced this pull request Dec 10, 2021
We have a number of different annotations all working together now, so
logging exclusions with our reasoning will help folks who are
debugging "why is my manifest not being reconciled?".  And while this
will create a number of logs for some profiles, we we already log each
reconciled manifest with every sync cycle, so the volume increase
isn't huge compared to our existing log rate.  This is primarily a
dev-time issue, which argues against log spew for production clusters,
but since 72599d3 (Exclude
featuregate.release.openshift/tech-preview=true manifests, 2021-04-13, openshift#694),
there's been the dynamic tech-preview business, so there is now more
utility in production logging around this than there was before.
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. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants