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

🌱 Switch e2e to kind #2209

Merged
merged 4 commits into from
Jan 24, 2025
Merged

Conversation

lentzi90
Copy link
Member

@lentzi90 lentzi90 commented Jan 21, 2025

What this PR does / why we need it:

Minikube is having troubles starting sometimes.
It was nice to work with since the VM could easily be attached to the same network as the BMH VMs, but it is possible to work around that also with kind.

It also removes overlays that are no longer used and the upgrade tests for ironic-24.0 as these are not supported/needed anymore. With that we also get rid of the last inspector code from the scripts 🎉

Fixture tests were only uploading artifacts on success. New they upload also on failure, just like e2e.

I have also tried stabilizing the fixture tests. It seems like there is some condition when namespace deletion takes a long time. From the BMO logs I saw that BMO was trying to create some resources in the terminating namespace. In an attempt to avoid this I added BMH deletion to the cleanup before deleting the namespace. That way at least BMO should be aware what is coming. Not sure if that is the root cause of the slow namespace deletion though.

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 #1783

@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 Jan 21, 2025
@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 requested review from mquhuy and zaneb January 21, 2025 13:59
@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 21, 2025
@lentzi90
Copy link
Member Author

Required tests have passed locally for redfish, redfish-virtualmedia and IPMI. Let's see what the CI thinks.

@lentzi90 lentzi90 marked this pull request as ready for review January 22, 2025 07:21
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.

Cleanup suggestion. Less work to update those.

config/overlays/e2e-release-0.4/ironic.env Outdated Show resolved Hide resolved
config/overlays/e2e-release-0.5/ironic.env Outdated Show resolved Hide resolved
@lentzi90 lentzi90 force-pushed the lentzi90/e2e-kind branch 2 times, most recently from 6480c59 to 8e3272e Compare January 23, 2025 08:44
@metal3-io-bot metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 23, 2025
Remove BMO overlays no longer used in the e2e tests.
Drop upgrade from ironic-24.0 as it is out of support and not needed to
be tested anymore.

Signed-off-by: Lennart Jern <[email protected]>
Minikube is having troubles starting sometimes.
It was nice to work with since the VM could easily be attached to the
same network as the BMH VMs, but it is possible to work around that also
with kind.

Signed-off-by: Lennart Jern <[email protected]>
@lentzi90 lentzi90 changed the title WIP: 🌱 Switch e2e to kind 🌱 Switch e2e to kind Jan 23, 2025
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2025
Sometimes the fixture tests hit the timeout for namespace deletion.
The BMO logs indicate that BMO is trying to create new objects while the
namespace is terminating. For example HardwareDetails. To avoid this, I
think we can trigger deletion of the BMHs before we delete the
namespace.

We are running a bit close to the 1m deadline on successful runs
in the re-inspection test.
I believe this is explained by an extra reconcile loop when the
hardwaredetails are updated because of the inspection. No other fixture
test is close to this deadline normally.

Signed-off-by: Lennart Jern <[email protected]>
@lentzi90
Copy link
Member Author

Alright I think this is ready for review. It became a bit larger than planned... Some of it can definitely be split into separate PRs if you wish.
I have run every test the is locally (including fixture and optional upgrade jobs) and all have passed. The fixture test is actually what I am most unsure about. It was quite flaky so I am not completely sure if I got it stable with this. The flakiness is anyway unrelated to this PR as far as I can tell.

@lentzi90 lentzi90 requested a review from tuminoid January 23, 2025 12:15
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.

One nit.

Also, @kashifest brought up https://github.com/medyagh/setup-minikube as a way to set up minikube in a GH action. I did not check it out myself if it would actually even be flexible enough for our needs. Did you check that?

@@ -2,6 +2,8 @@ images:
# Use locally built e2e images
- name: quay.io/metal3-io/baremetal-operator:e2e
loadBehavior: tryLoad
# - name: quay.io/metal3-io/ironic:local
Copy link
Member

Choose a reason for hiding this comment

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

Why have this added but commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I forgot about it!
@Rozzii you added the option to use a locally built ironic image recently. This is basically all that is required for it as far as I understand. If you uncomment this, the image is loaded. The rest is handled by changing the manifests directly or by pointing to another kustomization to deploy below.
Is this acceptable?

Previous process:

  • Set LOAD_LOCAL_IRONIC=true and build the image
  • Change e2e config to deploy ironic from e2e-local-ironic

Process after this PR:

  • Build the image and comment out these lines in e2e config
  • Change e2e config to deploy ironic from e2e-local-ironic

Copy link
Member

Choose a reason for hiding this comment

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

If we move to kind then the image transfer is not needed anymore as kind runs on the host VM so in that context I am fine with the changes.

I am fine with doing it via the config file although I prefer env variable based config because I am a bit worried that it is easy to accidentally commit changes to ironic.yaml.

@lentzi90
Copy link
Member Author

/cc @Rozzii

@metal3-io-bot metal3-io-bot requested a review from Rozzii January 24, 2025 06:05
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2025
@Rozzii
Copy link
Member

Rozzii commented Jan 24, 2025

/lgtm cancel

@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2025
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

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

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Rozzii

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2025
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

🚀 let's get rid of the minikube/docker flake!
Thank you @lentzi90 for taking time to do this.

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2025
@metal3-io-bot metal3-io-bot merged commit 0a49d5a into metal3-io:main Jan 24, 2025
29 checks passed
@metal3-io-bot metal3-io-bot deleted the lentzi90/e2e-kind branch January 24, 2025 14:23
@metal3-io-bot metal3-io-bot added this to the BMO - v0.10 milestone Jan 24, 2025
@lentzi90
Copy link
Member Author

/cherry-pick release-0.9

@metal3-io-bot
Copy link
Contributor

@lentzi90: #2209 failed to apply on top of branch "release-0.9":

Applying: E2E: Clean up unused overlays
Using index info to reconstruct a base tree...
M	hack/ci-e2e.sh
M	test/e2e/config/ironic.yaml
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/config/ironic.yaml
CONFLICT (content): Merge conflict in test/e2e/config/ironic.yaml
Removing ironic-deployment/overlays/e2e-with-inspector/kustomization.yaml
Removing ironic-deployment/overlays/e2e-with-inspector/ironic_bmo_configmap.env
Removing ironic-deployment/overlays/e2e-with-inspector/ironic-patch.yaml
Removing ironic-deployment/overlays/e2e-release-24.0-with-inspector/kustomization.yaml
Removing ironic-deployment/overlays/e2e-release-24.0-with-inspector/ironic_bmo_configmap.env
Removing ironic-deployment/overlays/e2e-release-24.0-with-inspector/ironic-patch.yaml
Auto-merging hack/ci-e2e.sh
Removing config/overlays/fixture-release-0.5/kustomization.yaml
Removing config/overlays/fixture-release-0.4/kustomization.yaml
Removing config/overlays/e2e-release-0.5/kustomization.yaml
Removing config/overlays/e2e-release-0.5/ironic.env
Removing config/overlays/e2e-release-0.4/kustomization.yaml
Removing config/overlays/e2e-release-0.4/ironic.env
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 E2E: Clean up unused overlays

In response to this:

/cherry-pick release-0.9

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-sigs/prow repository.

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[flake] Docker restart fails in BMO e2e
4 participants