:warning: E2E test now resolve CNI_RESOURCES without using env variables by fabriziopandini · Pull Request #3846 · kubernetes-sigs/cluster-api

fabriziopandini · 2020-10-22T10:25:00Z

This PR makes the E2E framework to resolve CNI_RESOURCES without using env variables, thus avoiding problems when the CNI manifest is too long.

As a consequence of this change the call to SetCNIEnvVar should be now replaced by one (or more) calls to createRepositoryInput.RegisterClusterResourceSetConfigMapTransformation

Fixes #3797
/assign @CecileRobertMichon

/hold

fabriziopandini · 2020-10-22T20:05:08Z

/hold cancel

fabriziopandini · 2020-10-22T20:05:37Z

/assign @CecileRobertMichon

CecileRobertMichon · 2020-10-22T20:21:51Z

@fabriziopandini were you able to test this with the longer calico manifest?

CecileRobertMichon · 2020-10-22T20:22:59Z

+// Deprecated: CNI manifest will be now automatically embedded into cluster templates during create repository.
+// The new approach does not uses env variables so we can avoid https://github.com/kubernetes-sigs/cluster-api/issues/3797;
+// This func is preserved for avoiding to break users in the v0.3 series, but it is now a noop.
 func SetCNIEnvVar(cniManifestPath string, cniEnvVar string) {


should we make this PR has "breaking" since this is now a no-op?

makes sense, done + updated PR comment

CecileRobertMichon

lgtm

This doesn't look to be breaking for other providers using CNI with CRS but assigning a few others to confirm 👀
/assign @sedefsavas @richardcase @cpanato

thanks for figuring out the root cause @fabriziopandini!

sedefsavas · 2020-10-23T00:12:19Z

+	CNIPath      = "CNI"
+	CNIResources = "CNI_RESOURCES"


If we define these here, I think we will limit the CNIs to only 1 per e2e tests.

Previously, to use a different CNI, these parameters were being changed and by setting the changed parameters to CRS as data was working fine.
CAPZ is using multiple CNIs. cc @CecileRobertMichon

Ooh great catch @sedefsavas. That's true, we do that for IPv6 https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/test/e2e/e2e_suite_test.go#L171 (and actually have a different CNIPath defined for that and rely on the SetCNIEnvVar function).

Good catch, I was not aware of the multiple CNI use case!

CecileRobertMichon · 2020-10-23T00:16:59Z

/hold

based on @sedefsavas's comment above

fabriziopandini · 2020-10-23T10:35:38Z

@CecileRobertMichon @sedefsavas I have proposed a solution that allows managing multiple CNI without requiring env variables.

/test pull-cluster-api-e2e-full-release-0-3

sedefsavas · 2020-10-23T16:42:17Z

+	cniPAth := config.GetVariable(CNIPath)
+	Expect(cniPAth).To(BeAnExistingFile(), "The %s variable should resolve to an existing file", CNIPath)
+
+	createRepositoryInput.RegisterClusterResourceSetConfigMapTransformation(cniPAth, CNIResources)


Just to understand, will createClusterctlLocalRepository be called multiple times for adding different CNIs?

No, there will be only one call to createClusterctlLocalRepository as of today.
However, inside this func you have to register one ClusterResourceSetConfigMapTransformation for each CNI you want to support.

sedefsavas · 2020-10-23T17:43:30Z

This lgtm now.
MachinPool test failed but may be a flake.

/test pull-cluster-api-e2e-full-release-0-3

CecileRobertMichon · 2020-10-23T18:13:13Z

I updated my Calico PR to use your branch and applied the changes to verify everything works as expected with two CNIs https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/991/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R37

fabriziopandini · 2020-10-23T18:39:13Z

kk, I will wait for test results in CAPZ before addressing the nits

CecileRobertMichon · 2020-10-23T20:03:14Z

CAPZ e2e passed 🎉

fabriziopandini · 2020-10-23T20:06:37Z

@CecileRobertMichon thanks for validating this change on CAPZ!
comment addresses and squashed

CecileRobertMichon · 2020-10-23T20:08:33Z

thanks for fixing it :)

/lgtm
/approve
/hold cancel

k8s-ci-robot · 2020-10-23T20:08:41Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

CecileRobertMichon · 2020-10-23T21:01:00Z

/retest

k8s-ci-robot · 2020-10-23T21:03:09Z

@fabriziopandini: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-e2e-main	`ed531f4`	link	`/test pull-cluster-api-e2e-main`
pull-cluster-api-make-main	`ed531f4`	link	`/test pull-cluster-api-make-main`
pull-cluster-api-build-main	`ed531f4`	link	`/test pull-cluster-api-build-main`
pull-cluster-api-test-main	`ed531f4`	link	`/test pull-cluster-api-test-main`
pull-cluster-api-apidiff-main	`ed531f4`	link	`/test pull-cluster-api-apidiff-main`
pull-cluster-api-apidiff-release-0-3	`14dba99`	link	`/test pull-cluster-api-apidiff-release-0-3`

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.

Details

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.

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 22, 2020

k8s-ci-robot requested review from detiber and vincepri October 22, 2020 10:25

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 22, 2020

fabriziopandini force-pushed the investigate-3797 branch from 54376e9 to 76b678a Compare October 22, 2020 10:55

fabriziopandini mentioned this pull request Oct 22, 2020

E2E tests break with long CNI spec #3797

Closed

fabriziopandini force-pushed the investigate-3797 branch from 76b678a to ed531f4 Compare October 22, 2020 19:04

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2020

fabriziopandini changed the base branch from master to release-0.3 October 22, 2020 19:09

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2020

fabriziopandini force-pushed the investigate-3797 branch 2 times, most recently from f6bbcb7 to c9132c9 Compare October 22, 2020 19:25

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 22, 2020

fabriziopandini changed the title ~~Investigate 3797~~ E2E test now resolve CNI_RESOURCES without using env variables Oct 22, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 22, 2020

k8s-ci-robot assigned CecileRobertMichon Oct 22, 2020

CecileRobertMichon reviewed Oct 22, 2020

View reviewed changes

fabriziopandini mentioned this pull request Oct 22, 2020

🌱 Update CAPI to v0.3.11-rc.0 and Calico addon template to v3.16.3 kubernetes-sigs/cluster-api-provider-azure#991

Merged

3 tasks

fabriziopandini changed the title ~~E2E test now resolve CNI_RESOURCES without using env variables~~ :Warning E2E test now resolve CNI_RESOURCES without using env variables Oct 22, 2020

CecileRobertMichon changed the title ~~:Warning E2E test now resolve CNI_RESOURCES without using env variables~~ ⚠️ E2E test now resolve CNI_RESOURCES without using env variables Oct 22, 2020

CecileRobertMichon approved these changes Oct 22, 2020

View reviewed changes

k8s-ci-robot assigned cpanato, richardcase and sedefsavas Oct 22, 2020

sedefsavas reviewed Oct 23, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 23, 2020

sedefsavas reviewed Oct 23, 2020

View reviewed changes

CecileRobertMichon reviewed Oct 23, 2020

View reviewed changes

Comment thread test/e2e/e2e_suite_test.go Outdated

CecileRobertMichon reviewed Oct 23, 2020

View reviewed changes

Comment thread test/e2e/e2e_suite_test.go Outdated

CecileRobertMichon reviewed Oct 23, 2020

View reviewed changes

Comment thread test/e2e/e2e_suite_test.go Outdated

E2E framework resolves CNI_RESOURCES without using env vars

14dba99

fabriziopandini force-pushed the investigate-3797 branch from cb51836 to 14dba99 Compare October 23, 2020 20:05

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 23, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2020

k8s-ci-robot merged commit 035db28 into kubernetes-sigs:release-0.3 Oct 23, 2020

fabriziopandini deleted the investigate-3797 branch October 26, 2020 11:02

fabriziopandini mentioned this pull request Oct 27, 2020

Forward-port E2E test now resolve CNI_RESOURCES without using env variables #3874

Closed

arghya88 mentioned this pull request Nov 3, 2020

⚠️ E2E framework resolves CNI_RESOURCES without using env vars #3896

Merged

sedefsavas mentioned this pull request Dec 1, 2020

🐛Fix for CAPI 3.11 upgrade kubernetes-sigs/cluster-api-provider-aws#2133

Merged

Conversation

fabriziopandini commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabriziopandini commented Oct 22, 2020

Uh oh!

fabriziopandini commented Oct 22, 2020

Uh oh!

CecileRobertMichon commented Oct 22, 2020

Uh oh!

CecileRobertMichon Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

fabriziopandini Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon left a comment

Choose a reason for hiding this comment

Uh oh!

sedefsavas Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

fabriziopandini Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon commented Oct 23, 2020

Uh oh!

fabriziopandini commented Oct 23, 2020

Uh oh!

sedefsavas Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

fabriziopandini Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

sedefsavas commented Oct 23, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CecileRobertMichon commented Oct 23, 2020

Uh oh!

fabriziopandini commented Oct 23, 2020

Uh oh!

CecileRobertMichon commented Oct 23, 2020

Uh oh!

fabriziopandini commented Oct 23, 2020

Uh oh!

CecileRobertMichon commented Oct 23, 2020

Uh oh!

k8s-ci-robot commented Oct 23, 2020

Uh oh!

CecileRobertMichon commented Oct 23, 2020

Uh oh!

k8s-ci-robot commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fabriziopandini commented Oct 22, 2020 •

edited

Loading

fabriziopandini Oct 23, 2020 •

edited

Loading

k8s-ci-robot commented Oct 23, 2020 •

edited

Loading