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.

Sign up for GitHub

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

Jump to bottom

Switch ipv6 template to external cloud-provider #3221

Merged
merged 1 commit into from
Mar 31, 2023
Merged

Switch ipv6 template to external cloud-provider #3221

merged 1 commit into from
Mar 31, 2023

Conversation

CecileRobertMichon
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon commented Mar 2, 2023 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it: Follow up on #3105 to switch the ipv6 template to out of tree. It was previously blocked due to kubernetes-sigs/cloud-provider-azure#3401. Root cause can be found here: kubernetes-sigs/cloud-provider-azure#3401 (comment). The fix is to remove the --node-ip flag from kubelet.

For IPv6, --node-ip should not be set either since it is actually a dual-stack cluster as for Nodes. When someday the cluster is truly IPv6 only, this option should be set.

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:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Switch ipv6 template to external cloud-provider

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 2, 2023
@k8s-ci-robot k8s-ci-robot requested review from marosset and nojnhuh March 2, 2023 23:47
@CecileRobertMichon CecileRobertMichon mentioned this pull request Mar 3, 2023
Closed
@lzhecheng
Copy link
Contributor

/test pull-cluster-api-provider-azure-e2e

1 similar comment
@lzhecheng
Copy link
Contributor

/test pull-cluster-api-provider-azure-e2e

@CecileRobertMichon
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2023
@lzhecheng
Copy link
Contributor

/test pull-cluster-api-provider-azure-e2e

@lzhecheng
Copy link
Contributor

could you please rebase the PR?

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 8, 2023
@CecileRobertMichon
Copy link
Contributor Author

@lzhecheng done

@CecileRobertMichon
Copy link
Contributor Author

ipv6 test passed but the VMSS test failed due to unrelated known flake #3227

/retest

@CecileRobertMichon
Copy link
Contributor Author

interesting... 2/2 passes for ipv6

/retest

@CecileRobertMichon
Copy link
Contributor Author

ipv6 test failed https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/3221/pull-cluster-api-provider-azure-e2e/1633888005896802304

/retest

@CecileRobertMichon
Copy link
Contributor Author

@lzhecheng ipv6 test failed 2 out of 4 runs so far

@lzhecheng
Copy link
Contributor

@lzhecheng ipv6 test failed 2 out of 4 runs so far

Thank you for testing. I will run ipv6 only test with CI pipeline.
#3260

@lzhecheng
Copy link
Contributor

/test pull-cluster-api-provider-azure-e2e

1 similar comment
@lzhecheng
Copy link
Contributor

/test pull-cluster-api-provider-azure-e2e

@CecileRobertMichon CecileRobertMichon mentioned this pull request Mar 20, 2023
Merged
3 tasks
@CecileRobertMichon
Copy link
Contributor Author

test pass 1/1

/test pull-cluster-api-provider-azure-e2e

@CecileRobertMichon
Copy link
Contributor Author

/retest

github api token rate limiting

@CecileRobertMichon
Copy link
Contributor Author

pass 2/2

/test pull-cluster-api-provider-azure-e2e

@CecileRobertMichon
Copy link
Contributor Author

pass 3/3

/test pull-cluster-api-provider-azure-e2e

@CecileRobertMichon
Copy link
Contributor Author

pass 4/4

/test pull-cluster-api-provider-azure-e2e

@CecileRobertMichon CecileRobertMichon changed the title [WIP] Switch ipv6 template to external cloud-provider Switch ipv6 template to external cloud-provider Mar 30, 2023
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 30, 2023
@CecileRobertMichon
Copy link
Contributor Author

/assign @lzhecheng

@lzhecheng
Copy link
Contributor

/lgtm
Thank you!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 30, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d074c5dffb5819ea87421c8648bc8a7ff3d18303

@CecileRobertMichon
Copy link
Contributor Author

/assign @mboersma @jackfrancis

mboersma
mboersma reviewed Mar 31, 2023
View reviewed changes
Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm

mboersma
mboersma reviewed Mar 31, 2023
View reviewed changes
Tiltfile
@@ -371,7 +371,7 @@ def deploy_worker_templates(template, substitutions):
else:
calico_values = "./templates/addons/calico/values.yaml"
flavor_cmd += "; " + helm_cmd + " repo add projectcalico https://docs.tigera.io/calico/charts; " + helm_cmd + " --kubeconfig ./${CLUSTER_NAME}.kubeconfig install calico projectcalico/tigera-operator -f " + calico_values + " --namespace tigera-operator --create-namespace"
if "intree-cloud-provider" not in flavor_name and "ipv6" not in flavor_name: # TODO: remove ipv6 once https://github.com/kubernetes-sigs/cloud-provider-azure/issues/3401 is fixed.
if "intree-cloud-provider" not in flavor_name:
Copy link
Contributor

@mboersma mboersma Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Does this fix kubernetes-sigs/cloud-provider-azure#3401 then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mboersma yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note there is a separate PR in k/k to "fix" the issue but this PR mitigates the symptoms: kubernetes/kubernetes#116879

@CecileRobertMichon
Copy link
Contributor Author

/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2023
@k8s-ci-robot k8s-ci-robot merged commit dddbac9 into kubernetes-sigs:main Mar 31, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.9 milestone Mar 31, 2023
@CecileRobertMichon CecileRobertMichon mentioned this pull request Apr 5, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mboersma mboersma mboersma left review comments

@marosset marosset Awaiting requested review from marosset

@nojnhuh nojnhuh Awaiting requested review from nojnhuh

Assignees

@mboersma mboersma

@jackfrancis jackfrancis

@lzhecheng lzhecheng

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.
5 participants
@CecileRobertMichon @lzhecheng @k8s-ci-robot @mboersma @jackfrancis