Bug 2033745: pkg/cvo/availableupdates: Acount for default upstream in recent-change throttling #718
Conversation
wking
force-pushed
the
default-upstream-in-recent-update-throttling
branch
from
ae7560a to
1640c28
Compare
openshift-ci
bot
added
the
approved
…e throttling
Since [1], clusters born in 4.7 or later default to not having an
explicit spec.upstream, so they rely on the CVO's internal default.
However, in that case, u.Upstream will be an empty string, while by
this point in syncAvailableUpdates, upstream will have been set to the
default value. This commit splits up the "do we really need to
check?" logic into a number of distinct cases, and gives them more
specific logging, to make it easier to understand and confirm the
desired behavior:
a. If we have no cached data, we need to pull a graph.
b. If it's been over minimumUpdateCheckInterval since our last check,
we need to pull a graph. Even if nothing has changed on our side,
our data is sufficiently stale to need a refresh.
c. If the channel has changed, we have different interests, and we
need to pull a graph to hear what the upstream recommends for this
new set of interests.
d. If the upstream hasn't changed, because:
i. The current upstream (explicitly or by default) matches the old
explicit upstream, or
ii. The current upstream (explicitly or by default) matches the
default, and the old upstream was unset.
then everything's the same on our side, and our cached graph is
recent, so we don't need to do anything.
e. Otherwise, the upstream has changed, and we need to pull a graph to
see what our new guide has to suggest.
Cases for upstream:
* A -> A: Handled by d.i.
* A -> B: Handled by e.
* A -> unset (defaulted) or default: Handled by e.
* Unset or default -> A: Handled by e.
* Default -> default: Handled in d.i.
* Default -> unset (defaulted): Handled in d.i.
* Unset -> default: Handled by d.ii, new in this commit, previously
resulted in an excessive pull.
* Unset -> unset (defaulted): Handled by d.ii, new in this commit,
previously resulted in an excessive pull.
[1]: openshift/installer#4112
wking
force-pushed
the
default-upstream-in-recent-update-throttling
branch
from
1640c28 to
36c670f
Compare
wking
changed the title
openshift-ci
bot
added
the
bugzilla/severity-low
|
@wking: This pull request references Bugzilla bug 2033745, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
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
bot
added
the
bugzilla/valid-bug
| if u != nil && u.Upstream == upstream && u.Channel == channel && u.RecentlyChanged(optr.minimumUpdateCheckInterval) { | ||
| klog.V(4).Infof("Available updates were recently retrieved, will try later.") | ||
| if u == nil { | ||
| klog.V(4).Info("First attempt to retrieve available updates") |
There was a problem hiding this comment.
Why not add upstream url for logging?
There was a problem hiding this comment.
We log that later, when we actually make the request:
$ git --no-pager grep 'to request updates from' pkg/cincinnati
pkg/cincinnati/cincinnati.go: klog.V(5).Infof("Using a root CA pool with 0 root CA subjects to request updates from %s", uri)
pkg/cincinnati/cincinnati.go: klog.V(5).Infof("Using a root CA pool with %n root CA subjects to request updates from %s", len(c.transport.TLSClientConfig.RootCAs.Subjects()), uri)
pkg/cincinnati/cincinnati.go: klog.V(5).Infof("Using proxy %s to request updates from %s", proxy.Host, uri)In between this line and that, we can still bail out for reasons like "channel is empty"
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@wking: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
@wking: All pull requests linked via external trackers have merged: Bugzilla bug 2033745 has been moved to the MODIFIED state. DetailsIn response to this:
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. |
3 participants
Since openshift/installer#4112, clusters born in 4.7 or later default to not having an explicit spec.upstream, so they rely on the CVO's internal default. However, in that case,
u.Upstreamwill be an empty string, while by this point insyncAvailableUpdates,upstreamwill have been set to the default value. This commit splits up the "do we really need to check?" logic into a number of distinct cases, and gives them more specific logging, to make it easier to understand and confirm the desired behavior:a. If we have no cached data, we need to pull a graph.
b. If it's been over
minimumUpdateCheckIntervalsince our last check, we need to pull a graph. Even if nothing has changed on our side, our data is sufficiently stale to need a refresh.c. If the
channelhas changed, we have different interests, and we need to pull a graph to hear what theupstreamrecommends for this new set of interests.d. If the
upstreamhasn't changed, because:i. The current
upstream(explicitly or by default) matches the old explicitupstream, orii. The current
upstream(explicitly or by default) matches the default, and the oldupstreamwas unset.then everything's the same on our side, and our cached graph is recent, so we don't need to do anything.
e. Otherwise, the
upstreamhas changed, and we need to pull a graph to see what our new guide has to suggest.Cases for upstream: