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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Fix kubernetes version for e2e tests #960

Merged
merged 1 commit into from
May 4, 2023

Conversation

kashifest
Copy link
Member

This PR fixes e2e tests with correct k8s versions. Currently we are wrongly initializing the e2e integration tests with one older minor version but we should always initialize it to the latest k8s. This PR also adds a matrix to better understand which k8s version we are using for different e2e tests.

@metal3-io-bot metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 3, 2023
@kashifest
Copy link
Member Author

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

@kashifest
Copy link
Member Author

/test-e2e-upgrade-main-from-release-0-5

4 similar comments
@kashifest
Copy link
Member Author

/test-e2e-upgrade-main-from-release-0-5

@kashifest
Copy link
Member Author

/test-e2e-upgrade-main-from-release-0-5

@kashifest
Copy link
Member Author

/test-e2e-upgrade-main-from-release-0-5

@kashifest
Copy link
Member Author

/test-e2e-upgrade-main-from-release-0-5

docs/e2e-test.md Outdated Show resolved Hide resolved
@kashifest
Copy link
Member Author

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

@kashifest
Copy link
Member Author


### Test matrix for k8s version

Current e2e tests use the following Kubernetes versions for source and target clusters:
Copy link
Member

Choose a reason for hiding this comment

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

I know we have this terminology everywhere, but I think we should try to use "bootstrap", "management", "workload" and "self-hosted" terminology instead of "source" and "target". In CAPI "source" and "target" is only used in the context of pivoting. In other contexts it is quite confusing to use these terms.

We start with the bootstrap cluster, we pivot to the target management cluster that is self-hosted.

Suggested change
Current e2e tests use the following Kubernetes versions for source and target clusters:
Current e2e tests use the following Kubernetes versions for bootstrap and self-hosted management clusters:

Copy link
Member

Choose a reason for hiding this comment

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

self-hosted management means the cluster is initialized with control providers however in this context tests like remediation will not initialize the "target" cluster with providers it is a workload cluster.

Copy link
Member

@lentzi90 lentzi90 May 4, 2023

Choose a reason for hiding this comment

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

Ah even more reason to not say target cluster! What about "CAPI cluster" or "metal3 cluster"? Or we can say "workload/management" cluster

Copy link
Member

@mboukhalfa mboukhalfa May 4, 2023

Choose a reason for hiding this comment

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

as management can also refer to bootstrap I suggest to use metal3 cluster or metal3 provisioned cluster

Copy link
Member Author

Choose a reason for hiding this comment

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

this is overcomplicating @mboukhalfa , anyways lets take this PR in so that I can test e2e with correct k8s versions and lets debate on the name of the cluster in a followup I would do soon.


Current e2e tests use the following Kubernetes versions for source and target clusters:

| tests | source cluster | target init | target final |
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
| tests | source cluster | target init | target final |
| tests | bootstrap cluster | self-hosted init | self-hosted final |

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review. I will push a change to this table soon with the uplift to v1.27.1. Can I address the comments in that?

Copy link
Member

Choose a reason for hiding this comment

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

Sure

@mboukhalfa
Copy link
Member

/approve

@metal3-io-bot
Copy link
Contributor

[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 4, 2023
@lentzi90
Copy link
Member

lentzi90 commented May 4, 2023

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label May 4, 2023
@metal3-io-bot metal3-io-bot merged commit 52ec2be into metal3-io:main May 4, 2023
1 check passed
@kashifest kashifest deleted the fix/e2e_k8s branch May 4, 2023 17:55
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants