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

Make minikube the default ephemeral cluster #1404

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kashifest
Copy link
Member

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.

@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 20, 2024
@kashifest
Copy link
Member Author

/hold
lets test it properly first.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2024
@kashifest
Copy link
Member Author

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

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.

Signed-off-by: Kashif Khan <kashif.khan@est.tech>
@kashifest kashifest force-pushed the kashif/make-minikube-default-ephemeral-cluster branch from 1ee79f9 to df02ad3 Compare May 20, 2024 12:27
@kashifest
Copy link
Member Author

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

@mboukhalfa
Copy link
Member

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

@kashifest
Copy link
Member Author

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

#1405

@kashifest
Copy link
Member Author

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

@metal3-io-bot
Copy link
Collaborator

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

  • /test markdownlint
  • /test metal3-centos-e2e-integration-test-release-1-7
  • /test metal3-dev-env-integration-test-ubuntu-main
  • /test shellcheck

The following commands are available to trigger optional jobs:

  • /test metal3-centos-e2e-basic-test-main
  • /test metal3-centos-e2e-basic-test-release-1-6
  • /test metal3-centos-e2e-basic-test-release-1-7
  • /test metal3-centos-e2e-feature-test-main
  • /test metal3-centos-e2e-feature-test-release-1-5
  • /test metal3-centos-e2e-feature-test-release-1-6
  • /test metal3-centos-e2e-feature-test-release-1-7
  • /test metal3-centos-e2e-integration-test-main
  • /test metal3-centos-e2e-integration-test-release-1-5
  • /test metal3-centos-e2e-integration-test-release-1-6
  • /test metal3-dev-env-integration-test-centos-main
  • /test metal3-dev-env-integration-test-centos-release-1-5
  • /test metal3-dev-env-integration-test-centos-release-1-6
  • /test metal3-dev-env-integration-test-centos-release-1-7
  • /test metal3-dev-env-integration-test-ubuntu-release-1-5
  • /test metal3-dev-env-integration-test-ubuntu-release-1-6
  • /test metal3-dev-env-integration-test-ubuntu-release-1-7
  • /test metal3-e2e-1-26-1-27-upgrade-test-main
  • /test metal3-e2e-1-26-1-27-upgrade-test-release-1-5
  • /test metal3-e2e-1-26-1-27-upgrade-test-release-1-6
  • /test metal3-e2e-1-26-1-27-upgrade-test-release-1-7
  • /test metal3-e2e-1-27-1-28-upgrade-test-main
  • /test metal3-e2e-1-27-1-28-upgrade-test-release-1-5
  • /test metal3-e2e-1-27-1-28-upgrade-test-release-1-6
  • /test metal3-e2e-1-27-1-28-upgrade-test-release-1-7
  • /test metal3-e2e-1-28-1-29-upgrade-test-main
  • /test metal3-e2e-1-28-1-29-upgrade-test-release-1-5
  • /test metal3-e2e-1-28-1-29-upgrade-test-release-1-6
  • /test metal3-e2e-1-28-1-29-upgrade-test-release-1-7
  • /test metal3-e2e-clusterctl-upgrade-test-main
  • /test metal3-e2e-clusterctl-upgrade-test-release-1-5
  • /test metal3-e2e-clusterctl-upgrade-test-release-1-6
  • /test metal3-e2e-clusterctl-upgrade-test-release-1-7
  • /test metal3-ubuntu-e2e-basic-test-main
  • /test metal3-ubuntu-e2e-basic-test-release-1-6
  • /test metal3-ubuntu-e2e-basic-test-release-1-7
  • /test metal3-ubuntu-e2e-feature-test-main
  • /test metal3-ubuntu-e2e-feature-test-release-1-5
  • /test metal3-ubuntu-e2e-feature-test-release-1-6
  • /test metal3-ubuntu-e2e-feature-test-release-1-7
  • /test metal3-ubuntu-e2e-integration-test-main
  • /test metal3-ubuntu-e2e-integration-test-release-1-5
  • /test metal3-ubuntu-e2e-integration-test-release-1-6
  • /test metal3-ubuntu-e2e-integration-test-release-1-7

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

  • shellcheck

In response to this:

/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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kashifest
Copy link
Member Author

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

@kashifest
Copy link
Member Author

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

@mboukhalfa
Copy link
Member

@metal3-io-bot
Copy link
Collaborator

[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:

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 May 21, 2024
@kashifest
Copy link
Member Author

/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
Copy link
Member

mboukhalfa commented May 21, 2024

/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
Copy link
Member Author

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
Copy link
Member Author

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

@kashifest
Copy link
Member Author

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
Copy link
Member Author

/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
Copy link
Collaborator

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

  • ,
  • a
  • centos
  • failing
  • for
  • is
  • it
  • need
  • patch
  • reasons
  • separate
  • unrelated
  • would

Only the following failed contexts/checkruns were expected:

  • metal3-centos-e2e-integration-test-release-1-7
  • metal3-dev-env-integration-test-ubuntu-main
  • metal3-dev-env-integration-test-ubuntu-release-1-6
  • metal3-dev-env-integration-test-ubuntu-release-1-7
  • metal3-ubuntu-e2e-integration-test-main
  • shellcheck
  • tide

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:

/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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kashifest
Copy link
Member Author

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

@metal3-io-bot
Copy link
Collaborator

@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:

/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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kashifest
Copy link
Member Author

@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
Copy link
Member Author

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 22, 2024
@Sunnatillo
Copy link
Member

/hold

Do we still need this change?

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2024
@kashifest
Copy link
Member Author

kashifest commented Jun 3, 2024

/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
Copy link
Member Author

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 ?

Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

I see no issues switching to use minikube throughout CI. However, I think we should keep it configurable.

else
export EPHEMERAL_CLUSTER="minikube"
fi
export EPHEMERAL_CLUSTER="minikube"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export EPHEMERAL_CLUSTER="minikube"
export EPHEMERAL_CLUSTER=${EPHEMERAL_CLUSTER:-"minikube"}

Copy link
Member Author

Choose a reason for hiding this comment

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

ow yeah, this I did intentionally here for the test because it gets dictated from different places like project-infra. CAPM3 etc

@Rozzii
Copy link
Member

Rozzii commented Jun 5, 2024

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

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2024
@kashifest
Copy link
Member Author

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
Copy link
Member

test ironic as a k8s deployment instead of local containers.

You sold it to me here 👍🏽

@elfosardo
Copy link
Member

definitely in favor of this!

@tuminoid
Copy link
Member

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
Copy link
Member

Rozzii commented Jun 12, 2024

@kashifest I think we can cancel the hold

@tuminoid
Copy link
Member

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

@metal3-io-bot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@metal3-io-bot metal3-io-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 10, 2024
@kashifest
Copy link
Member Author

I might need to re-consider this PR.
/remove-lifecycle stale

@metal3-io-bot metal3-io-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 10, 2024
@metal3-io-bot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@metal3-io-bot metal3-io-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 9, 2024
@tuminoid
Copy link
Member

tuminoid commented Dec 9, 2024

/remove-lifecycle stale

@metal3-io-bot metal3-io-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 9, 2024
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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants