Skip to content

Delete stale v6 address from cilium_host#25962

Merged
dylandreimerink merged 1 commit intocilium:v1.13from
jschwinger233:gray/fix-downgrade
Jun 8, 2023
Merged

Delete stale v6 address from cilium_host#25962
dylandreimerink merged 1 commit intocilium:v1.13from
jschwinger233:gray/fix-downgrade

Conversation

@jschwinger233
Copy link
Copy Markdown
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
Copy Markdown
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 brb and lmb June 7, 2023 06:24
@brb
Copy link
Copy Markdown
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 requested review from chancez, kaworu, nathanjsweet and viktor-kurchenko and removed request for chancez, kaworu and nathanjsweet June 7, 2023 06:59
@jschwinger233 jschwinger233 removed request for a team June 7, 2023 07:18
Comment thread pkg/datapath/loader/base.go Outdated
Comment thread pkg/datapath/loader/base.go Outdated
Comment thread pkg/datapath/loader/base.go Outdated
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
Copy Markdown
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
Copy Markdown
Member Author

/test

@jschwinger233
Copy link
Copy Markdown
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
Copy Markdown
Member Author

/test-backport-1.13

@brb
Copy link
Copy Markdown
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).

Comment thread pkg/datapath/loader/base.go
@jschwinger233
Copy link
Copy Markdown
Member Author

/test-1.19-4.19

@jschwinger233
Copy link
Copy Markdown
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
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

No open projects
Status: Released

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