Add E2E test for cluster class #2235
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
/test pull-cluster-api-provider-azure-e2e |
shysank
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/test ls |
|
@shysank: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
|
/test pull-cluster-api-provider-azure-e2e |
|
/test pull-cluster-api-provider-azure-e2e |
|
/test pull-cluster-api-provider-azure-ci-entrypoint |
|
/test pull-cluster-api-provider-azure-e2e |
|
@CecileRobertMichon @mboersma ptal, whenever you get a chance. |
| machineHealthCheck: | ||
| maxUnhealthy: 100% | ||
| unhealthyConditions: | ||
| - type: E2ENodeUnhealthy |
There was a problem hiding this comment.
Where does this value come from? I am looking into adding a health check for windows on the other e2e test but can not determine if this health condition is valid. I don't find this type in capz/capi repositories and the docs give examples using Ready on the machines: https://cluster-api.sigs.k8s.io/tasks/healthcheck.html#creating-a-machinehealthcheck
Using Ready on the machines makes sense but I wasn't sure if I was missing something
There was a problem hiding this comment.
I took it from the existing templates, but not sure how it works though.
|
/test pull-cluster-api-provider-azure-e2e-optional |
|
/test pull-cluster-api-provider-azure-e2e |
|
@CecileRobertMichon I think I've addressed all the comments. PTAL, whenever you get a chance. |
| identityRef: | ||
| apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 | ||
| kind: AzureClusterIdentity | ||
| location: replace_me |
There was a problem hiding this comment.
just curious, is "replace_me" an arbitrary string or is it some agreed upon contract/pattern for required variables in clusterclass?
There was a problem hiding this comment.
It's unfortunately just a workaround for fields that have to be set for the validating webhook and are then later overwritten per cluster.
I think this workaround kind of becomes a pattern right now, but there is no standardization (yet).
Not sure how we can improve this, maybe it's an option that these fields should be optional when the resource is referenced in a ClusterClass. But not sure how feasible this is.
(as far as I'm aware CAPA is using a similar but not the same string)
There was a problem hiding this comment.
CAPA is using REPLACEME. I don't particularly have a preference but I vaguely remember using replace_me being used in other places where a similar mechanism was involved.
There was a problem hiding this comment.
I'm inferring from reading this history that empty string val doesn't suffice?
There was a problem hiding this comment.
Afaik the validation webhook will complain
There was a problem hiding this comment.
Is there a capi issue tracking how we might improve this? IMO this is a fairly significant bit of UX friction for users to deal with, we should do this work on behalf of them (happy to help).
Everything else here actually looks fine to me and test signal is green, I'm going to mark this as /lgtm.
Thanks so much @shysank, really happy to have ClusterClass coverage in capz!
There was a problem hiding this comment.
I'm not sure, we probably have some guidance around cluster-specific fields and that they shouldn't be added to cluster templates (which was in general nicely done by CAPZ). But I"m not sure if that falls under the "cluster-specific" field category.
I think it makes sense to open an issue in CAPI and see what we have / need.
We can discuss documentation improvements or potential implementation changes to mitigate this issue (e.g. maybe some fields shouldn't be mandatory on a cluster template / machine template when it's used as template in a ClusterClass).
|
let's squash commits? |
shysank
force-pushed
the
cc_templates
branch
2 times, most recently
from
1766e20 to
238fcf5
Compare
|
/test pull-cluster-api-provider-azure-ci-entrypoint |
|
/test pull-cluster-api-provider-azure-e2e-optional |
k8s-ci-robot
added
the
needs-rebase
|
@shysank will you be able to rebase this or would you like someone else to take over? |
|
/assign Happy to inherit this by default if @shysank no longer has cycles |
cc @sonasingh46 (just fyi) |
k8s-ci-robot
removed
the
needs-rebase
|
@CecileRobertMichon I've rebased it with main. |
|
/milestone v1.4 |
jackfrancis
force-pushed
the
cc_templates
branch
from
7ef8fa5 to
15e070c
Compare
|
Got a passing test locally after rebasing this PR. I think I see some things that can be cleaned up, will do so now. But this looks generally good to go. |
|
/test pull-cluster-api-provider-azure-e2e-optional |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon 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 |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Adds e2e test for clusterclass. The e2e test works as follows:
ci-defaultci-defaultas topology.To do (1), I've created a new flavor called
topology, and use the generated template to create the cluster class. This is not ideal, but it helps to get started with the existing capi test framework.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 #2234
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs:
Release note: