Skip to content
This repository has been archived by the owner on Jun 28, 2023. It is now read-only.

IPv6 Dual-stack support for Calico Addon #2252

Merged
merged 3 commits into from
Oct 28, 2021
Merged

IPv6 Dual-stack support for Calico Addon #2252

merged 3 commits into from
Oct 28, 2021

Conversation

flawedmatrix
Copy link
Contributor

What this PR does / why we need it

This PR leverages existing support in Calico for dualstack configurations and sets the proper Calico configuration and environment variables for ipv4,ipv6 and ipv6,ipv4 dualstack IP families.

Details for the Release Notes (PLEASE PROVIDE)

Calico now supports ipv4,ipv6 and ipv6,ipv4 dualstack IP families

Which issue(s) this PR fixes

Part of: vmware-tanzu/tanzu-framework#616

Describe testing done for PR

We manually modified the calico package to test this new overlay change and deployed it to a docker workload cluster

Special notes for your reviewer

christianang and others added 2 commits October 11, 2021 18:13
@christianang @flawedmatrix
Calico overlay supports dualstack configurations
cc63fde 
Co-authored-by: Edwin Xie <exie@vmware.com>
@mcwumbly @tylerschultz
Remove CALICO_ROUTER_ID setting when dualstack
94e4185 
CALICO_ROUTER_ID with value hash is needed when a device has no IPv4
address to use for the device's router id. The generated hash may have
collisions in larger deployments. The calico documentation recommends
using it when in ipv6 only deployments.

https://docs.projectcalico.org/reference/node/configuration#setting-calico_router_id-for-ipv6-only-system

Co-authored-by: Tyler Schultz <tschultz@vmware.com>
@flawedmatrix flawedmatrix requested a review from a team as a code owner October 13, 2021 23:51
@github-actions github-actions bot added the owner/packages Work executed by a package's maintainer label Oct 13, 2021
@mcwumbly mcwumbly requested a review from 12345lcr October 14, 2021 00:48
@mcwumbly
Copy link
Contributor

We manually modified the calico package to test this new overlay change and deployed it to a docker workload cluster

Can you elaborate on this a bit?

I was under the impression that CAPD itself does not support dual stack yet. Does that not matter for some reason?

@12345lcr
Copy link
Contributor

12345lcr commented Oct 14, 2021 •
edited
Loading

And please update the readme for the ipFamily part for both 3.11.3 and 3.19.1

seemiller
seemiller suggested changes Oct 20, 2021
View reviewed changes
Copy link
Contributor

@seemiller seemiller left a comment

Choose a reason for hiding this comment

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

Files look good and tests pass. Just update the docs as requested and we can merge.

@github-actions github-actions bot added the owner/docs Work executed by VMware documentation team label Oct 20, 2021
@seemiller
Copy link
Contributor

Just realized that I am no longer in the pkg-calico-owners team. Someone on the team will have to do the final approval.

@12345lcr
Copy link
Contributor

There are a few things that worth to mention:

  1. Currently downstream is adding calico 3.19 and setting up automation test pipelines for it. And we are almost done.
  2. Previously when I was adding calico 3.19 in upstream tce, there were requirements that I should bring the upstream calico 3.19 packages to downstream pipelines for testing and bring back the result. Due to lack of automation tests back then, some hacky ways and bug fixes were involved and took some time.
  3. Once the downstream calico 3.19 is merged, it will be easier to take this pr changes to downstream for testing to make sure these changes won't break anything. And I can take care of that.
  4. I can bring the changes to downstream for testing before or after merge and I am ok with both. But I am still not sure about the process that should the downstream testing results block the upstream pr merge? @seemiller any comments? Thanks.

@seemiller
Copy link
Contributor

@12345lcr As a package owner, you are free to decide what is best regarding the package. From my perspective, if the package is documented and coded correctly, can deploy successfully to a cluster, runs correctly and has passing unit tests, it is good to merge. Downstream use of the package is outside our scope and control. And worst case, everything is in source control, so we can rollback or patch as necessary.

@12345lcr
Copy link
Contributor

LGTM

@12345lcr 12345lcr merged commit 0877439 into vmware-tanzu:main Oct 28, 2021
@12345lcr
Copy link
Contributor

12345lcr commented Oct 28, 2021 •
edited
Loading

Hey @seemiller and @mcwumbly , I think I missed something.

  1. Every time we touch something in the bundle, we need to run make push-package PACKAGE=<package> VERSION=<version> to build and push the package image and update the new image sha to the imgpkgBundle field in addons/packages/<package>/<version>/package.yaml, right? If that is the correct process then it seems we missed that in this pr.

  2. When we want to do some modifications to the bundle, the following image push step will actually overwrite the original image in registry. Which means the PR is not merged, yet the package image is already updated, is this our desired behavior?

  3. Do we need to revert this pr?

@tylerschultz
Copy link
Contributor

I have another question too:

  1. Is there convention for what version semver should be used in scenarios like this? The upstream calico version remains the same, but templating has changed?

@christianang
Copy link
Contributor

  1. Yeah you would need to make/push a package. I suppose ideally it would have been done in this PR, but I think you can just make a follow up PR to update the image with the new templates.

  2. The package.yaml should reference an image with a sha, so the new image won't be consumed until the PR with the image bump is merged.

  3. My 2 cents, I think it makes sense to make a follow up PR to bump the image rather than revert this only to re-PR with the new image.

@12345lcr
Copy link
Contributor

Hi @flawedmatrix , please follow the instructions below to push the new package image to the registry and raise another pr to update the package imgpkgBundle

https://tanzucommunityedition.io/docs/latest/designs/package-process/#6-bundle-configuration-and-deploy-to-registry
https://tanzucommunityedition.io/docs/latest/designs/package-process/#7-createmodify-a-package-cr

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@seemiller seemiller seemiller approved these changes

@12345lcr 12345lcr 12345lcr approved these changes

Assignees
No one assigned
Labels
cla-not-required owner/docs Work executed by VMware documentation team owner/packages Work executed by a package's maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@flawedmatrix @mcwumbly @12345lcr @seemiller @tylerschultz @christianang @vmwclabot