Make e2e Helm install idempotent #2915
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
/test ls |
k8s-ci-robot
added
the
size/S
|
@CecileRobertMichon: 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-csi-migration |
CecileRobertMichon
force-pushed
the
fix-csi-migration-test
branch
from
03aaa2a to
30aa6be
Compare
|
/test pull-cluster-api-provider-azure-e2e-csi-migration |
1 similar comment
|
/test pull-cluster-api-provider-azure-e2e-csi-migration |
k8s-ci-robot
added
size/M
|
/test pull-cluster-api-provider-azure-e2e-csi-migration |
2 similar comments
|
/test pull-cluster-api-provider-azure-e2e-csi-migration |
|
/test pull-cluster-api-provider-azure-e2e-csi-migration |
k8s-ci-robot
added
size/L
|
/test pull-cluster-api-provider-azure-e2e-csi-migration |
1 similar comment
|
/test pull-cluster-api-provider-azure-e2e-csi-migration |
|
/retest |
|
/test pull-cluster-api-provider-azure-e2e-csi-migration |
CecileRobertMichon
force-pushed
the
fix-csi-migration-test
branch
from
8c6d224 to
cacd07f
Compare
|
/test pull-cluster-api-provider-azure-e2e-csi-migration |
CecileRobertMichon
force-pushed
the
fix-csi-migration-test
branch
from
6c23c14 to
1b59ccb
Compare
CecileRobertMichon
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
CecileRobertMichon
changed the title
|
All the CSI migration specific errors are gone, the last run (https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/2915/pull-cluster-api-provider-azure-e2e-csi-migration/1603196398872825856) fails with the upgrade CoreDNS image pull known issue (kubernetes-sigs/cluster-api#7768) which is the same error we're seeing in https://testgrid.k8s.io/sig-cluster-lifecycle-cluster-api-provider-azure#capi-periodic-upgrade-main-1-23-1-24. I vote we merge this as an incremental improvement even if it doesn't completely fix the test and work on fixing the CoreDNS image pull issue separately (that will likely require changes in CAPI). /assign @marosset @jackfrancis |
| Values: []string{"controller.replicas=1", "controller.runOnControlPlane=true"}, | ||
| } | ||
| // TODO: make this always true once HostProcessContainers are on for all supported k8s versions. | ||
| if hasWindows { |
There was a problem hiding this comment.
Should we propose moving this logic into the official helm chart as well? (before injecting host process stuff into the actual resource spec you must enable the windows.useHostProcessContainers chart config and be installing onto a valid version of Kubernetes)
There was a problem hiding this comment.
That's what the Helm chart is validating actually (as part of the daemonset) and why that option wasn't valid and failing the Helm chart install for templates that didn't enable to correct feature flag for k8s < 1.26
There was a problem hiding this comment.
Do we support anything less than 1.22 now?
There was a problem hiding this comment.
No and in fact I disabled this for anything < 1.23 (https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2915/files#diff-2c7fc9c9c12632c5e550abeb31614356abd17710d948a8664f9d2f2163e38b0dR266). This is to cover the case between 1.23 and 1.26 where HostProcess containers still require the feature flag for this to work (otherwise the Helm install fails), so this makes sure we don't break e2e templates that don't have windows (and thus might not have the feature flag set, which is the case for some of the CSI migration test templates).
|
/test pull-cluster-api-provider-azure-e2e-optional |
|
/lgtm @CecileRobertMichon are these changes needed in release-1.6 and/or release-1.5 to fix broken tests therein? |
k8s-ci-robot
added
the
lgtm
no, #2495 wasn't cherry-picked (rightly so) so the regressions this is fixing were never in 1.5/1.6 |
|
/lgtm |
|
/retest known issue fixed by #2947 |
|
/retest |
|
[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
|
@CecileRobertMichon: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
/retest |
|
Good news: https://testgrid.k8s.io/sig-cluster-lifecycle-cluster-api-provider-azure#capi-periodic-upgrade-main-1-22-1-23 is now passing since this merged |
What type of PR is this?
/kind failing-test
What this PR does / why we need it: In order to fix CSI migration test errors (https://testgrid.k8s.io/sig-cluster-lifecycle-cluster-api-provider-azure#capi-periodic-upgrade-main-1-22-1-23) that started after #2495 merged, we need to make the Helm install idempotent for Calico and cloud provider. Also fixed the errors in the test by not installing CSI drivers before k8s 1.22 and not adding the HostProcess feature flag for windows when windows is not enabled on the template (and the right feature flags aren't in place).
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 #
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: