Skip to content

Adding ipi support to cluster-launch-installer-e2e.yaml#6631

Closed
derekhiggins wants to merge 9 commits intoopenshift:masterfrom
derekhiggins:ipi-e2e-job
Closed

Adding ipi support to cluster-launch-installer-e2e.yaml#6631
derekhiggins wants to merge 9 commits intoopenshift:masterfrom
derekhiggins:ipi-e2e-job

Conversation

@derekhiggins
Copy link
Copy Markdown
Contributor

Updating cluster-launch-installer-e2e template to for
jobs that test Bare Metal provided Installer Provisioned
Infrastructure. Use packet.net to spin up a single baremetal
server and test the ipi installer on it.

Continuation of #5016 (can't seem to reopen it)

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 10, 2020
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: derekhiggins
To complete the pull request process, please assign crawford
You can assign the PR to them by writing /assign @crawford in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details 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

@droslean
Copy link
Copy Markdown
Member

/test pj-rehearse

@derekhiggins derekhiggins force-pushed the ipi-e2e-job branch 6 times, most recently from 3a316f9 to 15b4a8d Compare January 30, 2020 09:43
@jstuever
Copy link
Copy Markdown
Contributor

/uncc @jstuever

@andfasano
Copy link
Copy Markdown
Contributor

/test pj-rehearse

1 similar comment
@andfasano
Copy link
Copy Markdown
Contributor

/test pj-rehearse

derekhiggins and others added 6 commits February 20, 2020 16:58
Updating cluster-launch-installer-e2e template to for
jobs that test Bare Metal provided Installer Provisioned
Infrastructure. Use packet.net to spin up a single baremetal
server and test the ipi installer on it.
dev-scripts can go a long time without outputing
anything, add a keepalive to prevent ssh timeing out
Also added run-remote-smoke-tests
@andfasano
Copy link
Copy Markdown
Contributor

/test pj-rehearse

1 similar comment
@andfasano
Copy link
Copy Markdown
Contributor

/test pj-rehearse

@andfasano
Copy link
Copy Markdown
Contributor

/assign @crawford

@sdodson
Copy link
Copy Markdown
Member

sdodson commented Feb 24, 2020

/cc

Copy link
Copy Markdown
Contributor

@patrickdillon patrickdillon left a comment

Choose a reason for hiding this comment

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

I only took a brief look at this, but it seems like this would add pods to run on every e2e job, whether they need packet or not.

This seems like it needs a separate template, can you clarify?

ci-operator.openshift.io/wait-for-container-artifacts: teardown
ci-operator.openshift.io/save-container-logs: "true"
ci-operator.openshift.io/container-sub-tests: "setup,test,teardown"
ci-operator.openshift.io/container-sub-tests: "setup,test,teardown,setup-packet"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This template is shared by all IPI jobs. Does this addition have an effect on IPI jobs that don't run packet?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It shouldn't have since it execution is guarded by a condition on the CLUSTER_TYPE (see https://github.com/derekhiggins/release/blob/44d62d646ac884ec448f9e0b36f216800af2fd53/ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml#L1002) - same approach used on the other containers

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This annotation determines which tests are returned as JUnit tests so setup-packet would be displayed as a test on every test, even those not using Packet.


# The setup-packet and test containers need libnns_wrapper to use ssh
# TODO(derekh): investigate if it can be added to that container images
- name: nss-wrapper-hack
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Am I right to think we would be running this pod on every e2e install, whether it is needed or not?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, it copies a bunch of files to a shared folder since they are needed by the setup-packet container

cp /bin/mock-nss.sh /usr/lib64/libnss_wrapper.so /tmp/shared/
# We need to have a seperate setup container for packet.net servers
# as we need an image with terrafrom
- name: setup-packet
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above: Am I right to think we would be running this pod on every e2e install, whether it is needed or not?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See previous comment on CLUSTER_TYPE guard

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah so it would run and return 0 so the setup-packet junit test I mentioned above would pass on non-packet e2e-tests. makes sense.

Copy link
Copy Markdown
Contributor

@andfasano andfasano left a comment

Choose a reason for hiding this comment

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

I only took a brief look at this, but it seems like this would add pods to run on every e2e job, whether they need packet or not.

This seems like it needs a separate template, can you clarify?

In a previously opened PR I think that @smarterclayton suggested to include it in the current template, as originally was separate (see #5016 (comment)).

@patrickdillon
Copy link
Copy Markdown
Contributor

In a previously opened PR I think that @smarterclayton suggested to include it in the current template, as originally was separate (see #5016 (comment)).

To me, adding new pods would call for creating a new template. I could see this as a template that VSphere might also need. I know there are maintenance issues with creating new templates, but I also don't want to create unnecessary dependencies.

@wking I know you have informed opinions on templates, can you take a quick look?

I don't feel comfortable on making that call without more informed opinions but will review the rest of the PR independent of that.

@patrickdillon
Copy link
Copy Markdown
Contributor

after bugging the test team it looks like we should be using this: https://steps.svc.ci.openshift.org/

I will look into this.

@derekhiggins
Copy link
Copy Markdown
Contributor Author

Addressed here #7727

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@derekhiggins: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/rehearse/openshift/installer/master/e2e-ipi 4f6447fed38d954ebb46ff93030840931a958249 link /test pj-rehearse
ci/prow/ordered-prow-config 54b35cb link /test ordered-prow-config
ci/prow/pj-rehearse 44d62d6 link /test pj-rehearse
ci/rehearse/openshift/cloud-credential-operator/master/e2e-gcp 44d62d6 link /test pj-rehearse
ci/rehearse/cri-o/cri-o/release-1.13/e2e-aws 44d62d6 link /test pj-rehearse
ci/prow/app-ci-config 44d62d6 link /test app-ci-config
ci/prow/ci-operator-config 44d62d6 link /test ci-operator-config

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.

Details

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

9 participants