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

✨ Add e2e tests #1303

Merged
merged 1 commit into from
Sep 29, 2023
Merged

✨ Add e2e tests #1303

merged 1 commit into from
Sep 29, 2023

Conversation

lentzi90
Copy link
Member

@lentzi90 lentzi90 commented Jun 28, 2023 •
edited
Loading

What this PR does / why we need it:

Adds e2e tests for BMO. This is not in any way ready to replace the existing tests we run with the full Metal3 stack, but eventually they may become enough to test at least BMO in an end to end fashion. So far there is just tests for registering -> inspecting -> available.
My hope is that the tests will prove flexible enough to be able to run also on real hardware, but for CI purposes we will use VMs.

Reviewer guide

Thanks for reviewing this (very large) PR! I hope the line count does not scare you away. About 50 % of it is anyway just go.{mod|sum}. To help with the review I want to explain a few things here.

  • I made test its own module to avoid any issues with "breaking APIs" when we change the tests. We should not give any promises about stability of the test module at this time.
  • Large parts of the common.go is just adapted code from the CAPI framework. Specifically everything about the e2e config is heavily inspired by CAPI.
  • The additions to the Makefile are also largely inspired by CAPI/CAPM3 where we already have e2e tests.
  • There is a README.md and I hope it is useful. It is probably a good place to start the review.

Then some answers to questions I expect to pop up:

Q: Why should we have a config file for e2e tests?
A: The idea is to be able to reuse these tests and then we need some flexibility that the config file provides. For example, it should be possible to specify the BMC details for a real server and run the tests against it.

Q: Why not take BMH manifests as input to the test instead of providing only BMC details?
A: This would take away too much control from the tests. We need to be able to create the BMH objects to fit the tests.

Q: Is the config file format set in stone?
A: Absolutely not! I expect that we will extend/modify it in the future as we add more tests.

Q: Why use cmctl?
A: It is a convenient way of installing cert-manager and ensuring that it is working. Cert-manager can otherwise be a bit tricky because it is not enough to wait for the pods to be running. It also needs a little time to initialize and register the webhooks. I do not expect this is how we will install cert-manager in the long run though.

Future work

I expect that we will want to work with this in the future and add more tests, configure things differently, etc.
Here are some of my ideas for what we should add.

  • Test provisioning
  • Test deprovisioning
  • Test re-inspection
  • Test detach annotation
  • Test status annotation to skip inspection
  • Test basic operations like power on/off
  • Configure BMO/Ironic to use TLS and basic auth
  • Remove use of os.Exec. We can write native go code to install cert-manager, BMO and Ironic instead of using kubectl and cmctl.
  • Rewrite (parts of) the ci-e2e.sh script as a go library. Instead of relying on virsh and virt-install to set up VMs and network, we could have a small go library to do the same. This would be extremely useful also for CAPM3.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2023
@metal3-io-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@metal3-io-bot metal3-io-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 28, 2023
tuminoid
tuminoid reviewed Jun 28, 2023
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Just noting somethings.

test/go.mod Show resolved Hide resolved
test/go.mod Show resolved Hide resolved
dtantsur
dtantsur reviewed Jul 3, 2023
View reviewed changes
test/e2e/fixture_test.go Outdated Show resolved Hide resolved
dtantsur
dtantsur reviewed Jul 3, 2023
View reviewed changes
test/e2e/fixture_test.go Outdated Show resolved Hide resolved
@lentzi90 lentzi90 force-pushed the lentzi90/e2e-tests branch 3 times, most recently from 454820d to bbdc6a7 Compare August 21, 2023 12:54
@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2023
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2023
@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2023
@lentzi90 lentzi90 force-pushed the lentzi90/e2e-tests branch 3 times, most recently from d2cf4ff to 9e3c194 Compare September 15, 2023 12:06
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2023
@lentzi90 lentzi90 force-pushed the lentzi90/e2e-tests branch 9 times, most recently from bddcc24 to ab47a4b Compare September 20, 2023 06:59
@lentzi90
Copy link
Member Author

Successful run: https://jenkins.nordix.org/job/metal3-bmo-e2e/22/ 🎉
I'll set up the pipeline properly new 🙂

@lentzi90
Copy link
Member Author

/bmo-e2e-tests

@metal3-io-bot
Copy link
Contributor

Failed

@kashifest
Copy link
Member

/metal3_baremetal-operator_e2e_tests

@lentzi90
Copy link
Member Author

/metal3-bmo-e2e-tests

4 similar comments
@lentzi90
Copy link
Member Author

/metal3-bmo-e2e-tests

@kashifest
Copy link
Member

/metal3-bmo-e2e-tests

@kashifest
Copy link
Member

/metal3-bmo-e2e-tests

@kashifest
Copy link
Member

/metal3-bmo-e2e-tests

@lentzi90
Copy link
Member Author

/metal3-bmo-e2e-test-pull

@lentzi90
Copy link
Member Author

/metal3-bmo-e2e-test

@lentzi90
Copy link
Member Author

/test-ubuntu-integration-main
/test-centos-e2e-integration-main

@lentzi90 lentzi90 changed the title ✨ Add e2e tests ✨ Add e2e tests Sep 28, 2023
kashifest
kashifest previously approved these changes Sep 28, 2023
View reviewed changes
@lentzi90
Copy link
Member Author

All looks good! Can I get an lgtm 🙏
/cc @dtantsur @tuminoid

tuminoid
tuminoid reviewed Sep 28, 2023
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Two nits which you can feel free to ignore too.

docs/releasing.md Outdated Show resolved Hide resolved
test/e2e/README.md Show resolved Hide resolved
@lentzi90
Add e2e tests
d16fb45 
This adds a test suite for e2e tests. So far there is only a few tests
related to inspection, but it should be enough to validate the approach.

My hope is that this framework will be flexible enough to allow tests
both in CI, using VMs, and on actual hardware. The tests can be
configured through a config file. They can run on an existing cluster or
set up a temporary kind cluster automatically. Similarly, BMO, Ironic
and Cert-manager can all be deployed or not depending on configuration.

Logs are collected for BMO and Ironic when deployed automatically, even
in existing clusters.
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

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

1 similar comment
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

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

@lentzi90
Copy link
Member Author

Formatted both docs @tuminoid 🙂

@lentzi90
Copy link
Member Author

/metal3-bmo-e2e-test
/test-ubuntu-integration-main
/test-centos-e2e-integration-main

@lentzi90
Copy link
Member Author 

stderr: fatal: unable to access 'https://github.com/metal3-io/project-infra.git/': Could not resolve host: github.com

🙄

/metal3-bmo-e2e-test

@lentzi90
Copy link
Member Author

/metal3-bmo-e2e-test

@lentzi90
Copy link
Member Author

/test-ubuntu-integration-main

tuminoid
tuminoid reviewed Sep 29, 2023
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks for the massive contribution. This is a great step forward in untangling the testing setup we have today.

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2023
@metal3-io-bot metal3-io-bot merged commit 6a4b794 into metal3-io:main Sep 29, 2023
14 checks passed
@lentzi90 lentzi90 deleted the lentzi90/e2e-tests branch September 29, 2023 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuminoid tuminoid tuminoid left review comments

@mboukhalfa mboukhalfa mboukhalfa left review comments

@kashifest kashifest kashifest left review comments

@honza honza Awaiting requested review from honza

@zaneb zaneb Awaiting requested review from zaneb

@dtantsur dtantsur Awaiting requested review from dtantsur

Assignees
No one assigned
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lentzi90 @metal3-io-bot @tuminoid @kashifest @dtantsur @mboukhalfa