MON-1693: Add periodics to verify clusters with UMW can be safely upgraded #19545
Conversation
This file contains hidden or 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
vrutkovs
force-pushed
the
umw-upgrade-workflow
branch
from
ca57926 to
edb6a77
Compare
vrutkovs
changed the title
wking
reviewed
Jun 22, 2021
Member
There was a problem hiding this comment.
Existing step touching this ConfigMap:
$ git --no-pager grep cluster-monitoring-config ci-operator/step-registry/
ci-operator/step-registry/ipi/install/monitoringpvc/ipi-install-monitoringpvc-commands.sh: name: cluster-monitoring-configYou'll probably want both steps to try to be more clever about YAML parsing (via Python?) to manage just the settings that their step is about, instead of having each of them claiming complete ownership of the config and possibly stomping the other.
Contributor
Author
There was a problem hiding this comment.
Oh, hmm, this one is critical for upgrades.
I'll use yq to add modifications
vrutkovs
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
There was a problem hiding this comment.
You don't need upgrade in the name, everything in this file is an upgrade. e2e-aws-umw
Contributor
Author
There was a problem hiding this comment.
I'd prefer to have upgrade to be in the prowjob name, without it folks may assume its an install job
Member
There was a problem hiding this comment.
Won't the prow job get called ci-openshift-release-master-ci-4.7-upgrade-from-stable-4.6-e2e-aws-umw-upgrade, i.e. upgrade appears twice?
Contributor
Author
There was a problem hiding this comment.
Ah, okay, sure. Renameing this
vrutkovs
force-pushed
the
umw-upgrade-workflow
branch
5 times, most recently
from
80ff9d7 to
2d38c08
Compare
vrutkovs
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
Ready for review, this updates both steps to add modifications to a single manifest using |
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
vrutkovs
force-pushed
the
umw-upgrade-workflow
branch
from
2d38c08 to
5189b37
Compare
wking
reviewed
Jun 24, 2021
ci-operator/step-registry/ipi/conf/user-workload-monitoring/OWNERS
Outdated
Show resolved
Hide resolved
wking
reviewed
Jun 24, 2021
Member
There was a problem hiding this comment.
nit: Can we use JSON / jq for the manifest, so we don't have to curl-install yq?
Contributor
Author
There was a problem hiding this comment.
I guess that would hamper the readability - and we've already settled on yaml manifests in most steps. Some other steps are pulling yq too, so I guess its "canon" now :)
wking
reviewed
Jun 24, 2021
Member
There was a problem hiding this comment.
the config.yaml property is a string, right? Is yq smart enough to say "ah, the body of this string is also YAML, let me parse as YAML and add the new keys..."?
Contributor
Author
There was a problem hiding this comment.
Yup, it merges keys as YAML. Let me add a step to setup PVC for these jobs too
Contributor
Author
There was a problem hiding this comment.
Ah, I was wrong, we have to extract it ourselves:
vrutkovs
force-pushed
the
umw-upgrade-workflow
branch
8 times, most recently
from
e890f37 to
e09a85d
Compare
Contributor
Author
|
/retest |
openshift-ci
bot
added
the
needs-rebase
vrutkovs
force-pushed
the
umw-upgrade-workflow
branch
from
0e877f1 to
8075c0b
Compare
openshift-ci
bot
removed
the
needs-rebase
vrutkovs
force-pushed
the
umw-upgrade-workflow
branch
2 times, most recently
from
8e49c0a to
01c769a
Compare
vrutkovs
force-pushed
the
umw-upgrade-workflow
branch
2 times, most recently
from
1ceb57a to
451d7f7
Compare
wking
reviewed
Aug 3, 2021
ci-operator/step-registry/ipi/OWNERS
Outdated
Member
There was a problem hiding this comment.
Does it make sense to drop TRT in here, or one level up to cover the whole step registry?
Contributor
Author
There was a problem hiding this comment.
I'd prefer to keep it here. We can rearrange owners when TRT is ready to handle the whole step registry
wking
reviewed
Aug 3, 2021
Member
There was a problem hiding this comment.
nit: seems like holding the patch in the shared directory is traditional, but I don't see why we'd ever want to share patches. I've filed #20865 to fix our existing use-cases, so you should be able to fix this without worrying about diverging from the pattern ;).
wking
approved these changes
Aug 4, 2021
Member
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
Just the two nits I mentioned above, and I'm fine if those get punted on. I'm also not an approver for the release jobs, so:
/lgtm
and there's time to circle back and address the nits if you want before we can get a release-job approver over to approve this.
vrutkovs
force-pushed
the
umw-upgrade-workflow
branch
from
451d7f7 to
7e13e21
Compare
vrutkovs
force-pushed
the
umw-upgrade-workflow
branch
from
7e13e21 to
13a30eb
Compare
These periodics would help us catch issues like https://bugzilla.redhat.com/show_bug.cgi?id=1953518
vrutkovs
force-pushed
the
umw-upgrade-workflow
branch
from
d184a73 to
5f73bc5
Compare
This step would ensure cluster is configured with user workload monitoring feature enabled
vrutkovs
force-pushed
the
umw-upgrade-workflow
branch
from
5f73bc5 to
7ef62d8
Compare
Contributor
|
@vrutkovs: The following tests failed, say
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. |
2 tasks
Member
|
/approve /lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, vrutkovs, 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 |
openshift-ci
bot
added
the
approved
Contributor
|
@vrutkovs: Updated the following 3 configmaps:
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
These periodics would help us catch issues like https://bugzilla.redhat.com/show_bug.cgi?id=1953518