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.

Sign up for GitHub

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

Jump to bottom

[Prow CI Step Enhancement] update install step to support QE e2e test #32647

Merged
merged 3 commits into from
Oct 10, 2022
Merged

[Prow CI Step Enhancement] update install step to support QE e2e test #32647

merged 3 commits into from
Oct 10, 2022

Conversation

LiangquanLi930
Copy link
Member

@LiangquanLi930 LiangquanLi930 commented Sep 27, 2022 •
edited
Loading

extended aws together with PR
For QE e2e test, we need to use QE's quota, so we should support dynamic switching of --aws-creds and --base-domain

@LiangquanLi930 LiangquanLi930 force-pushed the hypershift-workflow4 branch 7 times, most recently from 4203be2 to 5368841 Compare September 28, 2022 06:03
@LiangquanLi930
Copy link
Member Author

The error of periodic-ci-openshift-openshift-tests-private-release-4.11-amd64 and periodic-ci-openshift-openshift-tests-private-release-4.12-amd64 are because of the vpc limit

enxebre
enxebre reviewed Sep 28, 2022
View reviewed changes
ci-operator/step-registry/hypershift/aws/create/hypershift-aws-create-chain.yaml
@@ -19,8 +19,15 @@ chain:
- name: EXTRA_ARGS
default: ""
documentation: "Extra args to pass to the create cluster aws command"
- name: HYPERSHIFT_GUEST_INFRA_OCP_ACCOUNT
default: "false"
Copy link
Member

Choose a reason for hiding this comment

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

fwiw I consider hypershift-aws-create-chain.yaml deprecated. There's no semantic difference with hypershift-aws-setup-nested-management-cluster-chain.yaml which we should rename to just `hypershift-aws-create. Then is up to the workload to chain multiples creates to have nested clusters.

However I'm fine with the change for now and I'll follow up later on with the refactor above.

@LiangquanLi930
Copy link
Member Author

@enxebre Hi, add

if [[ $HYPERSHIFT_BASE_DOMAIN != "qe.devcluster.openshift.com" ]]; then
  HYPERSHIFT_BASE_DOMAIN=origin-ci-int-aws.dev.rhcloud.com
fi

This is added to support qe's install workflow, could you help me review again? thanks

@enxebre enxebre mentioned this pull request Sep 28, 2022
Closed
enxebre
enxebre reviewed Sep 28, 2022
View reviewed changes
...ws/setup-nested-management-cluster/hypershift-aws-setup-nested-management-cluster-chain.yaml Outdated

if [[ $HYPERSHIFT_GUEST_INFRA_OCP_ACCOUNT == "true" ]]; then
AWS_GUEST_INFRA_CREDENTIALS_FILE="${CLUSTER_PROFILE_DIR}/.awscred"
BASE_DOMAIN=origin-ci-int-aws.dev.rhcloud.com
if [[ $HYPERSHIFT_BASE_DOMAIN != "qe.devcluster.openshift.com" ]]; then
Copy link
Member

@enxebre enxebre Sep 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can you please don't default the env variable, keep the logic as before this PR, and then just let the environment variable be authoritative if it was set?

The likes of

      AWS_GUEST_INFRA_CREDENTIALS_FILE="/etc/hypershift-ci-jobs-awscreds/credentials"
      DEFAULT_BASE_DOMAIN=ci.hypershift.devcluster.openshift.com
      
      if [[ $HYPERSHIFT_GUEST_INFRA_OCP_ACCOUNT == "true" ]]; then
        AWS_GUEST_INFRA_CREDENTIALS_FILE="${CLUSTER_PROFILE_DIR}/.awscred"
        DEFAULT_BASE_DOMAIN=origin-ci-int-aws.dev.rhcloud.com
      fi
      
      DOMAIN :=${HYPERSHIFT_BASE_DOMAIN:-$DEFAULT_BASE_DOMAIN}

That way the behaviour is generic

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, thanks for your comment, good idea

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, it assumes HYPERSHIFT_BASE_DOMAIN has the highest weight. If it is set in jobs, HYPERSHIFT_GUEST_INFRA_OCP_ACCOUNT won't work any more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then for existing chains, we need remove all existing env HYPERSHIFT_BASE_DOMAIN with default value ? so that it won't affect HYPERSHIFT_GUEST_INFRA_OCP_ACCOUNT

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2022
@LiangquanLi930
Copy link
Member Author

@enxebre could you help me review again?

@enxebre
Copy link
Member

enxebre commented Sep 30, 2022

looks to me overall now. CI is currently broken pending openshift/hypershift#1773

@LiangquanLi930
Copy link
Member Author

/retest

@LiangquanLi930
Copy link
Member Author

looks to me overall now. CI is currently broken pending openshift/hypershift#1773

hi @enxebre 1773 merge, could you help review again when you free? thanks

@LiangquanLi930
Copy link
Member Author

/test pj-rehearse

@enxebre
Copy link
Member

enxebre commented Oct 3, 2022

/hold
while trying to get CI stable

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 3, 2022
@YuLi517
Copy link

YuLi517 commented Oct 5, 2022

@enxebre Could we merge this PR? As it's a blocker for QE to continue run e2e test for openshift 4.12 release.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 7, 2022

@LiangquanLi930: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/rehearse/periodic-ci-openshift-hypershift-main-periodics-e2e-aws-periodic 33a3178 link unknown /test pj-rehearse
ci/rehearse/openshift/apiserver-network-proxy/master/e2e-hypershift 9859599 link unknown /test pj-rehearse
ci/rehearse/openshift/hypershift/main/e2e-aws-metrics 9859599 link unknown /test pj-rehearse
ci/rehearse/openshift/hypershift/main/e2e-aws 9859599 link unknown /test pj-rehearse
ci/rehearse/periodic-ci-openshift-hypershift-main-periodics-conformance-proxy-aws-ovn-4-12 9859599 link unknown /test pj-rehearse
ci/rehearse/periodic-ci-openshift-openshift-tests-private-release-4.11-amd64-nightly-e2e-aws-ipi-hypershift 9859599 link unknown /test pj-rehearse
ci/rehearse/openshift/ovn-kubernetes/master/e2e-aws-ovn-hypershift 9859599 link unknown /test pj-rehearse
ci/rehearse/periodic-ci-openshift-openshift-tests-private-release-4.12-amd64-nightly-e2e-aws-ipi-hypershift-p2 9859599 link unknown /test pj-rehearse

Full PR test history. Your PR dashboard.

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.

@YuLi517
Copy link

YuLi517 commented Oct 9, 2022

@csrwng @Xia-Zhao-rh Could you help to review it also?

@enxebre
Copy link
Member

enxebre commented Oct 10, 2022

/hold cancel
/lgtm

@csrwng
Copy link
Contributor

csrwng commented Oct 10, 2022

/lgtm
/approve

@enxebre
Copy link
Member

enxebre commented Oct 10, 2022

/hold
can you remove "merges" and squash into a single commit before merging

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 10, 2022
@csrwng
Copy link
Contributor

csrwng commented Oct 10, 2022

/label tide/merge-method-squash

@openshift-ci openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 10, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 10, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, enxebre, LiangquanLi930

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 10, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, enxebre, LiangquanLi930

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

@csrwng
Copy link
Contributor

csrwng commented Oct 10, 2022

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit 089a6b0 into openshift:master Oct 10, 2022
@LiangquanLi930 LiangquanLi930 deleted the hypershift-workflow4 branch February 1, 2023 10:01
@LiangquanLi930 LiangquanLi930 changed the title hypershift-chain: update install HyperShift QE: update install step to support QE e2e test Dec 15, 2023
@LiangquanLi930 LiangquanLi930 changed the title HyperShift QE: update install step to support QE e2e test [Prow CI Step Enablement] update install step to support QE e2e test Dec 20, 2023
@LiangquanLi930 LiangquanLi930 changed the title [Prow CI Step Enablement] update install step to support QE e2e test [Prow CI Step Enhancement] update install step to support QE e2e test Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heliubj18 heliubj18 heliubj18 left review comments

@enxebre enxebre enxebre left review comments

@csrwng csrwng Awaiting requested review from csrwng

@Xia-Zhao-rh Xia-Zhao-rh Awaiting requested review from Xia-Zhao-rh

Assignees

@enxebre enxebre

@csrwng csrwng

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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@LiangquanLi930 @enxebre @YuLi517 @csrwng @heliubj18 @openshift-merge-robot