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.

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

Delete stale v6 address from cilium_host #25962

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Jun 7, 2023

Prior to 1.14, Cilium set the cilium_host IPv6 addr to the same one as the native iface, but #24208 replaces the native IPv6 with the one allocated from IPAM. That change breaks the downgrade path due to failures on installing CIDR routes.

To fix the downgrade path, the ideal way is to delete the stale IPv6 on cilium_host, as long as the IPv6 is from IPAM; but in practical, we don't have a perfect approach to tell if an IPv6 is from IPAM due to the complicated situations for multi-pool IPAM, ENI IPAM, and so on.

Therefore, this PR deletes global scope IPv6 on cilium_host as long as the address is not the one we want. We believe this is so far the most robust way to make sure stale addresses are gone.

Fixes: #25938

Signed-off-by: Zhichuan Liang gray.liang@isovalent.com

Fix downgrade path from 1.14 to 1.13 due to stale IPAM-allocated IPv6 on cilium_host

@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 Jun 7, 2023
@jschwinger233 jschwinger233 added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jun 7, 2023
@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 Jun 7, 2023
@jschwinger233 jschwinger233 added upgrade-impact This PR has potential upgrade or downgrade impact. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 7, 2023
@jschwinger233
Copy link
Member Author

The patch has been tested and confirmed to fix the issue.

@jschwinger233 jschwinger233 marked this pull request as ready for review June 7, 2023 06:24
@jschwinger233 jschwinger233 requested a review from a team as a code owner June 7, 2023 06:24
@jschwinger233 jschwinger233 requested review from lmb and brb June 7, 2023 06:24
@brb
Copy link
Member

brb commented Jun 7, 2023

Thanks! I think we should open this PR for the v1.13, but not the main branch?

@jschwinger233 jschwinger233 changed the base branch from main to v1.13 June 7, 2023 06:59
@jschwinger233 jschwinger233 requested review from a team as code owners June 7, 2023 06:59
@jschwinger233 jschwinger233 removed request for a team June 7, 2023 07:18
pkg/datapath/loader/base.go Outdated Show resolved Hide resolved
pkg/datapath/loader/base.go Outdated Show resolved Hide resolved
Prior to 1.14, Cilium set the cilium_host IPv6 addr to the same one as
the native iface, but cilium#24208
replaces the native IPv6 with the one allocated from IPAM. That change
breaks the downgrade path due to failures on installing CIDR routes.

To fix the downgrade path, the ideal way is to delete the stale IPv6 on
cilium_host, as long as the IPv6 is from IPAM; but in practical, we
don't have a perfect approach to tell if an IPv6 is from IPAM due to the
complicated situations for multi-pool IPAM, ENI IPAM, and so on.

Therefore, this commit deletes global scope IPv6 on cilium_host as long
as the address is not the one we want. We believe this is so far the
most robust way to make sure stale addresses are gone.

Fixes: cilium#25938

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks!

@jschwinger233
Copy link
Member Author

/test

@jschwinger233
Copy link
Member Author

jschwinger233 commented Jun 7, 2023

Opened an empty PR based on v1.13 to see which jobs have been broken: #25981
Let's use /test-backport-1.13

@jschwinger233
Copy link
Member Author

/test-backport-1.13

@brb
Copy link
Member

brb commented Jun 7, 2023

Tested with the downgrade CI - it works! https://github.com/cilium/cilium/actions/runs/5198987404/jobs/9375802753 (please ignore the red builds, as they are suffering from other problems).

@jschwinger233
Copy link
Member Author

/test-1.19-4.19

@jschwinger233
Copy link
Member Author

jschwinger233 commented Jun 8, 2023

All the required CI jobs have passed, the red ones are triggered by /test and are supposed to fail for 1.13 according to the probe PR #25981.

@jschwinger233 jschwinger233 added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 8, 2023
@dylandreimerink dylandreimerink merged commit f2cfffd into cilium:v1.13 Jun 8, 2023
102 of 108 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. upgrade-impact This PR has potential upgrade or downgrade impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datapath: When doing downgrade from v1.14 to v1.13 cilium_host IPv6 addr is not changed
4 participants