-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Add MachinePools to Topology Quickstart E2E Templates #9393
🌱 Add MachinePools to Topology Quickstart E2E Templates #9393
Conversation
ce04f67
to
40f254c
Compare
/retest |
Looking into the test failure:
Based on the YAML, we have the following ownerRef on DockerMachinePool - apiVersion: cluster.x-k8s.io/v1beta1
blockOwnerDeletion: true
controller: true
kind: MachinePool
name: quick-start-pysxnr-mp-0-zbnfg
uid: f2c874ee-5eb1-4ae5-aad1-a25a366f53a1 Based on our test code we check for machinePoolOwner dockerMachinePoolKind: func(owners []metav1.OwnerReference) error {
// DockerMachinePool must be owned by a MachinePool.
return HasExactOwners(owners, machinePoolOwner)
}, The reason why this fails should be So I think you have to change it to
It looks like we already have the same for kubeadmConfig:
Please also change the docs here:
Basically the InfrastructureMachinePool row should be:
Please also update:
|
670d3b9
to
4f00f79
Compare
4f00f79
to
42aee3b
Compare
42aee3b
to
0a3fa82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nits
test/e2e/data/infrastructure-docker/main/clusterclass-quick-start.yaml
Outdated
Show resolved
Hide resolved
test/e2e/data/infrastructure-docker/main/clusterclass-quick-start.yaml
Outdated
Show resolved
Hide resolved
test/e2e/data/infrastructure-docker/main/clusterclass-quick-start.yaml
Outdated
Show resolved
Hide resolved
test/e2e/data/infrastructure-docker/main/clusterclass-quick-start.yaml
Outdated
Show resolved
Hide resolved
test/e2e/data/infrastructure-docker/main/clusterclass-quick-start-runtimesdk.yaml
Show resolved
Hide resolved
test/e2e/data/infrastructure-docker/main/clusterclass-quick-start-runtimesdk.yaml
Outdated
Show resolved
Hide resolved
test/e2e/data/infrastructure-docker/main/clusterclass-quick-start.yaml
Outdated
Show resolved
Hide resolved
/test pull-cluster-api-e2e-full-main |
1 similar comment
/test pull-cluster-api-e2e-full-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/retest
I think there is something broken (even if not 100% of the time). It looks like some controllers sometimes don't come up on self-hosted clusters |
432f4a4
to
890f693
Compare
/test pull-cluster-api-e2e-full-main |
I think someone has to run the test locally and check why the Pods are not coming up. I think we don't collect that data in Prow (but not entirely sure there) |
test/e2e/data/infrastructure-docker/main/clusterclass-quick-start-runtimesdk.yaml
Show resolved
Hide resolved
Yup I'll try running this locally. I dug through all the prow artifacts and couldn't find anything of note |
Could be worth a follow-up issue to explore if we could collect controller logs in this situation. Isn't the first time that this is a bit annoying :) |
890f693
to
cb22353
Compare
@sbueringer Looks like the same test passed locally. This does make it tricky to find out why the pods aren't coming up on prow... With
Full build log``` When testing Cluster API working on self-hosted clusters using ClusterClass [ClusterClass] Should pivot the bootstrap cluster to a self-hosted cluster Captured StdOut/StdErr Output >> I0914 20:28:16.822081 446901 warning_handler.go:65] "KubeAPIWarningLogger: Cluster refers to ClusterClass quick-start but this object which hasn't yet been reconciled. Cluster topology has not been fully validated. " Timeline >>
|
5060e47
to
19dd3f3
Compare
Thx! /lgtm /test pull-cluster-api-e2e-full-main Let's confirm by checking the artifacts that DockerMachinePool is created in the self-hosted cased and controllers are coming up |
LGTM label has been added. Git tree hash: 82e3d4ce1eaea19d55182748d520aa13ea62dc5b
|
I introduced this in: #9389 To summarize:
I think this can be fixed with dockerMachinePoolKind: func(owners []metav1.OwnerReference) error {
// DockerMachinePool must be owned and controlled by a MachinePool.
return HasExactOwners(owners, machinePoolController, clusterOwner)
}, kubeadmConfigKind: func(owners []metav1.OwnerReference) error {
// The KubeadmConfig must be owned and controlled by a Machine or MachinePool.
return HasOneOfExactOwners(owners, []metav1.OwnerReference{machineController}, []metav1.OwnerReference{machinePoolController, clusterOwner})
}, |
^^ @killianmuldoon please double-check if the test change sounds reasonable based on #9389 cc @willie-yao Feel free to make the test change already, squash the commits and trigger e2e-full again afterwards |
19dd3f3
to
d9147e4
Compare
/test pull-cluster-api-e2e-full-main |
/lgtm (approval pending green e2e-full + manual verification via artifacts that the self-hosted case works as expected, i.e. it successfully reconciles a MachinePool) |
LGTM label has been added. Git tree hash: c8ce92451ed6f96a697a43f447a0142a9cdd3978
|
Looks like the MachinePool was reconciled succesfully according to the status here: https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/9393/pull-cluster-api-e2e-full-main/1704195435809214464/artifacts/clusters/bootstrap/resources/self-hosted-8h76vm/DockerMachinePool/self-hosted-2mrbjd-mp-0-7q9k6.yaml For my own understanding: Is there a better way to check for whether it is reconciled successfully or not? |
You could also check the MachinePool and the Cluster (the latter for TopologyReconciled: true) But ideally this should not be necessary because the e2e test should validate it. But I think we'll have other e2e tests that will verify this explicitly once we adjusted all e2e tests. Independent of that it would be good to create a follow-up issue to consider adding more validation to our e2e tests. It would be good if we at least validate that TopologyReconciled is true. |
@willie-yao Thank you very much!! Also thx @chrischdi @killianmuldoon for the reviews & troubleshooting. Really appreciate it! /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
/retest flake |
What this PR does / why we need it:
This PR adds MachinePools to existing topology templates for the quickstart E2E tests by changing the
cluster-with-topology.yaml
base, specifically the following templates: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 part of #5991
/area testing