search

metal3-io / metal3-dev-env

Metal³ Development Environment
Apache License 2.0
109 stars 117 forks source link

Make minikube the default ephemeral cluster #1404

Open kashifest opened 1 month ago

kashifest commented 1 month ago

This PR makes minikube the default ephemeral cluster irrespective of container runtime or image OS. The idea is to run Ubuntu tests only for the time being in different repos until CI is stable. Centos e2es are failing more often. Making minikube ephemeral cluster makes sure that we test ironic as a k8s deployment instead of local containers.

kashifest commented 1 month ago

/hold lets test it properly first.

kashifest commented 1 month ago

/test metal3-dev-env-integration-test-ubuntu-main

kashifest commented 1 month ago

/test metal3-dev-env-integration-test-ubuntu-main

mboukhalfa commented 1 month ago

Can you create an issue to keep track and add TODOs once centos back to normal

kashifest commented 1 month ago

Can you create an issue to keep track and add TODOs once centos back to normal

https://github.com/metal3-io/metal3-dev-env/issues/1405

kashifest commented 1 month ago

/test metal3-dev-env-integration-test-ubuntu-release-1.7 /test metal3-dev-env-integration-test-ubuntu-release-1.6

metal3-io-bot commented 1 month ago

@kashifest: The specified target(s) for /test were not found. The following commands are available to trigger required jobs:

The following commands are available to trigger optional jobs:

Use /test all to run the following jobs that were automatically triggered:

In response to [this](https://github.com/metal3-io/metal3-dev-env/pull/1404#issuecomment-2121835043): >/test metal3-dev-env-integration-test-ubuntu-release-1.7 >/test metal3-dev-env-integration-test-ubuntu-release-1.6 > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
kashifest commented 1 month ago

/test metal3-dev-env-integration-test-ubuntu-release-1-7 /test metal3-dev-env-integration-test-ubuntu-release-1-6

kashifest commented 1 month ago

/test metal3-dev-env-integration-test-ubuntu-release-1-6

mboukhalfa commented 1 month ago

/approve /hold We must check e2e tests https://github.com/metal3-io/cluster-api-provider-metal3/blob/95ac037d51099c05255993c53fbb068ec73bf996/scripts/environment.sh#L45

metal3-io-bot commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboukhalfa

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: - ~~[OWNERS](https://github.com/metal3-io/metal3-dev-env/blob/main/OWNERS)~~ [mboukhalfa] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
kashifest commented 1 month ago

/hold We must check e2e tests https://github.com/metal3-io/cluster-api-provider-metal3/blob/95ac037d51099c05255993c53fbb068ec73bf996/scripts/environment.sh#L45

I am actually hardcoding it here , so exporting elsewhere shouldn't be an issue, we also have an export in project-infra.

mboukhalfa commented 1 month ago

/hold We must check e2e tests https://github.com/metal3-io/cluster-api-provider-metal3/blob/95ac037d51099c05255993c53fbb068ec73bf996/scripts/environment.sh#L45

I am actually hardcoding it here , so exporting elsewhere shouldn't be an issue, we also have an export in project-infra.

I am just thinking about when e2e export that in their environment var should not trigger any conflict later ? If I understand correctly EPHEMERAL_CLUSTER="minikube" will be exported in devenv but for e2e still sees EPHEMERAL_CLUSTER="kind" is there any unwanted behavior when pivoting back or something ?

kashifest commented 1 month ago

I am just thinking about when e2e export that in their environment var should not trigger any conflict later ? If I understand correctly EPHEMERAL_CLUSTER="minikube" will be exported in devenv but for e2e still sees EPHEMERAL_CLUSTER="kind" is there any unwanted behavior when pivoting back or something ?

In that case lets run it for safety here, /test metal3-dev-env-integration-test-ubuntu-release-1-6 /test metal3-ubuntu-e2e-integration-test-main

kashifest commented 1 month ago

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

kashifest commented 1 month ago

You were right @mboukhalfa

e2e wont pass with just only this I can see it is now trying to fetch ironic local container logs which it shouldnt , so e2e export is coming into play and needs to change as well if we go this way :

STEP: Fetch logs for container ironic @ 05/22/24 07:02:31.24
10:02:39  2024/05/22 07:02:31 exit status 1
10:02:39  Could not open /home/metal3ci/metal3/test/e2e/junit.e2e_suite.1.xml:
10:02:39  open /home/metal3ci/metal3/test/e2e/junit.e2e_suite.1.xml: no such file or directory
kashifest commented 1 month ago

/override metal3-ubuntu-e2e-integration-test-main , it would need a separate patch /override metal3-centos-e2e-integration-test-release-1-7 centos is failing for unrelated reasons

metal3-io-bot commented 1 month ago

@kashifest: /override requires failed status contexts, check run or a prowjob name to operate on. The following unknown contexts/checkruns were given:

Only the following failed contexts/checkruns were expected:

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to [this](https://github.com/metal3-io/metal3-dev-env/pull/1404#issuecomment-2124042523): >/override metal3-ubuntu-e2e-integration-test-main , it would need a separate patch >/override metal3-centos-e2e-integration-test-release-1-7 centos is failing for unrelated reasons Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
kashifest commented 1 month ago

/override metal3-ubuntu-e2e-integration-test-main /override metal3-centos-e2e-integration-test-release-1-7

metal3-io-bot commented 1 month ago

@kashifest: Overrode contexts on behalf of kashifest: metal3-centos-e2e-integration-test-release-1-7, metal3-ubuntu-e2e-integration-test-main

In response to [this](https://github.com/metal3-io/metal3-dev-env/pull/1404#issuecomment-2124043366): >/override metal3-ubuntu-e2e-integration-test-main >/override metal3-centos-e2e-integration-test-release-1-7 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
kashifest commented 1 month ago

@dtantsur @elfosardo PTAL here, after this change, plan is to run only ubuntu based tests as required in PRs, until we have a solution for centos based jobs. After this change, Ubuntu will run minikube as ephemeral cluster and run ironic as a k8s deployment (not as local containers as it is currently in kind cluster)

kashifest commented 1 month ago

/hold cancel

Sunnatillo commented 1 month ago

/hold

Do we still need this change?

kashifest commented 1 month ago

/hold

Do we still need this change?

We can discuss on it, I still think this is a better option given that:

  1. We can reduce the tests being run
  2. Not being dependant on CentOS packages which breaks more often compared to Ubuntu
kashifest commented 1 month ago

We can discuss on it, I still think this is a better option given that:

  1. We can reduce the tests being run
  2. Not being dependant on CentOS packages which breaks more often compared to Ubuntu

@Rozzii @lentzi90 @elfosardo how do you folks feel about this ?

Rozzii commented 1 month ago

I was already proponent of this change and I still support it. /lgtm

kashifest commented 3 weeks ago

I am guessing no one has any objections on this one, I will push an update on this PR and then we can take it in

dtantsur commented 3 weeks ago

test ironic as a k8s deployment instead of local containers.

You sold it to me here 👍🏽

elfosardo commented 3 weeks ago

definitely in favor of this!

tuminoid commented 3 weeks ago

If we keep it configurable and mean to support it, we need some periodic test at minimum to verify it keeps working. It is already sometimes breaking after we stopped requiring both ubuntu and centos e2e to be run on PRs.

Rozzii commented 3 weeks ago

@kashifest I think we can cancel the hold

tuminoid commented 3 weeks ago

If we keep it configurable and mean to support it, we need some periodic test at minimum to verify it keeps working. It is already sometimes breaking after we stopped requiring both ubuntu and centos e2e to be run on PRs.

Added https://github.com/metal3-io/metal3-dev-env/issues/1418 for that. It must be implemented asap, if we merge this and plan to keep local ironic support, otherwise the feature will rot fast and be unsupportable.