Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Sep 10, 2021

So we can explain why we're blocking the different edges: openshift/enhancements#821.

The zz in the filename for the vSphere hostname block ensures that one sorts last alphabetically, because it's the broadest block, and legacy Cincinnati services will prefer the final regular expression they load for a given to target.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2021
@wking wking force-pushed the targeted-edge-blocking branch 2 times, most recently from 86082bb to e75dffb Compare September 10, 2021 17:59
@wking wking changed the title blocked-edges/4.7.4*: Shard into per-bug entries blocked-edges/4.7.4*: Targeted edge blocking and version 1.1.0 Sep 10, 2021
@wking
Copy link
Member Author

wking commented Sep 10, 2021

/hold

Don't want to rush this one in ;)

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2021
@wking wking force-pushed the targeted-edge-blocking branch from e75dffb to 676e5b1 Compare September 10, 2021 22:33
@wking wking force-pushed the targeted-edge-blocking branch 2 times, most recently from 4722ce7 to a66bd51 Compare September 11, 2021 01:32
@wking
Copy link
Member Author

wking commented Sep 11, 2021

Checking a66bd51 on a 4.6.0 OSUS installed on 4.8.11 OpenShift (via #1057 pointed at this branch):

$ curl -ksH accept:application/json 'https://example-policy-engine-route-openshift-update-service.apps.ci-ln-l4qnyb2-f76d1.origin-ci-int-gce.dev.openshift.com/api/upgrades_info/v1/graph?channel=candidate-4.7' | jq -r '.nodes[] | select(.version == "4.7.4").metadata["io.openshift.upgrades.graph.previous.remove_regex"]'
.*
$ curl -ksH accept:application/json 'https://example-policy-engine-route-openshift-update-service.apps.ci-ln-l4qnyb2-f76d1.origin-ci-int-gce.dev.openshift.com/api/upgrades_info/v1/graph?channel=candidate-4.7' | jq -r '(.nodes | with_entries(.key |= tostring)) as $nodes_by_index | .edges[] | $nodes_by_index[.[0] | tostring].version + " -> " + $nodes_by_index[.[1] | tostring].version' | grep ' -> 4.7.4'
...no hits...

So looks good to me vs. an old Cincinnati.

* `from` (required, [string][json-string]) is a regex for the previous release versions.
If you want to require `from` to match the full version string (and not just a substring), you must include explicit `^` and `$` anchors.
Release version strings will receive [the architecture-suffix](#release-names) before being compared to this regular expression.
* `url` (optional, [string][json-string]), with a URI documenting the blocking reason.

Choose a reason for hiding this comment

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

IMO justification link should not be optional

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should, and there's a section in the in-flight enhancement explaining my motivation, but let's hash this out in the relevant enhancements thread.

Release version strings will receive [the architecture-suffix](#release-names) before being compared to this regular expression.
* `url` (optional, [string][json-string]), with a URI documenting the blocking reason.
For example, this could link to a bug's impact statement or knowledge-base article.
* `name` (optional, [string][json-string]), with a CamelCase reason suitable for [a `ClusterOperatorStatusCondition` `reason` property][api-reason].

Choose a reason for hiding this comment

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

Do we need to come up with a name when its tied to a bug is (almost) all cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

This slug eventually feeds the Recommended condition's reason property. I think we want that, although see the Metadata to include when blocking a conditional request section in the enhancement for discussion.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we maintain the list of name i.e. ClusterOperatorStatusCondition reason it might be useful to create some kind of glossary

So we can explain why we're blocking the different edges [1].

The zz in the filename for the vSphere hostname block ensures that one
sorts last alphabetically, because it's the broadest block, and legacy
Cincinnati services will prefer the final regular expression they load
for a given 'to' target.

[1]: openshift/enhancements#821
@wking wking force-pushed the targeted-edge-blocking branch from a66bd51 to 39bc2fb Compare September 15, 2021 02:29
Copy link

@vrutkovs vrutkovs left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2021
Copy link
Member

@LalatenduMohanty LalatenduMohanty left a comment

Choose a reason for hiding this comment

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

/lgtm

Release version strings will receive [the architecture-suffix](#release-names) before being compared to this regular expression.
* `url` (optional, [string][json-string]), with a URI documenting the blocking reason.
For example, this could link to a bug's impact statement or knowledge-base article.
* `name` (optional, [string][json-string]), with a CamelCase reason suitable for [a `ClusterOperatorStatusCondition` `reason` property][api-reason].
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we maintain the list of name i.e. ClusterOperatorStatusCondition reason it might be useful to create some kind of glossary

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 16, 2021

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@LalatenduMohanty
Copy link
Member

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2021
@openshift-merge-robot openshift-merge-robot merged commit 00cf124 into openshift:master Sep 20, 2021
@wking wking deleted the targeted-edge-blocking branch September 20, 2021 16:20
@wking
Copy link
Member Author

wking commented Sep 20, 2021

Checking now that this is live. From shortly before the PR merged:

$ curl -s 'https://api.openshift.com/api/upgrades_info/v1/graph?channel=fast-4.7' >fast-4.7-pre.json
$ curl -s 'https://api.openshift.com/api/upgrades_info/v1/graph?channel=fast-4.8' >fast-4.8-pre.json

And from after the PR merged:

$ curl -s 'https://api.openshift.com/api/upgrades_info/v1/graph?channel=fast-4.7' >fast-4.7-post.json
$ curl -s 'https://api.openshift.com/api/upgrades_info/v1/graph?channel=fast-4.8' >fast-4.8-post.json

Writing a comparer:

$ edges() {
>   jq -r '(.nodes | with_entries(.key |= tostring)) as $nodes_by_index | .edges[] | $nodes_by_index[(.[0] | tostring)].version + " -> " + $nodes_by_index[(.[1] | tostring)].version' "${1}" | sort -V
> }

And using it:

$ diff -u <(edges fast-4.7-pre.json) <(edges fast-4.7-post.json) && echo 'no changes'
no changes
$ diff -u <(edges fast-4.8-pre.json) <(edges fast-4.8-post.json) && echo 'no changes'
no changes

So hooray, we're good vs. modern Cincinnati too.

wking added a commit to wking/cincinnati-graph-data that referenced this pull request Sep 23, 2021
So we can explain why we're blocking the different edges [1] (the
promql -> PromQL type change is in flight with [2]).

The zz in the filename for the vSphere hostname block ensures that one
sorts last alphabetically, because it's the broadest block, and legacy
Cincinnati services will prefer the final regular expression they load
for a given 'to' target.

This is basically a second attempt at my earlier 39bc2fb
(blocked-edges/4.7.4*: Targeted edge blocking and version 1.1.0,
2021-09-01, openshift#1056), which ended up getting reverted in da1254a
(Revert "blocked-edges/4.7.4*: Targeted edge blocking and version
1.1.0", 2021-09-21, openshift#1078) because the production Cincinnati was mad
about the 1.1.0 version string.  [3] taught Cincinnati to relax, and
now that's live (and we never shipped any versions that would be mad
about 1.1.0 to customers, the 4.6.0 Update Service operator went out
before [4]).

[1]: openshift/enhancements#821
[2]: openshift/enhancements#910
[3]: openshift/cincinnati#538
[4]: openshift/cincinnati#314
wking added a commit to wking/cincinnati-graph-data that referenced this pull request Sep 27, 2021
So we can explain why we're blocking the different edges [1] (the
promql -> PromQL type change is in flight with [2]).

The zz in the filename for the vSphere hostname block ensures that one
sorts last alphabetically, because it's the broadest block, and legacy
Cincinnati services will prefer the final regular expression they load
for a given 'to' target.

This is basically a second attempt at my earlier 39bc2fb
(blocked-edges/4.7.4*: Targeted edge blocking and version 1.1.0,
2021-09-01, openshift#1056), which ended up getting reverted in da1254a
(Revert "blocked-edges/4.7.4*: Targeted edge blocking and version
1.1.0", 2021-09-21, openshift#1078) because the production Cincinnati was mad
about the 1.1.0 version string.  [3] taught Cincinnati to relax, and
now that's live (and we never shipped any versions that would be mad
about 1.1.0 to customers, the 4.6.0 Update Service operator went out
before [4]).

[1]: openshift/enhancements#821
[2]: openshift/enhancements#910
[3]: openshift/cincinnati#538
[4]: openshift/cincinnati#314
wking added a commit to wking/cincinnati-graph-data that referenced this pull request Sep 27, 2021
So we can explain why we're blocking the different edges [1] (the
promql -> PromQL type change is in flight with [2]).

The zz in the filename for the vSphere hostname block ensures that one
sorts last alphabetically, because it's the broadest block, and legacy
Cincinnati services will prefer the final regular expression they load
for a given 'to' target.

This is basically a second attempt at my earlier 39bc2fb
(blocked-edges/4.7.4*: Targeted edge blocking and version 1.1.0,
2021-09-01, openshift#1056), which ended up getting reverted in da1254a
(Revert "blocked-edges/4.7.4*: Targeted edge blocking and version
1.1.0", 2021-09-21, openshift#1078) because the production Cincinnati was mad
about the 1.1.0 version string.  [3] taught Cincinnati to relax, and
now that's live (and we never shipped any versions that would be mad
about 1.1.0 to customers, the 4.6.0 Update Service operator went out
before [4]).

[1]: openshift/enhancements#821
[2]: openshift/enhancements#910
[3]: openshift/cincinnati#538
[4]: openshift/cincinnati#314
wking added a commit to wking/cincinnati-graph-data that referenced this pull request Jan 31, 2022
… conditional update

Since 39bc2fb (blocked-edges/4.7.4*: Targeted edge blocking and
version 1.1.0, 2021-09-01, openshift#1056), we've had some example conditional
updates back on * -> 4.7.4.  But now that 4.10 has feature candidates,
this commit adds a conditional edge based on a minor 4.10.0-fc.2 issue
that was fixed in fc.3.  Folks interested in seeing conditional edges
in action can install fc.0 or fc.1 on AWS, and see that this
conditional edge is not recommended.  Or they can install fc.0 or fc.1
on a non-AWS platform to see that the conditional edge is recommended.
wking added a commit to wking/cincinnati-graph-data that referenced this pull request Jan 31, 2022
… conditional update

Since 39bc2fb (blocked-edges/4.7.4*: Targeted edge blocking and
version 1.1.0, 2021-09-01, openshift#1056), we've had some example conditional
updates back on * -> 4.7.4.  But now that 4.10 has feature candidates,
this commit adds a conditional edge based on a minor 4.10.0-fc.2 issue
that was fixed in fc.3.  Folks interested in seeing conditional edges
in action can install fc.0 or fc.1 on AWS, and see that this
conditional edge is not recommended.  Or they can install fc.0 or fc.1
on a non-AWS platform to see that the conditional edge is recommended.
wking added a commit to wking/cincinnati-graph-data that referenced this pull request Jan 31, 2022
… conditional update

Since 39bc2fb (blocked-edges/4.7.4*: Targeted edge blocking and
version 1.1.0, 2021-09-01, openshift#1056), we've had some example conditional
updates back on * -> 4.7.4.  But now that 4.10 has feature candidates,
this commit adds a conditional edge based on a minor 4.10.0-fc.2 issue
that was fixed in fc.3.  Folks interested in seeing conditional edges
in action can install fc.0 or fc.1 on AWS, and see that this
conditional edge is not recommended.  Or they can install fc.0 or fc.1
on a non-AWS platform to see that the conditional edge is recommended.
wking added a commit to wking/cincinnati-graph-data that referenced this pull request Jan 31, 2022
… conditional update

Since 39bc2fb (blocked-edges/4.7.4*: Targeted edge blocking and
version 1.1.0, 2021-09-01, openshift#1056), we've had some example conditional
updates back on * -> 4.7.4.  But now that 4.10 has feature candidates,
this commit adds a conditional edge based on a minor 4.10.0-fc.2 issue
that was fixed in fc.3.  Folks interested in seeing conditional edges
in action can install fc.0 or fc.1 on AWS, and see that this
conditional edge is not recommended.  Or they can install fc.0 or fc.1
on a non-AWS platform to see that the conditional edge is recommended.
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.

4 participants