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

Introduce v3 backend maps #21797

Merged
merged 2 commits into from
Dec 9, 2022
Merged

Introduce v3 backend maps #21797

merged 2 commits into from
Dec 9, 2022

Conversation

YutaroHayakawa
Copy link
Member

Introduce new v3 backend maps for IPv4 and IPv6. Since it modifies the value size of the backend maps, this PR contains the map migration logic as well. The change from the previous map version now includes the cluster_id field. This field will be filled by the control plane that implements Cluster Mesh with overlapping PodCIDR support. When the global service contains backends from the remote clusters with overlapping PodCIDR, we can distinguish them by cluster-aware addressing scheme (see #21161). Note that since the current agent doesn't have actual logic to realize global service with cluster-aware addressing, cluster_id should always be zero for now.

Introduce v3 backend maps

@YutaroHayakawa YutaroHayakawa requested review from a team as code owners October 19, 2022 05:48
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 19, 2022
@YutaroHayakawa YutaroHayakawa added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/clustermesh Relates to multi-cluster routing functionality in Cilium. release-note/misc This PR makes changes that have no direct user impact. labels Oct 19, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 19, 2022
@YutaroHayakawa YutaroHayakawa marked this pull request as draft October 19, 2022 05:50
@YutaroHayakawa YutaroHayakawa force-pushed the yutaro/clustermesh-overlapping-podcidr-oss branch 3 times, most recently from a25440c to 0fcc61a Compare October 19, 2022 07:07
@YutaroHayakawa YutaroHayakawa marked this pull request as ready for review October 19, 2022 08:07
@aanm aanm added the upgrade-impact This PR has potential upgrade or downgrade impact. label Oct 21, 2022
@brb
Copy link
Member

brb commented Oct 26, 2022

Have we decided to have or not to have the V3 -> V2 downgrade path?

@YutaroHayakawa
Copy link
Member Author

Yep, we should have it. Should be covered with separate PRs.

ti-mo
ti-mo approved these changes Nov 14, 2022
View reviewed changes
Copy link
Contributor

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Looks okay from my side given up/downgrade path is added later.

Just curious, why do we bother renaming LB6_BACKEND_MAP_V2 to LB6_BACKEND_MAP_V3 if it's a macro? Updating the cDefinesMap entry should be sufficient.

@ldelossa
Copy link
Contributor

Small nit, but the first two commits could probably be combined into one. They are both small and edit the same file.

ldelossa
ldelossa approved these changes Nov 15, 2022
View reviewed changes
Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

LGTM.

@YutaroHayakawa YutaroHayakawa force-pushed the yutaro/clustermesh-overlapping-podcidr-oss branch from 0fcc61a to 710860c Compare November 16, 2022 06:41
@YutaroHayakawa
Copy link
Member Author

Squashed the first two commits, as Louis mentioned. Also, I noticed third commit was not used anywhere at all. I mistakenly ported it from my dev branch, so I dropped the third commit.

@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Nov 16, 2022 •
edited by maintainer-s-little-helper bot
Loading

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: migrate-svc restart count values do not match

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.9 so I can create one.

@YutaroHayakawa
Copy link
Member Author

Ok, the Upgrade/Downgrade test passed. I'll do run rest of the tests and if they pass, I'll revert the Don't merge commit and make this ready-to-merge.

@YutaroHayakawa
Copy link
Member Author

/test

@YutaroHayakawa YutaroHayakawa added dont-merge/blocked Another PR must be merged before this one. and removed dont-merge/blocked Another PR must be merged before this one. labels Dec 9, 2022
@YutaroHayakawa
clustermesh: Add some helper functions required for v3 backend map
6803960 
Add some missing helper functions to implement v3 backend map.
1. A new AddrCluster constructor AddrClusterFrom
2. A new method of AddrCluster to extract ClusterID

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa
Introduce v3 backend maps
ac14a28 
Extend current backend map values (struct lb4_backend and struct
lb6_backend) to contain a new cluster_id field. With this field, we can
distinguish two backends which have the same IP address, but belong to
different clusters. Since we don't have available padding bit for both
structs anymore, we need to extend the structs and bump the map version
as well. This commit also contains migration code from v2 backend map to
v3 backend map.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa
Copy link
Member Author

Some tests failed with Error: Unable to install Cilium: roles.rbac.authorization.k8s.io "cilium-config-agent" already exists. Perhaps this is related to the recent reverting of the per-node configuration feature. I'll try with rebased tree.

@YutaroHayakawa YutaroHayakawa force-pushed the yutaro/clustermesh-overlapping-podcidr-oss branch from 5e31186 to 2b9cc3b Compare December 9, 2022 08:15
@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Dec 9, 2022 •
edited by maintainer-s-little-helper bot
Loading

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN

Failure Output

FAIL: Failed to reach 192.168.56.11:80 from testclient-9szkf

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

@YutaroHayakawa
Copy link
Member Author

/ci-eks

@YutaroHayakawa
Copy link
Member Author

/ci-aks

@aanm aanm added the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label Dec 9, 2022
@YutaroHayakawa
Copy link
Member Author

/test-runtime

@YutaroHayakawa
Copy link
Member Author

/test-1.25-4.19

@YutaroHayakawa
Copy link
Member Author

/test-runtime

@YutaroHayakawa YutaroHayakawa removed the dont-merge/blocked Another PR must be merged before this one. label Dec 9, 2022
@YutaroHayakawa
Copy link
Member Author

Ok, now it's all green. I'll revert the changes for testing and push again, but since it passed all tests, I'll make this ready to merge.

スクリーンショット 2022-12-10 0 49 12

@YutaroHayakawa YutaroHayakawa force-pushed the yutaro/clustermesh-overlapping-podcidr-oss branch from 2b9cc3b to ac14a28 Compare December 9, 2022 15:53
@YutaroHayakawa YutaroHayakawa added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 9, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.13.0-rc4 Dec 9, 2022
@pchaigno
Copy link
Member

pchaigno commented Dec 9, 2022

Seeing a lot of CI job reruns without justifications here 😬

@pchaigno pchaigno merged commit c6e53ea into cilium:master Dec 9, 2022
@joestringer joestringer mentioned this pull request Dec 21, 2022
Merged
@joestringer joestringer added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Dec 21, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.13 in 1.13.0-rc4 Dec 21, 2022
@joestringer joestringer added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Dec 22, 2022
@joestringer joestringer moved this from Backport pending to v1.13 to Backport done to v1.10 in 1.13.0-rc4 Dec 22, 2022
@joestringer joestringer mentioned this pull request Dec 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrajahalme jrajahalme jrajahalme left review comments

@nebril nebril nebril approved these changes

@ti-mo ti-mo ti-mo approved these changes

@ldelossa ldelossa ldelossa approved these changes

@aspsk aspsk Awaiting requested review from aspsk aspsk was automatically assigned from cilium/sig-lb

@borkmann borkmann Awaiting requested review from borkmann borkmann was automatically assigned from cilium/loader

Assignees
No one assigned
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.13 This issue will prevent the release of the next version of Cilium. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. upgrade-impact This PR has potential upgrade or downgrade impact.
Projects
No open projects
1.13.0-rc4
Backport done to v1.13
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@YutaroHayakawa @brb @ldelossa @pchaigno @nebril @ti-mo @jrajahalme @joestringer @aanm