Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Targeted edge blocking #663

Merged
merged 7 commits into from
Nov 23, 2021

Conversation

wking
Copy link
Member

@wking wking commented Sep 23, 2021

@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 Sep 23, 2021
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2021
@wking wking force-pushed the targeted-edge-blocking branch 2 times, most recently from b5f1d1d to facfbe2 Compare September 24, 2021 19:43
wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 24, 2021
This is ART's default for OpenShift 4.10 [1], and we need [1.16's new
`io/fs` package][2] to avoid [3]:

  INFO[2021-09-24T19:45:46Z] vendor/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics.go:21:2: cannot find package "." in:
  	/go/src/github.com/openshift/cluster-version-operator/vendor/io/fs

for the 1.22 Kube bumps needed for targeted edge blocking [4].

[1]: https://github.com/openshift/ocp-build-data/blob/f14ea97fcb9893c325f12bed9d9afb9cd2f10857/streams.yml#L31
[2]: https://golang.org/doc/go1.16#fs
[3]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-version-operator/663/pull-ci-openshift-cluster-version-operator-master-unit/1441488679963987968#1:build-log.txt%3A13
[4]: openshift#663
@wking
Copy link
Member Author

wking commented Sep 25, 2021

#665 landed.

/retest

@wking wking force-pushed the targeted-edge-blocking branch 3 times, most recently from 488d17a to 7ba19cd Compare September 27, 2021 19:52
@wking wking force-pushed the targeted-edge-blocking branch 11 times, most recently from b4e03e0 to d50479d Compare November 9, 2021 00:20
@wking wking changed the title WIP: Targeted edge blocking Targeted edge blocking Nov 9, 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 9, 2021
Separated from the rest of the CVO stuff, because the insights folks
might want to use this too [1].

[1]: openshift/enhancements#837
…ttling

Per [1]:

  Additionally, the operator will continually re-evaluate the blocking
  conditionals in conditionalUpdates and update
  conditionalUpdates[].risks accordingly. The timing of the evaluation
  and freshness are largely internal details, but to avoid consuming
  excessive monitoring resources and because the rules should be based
  on slowly-changing state, the operator will handle polling with the
  following restrictions:

  * The cluster-version operator will cache polling results for each
    query, so a single query which is used in evaluating multiple
    risks over multiple conditional update targets will only be
    evaluated once per round.

  * After evaluating a PromQL query, the cluster-version operator will
    wait at least 10 minutes before evaluating any PromQL. This delay
    will not be persisted between operator restarts, so a
    crash-looping CVO may result in higher PromQL load. But a
    crash-looping CVO will also cause the KubePodCrashLooping alert to
    fire, which will summon the cluster administrator.

  * After evaluating a PromQL query, the cluster-version operator will
    wait at least an hour before evaluating that PromQL query again.

That's what this commit sets up.  The tests are a bit fiddly, since I
wanted to excercise "I have so many queries that I'd like to run, and
they're expiring before I can get through them all".  I'm trying to
show that if you give it enough tries, we won't consistently starve
out surprisingly many conditions, even though in that overloaded case,
someone is always getting starved out.  Unlikely to happen in the
wild, but the enhancement section is intentionally addressing the
"what if some malicious/misconfigured graph floods the CVO with PromQL
suggestions?".

[1]: https://github.com/openshift/enhancements/blob/2cc2d9b331532c852878a7c793f3a754914c824e/enhancements/update/targeted-update-edge-blocking.md#cluster-version-operator-support-for-the-enhanced-schema
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2021
@wking
Copy link
Member Author

wking commented Nov 23, 2021

I've pushed 6dfc418 -> ca186ed with the simplifications recommended by gofmt

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

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

openshift-ci bot commented Nov 23, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LalatenduMohanty, wking

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 [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 Author

wking commented Nov 23, 2021

e2e-agnostic seems to have hung mid-install for some reason after bootstrap-destroy:

level=info msg=Waiting up to 40m0s (until 5:47AM) for the cluster at https://api.ci-op-dm3ziilg-3302f.ci.azure.devcluster.openshift.com:6443 to initialize...
I1123 05:08:03.667720      69 trace.go:205] Trace[1592433353]: "Reflector ListAndWatch" name:k8s.io/client-go/tools/watch/informerwatcher.go:146 (23-Nov-2021 05:07:33.664) (total time: 30003ms):
Trace[1592433353]: [30.003338054s] [30.003338054s] END
E1123 05:08:03.667745      69 reflector.go:138] k8s.io/client-go/tools/watch/informerwatcher.go:146: Failed to watch *v1.ClusterVersion: failed to list *v1.ClusterVersion: Get "https://api.ci-op-dm3ziilg-3302f.ci.azure.devcluster.openshift.com:6443/apis/config.openshift.io/v1/clusterversions?fieldSelector=metadata.name%3Dversion&limit=500&resourceVersion=0": dial tcp 13.86.35.5:6443: i/o timeout
I1123 05:08:20.285699      69 trace.go:205] Trace[602073046]: "Reflector ListAndWatch" name:k8s.io/client-go/tools/watch/informerwatcher.go:146 (23-Nov-2021 05:08:04.473) (total time: 15812ms):
Trace[602073046]: ---"Objects listed" 15812ms (05:08:20.285)
Trace[602073046]: [15.812042182s] [15.812042182s] END
level=error msg=Attempted to gather ClusterOperator status after installation failure: listing ClusterOperator objects: Get "https://api.ci-op-dm3ziilg-3302f.ci.azure.devcluster.openshift.com:6443/apis/config.openshift.io/v1/clusteroperators": dial tcp 13.86.35.5:6443: i/o timeout
level=error msg=Cluster initialization failed because one or more operators are not functioning properly.
level=error msg=The cluster should be accessible for troubleshooting as detailed in the documentation linked below,
level=error msg=https://docs.openshift.com/container-platform/latest/support/troubleshooting/troubleshooting-installations.html
level=error msg=The 'wait-for install-complete' subcommand can then be used to continue the installation
level=fatal msg=failed to initialize the cluster: Working towards 4.10.0-0.ci.test-2021-11-23-043104-ci-op-dm3ziilg-latest: 717 of 818 done (87% complete)

By the time the gather steps rolled around, it was request canceled while waiting for connection and such, so not clear why that stuck. Trying again:

/test e2e-agnostic

@wking
Copy link
Member Author

wking commented Nov 23, 2021

e2e-agnostic:

error: timed out waiting for the condition on clusteroperators/network
{"component":"entrypoint","error":"wrapped process failed: exit status 1","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-11-23T07:21:57Z"}

But by gather-time, things look ok:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-version-operator/663/pull-ci-openshift-cluster-version-operator-master-e2e-agnostic/1463030884557918208/artifacts/e2e-agnostic/gather-extra/artifacts/clusteroperators.json | jq -r '.items[] | select(.metadata.name == "network").status.conditions[] | .lastTransitionTime + " " + .type + "=" + .status + " " + (.reason // "-") + ": " + (.message // "-")' | sort
2021-11-23T06:52:08Z ManagementStateDegraded=False -: -
2021-11-23T06:52:08Z Upgradeable=True -: -
2021-11-23T06:53:45Z Available=True -: -
2021-11-23T07:22:46Z Degraded=False -: -
2021-11-23T07:22:46Z Progressing=False -: -

Those 7:22:46 transitions are just after the 7:21:57 timeout. From the network operator's logs

...
I1123 07:18:25.403813       1 log.go:184] Set ClusterOperator conditions:
- lastTransitionTime: "2021-11-23T06:52:08Z"
  status: "False"
  type: ManagementStateDegraded
- lastTransitionTime: "2021-11-23T07:18:25Z"
  message: DaemonSet "openshift-multus/multus-additional-cni-plugins" rollout is not
    making progress - last change 2021-11-23T07:07:52Z
  reason: RolloutHung
  status: "True"
  type: Degraded
- lastTransitionTime: "2021-11-23T06:52:08Z"
  status: "True"
  type: Upgradeable
- lastTransitionTime: "2021-11-23T06:52:10Z"
  message: DaemonSet "openshift-multus/multus-additional-cni-plugins" is not available
    (awaiting 1 nodes)
  reason: Deploying
  status: "True"
  type: Progressing
- lastTransitionTime: "2021-11-23T06:53:45Z"
  status: "True"
  type: Available
...
I1123 07:22:46.848875       1 log.go:184] Set operator conditions:
- lastTransitionTime: "2021-11-23T06:52:08Z"
  status: "False"
  type: ManagementStateDegraded
- lastTransitionTime: "2021-11-23T07:18:25Z"
  message: DaemonSet "openshift-multus/multus-additional-cni-plugins" rollout is not
    making progress - last change 2021-11-23T07:07:52Z
  reason: RolloutHung
  status: "True"
  type: Degraded
- lastTransitionTime: "2021-11-23T06:52:08Z"
  status: "True"
  type: Upgradeable
- lastTransitionTime: "2021-11-23T07:22:46Z"
  status: "False"
  type: Progressing
- lastTransitionTime: "2021-11-23T06:53:45Z"
  status: "True"
  type: Available
...
I1123 07:22:46.908071       1 log.go:184] Set ClusterOperator conditions:
- lastTransitionTime: "2021-11-23T06:52:08Z"
  status: "False"
  type: ManagementStateDegraded
- lastTransitionTime: "2021-11-23T07:22:46Z"
  status: "False"
  type: Degraded
- lastTransitionTime: "2021-11-23T06:52:08Z"
  status: "True"
  type: Upgradeable
- lastTransitionTime: "2021-11-23T07:22:46Z"
  status: "False"
  type: Progressing
- lastTransitionTime: "2021-11-23T06:53:45Z"
  status: "True"
  type: Available
...

Perhaps a slow node? I dunno. Trying again:

/test e2e-agnostic

@wking
Copy link
Member Author

wking commented Nov 23, 2021

Poking some more in that last failed run, the nodes were all Ready=True well before 7:22:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-version-operator/663/pull-ci-openshift-cluster-version-operator-master-e2e-agnostic/1463030884557918208/artifacts/e2e-agnostic/gather-extra/artifacts/nodes.json | jq -r '.items[] | .metadata.name as $n | .status.conditions[] | select(.type == "Ready") | .lastTransitionTime + " " + .state + " " + $n' | sort
2021-11-23T06:52:40Z  ci-op-tnvjkz61-3302f-cx8rj-master-0
2021-11-23T06:52:48Z  ci-op-tnvjkz61-3302f-cx8rj-master-1
2021-11-23T06:52:51Z  ci-op-tnvjkz61-3302f-cx8rj-master-2
2021-11-23T07:05:24Z  ci-op-tnvjkz61-3302f-cx8rj-worker-centralus1-8tnn7
2021-11-23T07:05:49Z  ci-op-tnvjkz61-3302f-cx8rj-worker-centralus3-85dqc
2021-11-23T07:07:28Z  ci-op-tnvjkz61-3302f-cx8rj-worker-centralus2-zm4wt

One of the DaemonSet pods was definitely slow to go Ready=True though:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-version-operator/663/pull-ci-openshift-cluster-version-operator-master-e2e-agnostic/1463030884557918208/artifacts/e2e-agnostic/gather-extra/artifacts/pods.json | jq -r '.items[] | . as $p | select(.metadata.name | startswith("multus-additional-cni-plugins-")).status.conditions[] | select(.type == "Ready") | .lastTransitionTime + " " + .status + " " + $p.metadata.name + " " + $p.spec.nodeName' | sort
2021-11-23T06:53:08Z True multus-additional-cni-plugins-4lfnf ci-op-tnvjkz61-3302f-cx8rj-master-1
2021-11-23T06:53:13Z True multus-additional-cni-plugins-l27nd ci-op-tnvjkz61-3302f-cx8rj-master-2
2021-11-23T06:53:45Z True multus-additional-cni-plugins-g2nh7 ci-op-tnvjkz61-3302f-cx8rj-master-0
2021-11-23T07:06:47Z True multus-additional-cni-plugins-8vsc5 ci-op-tnvjkz61-3302f-cx8rj-worker-centralus1-8tnn7
2021-11-23T07:07:52Z True multus-additional-cni-plugins-sjs99 ci-op-tnvjkz61-3302f-cx8rj-worker-centralus2-zm4wt
2021-11-23T07:22:46Z True multus-additional-cni-plugins-zp4tg ci-op-tnvjkz61-3302f-cx8rj-worker-centralus3-85dqc

Digging into that slow pod:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-version-operator/663/pull-ci-openshift-cluster-version-operator-master-e2e-agnostic/1463030884557918208/artifacts/e2e-agnostic/gather-extra/artifacts/pods.json | jq -r '.items[] | select(.metadata.name == "multus-additional-cni-plugins-zp4tg").status.initContainerStatuses[] | (.state.terminated | .startedAt + " " + .finishedAt) + " " + (.restartCount | tostring) + " " + .name'
2021-11-23T07:05:35Z 2021-11-23T07:05:35Z 0 egress-router-binary-copy
2021-11-23T07:05:44Z 2021-11-23T07:05:44Z 0 cni-plugins
2021-11-23T07:05:47Z 2021-11-23T07:05:47Z 0 bond-cni-plugin
2021-11-23T07:05:59Z 2021-11-23T07:05:59Z 0 routeoverride-cni
2021-11-23T07:22:44Z 2021-11-23T07:22:44Z 0 whereabouts-cni-bincopy
2021-11-23T07:22:44Z 2021-11-23T07:22:44Z 0 whereabouts-cni

That is a huge delay between routeoverride-cni and whereabouts-cni-bincopy. Ah, because the CI registry (or the connection to is) was slow/broken:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-version-operator/663/pull-ci-openshift-cluster-version-operator-master-e2e-agnostic/1463030884557918208/artifacts/e2e-agnostic/gather-extra/artifacts/events.json | jq -r '.items[] | select(tostring | contains("multus-additional-cni-plugins-zp4tg")) | .metadata.creationTimestamp + " " + .reason + ": " + .message'
...
2021-11-23T07:06:00Z Pulling: Pulling image "registry.build01.ci.openshift.org/ci-op-tnvjkz61/stable@sha256:b1418e6400569e5a31bd3708198fe2f0e202d2084001d1bcafe79d724fff3483"
2021-11-23T07:22:26Z Failed: Failed to pull image "registry.build01.ci.openshift.org/ci-op-tnvjkz61/stable@sha256:b1418e6400569e5a31bd3708198fe2f0e202d2084001d1bcafe79d724fff3483": rpc error: code = Unknown desc = reading blob sha256:35a67cc5ac632c9d7fa635dcbde51c7ca1d002f042a7e235ff696eed0382ee02: Get "https://build01-9hdwj-image-registry-us-east-1-nucqrmelsxtgndkbvchwdkw.s3.dualstack.us-east-1.amazonaws.com/docker/registry/v2/blobs/sha256/35/35a67cc5ac632c9d7fa635dcbde51c7ca1d002f042a7e235ff696eed0382ee02/data?...": read tcp 10.0.128.4:48550->52.217.198.250:443: read: connection timed out
2021-11-23T07:22:26Z Failed: Error: ErrImagePull
2021-11-23T07:22:27Z BackOff: Back-off pulling image "registry.build01.ci.openshift.org/ci-op-tnvjkz61/stable@sha256:b1418e6400569e5a31bd3708198fe2f0e202d2084001d1bcafe79d724fff3483"
2021-11-23T07:22:27Z Failed: Error: ImagePullBackOff
2021-11-23T07:22:43Z Pulled: Successfully pulled image "registry.build01.ci.openshift.org/ci-op-tnvjkz61/stable@sha256:b1418e6400569e5a31bd3708198fe2f0e202d2084001d1bcafe79d724fff3483" in 5.151277654s
2021-11-23T07:22:44Z Created: Created container whereabouts-cni-bincopy
2021-11-23T07:22:44Z Started: Started container whereabouts-cni-bincopy
2021-11-23T07:22:44Z Pulled: Container image "registry.build01.ci.openshift.org/ci-op-tnvjkz61/stable@sha256:b1418e6400569e5a31bd3708198fe2f0e202d2084001d1bcafe79d724fff3483" already present on machine
2021-11-23T07:22:44Z Created: Created container whereabouts-cni
...

@openshift-merge-robot openshift-merge-robot merged commit b572cb1 into openshift:master Nov 23, 2021
@wking wking deleted the targeted-edge-blocking branch November 23, 2021 14:50
if target == nil {
return current, updates, nil, &Error{
Reason: "ResponseInvalid",
Message: fmt.Sprintf("no node for conditional update %s", edge.To),
Copy link
Contributor

Choose a reason for hiding this comment

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

In the case target node is null, it seems edge.To would be null as well. If that's true, seems that's no need to log it in the message.

Copy link
Member Author

Choose a reason for hiding this comment

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

edge.To might have a version string that just happens to not be listed in nodes. Not something that should exist in well-formed Cincinnati graph JSON, but still worth logging.

Having edge.To be empty would also be a problem, and we could also complain about that without even looking through nodes. As this code stands, I guess we could have an empty edge.To and an entry in nodes with an empty-string version and maybe not get a ResponseInvalid complaint? Seems fair to open a bug if we aren't setting ResponseInvalid if nodes[].version == "" or unset today.

wking added a commit to wking/cluster-version-operator that referenced this pull request Mar 6, 2023
…rget version

When changing channels it's possible that multiple new conditional
update risks will need to be evaluated.  For instance, a cluster
running 4.10.34 in a 4.10 channel might only have to evaluate
OpenStackNodeCreationFails.  But when the channel is changed to a 4.11
channel multiple new risks require evaluation and the evaluation of
new risks was throttled at one every 10 minutes.  This means if there
are three new risks it may take up to 20 minutes after the channel has
changed for the full set of conditional updates to be computed.

With this commit, I'm sorting the conditional updates in
version-descending order, which is the order we've used in the
ClusterVersion status since c9dd479 (pkg/cvo/availableupdates:
Sort (conditional)updates, 2021-09-29, openshift#663).  This prioritizes the
longest-hop risks.  For example, 4.10.34 currently has the following
updates:

* 4.10.(z!=38): no risks
* 4.10.38: OpenStackNodeCreationFails
* 4.11.(z<10): ARM64SecCompError524, AWSOldBootImagesLackAfterburn, MachineConfigRenderingChurn, OVNNetworkPolicyLongName
* 4.11.(10<=z<26): ARM64SecCompError524, AWSOldBootImagesLackAfterburn, MachineConfigRenderingChurn
* 4.11.26: ARM64SecCompError524, AWSOldBootImagesLackAfterburn
* 4.11.(27<=z<...): AWSOldBootImagesLackAfterburn

By focusing on the largest target (say 4.11.30), we'd evaluate
AWSOldBootImagesLackAfterburn first.  If it did not match the current
cluster, 4.11.27 and later would be quickly recommended.  It would
take another 10m before the self-throttling allowed us to evaluate
ARM64SecCompError524, and once we had, that would unblock 4.11.26.
Ten minutes after that, we'd evaluate MachineConfigRenderingChurn, and
unblock 4.11.(10<=z<26).  And so on.  But folks on 4.10.34 today are
much more likely to be interested in 4.11.30 and other tip releases
than they are to care about 4.11.10 and other relatively old releases.
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/cluster-version-operator that referenced this pull request Mar 14, 2023
…rget version

When changing channels it's possible that multiple new conditional
update risks will need to be evaluated.  For instance, a cluster
running 4.10.34 in a 4.10 channel might only have to evaluate
OpenStackNodeCreationFails.  But when the channel is changed to a 4.11
channel multiple new risks require evaluation and the evaluation of
new risks was throttled at one every 10 minutes.  This means if there
are three new risks it may take up to 20 minutes after the channel has
changed for the full set of conditional updates to be computed.

With this commit, I'm sorting the conditional updates in
version-descending order, which is the order we've used in the
ClusterVersion status since c9dd479 (pkg/cvo/availableupdates:
Sort (conditional)updates, 2021-09-29, openshift#663).  This prioritizes the
longest-hop risks.  For example, 4.10.34 currently has the following
updates:

* 4.10.(z!=38): no risks
* 4.10.38: OpenStackNodeCreationFails
* 4.11.(z<10): ARM64SecCompError524, AWSOldBootImagesLackAfterburn, MachineConfigRenderingChurn, OVNNetworkPolicyLongName
* 4.11.(10<=z<26): ARM64SecCompError524, AWSOldBootImagesLackAfterburn, MachineConfigRenderingChurn
* 4.11.26: ARM64SecCompError524, AWSOldBootImagesLackAfterburn
* 4.11.(27<=z<...): AWSOldBootImagesLackAfterburn

By focusing on the largest target (say 4.11.30), we'd evaluate
AWSOldBootImagesLackAfterburn first.  If it did not match the current
cluster, 4.11.27 and later would be quickly recommended.  It would
take another 10m before the self-throttling allowed us to evaluate
ARM64SecCompError524, and once we had, that would unblock 4.11.26.
Ten minutes after that, we'd evaluate MachineConfigRenderingChurn, and
unblock 4.11.(10<=z<26).  And so on.  But folks on 4.10.34 today are
much more likely to be interested in 4.11.30 and other tip releases
than they are to care about 4.11.10 and other relatively old releases.
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/cluster-version-operator that referenced this pull request Mar 17, 2023
…rget version

When changing channels it's possible that multiple new conditional
update risks will need to be evaluated.  For instance, a cluster
running 4.10.34 in a 4.10 channel might only have to evaluate
OpenStackNodeCreationFails.  But when the channel is changed to a 4.11
channel multiple new risks require evaluation and the evaluation of
new risks was throttled at one every 10 minutes.  This means if there
are three new risks it may take up to 20 minutes after the channel has
changed for the full set of conditional updates to be computed.

With this commit, I'm sorting the conditional updates in
version-descending order, which is the order we've used in the
ClusterVersion status since c9dd479 (pkg/cvo/availableupdates:
Sort (conditional)updates, 2021-09-29, openshift#663).  This prioritizes the
longest-hop risks.  For example, 4.10.34 currently has the following
updates:

* 4.10.(z!=38): no risks
* 4.10.38: OpenStackNodeCreationFails
* 4.11.(z<10): ARM64SecCompError524, AWSOldBootImagesLackAfterburn, MachineConfigRenderingChurn, OVNNetworkPolicyLongName
* 4.11.(10<=z<26): ARM64SecCompError524, AWSOldBootImagesLackAfterburn, MachineConfigRenderingChurn
* 4.11.26: ARM64SecCompError524, AWSOldBootImagesLackAfterburn
* 4.11.(27<=z<...): AWSOldBootImagesLackAfterburn

By focusing on the largest target (say 4.11.30), we'd evaluate
AWSOldBootImagesLackAfterburn first.  If it did not match the current
cluster, 4.11.27 and later would be quickly recommended.  It would
take another 10m before the self-throttling allowed us to evaluate
ARM64SecCompError524, and once we had, that would unblock 4.11.26.
Ten minutes after that, we'd evaluate MachineConfigRenderingChurn, and
unblock 4.11.(10<=z<26).  And so on.  But folks on 4.10.34 today are
much more likely to be interested in 4.11.30 and other tip releases
than they are to care about 4.11.10 and other relatively old releases.
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/cluster-version-operator that referenced this pull request Mar 22, 2023
…rget version

When changing channels it's possible that multiple new conditional
update risks will need to be evaluated.  For instance, a cluster
running 4.10.34 in a 4.10 channel might only have to evaluate
OpenStackNodeCreationFails.  But when the channel is changed to a 4.11
channel multiple new risks require evaluation and the evaluation of
new risks was throttled at one every 10 minutes.  This means if there
are three new risks it may take up to 20 minutes after the channel has
changed for the full set of conditional updates to be computed.

With this commit, I'm sorting the conditional updates in
version-descending order, which is the order we've used in the
ClusterVersion status since c9dd479 (pkg/cvo/availableupdates:
Sort (conditional)updates, 2021-09-29, openshift#663).  This prioritizes the
longest-hop risks.  For example, 4.10.34 currently has the following
updates:

* 4.10.(z!=38): no risks
* 4.10.38: OpenStackNodeCreationFails
* 4.11.(z<10): ARM64SecCompError524, AWSOldBootImagesLackAfterburn, MachineConfigRenderingChurn, OVNNetworkPolicyLongName
* 4.11.(10<=z<26): ARM64SecCompError524, AWSOldBootImagesLackAfterburn, MachineConfigRenderingChurn
* 4.11.26: ARM64SecCompError524, AWSOldBootImagesLackAfterburn
* 4.11.(27<=z<...): AWSOldBootImagesLackAfterburn

By focusing on the largest target (say 4.11.30), we'd evaluate
AWSOldBootImagesLackAfterburn first.  If it did not match the current
cluster, 4.11.27 and later would be quickly recommended.  It would
take another 10m before the self-throttling allowed us to evaluate
ARM64SecCompError524, and once we had, that would unblock 4.11.26.
Ten minutes after that, we'd evaluate MachineConfigRenderingChurn, and
unblock 4.11.(10<=z<26).  And so on.  But folks on 4.10.34 today are
much more likely to be interested in 4.11.30 and other tip releases
than they are to care about 4.11.10 and other relatively old releases.
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/cluster-version-operator that referenced this pull request Mar 23, 2023
…rget version

When changing channels it's possible that multiple new conditional
update risks will need to be evaluated.  For instance, a cluster
running 4.10.34 in a 4.10 channel might only have to evaluate
OpenStackNodeCreationFails.  But when the channel is changed to a 4.11
channel multiple new risks require evaluation and the evaluation of
new risks was throttled at one every 10 minutes.  This means if there
are three new risks it may take up to 20 minutes after the channel has
changed for the full set of conditional updates to be computed.

With this commit, I'm sorting the conditional updates in
version-descending order, which is the order we've used in the
ClusterVersion status since c9dd479 (pkg/cvo/availableupdates:
Sort (conditional)updates, 2021-09-29, openshift#663).  This prioritizes the
longest-hop risks.  For example, 4.10.34 currently has the following
updates:

* 4.10.(z!=38): no risks
* 4.10.38: OpenStackNodeCreationFails
* 4.11.(z<10): ARM64SecCompError524, AWSOldBootImagesLackAfterburn, MachineConfigRenderingChurn, OVNNetworkPolicyLongName
* 4.11.(10<=z<26): ARM64SecCompError524, AWSOldBootImagesLackAfterburn, MachineConfigRenderingChurn
* 4.11.26: ARM64SecCompError524, AWSOldBootImagesLackAfterburn
* 4.11.(27<=z<...): AWSOldBootImagesLackAfterburn

By focusing on the largest target (say 4.11.30), we'd evaluate
AWSOldBootImagesLackAfterburn first.  If it did not match the current
cluster, 4.11.27 and later would be quickly recommended.  It would
take another 10m before the self-throttling allowed us to evaluate
ARM64SecCompError524, and once we had, that would unblock 4.11.26.
Ten minutes after that, we'd evaluate MachineConfigRenderingChurn, and
unblock 4.11.(10<=z<26).  And so on.  But folks on 4.10.34 today are
much more likely to be interested in 4.11.30 and other tip releases
than they are to care about 4.11.10 and other relatively old releases.
wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 20, 2023
530a509 (pkg/cvo/availableupdates: Prioritize conditional risks for
largest target version, 2023-03-06, openshift#909) prioritized the order in
which risks were evaluated.  But we were still waiting 10 minutes
between different PromQL evaluations while evaluating conditional
update risks.  The original 10m requirement is from the enhancement
[1], and was implemented in ca186ed (pkg/clusterconditions/cache:
Add a cache wrapper for client-side throttling, 2021-11-10, openshift#663).
But discussing with Lala, Scott, and Ben, we feel like the addressing
the demonstrated user experience need of low-latency risk evaluation
[2] is worth reducing the throttling to 1s per expression evaluation.
We still have MinForCondition set to an hour, so with this commit, a
cluster-version operator evaluating three risks will move from a
timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 10m, evaluate B for the first time (MinBetweenMatches after 1).
3. 20m, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

to a timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatches after 1).
3. 2s, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h1s, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h2s, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h1s, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h2s, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

We could deliver faster cache warming while preserving spaced out
refresh evaluation by splitting MinBetweenMatches into a 1s
MinBetweenMatchesInitial and 10m MinBetweenMatchesWhenCached, which
would produce timelines like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatchesInitial after 1).
3. 2s, evaluate C for the first time (MinBetweenMatchesInitial after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatchesWhenCached after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatchesWhenCached after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatchesWhenCached after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatchesWhenCached after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatchesWhenCached after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatchesWhenCached after 8).

but again discussing with Lala, Scott, and Ben, the code complexity to
deliver that distinction does not seem to be worth thet protection it
delivers to the PromQL engine.  And really, PromQL engines concerned
about load should harden themselves, including via Retry-After [3]
that allow clients to back off gracefully when the service needs that,
instead of relying on clients to guess about the load the service
could handle and back off without insight into actual server capacity.

[1]: https://github.com/openshift/enhancements/blame/158111ce156aac7fa6063a47c00e129c13033aec/enhancements/update/targeted-update-edge-blocking.md#L323-L325
[2]: https://issues.redhat.com/browse/OCPBUGS-19512
[3]: https://www.rfc-editor.org/rfc/rfc9110#name-retry-after
wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 22, 2023
530a509 (pkg/cvo/availableupdates: Prioritize conditional risks for
largest target version, 2023-03-06, openshift#909) prioritized the order in
which risks were evaluated.  But we were still waiting 10 minutes
between different PromQL evaluations while evaluating conditional
update risks.  The original 10m requirement is from the enhancement
[1], and was implemented in ca186ed (pkg/clusterconditions/cache:
Add a cache wrapper for client-side throttling, 2021-11-10, openshift#663).
But discussing with Lala, Scott, and Ben, we feel like the addressing
the demonstrated user experience need of low-latency risk evaluation
[2] is worth reducing the throttling to 1s per expression evaluation.
We still have MinForCondition set to an hour, so with this commit, a
cluster-version operator evaluating three risks will move from a
timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 10m, evaluate B for the first time (MinBetweenMatches after 1).
3. 20m, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

to a timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatches after 1).
3. 2s, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h1s, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h2s, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h1s, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h2s, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

We could deliver faster cache warming while preserving spaced out
refresh evaluation by splitting MinBetweenMatches into a 1s
MinBetweenMatchesInitial and 10m MinBetweenMatchesWhenCached, which
would produce timelines like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatchesInitial after 1).
3. 2s, evaluate C for the first time (MinBetweenMatchesInitial after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatchesWhenCached after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatchesWhenCached after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatchesWhenCached after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatchesWhenCached after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatchesWhenCached after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatchesWhenCached after 8).

but again discussing with Lala, Scott, and Ben, the code complexity to
deliver that distinction does not seem to be worth thet protection it
delivers to the PromQL engine.  And really, PromQL engines concerned
about load should harden themselves, including via Retry-After [3]
that allow clients to back off gracefully when the service needs that,
instead of relying on clients to guess about the load the service
could handle and back off without insight into actual server capacity.

[1]: https://github.com/openshift/enhancements/blame/158111ce156aac7fa6063a47c00e129c13033aec/enhancements/update/targeted-update-edge-blocking.md#L323-L325
[2]: https://issues.redhat.com/browse/OCPBUGS-19512
[3]: https://www.rfc-editor.org/rfc/rfc9110#name-retry-after
wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 22, 2023
530a509 (pkg/cvo/availableupdates: Prioritize conditional risks for
largest target version, 2023-03-06, openshift#909) prioritized the order in
which risks were evaluated.  But we were still waiting 10 minutes
between different PromQL evaluations while evaluating conditional
update risks.  The original 10m requirement is from the enhancement
[1], and was implemented in ca186ed (pkg/clusterconditions/cache:
Add a cache wrapper for client-side throttling, 2021-11-10, openshift#663).
But discussing with Lala, Scott, and Ben, we feel like the addressing
the demonstrated user experience need of low-latency risk evaluation
[2] is worth reducing the throttling to 1s per expression evaluation.
We still have MinForCondition set to an hour, so with this commit, a
cluster-version operator evaluating three risks will move from a
timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 10m, evaluate B for the first time (MinBetweenMatches after 1).
3. 20m, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

to a timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatches after 1).
3. 2s, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h1s, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h2s, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h1s, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h2s, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

We could deliver faster cache warming while preserving spaced out
refresh evaluation by splitting MinBetweenMatches into a 1s
MinBetweenMatchesInitial and 10m MinBetweenMatchesWhenCached, which
would produce timelines like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatchesInitial after 1).
3. 2s, evaluate C for the first time (MinBetweenMatchesInitial after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatchesWhenCached after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatchesWhenCached after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatchesWhenCached after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatchesWhenCached after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatchesWhenCached after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatchesWhenCached after 8).

but again discussing with Lala, Scott, and Ben, the code complexity to
deliver that distinction does not seem to be worth thet protection it
delivers to the PromQL engine.  And really, PromQL engines concerned
about load should harden themselves, including via Retry-After [3]
that allow clients to back off gracefully when the service needs that,
instead of relying on clients to guess about the load the service
could handle and back off without insight into actual server capacity.

[1]: https://github.com/openshift/enhancements/blame/158111ce156aac7fa6063a47c00e129c13033aec/enhancements/update/targeted-update-edge-blocking.md#L323-L325
[2]: https://issues.redhat.com/browse/OCPBUGS-19512
[3]: https://www.rfc-editor.org/rfc/rfc9110#name-retry-after
wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 22, 2023
530a509 (pkg/cvo/availableupdates: Prioritize conditional risks for
largest target version, 2023-03-06, openshift#909) prioritized the order in
which risks were evaluated.  But we were still waiting 10 minutes
between different PromQL evaluations while evaluating conditional
update risks.  The original 10m requirement is from the enhancement
[1], and was implemented in ca186ed (pkg/clusterconditions/cache:
Add a cache wrapper for client-side throttling, 2021-11-10, openshift#663).
But discussing with Lala, Scott, and Ben, we feel like the addressing
the demonstrated user experience need of low-latency risk evaluation
[2] is worth reducing the throttling to 1s per expression evaluation.
We still have MinForCondition set to an hour, so with this commit, a
cluster-version operator evaluating three risks will move from a
timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 10m, evaluate B for the first time (MinBetweenMatches after 1).
3. 20m, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

to a timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatches after 1).
3. 2s, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h1s, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h2s, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h1s, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h2s, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

We could deliver faster cache warming while preserving spaced out
refresh evaluation by splitting MinBetweenMatches into a 1s
MinBetweenMatchesInitial and 10m MinBetweenMatchesWhenCached, which
would produce timelines like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatchesInitial after 1).
3. 2s, evaluate C for the first time (MinBetweenMatchesInitial after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatchesWhenCached after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatchesWhenCached after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatchesWhenCached after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatchesWhenCached after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatchesWhenCached after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatchesWhenCached after 8).

but again discussing with Lala, Scott, and Ben, the code complexity to
deliver that distinction does not seem to be worth thet protection it
delivers to the PromQL engine.  And really, PromQL engines concerned
about load should harden themselves, including via Retry-After [3]
that allow clients to back off gracefully when the service needs that,
instead of relying on clients to guess about the load the service
could handle and back off without insight into actual server capacity.

[1]: https://github.com/openshift/enhancements/blame/158111ce156aac7fa6063a47c00e129c13033aec/enhancements/update/targeted-update-edge-blocking.md#L323-L325
[2]: https://issues.redhat.com/browse/OCPBUGS-19512
[3]: https://www.rfc-editor.org/rfc/rfc9110#name-retry-after
wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 25, 2023
530a509 (pkg/cvo/availableupdates: Prioritize conditional risks for
largest target version, 2023-03-06, openshift#909) prioritized the order in
which risks were evaluated.  But we were still waiting 10 minutes
between different PromQL evaluations while evaluating conditional
update risks.  The original 10m requirement is from the enhancement
[1], and was implemented in ca186ed (pkg/clusterconditions/cache:
Add a cache wrapper for client-side throttling, 2021-11-10, openshift#663).
But discussing with Lala, Scott, and Ben, we feel like the addressing
the demonstrated user experience need of low-latency risk evaluation
[2] is worth reducing the throttling to 1s per expression evaluation.
We still have MinForCondition set to an hour, so with this commit, a
cluster-version operator evaluating three risks will move from a
timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 10m, evaluate B for the first time (MinBetweenMatches after 1).
3. 20m, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

to a timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatches after 1).
3. 2s, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h1s, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h2s, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h1s, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h2s, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

We could deliver faster cache warming while preserving spaced out
refresh evaluation by splitting MinBetweenMatches into a 1s
MinBetweenMatchesInitial and 10m MinBetweenMatchesWhenCached, which
would produce timelines like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatchesInitial after 1).
3. 2s, evaluate C for the first time (MinBetweenMatchesInitial after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatchesWhenCached after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatchesWhenCached after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatchesWhenCached after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatchesWhenCached after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatchesWhenCached after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatchesWhenCached after 8).

but again discussing with Lala, Scott, and Ben, the code complexity to
deliver that distinction does not seem to be worth thet protection it
delivers to the PromQL engine.  And really, PromQL engines concerned
about load should harden themselves, including via Retry-After [3]
that allow clients to back off gracefully when the service needs that,
instead of relying on clients to guess about the load the service
could handle and back off without insight into actual server capacity.

[1]: https://github.com/openshift/enhancements/blame/158111ce156aac7fa6063a47c00e129c13033aec/enhancements/update/targeted-update-edge-blocking.md#L323-L325
[2]: https://issues.redhat.com/browse/OCPBUGS-19512
[3]: https://www.rfc-editor.org/rfc/rfc9110#name-retry-after
wking added a commit to wking/cluster-version-operator that referenced this pull request Sep 25, 2023
530a509 (pkg/cvo/availableupdates: Prioritize conditional risks for
largest target version, 2023-03-06, openshift#909) prioritized the order in
which risks were evaluated.  But we were still waiting 10 minutes
between different PromQL evaluations while evaluating conditional
update risks.  The original 10m requirement is from the enhancement
[1], and was implemented in ca186ed (pkg/clusterconditions/cache:
Add a cache wrapper for client-side throttling, 2021-11-10, openshift#663).
But discussing with Lala, Scott, and Ben, we feel like the addressing
the demonstrated user experience need of low-latency risk evaluation
[2] is worth reducing the throttling to 1s per expression evaluation.
We still have MinForCondition set to an hour, so with this commit, a
cluster-version operator evaluating three risks will move from a
timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 10m, evaluate B for the first time (MinBetweenMatches after 1).
3. 20m, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

to a timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatches after 1).
3. 2s, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h1s, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h2s, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h1s, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h2s, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

We could deliver faster cache warming while preserving spaced out
refresh evaluation by splitting MinBetweenMatches into a 1s
MinBetweenMatchesInitial and 10m MinBetweenMatchesWhenCached, which
would produce timelines like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatchesInitial after 1).
3. 2s, evaluate C for the first time (MinBetweenMatchesInitial after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatchesWhenCached after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatchesWhenCached after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatchesWhenCached after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatchesWhenCached after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatchesWhenCached after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatchesWhenCached after 8).

but again discussing with Lala, Scott, and Ben, the code complexity to
deliver that distinction does not seem to be worth thet protection it
delivers to the PromQL engine.  And really, PromQL engines concerned
about load should harden themselves, including via Retry-After [3]
that allow clients to back off gracefully when the service needs that,
instead of relying on clients to guess about the load the service
could handle and back off without insight into actual server capacity.

[1]: https://github.com/openshift/enhancements/blame/158111ce156aac7fa6063a47c00e129c13033aec/enhancements/update/targeted-update-edge-blocking.md#L323-L325
[2]: https://issues.redhat.com/browse/OCPBUGS-19512
[3]: https://www.rfc-editor.org/rfc/rfc9110#name-retry-after
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/cluster-version-operator that referenced this pull request Sep 27, 2023
530a509 (pkg/cvo/availableupdates: Prioritize conditional risks for
largest target version, 2023-03-06, openshift#909) prioritized the order in
which risks were evaluated.  But we were still waiting 10 minutes
between different PromQL evaluations while evaluating conditional
update risks.  The original 10m requirement is from the enhancement
[1], and was implemented in ca186ed (pkg/clusterconditions/cache:
Add a cache wrapper for client-side throttling, 2021-11-10, openshift#663).
But discussing with Lala, Scott, and Ben, we feel like the addressing
the demonstrated user experience need of low-latency risk evaluation
[2] is worth reducing the throttling to 1s per expression evaluation.
We still have MinForCondition set to an hour, so with this commit, a
cluster-version operator evaluating three risks will move from a
timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 10m, evaluate B for the first time (MinBetweenMatches after 1).
3. 20m, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

to a timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatches after 1).
3. 2s, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h1s, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h2s, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h1s, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h2s, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

We could deliver faster cache warming while preserving spaced out
refresh evaluation by splitting MinBetweenMatches into a 1s
MinBetweenMatchesInitial and 10m MinBetweenMatchesWhenCached, which
would produce timelines like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatchesInitial after 1).
3. 2s, evaluate C for the first time (MinBetweenMatchesInitial after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatchesWhenCached after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatchesWhenCached after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatchesWhenCached after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatchesWhenCached after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatchesWhenCached after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatchesWhenCached after 8).

but again discussing with Lala, Scott, and Ben, the code complexity to
deliver that distinction does not seem to be worth thet protection it
delivers to the PromQL engine.  And really, PromQL engines concerned
about load should harden themselves, including via Retry-After [3]
that allow clients to back off gracefully when the service needs that,
instead of relying on clients to guess about the load the service
could handle and back off without insight into actual server capacity.

[1]: https://github.com/openshift/enhancements/blame/158111ce156aac7fa6063a47c00e129c13033aec/enhancements/update/targeted-update-edge-blocking.md#L323-L325
[2]: https://issues.redhat.com/browse/OCPBUGS-19512
[3]: https://www.rfc-editor.org/rfc/rfc9110#name-retry-after
wking added a commit to wking/cluster-version-operator that referenced this pull request Oct 25, 2023
530a509 (pkg/cvo/availableupdates: Prioritize conditional risks for
largest target version, 2023-03-06, openshift#909) prioritized the order in
which risks were evaluated.  But we were still waiting 10 minutes
between different PromQL evaluations while evaluating conditional
update risks.  The original 10m requirement is from the enhancement
[1], and was implemented in ca186ed (pkg/clusterconditions/cache:
Add a cache wrapper for client-side throttling, 2021-11-10, openshift#663).
But discussing with Lala, Scott, and Ben, we feel like the addressing
the demonstrated user experience need of low-latency risk evaluation
[2] is worth reducing the throttling to 1s per expression evaluation.
We still have MinForCondition set to an hour, so with this commit, a
cluster-version operator evaluating three risks will move from a
timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 10m, evaluate B for the first time (MinBetweenMatches after 1).
3. 20m, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

to a timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatches after 1).
3. 2s, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h1s, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h2s, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h1s, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h2s, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

We could deliver faster cache warming while preserving spaced out
refresh evaluation by splitting MinBetweenMatches into a 1s
MinBetweenMatchesInitial and 10m MinBetweenMatchesWhenCached, which
would produce timelines like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatchesInitial after 1).
3. 2s, evaluate C for the first time (MinBetweenMatchesInitial after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatchesWhenCached after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatchesWhenCached after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatchesWhenCached after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatchesWhenCached after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatchesWhenCached after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatchesWhenCached after 8).

but again discussing with Lala, Scott, and Ben, the code complexity to
deliver that distinction does not seem to be worth thet protection it
delivers to the PromQL engine.  And really, PromQL engines concerned
about load should harden themselves, including via Retry-After [3]
that allow clients to back off gracefully when the service needs that,
instead of relying on clients to guess about the load the service
could handle and back off without insight into actual server capacity.

[1]: https://github.com/openshift/enhancements/blame/158111ce156aac7fa6063a47c00e129c13033aec/enhancements/update/targeted-update-edge-blocking.md#L323-L325
[2]: https://issues.redhat.com/browse/OCPBUGS-19512
[3]: https://www.rfc-editor.org/rfc/rfc9110#name-retry-after
wking added a commit to wking/cluster-version-operator that referenced this pull request Oct 25, 2023
530a509 (pkg/cvo/availableupdates: Prioritize conditional risks for
largest target version, 2023-03-06, openshift#909) prioritized the order in
which risks were evaluated.  But we were still waiting 10 minutes
between different PromQL evaluations while evaluating conditional
update risks.  The original 10m requirement is from the enhancement
[1], and was implemented in ca186ed (pkg/clusterconditions/cache:
Add a cache wrapper for client-side throttling, 2021-11-10, openshift#663).
But discussing with Lala, Scott, and Ben, we feel like the addressing
the demonstrated user experience need of low-latency risk evaluation
[2] is worth reducing the throttling to 1s per expression evaluation.
We still have MinForCondition set to an hour, so with this commit, a
cluster-version operator evaluating three risks will move from a
timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 10m, evaluate B for the first time (MinBetweenMatches after 1).
3. 20m, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

to a timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatches after 1).
3. 2s, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h1s, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h2s, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h1s, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h2s, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

We could deliver faster cache warming while preserving spaced out
refresh evaluation by splitting MinBetweenMatches into a 1s
MinBetweenMatchesInitial and 10m MinBetweenMatchesWhenCached, which
would produce timelines like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatchesInitial after 1).
3. 2s, evaluate C for the first time (MinBetweenMatchesInitial after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatchesWhenCached after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatchesWhenCached after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatchesWhenCached after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatchesWhenCached after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatchesWhenCached after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatchesWhenCached after 8).

but again discussing with Lala, Scott, and Ben, the code complexity to
deliver that distinction does not seem to be worth thet protection it
delivers to the PromQL engine.  And really, PromQL engines concerned
about load should harden themselves, including via Retry-After [3]
that allow clients to back off gracefully when the service needs that,
instead of relying on clients to guess about the load the service
could handle and back off without insight into actual server capacity.

[1]: https://github.com/openshift/enhancements/blame/158111ce156aac7fa6063a47c00e129c13033aec/enhancements/update/targeted-update-edge-blocking.md#L323-L325
[2]: https://issues.redhat.com/browse/OCPBUGS-19512
[3]: https://www.rfc-editor.org/rfc/rfc9110#name-retry-after
wking added a commit to wking/cluster-version-operator that referenced this pull request Nov 2, 2023
530a509 (pkg/cvo/availableupdates: Prioritize conditional risks for
largest target version, 2023-03-06, openshift#909) prioritized the order in
which risks were evaluated.  But we were still waiting 10 minutes
between different PromQL evaluations while evaluating conditional
update risks.  The original 10m requirement is from the enhancement
[1], and was implemented in ca186ed (pkg/clusterconditions/cache:
Add a cache wrapper for client-side throttling, 2021-11-10, openshift#663).
But discussing with Lala, Scott, and Ben, we feel like the addressing
the demonstrated user experience need of low-latency risk evaluation
[2] is worth reducing the throttling to 1s per expression evaluation.
We still have MinForCondition set to an hour, so with this commit, a
cluster-version operator evaluating three risks will move from a
timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 10m, evaluate B for the first time (MinBetweenMatches after 1).
3. 20m, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

to a timeline like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatches after 1).
3. 2s, evaluate C for the first time (MinBetweenMatches after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatches after 3).
5. 1h1s, evaluate B again (MinForCondition after 2 and MinBetweenMatches after 4).
6. 1h2s, evaluate C again (MinForCondition after 3 and MinBetweenMatches after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatches after 6).
8. 2h1s, evaluate B again (MinForCondition after 5 and MinBetweenMatches after 7).
9. 2h2s, evaluate C again (MinForCondition after 6 and MinBetweenMatches after 8).

We could deliver faster cache warming while preserving spaced out
refresh evaluation by splitting MinBetweenMatches into a 1s
MinBetweenMatchesInitial and 10m MinBetweenMatchesWhenCached, which
would produce timelines like:

1. 0s, hear about risks that depend on PromQL A, B, and C.  Evaluate A for the first time.
2. 1s, evaluate B for the first time (MinBetweenMatchesInitial after 1).
3. 2s, evaluate C for the first time (MinBetweenMatchesInitial after 2).
4. 1h, evaluate A again (MinForCondition after 1, also well past MinBetweenMatchesWhenCached after 3).
5. 1h10m, evaluate B again (MinForCondition after 2 and MinBetweenMatchesWhenCached after 4).
6. 1h20m, evaluate C again (MinForCondition after 3 and MinBetweenMatchesWhenCached after 5).
7. 2h, evaluate A again (MinForCondition after 4, also well past MinBetweenMatchesWhenCached after 6).
8. 2h10m, evaluate B again (MinForCondition after 5 and MinBetweenMatchesWhenCached after 7).
9. 2h20m, evaluate C again (MinForCondition after 6 and MinBetweenMatchesWhenCached after 8).

but again discussing with Lala, Scott, and Ben, the code complexity to
deliver that distinction does not seem to be worth thet protection it
delivers to the PromQL engine.  And really, PromQL engines concerned
about load should harden themselves, including via Retry-After [3]
that allow clients to back off gracefully when the service needs that,
instead of relying on clients to guess about the load the service
could handle and back off without insight into actual server capacity.

[1]: https://github.com/openshift/enhancements/blame/158111ce156aac7fa6063a47c00e129c13033aec/enhancements/update/targeted-update-edge-blocking.md#L323-L325
[2]: https://issues.redhat.com/browse/OCPBUGS-19512
[3]: https://www.rfc-editor.org/rfc/rfc9110#name-retry-after
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.

5 participants