Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Jun 6, 2020

It has been in stable-4.4 since dc06ce1 (#263) landed. Promoting into fast/stable channels for 4.5 makes it clear that we support 4.4.6 regardless of whether a cluster is interested in 4.5 update recommendations or not.

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2020
@sdodson
Copy link
Member

sdodson commented Jun 7, 2020

I think for fast and stable channels this should be driven solely by the version at which the console starts allowing them to change to the 4.5 channels. I don't know what version that is or will be, but we should check on that, if those channels haven't been added to the console already then it'll be 4.4.9 at the earliest that they're added.

@wking
Copy link
Member Author

wking commented Jun 7, 2020

If we land this, we can land the console change in any future version. We don't need to synchronize beyond that, do we?

@sdodson
Copy link
Member

sdodson commented Jun 8, 2020

I guess my real concern is that we don't know that 4.4.6 will be the minimum only that it's sufficient for the candidate channel today. We may alter this to bring in fixes in the future which change things like openshift/machine-config-operator#1780 but the floor should always be the version that we add the channels to the console until such a time that we're doing the channel list dynamically.

@wking
Copy link
Member Author

wking commented Jun 8, 2020

In #116, you proposed backfilling stable-4.3 with the content of stable-4.2, and that looked good to me (but predated my ability to approve channel promotions). The constraint is that we need 4.4 releases in stable at least before the console's list includes stable-4.5. There is no harm I can see in also promoting some earlier 4.4.z into stable-4.5. Especially since a non-RC in candidate but not in the associated fast/stable might seem to be tombstoned, and we aren't tombstoning 4.4.6.

@vrutkovs
Copy link

vrutkovs commented Jun 9, 2020

GCP LB fixes have not been yet backported to 4.4 - that would make some CI runs fail on disruption test. Could we hold this one and ensure minimum version required for 4.5 has this fix?

Another issue which can bite us is https://bugzilla.redhat.com/show_bug.cgi?id=1834925 - 4.1 -> ... 4.x vSphere updates

@wking
Copy link
Member Author

wking commented Jun 9, 2020

GCP LB fixes have not been yet backported to 4.4 - that would make some CI runs fail on disruption test.

I would rather handle those in the release-building phase by not baking in 4.4.z releases known to be impacted as update sources when cutting the 4.5 GA and later. And if we slip up, to handle that by explicitly blocking edges in this repo. I don't think holding supported releases out of stable channels is the right approach to this issue.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2020
@wking wking force-pushed the stabilize-446-in-45-channels branch from 983920a to ce9bc7e Compare July 1, 2020 01:22
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2020
@wking
Copy link
Member Author

wking commented Jul 1, 2020

Rebased onto master with 983920a -> ce9bc7e.

@wking wking force-pushed the stabilize-446-in-45-channels branch from ce9bc7e to 32562e5 Compare July 1, 2020 01:31
@wking
Copy link
Member Author

wking commented Jul 1, 2020

And picking up similar handling for 4.4.8 and 4.4.9 with ce9bc7e -> 32562e5.

@wking wking changed the title channels/*-4.5: Add 4.4.6 to fast-4.5 and stable-4.5 channels/*-4.5: Add 4.4.[689] to fast-4.5 and stable-4.5 Jul 1, 2020
@wking wking force-pushed the stabilize-446-in-45-channels branch from 32562e5 to 8b72308 Compare July 1, 2020 01:51
@sdodson
Copy link
Member

sdodson commented Jul 1, 2020

I still don't understand why we want this. If we want this then why not 4.4.3-4.4.5? We already know that our baseline requirement for 4.4 to 4.5 at GA will be at least 4.4.10.

4.4.6 has been in stable-4.4 since dc06ce1 (Enable 4.4.6 in stable
channel(s), 2020-05-27, openshift#263) landed.  Promoting into fast/stable
channels for 4.5 makes it clear that we support 4.4.6 regardless of
whether a cluster is interested in 4.5 update recommendations or not.

And also promote 4.4.8, 4.4.9, and 4.4.10 to catch up with 61e8f69
(Enable 4.4.8 in stable channel(s), 2020-06-10, openshift#280), 64506bb
(Enable 4.4.9 in stable channel(s), 2020-06-18, openshift#289), and 92080fe
(Enable 4.4.10 in stable channel(s), 2020-06-24, openshift#296).
@wking wking force-pushed the stabilize-446-in-45-channels branch from 8b72308 to 50a498f Compare July 1, 2020 03:40
@wking
Copy link
Member Author

wking commented Jul 1, 2020

If we want this then why not 4.4.3-4.4.5?

Because in the current master (127c028).

$ sort channels/stable-4.4.yaml channels/candidate-4.5.yaml | uniq -c | grep ' 2 '
      2 - 4.4.10
      2 - 4.4.6
      2 - 4.4.8
      2 - 4.4.9

So those releases are supported (via stable-4.4) and in the 4.5 feeder (via candidate-4.5). They should be in the 4.5 fast/stable channels too. The alternative is tombstoning them in 4.5, and... I dunno what we'd use to justify that. I'm still on board with not including them as baked-in edges when cutting new 4.5 releases and using blocked edges for any places where we forget and bake them in anyway.

Also added 4.4.10 with 8b72308 -> 50a498f.

@wking wking changed the title channels/*-4.5: Add 4.4.[689] to fast-4.5 and stable-4.5 channels/*-4.5: Add 4.4.6 through 4.4.10 to fast-4.5 and stable-4.5 Jul 1, 2020
@wking
Copy link
Member Author

wking commented Jul 1, 2020

Current candidate-4.5 graph:

graph-4 5

This PR would bring the fast-4.5 and stable-4.5 into a similar state, except without any 4.5 RCs. We should round with ART to make sure that the next 4.5 RC only includes 4.4.10+.

@wking
Copy link
Member Author

wking commented Jul 1, 2020

Quay 500ed our blob pull.

/retest

@sdodson
Copy link
Member

sdodson commented Jul 1, 2020

So those releases are supported (via stable-4.4) and in the 4.5 feeder (via candidate-4.5). They should be in the 4.5 fast/stable channels too.

We've never asserted that everything in the candidate channel ends up in fast and stable channels. In fact we assert that there's no guarantee that you'll be able to upgrade from candidate versions to GA versions. Finnaly, 4.4.10 is the first version of the product which presents 4.5 channels to the admin.

@sdodson
Copy link
Member

sdodson commented Jul 1, 2020

I disagree with this pattern but I'll leave it up to others to make a final decision as I don't think there's any real danger. Please make sure that whatever the decision is gets written up in our SOPs so that we do the same thing next time.
/assign @vrutkovs @LalatenduMohanty
/uncc @derekwaynecarr

@openshift-ci-robot openshift-ci-robot removed the request for review from derekwaynecarr July 1, 2020 12:43
@vrutkovs
Copy link

vrutkovs commented Jul 1, 2020

There is no harm I can see in also promoting some earlier 4.4.z into stable-4.5

There's certainly harm on at least GCP - even if user changes the channel manually they'd hit LB bug and the workload would be distrupted.
Also, some admins are aware of this repo, they might check the yamls and wonder why their 4.4.6 can't be updated via Console (these builds don't have stable-4.5 channel added to console). This makes us look inconsistent - why would enable pre-4.4.10 releases having no means to do that via console?

/hold

@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 Jul 1, 2020
@wking
Copy link
Member Author

wking commented Jul 1, 2020

We've never asserted that everything in the candidate channel ends up in fast and stable channels.

Agreed, but the pattern #75 is trying to set is that holding in candidate needs a tombstone reason. And I think fast promotion in any series should be gated only by "is there a public errata?". Post errata issues should be represented by explicitly pulled edges, when necessary.

There's certainly harm on at least GCP - even if user changes the channel manually they'd hit LB bug and the workload would be distrupted.

I'd buy that if there were any 4.5 releases in fast-4.5 or stable-4.5 (which there aren't yet) which included older 4.4.z as baked-in sources (which we have told ART not to do) which we had not blocked (which is our backstop if ART slips up). So I see no risk now, minimal risk in the future, and the upside of not haviing a risk of VersionNotFound or a need to explain why we're tombstoning releases with public errata.

Also, some admins are aware of this repo, they might check the yamls and wonder why their 4.4.6 can't be updated via Console...

I agree that having the console pull channels from ClusterVersion status. Until then, confusion here seems like a small risk to me.

@LalatenduMohanty
Copy link
Member

LalatenduMohanty commented Jul 2, 2020

Also, some admins are aware of this repo, they might check the yamls and wonder why their 4.4.6 can't be updated via Console (these builds don't have stable-4.5 channel added to console). This makes us look inconsistent - why would enable pre-4.4.10 releases having no means to do that via console?

+1

Please make sure that whatever the decision is gets written up in our SOPs so that we do the same thing next time.

+1. Lets discuss this on slack.

@LalatenduMohanty
Copy link
Member

FYI, In the team meeting we discussed and everyone was in the agreement that we should not have more versions than the primary metadata from image manifest.

@LalatenduMohanty
Copy link
Member

/close

@openshift-ci-robot
Copy link

@LalatenduMohanty: Closed this PR.

Details

In response to this:

/close

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.

@LalatenduMohanty
Copy link
Member

/reopen

@openshift-ci-robot
Copy link

@LalatenduMohanty: Reopened this PR.

Details

In response to this:

/reopen

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

@wking: PR needs rebase.

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.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2020
@openshift-ci-robot
Copy link

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

Test name Commit Details Rerun command
ci/prow/publish 50a498f link /test publish
ci/prow/images 50a498f link /test images

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.

@sdodson
Copy link
Member

sdodson commented Dec 8, 2020

/close
We've already done something similar in another PR

@openshift-ci-robot
Copy link

@sdodson: Closed this PR.

Details

In response to this:

/close
We've already done something similar in another PR

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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants