ci-operator/step-registry/ipi/conf: Split out per-platform steps #7625
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
size/XL
wking
force-pushed
the
per-platform-conf-steps
branch
from
7d00a4c
to
c335498
Compare
dcbw
reviewed
Mar 12, 2020
ci-operator/step-registry/ipi/conf/generic/ipi-conf-generic-commands.sh
Outdated
Show resolved
Hide resolved
wking
force-pushed
the
per-platform-conf-steps
branch
from
c335498
to
c2982ee
Compare
|
So, if we were to add bits that switch the network plugin to OVN, given: we could do: |
wking
force-pushed
the
per-platform-conf-steps
branch
from
c2982ee
to
3807f1a
Compare
With my preference for a single chain, this would be either: chain:
as: ipi-conf
steps:
- ref: ipi-conf-generic
- ref: ipi-conf-aws
- ref: ipi-conf-azure
- ref: ipi-conf-gcp
- ref: ipi-conf-ovn
documentation: |-
The IPI configure step chain generates the install-config.yaml file based on the cluster profile and optional input files.Or a |
wking
force-pushed
the
per-platform-conf-steps
branch
2 times, most recently
from
21507d4
to
05393a0
Compare
@wking sure, that would work for a simple case. But we also need to address additional network configuration:
|
|
@wking different question, is there any way to move the bits in into the conf steps rather than install? |
|
@dcbw, I've pushed fca8b6efeec with a stab at an OVN/networking step. How's that look to you? |
We don't want to move those into the conf step, because you need the creds for installation, not for creating the |
wking
force-pushed
the
per-platform-conf-steps
branch
from
fca8b6e
to
055e091
Compare
dcbw
reviewed
Mar 12, 2020
ci-operator/step-registry/ipi/conf/networking/ipi-conf-networking-commands.sh
Outdated
Show resolved
Hide resolved
dcbw
reviewed
Mar 12, 2020
ci-operator/step-registry/ipi/conf/networking/ipi-conf-networking-commands.sh
Outdated
Show resolved
Hide resolved
wking
force-pushed
the
per-platform-conf-steps
branch
from
055e091
to
df5d688
Compare
dcbw
reviewed
Mar 12, 2020
ci-operator/step-registry/ipi/conf/networking/ipi-conf-networking-commands.sh
Outdated
Show resolved
Hide resolved
Sounds good to me. Can I punt on that for this PR? I think this PR is a clear enough example of how we can split platform/variant-specific functionality into separate steps while keeping a single, unified chain. I'd rather not grow it outside of |
wking
force-pushed
the
per-platform-conf-steps
branch
from
df5d688
to
d589e74
Compare
ci-operator/step-registry/ipi/conf/aws/ipi-conf-aws-commands.sh
Outdated
Show resolved
Hide resolved
| approvers: | ||
| - smarterclayton | ||
| - wking | ||
| - stevekuznetsov |
There was a problem hiding this comment.
I am blindly copying from the parent directory, because some CI presubmit required per-directory OWNERS here. If you have suggested subsets or different sets or whatever. Or just want me to set an empty/dummy OWNERS to make the presubmit job happy, let me know.
There was a problem hiding this comment.
If you have suggested subsets or different sets or whatever.
What I really want here is access to the installer's gcp-approvers alias, but since we're no longer mirroring aliases into this repo (here and, internally, here), I can't do that.
wking
force-pushed
the
per-platform-conf-steps
branch
from
d589e74
to
cd129fb
Compare
|
Blocked on #7629 fixing the existing steps. |
@wking yes for sure. that's a separate step and @JacobTanenbaum can take that on |
wking
force-pushed
the
per-platform-conf-steps
branch
from
cd129fb
to
2e2e404
Compare
|
Rebased around #7629 with cd129fb -> 2e2e404. |
|
I've pushed 4cd536e with an attempt at pivoting to @stevekuznetsov's preferred tailored-workflow approach (as I understand it, I may be a bit off-target) and dropping unused steps (which can be dragged back up out of Git if/when they are needed). |
ci-operator/step-registry/ipi/conf/aws/ipi-conf-aws-commands.sh
Outdated
Show resolved
Hide resolved
wking
force-pushed
the
per-platform-conf-steps
branch
2 times, most recently
from
1e5e858
to
eacc308
Compare
wking
added a commit
to wking/openshift-release
that referenced
this pull request
Mar 13, 2020
It's been 1000m since it landed in de3de20 (step-registry: add configure and install IPI steps, 2020-01-14, openshift#6708), but it's a pretty simple container (just write a config file), so Steve suggestes 10m as more appropriate [1]. [1]: openshift#7625 (comment)
wking
force-pushed
the
per-platform-conf-steps
branch
2 times, most recently
from
5d8cf99
to
e673d09
Compare
|
@wking: the implementation of the separation LGTM. We're done with the back-end improvements in From your perspective, are there any remaining issues that would prevent us from replacing the definition of
WDYT? |
To make it easier to add new platforms, by avoiding a giant platform switch statement in a single, long configuration file. Also conditionally add the fips setting. And switch to tab-indented Bash for ipi-conf-generic-commands.sh, so I can indent the here-document payload with <<- [1]. Personally, I'd use tab-intents for all of this shell, but I've left the four-space indents alone in the other files to decrease churn. [1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_04
To support the ovn and ipv6 variants, as well as folks providing their own custom network type directly via a cluster-network-type.txt file.
wking
force-pushed
the
per-platform-conf-steps
branch
from
e673d09
to
8727d36
Compare
Why not? Doesn't seem like a big deal to rename it, and the work is already done. |
wking
force-pushed
the
per-platform-conf-steps
branch
from
8727d36
to
5a22630
Compare
And drop all the variant stuff out. Personally, I prefer a single chain with a variant knob to make it easy to say "I don't care, pick something for me" without having to ask for a different workflow. But Steve prefers having workflows and jobs tailored to specific provider/variant combinations. This commit is an attempt to move in that direction. I've dropped the unused refs, but they can be dug back up out of Git if/when there are jobs that consume them (e.g. GCP jobs, OVN jobs, pre-existing subnet jobs, etc.). Shifting the job definitions to their new alphabetical location is required by the generated-config presubmit [1]. [1]: https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_release/7625/pull-ci-openshift-release-master-generated-config/17978
wking
force-pushed
the
per-platform-conf-steps
branch
from
5a22630
to
6db1f86
Compare
stevekuznetsov
approved these changes
Mar 17, 2020
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stevekuznetsov, 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@wking: Updated the following 30 configmaps:
In 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. |
petr-muller
added a commit
to petr-muller/rh-op-ecosystem-release
that referenced
this pull request
Mar 17, 2020
The `ipi` workflow was removed in openshift/release#7625 and is reintroduced as `ipi-aws` in openshift/release#7734 (hopefully). So retarget the AWS test to the `ipi-aws` workflow, and for now delete the GCP test because it does not have its workflow yet.
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
petr-muller
added a commit
to petr-muller/rh-op-ecosystem-release
that referenced
this pull request
Mar 18, 2020
The `ipi` workflow was removed in openshift/release#7625 and is reintroduced as `ipi-aws` in openshift/release#7734 (hopefully). So retarget the AWS test to the `ipi-aws` workflow, and for now delete the GCP test because it does not have its workflow yet.
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.
To make it easier to add new platforms, by avoiding a giant platform switch statement in a single, long configuration file.