graph-builder: add support for configuration file #69
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
approved
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
needs-rebase
|
Rebased to master and added graph-builder configuration options for #72 (comment). |
steveej
reviewed
Mar 19, 2019
lucab
commented
Mar 19, 2019
lucab
force-pushed
the
ups/config-toml
branch
2 times, most recently
from
24e4382 to
f58304e
Compare
steveej
reviewed
Mar 20, 2019
lucab
force-pushed
the
ups/config-toml
branch
3 times, most recently
from
59eba02 to
d5206e0
Compare
|
@steveej I did a major rework on the PR in the direction of #69 (comment). This means that the While this definitely still needs some polishing, can you please another pass to see if suits your taste better than the previous iteration? |
lucab
commented
Apr 1, 2019
steveej
reviewed
Apr 2, 2019
There was a problem hiding this comment.
The overall approach looks very clean to me, especially the simplicity of the assemble function.
I would've wished for some code generation (macros) for the merge implementations but I wouldn't block on that if there exists no ready-made solution right now.
I have added a couple of comments to be addressed and there are also some leftover from the previous review. Thanks for exploring this so thoroughly!
steveej
reviewed
Apr 2, 2019
lucab
force-pushed
the
ups/config-toml
branch
3 times, most recently
from
f2d6f09 to
922ed46
Compare
lucab
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
I added the final tests and documentation, completing the last TODOs. Dropped the WIP label, this is ready for the last round of review. |
steveej
previously requested changes
Apr 9, 2019
This updates cincinnati pod definition with newer CLI options names.
This adds a user-doc listing graph-builder TOML configuration options.
|
Pushed another update addressing last comments, PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lucab, steveeJ 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:
Approvers can indicate their approval by writing |
wking
added a commit
to wking/cincinnati
that referenced
this pull request
Apr 3, 2020
The default seems to have lived in the 30s range since at least 517af6e (graph-builder: add support for configuration file, 2019-03-12, openshift#69). But we can allow some latency here. On the cluster-version operator (CVO) side: * cvo.New takes an argument for the poll interval [1]. * That is fed by resyncPeriod(o.ResyncInterval)() [2]. * ResyncInterval is set from minResyncPeriod [3]. * minResyncPeriod is 2 minutes [4]. * resyncPeriod adds some jitter (returning between 1x and 2x the input value) [5]. So should be uniformly distributed between 2 and 4 minutes. That puts a floor on latency in getting this information out to clusters. It's also not a problem to add latency above the CVO-enforced floor. A delay in minutes in getting a new release out into a channel is not significant, because the biggest publication rush would be releasing a CVE fix into the stable channel. And that's already delayed by needing to cook in the candidate and fast channels, and eventually having the push to stable being a phased rollout. A delay in pulling an edge that has proven itself unstable is more serious, but we already have latencies in the minutes before alerts fire, and Telemetry/Insights that report issues to Red Hat add additional latencies in the minutes (Telemetry) to hours (Insights) range. Delaying the edge pull by a few more minutes is not going to greatly increase overall cluster-breaks -> edge-pulled latency here. The above discussion hopefully shows that there is little downside to growing the latency up to several minutes. With this commit, the delay is 5 minutes plus the time it takes the plugins to execute (I'd initially argued for 10m, but was bargained back down to 5 [6]). The upside is that it decreases our load on our external dependencies (quay.io, GitHub), reducing issues like: [2020-03-31T21:35:16Z DEBUG graph_builder::graph] graph update triggered [2020-03-31T21:35:20Z ERROR graph_builder::graph] Checking for new commit [2020-03-31T21:35:20Z ERROR graph_builder::graph] Parsing {"message":"API rate limit exceeded for user ID ...","documentation_url":"https://developer.github.com/v3/#rate-limiting"} to Vec<Branch> [2020-03-31T21:35:20Z ERROR graph_builder::graph] invalid type: map, expected a sequence at line 1 column 0 [2020-03-31T21:35:50Z DEBUG graph_builder::graph] graph update triggered It's also reduces our CPU and network load, since the graph-builder pod spends more time resting in between recalculate-the-graph spikes. Just the load consideration is enough for me to want to tune this latency as high as we can while keeping it well below other edge-pulling latency components. [1]: https://github.com/openshift/cluster-version-operator/blob/12623332615a0657a5468fae27cff1998a70bfef/pkg/cvo/cvo.go#L165 [2]: https://github.com/openshift/cluster-version-operator/blob/12623332615a0657a5468fae27cff1998a70bfef/pkg/start/start.go#L341 [3]: https://github.com/openshift/cluster-version-operator/blob/12623332615a0657a5468fae27cff1998a70bfef/pkg/start/start.go#L93 [4]: https://github.com/openshift/cluster-version-operator/blob/12623332615a0657a5468fae27cff1998a70bfef/pkg/start/start.go#L43 [5]: https://github.com/openshift/cluster-version-operator/blob/12623332615a0657a5468fae27cff1998a70bfef/pkg/start/start.go#L250-L251 [6]: openshift#264 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reworks plugins setup and graph-builder CLI parser to allow for sourcing complex/structured options from configuration file(s). In particular, configuration logic now cares about the following entities:
Ref: https://jira.coreos.com/browse/CORS-967
/cc @steveej