Bug 1991730: fixing zones mismatch issue #20359
Conversation
|
/retest |
e-tienne
left a comment
e-tienne
left a comment
There was a problem hiding this comment.
Glad you found this issue!
I think we should use the same zones across the board for all the steps.
Right not, we are using still using different zones in ipi-conf-aws and ipi-conf-aws-blackholenetwork
|
@wking Is there some other things need I do to merge this PR? |
...erator/step-registry/ipi/conf/aws/blackholenetwork/ipi-conf-aws-blackholenetwork-commands.sh
Show resolved
Hide resolved
| - name: ZONES_COUNT | ||
| default: "2" | ||
| documentation: |- | ||
| The number of AZs to present in the cluster. Accepted values are 1 and 3. |
There was a problem hiding this comment.
nit: 2 is the default, so it's not that likely that callers would set it explicitly. But it's probably worth including it in the set of acceptable values anyway, like Accepted values are 1, 2, and 3..
| mapfile -t AVAILABILITY_ZONES < <(aws --region "${REGION}" ec2 describe-availability-zones | jq -r '.AvailabilityZones[] | select(.State == "available") | .ZoneName' | sort -u) | ||
| ZONES=("${AVAILABILITY_ZONES[@]:0:${ZONES_COUNT}}") | ||
| ZONES_STR="[ " | ||
| ZONES_STR+=$(join_by , "${ZONES[@]}") |
There was a problem hiding this comment.
nit with the existing master code you're copying here: this would be simpler as:
ZONES_STR="[ $(join_by , "${ZONES[@]}") ]"… step pull the zones off the pre-existing infra
…nerating step pull the zones off the pre-existing infra
|
re-testing e2e-aws-proxy for consistency |
|
@e-tienne: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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. |
|
/test pj-rehearse |
|
I polish this PR to put the blackhole step first, and have the install-config-generating step pull the zones off the pre-existing infra. Go through the ci jobs triggered by ci robot, the failed job sound like not related to this PR change. Also https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_release/20359/rehearse-20359-pull-ci-openshift-installer-release-4.9-e2e-aws-shared-vpc/1422474381963038720 and https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_release/20359/rehearse-20359-periodic-ci-openshift-release-master-nightly-4.9-e2e-aws-proxy/1422474399247765504 proved the PR is working well. Pls have a review. |
ci-operator/step-registry/ipi/conf/aws/ipi-conf-aws-commands.sh
Outdated
Show resolved
Hide resolved
|
/approve |
|
Confirmed e2e-aws-proxy and e2e-aws-shared-vpc built the infra successfully again; will monitor future jobs for consistency. |
ci-operator/step-registry/ipi/conf/aws/ipi-conf-aws-commands.sh
Outdated
Show resolved
Hide resolved
openshift-ci
bot
removed
the
needs-rebase
| fi | ||
|
|
||
| cat >> "${CONFIG}" << EOF | ||
| PATCH="${SHARED_DIR}/install-config-common.yaml.patch" |
There was a problem hiding this comment.
nit: I think we want PATCH to live in /tmp, see #20865 where I try to fix up our existing steps. I'm fine rebasing my PR if yours lands first with the SHARED_DIR here, though.
There was a problem hiding this comment.
I thought I can find these patch file in job's artifacts for quick troubleshooting, but sound like I did not find out them. So I am okay with moving these patch files to /tmp.
Yeah, I prefer to land this PR firstly, then your rebase them. Thanks in advance.
ci-operator/step-registry/ipi/conf/aws/ipi-conf-aws-commands.sh
Outdated
Show resolved
Hide resolved
|
@wking I think I already resolved all the issues mentioned in review, can you help double confirm? Or some other things I missed to move forward for merging? |
|
I see this failing a bunch on aws-proxy jobs the last few days: |
|
/retitle Bug 1991730: fixing zones mismatch issue |
openshift-ci
bot
changed the title
openshift-ci
bot
added
the
bugzilla/severity-high
|
@jianlinliu: This pull request references Bugzilla bug 1991730, which is invalid:
Comment 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/invalid-bug
|
/bugzilla refresh |
openshift-ci
bot
added
the
bugzilla/valid-bug
|
@stbenjam: This pull request references Bugzilla bug 1991730, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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
removed
the
bugzilla/invalid-bug
|
/assign |
|
@jianlinliu: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, e-tienne, jianlinliu, vrutkovs 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
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
close this PR, use #21145 to replace this one. |
|
@jianlinliu: This pull request references Bugzilla bug 1991730. The bug has been updated to no longer refer to the pull request using the external bug tracker. 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. |
7 participants
In https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/periodic-ci-openshift-release-master-nightly-4.9-e2e-aws-proxy/1415490000736227328, get this error:
That is because in ./ci-operator/step-registry/ipi/conf/aws/ipi-conf-aws-commands.sh, it randomly selected "us-east-1f", "us-east-1a" as test target zones, but in the following ./ci-operator/step-registry/ipi/conf/aws/blackholenetwork/ipi-conf-aws-blackholenetwork-commands.sh, it is using vpc cf to create subnets, while the cf is creating subnets for zones in order, e.g: when AvailabilityZoneCount = 2, the cf will select the top 2 zones to create subnets, that means it is not random, that lead to no subnets created for "us-east-1f", so the error is thrown out.
This PR is fixing that issue