Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openshift/installer: add IaaS-agnostic E2E test #4148

Merged
merged 1 commit into from
Jun 26, 2019

Conversation

bbguimaraes
Copy link
Contributor

@bbguimaraes bbguimaraes commented Jun 21, 2019

Generated using openshift/ci-tools#6.

I've created the CM manually until we decide how to manage it:

oc -n ci create -f - <<'EOF'
apiVersion: v1
kind: ConfigMap
metadata:
  name: e2e-targets
data:
  e2e-targets: |
    0.25 aws
    0.25 aws-upi
    0.25 azure
    0.25 vsphere
EOF

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 21, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2019
set -eux
target=$(awk < /usr/local/e2e-targets \
--assign "r=$RANDOM" \
'BEGIN { r /= 32767 } (r -= $1) <= 0 { print $2; exit }')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbguimaraes curious - what does 32767 represent? in other words how you picked this value ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.gnu.org/software/bash/manual/html_node/Bash-Variables.html#index-RANDOM

RANDOM

Each time this parameter is referenced, a random integer between 0 and 32767 is generated. Assigning a value to this variable seeds the random number generator.

We want a value in the range [0, 1] because that's how the ratios are expressed in the ConfigMap. That is accomplished by dividing the value we get from $RANDOM by the maximum value we can get — note: bash doesn't do floating point math.

awk has a rand function, but it has to be seeded; we could also do this:

awk < /usr/local/e2e-targets \
    --assign "r=$RANDOM" \
    'BEGIN { srand(r); r = rand() } …'

@petr-muller
Copy link
Member

I've created the CM manually until we decide how to manage it

I propose to add it to the post-#4143 core-services dir and track the changes with config-updater.

@bbguimaraes
Copy link
Contributor Author

I propose to add it to the post-#4143 core-services dir and track the changes with config-updater.

If we want to make the version in openshift/release authoritative, yes. From the discussion in Jira:

We don't need to commit the breakdown to openshift/release (although we might want to commit a "there is no ConfigMap yet" fallback, in case the CI cluster gets wiped and we have to rebuild it from scratch).

Using config-updater would (AIUI) overwrite any modifications — which may be what we want.

@bbguimaraes
Copy link
Contributor Author

cannot rehearse jobs that have Command different from simple 'ci-operator'

=(

@bbguimaraes
Copy link
Contributor Author

@wking, @abhinavdahiya, @cuppett: PTAL.

Copy link
Contributor

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impl here LGTM -- @bbguimaraes feel free to /hold cancel whenever you feel like you've gotten a sufficient amount of other reviews

/lgtm
/approve
/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 24, 2019
@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2019
@bbguimaraes
Copy link
Contributor Author

error: could not build execution graph: the following names were not found in the config or were duplicates: core-dry (from [input:root], src, core-valid, [output-images], [images])

Hmm…

/retest

@bbguimaraes
Copy link
Contributor Author

/retest

@bbguimaraes
Copy link
Contributor Author

Tests in the staging namespace succeeded with aws and aws-upi.

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2019
@bbguimaraes
Copy link
Contributor Author

/hold

I want to retest this in light of yesterday's "incident".

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2019
@stevekuznetsov
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, bbguimaraes, stevekuznetsov

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@stevekuznetsov
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2019
@openshift-merge-robot openshift-merge-robot merged commit 8ef620e into openshift:master Jun 26, 2019
@openshift-ci-robot
Copy link
Contributor

@bbguimaraes: Updated the following 3 configmaps:

  • ci-operator-master-configs configmap in namespace ci using the following files:
    • key openshift-installer-master.yaml using file ci-operator/config/openshift/installer/openshift-installer-master.yaml
  • ci-operator-master-configs configmap in namespace ci-stg using the following files:
    • key openshift-installer-master.yaml using file ci-operator/config/openshift/installer/openshift-installer-master.yaml
  • job-config-master-presubmits configmap in namespace ci using the following files:
    • key openshift-installer-master-presubmits.yaml using file ci-operator/jobs/openshift/installer/openshift-installer-master-presubmits.yaml

In response to this:

Generated using openshift/ci-tools#6.

I've created the CM manually until we decide how to manage it:

oc -n ci create -f - <<'EOF'
apiVersion: v1
kind: ConfigMap
metadata:
 name: e2e-targets
data:
 e2e-targets: |
   0.25 aws
   0.25 aws-upi
   0.25 azure
   0.25 vsphere
EOF

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.

@bbguimaraes bbguimaraes deleted the iaas branch June 26, 2019 15:36
wking added a commit to wking/openshift-release that referenced this pull request Sep 18, 2019
… Random Azure regions

Shard over four regions to increase our capacity, because Azure allows
auto-bumps for vCPU limits up to 200 in each region, but it's a longer
process to get the limits raised beyond that in a single region.  This
sets us up for more lease-restriction relaxing after 78ade84 (Prow:
drop max azure CI clusters to 5, 2019-09-18, openshift#5074) and 594930f
(Prow: increase max Azure CI clusters to 20, 2019-09-18, openshift#5084).  We
may want per-region quotas, but for the moment I'm just hoping that
our random choices are even enough that an Azure-wide quota is
sufficient.  And conveniently, $RANDOM runs from 0 through 32767 [1],
so our modulo 4 value is evenly weighted :).

The change to Bash is because Bash supports $RANDOM [1], but POSIX
does not [2].  We already use RANDOM in
openshift-installer-master-presubmits.yaml since ca464ed
(openshift/installer: add IaaS-agnostic E2E test, 2019-06-20, openshift#4148).

Ideally we'd be loading the set of region choices (and possibly
weights) from some shared location somewhat like ca464ed has
things.  And we'd be reporting the chosen region in a structured way
for convenient monitoring.  But the plan is to break up these
templates into more composable chunks soon anyway, so I'm ok if we
aren't all that DRY in the short term.

[1]: https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#index-RANDOM
[2]: https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xcu_chap02.html#tag_23_02_05_03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants