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

Populates backend map from V2 backend map #17308

Merged
merged 1 commit into from
Sep 13, 2021
Merged

Populates backend map from V2 backend map #17308

merged 1 commit into from
Sep 13, 2021

Conversation

Weil0ng
Copy link
Contributor

@Weil0ng Weil0ng commented Sep 3, 2021 •
edited

This is a lossy process, we copy everything from v2 map to v1 map but
since v1 map's key type is smaller (16b vs 32b in v2), for entries with
and ID that is larger than 64k, we drop them. This means existing
connection could potentially be interrupted when downgrading from v2 to
v1.

V2 map introduced in #17235, this is the downgrade path.

Signed-off-by: Weilong Cui cuiwl@google.com

Fixes: #17262

@Weil0ng Weil0ng added the release-note/misc This PR makes changes that have no direct user impact. label Sep 3, 2021
@Weil0ng Weil0ng requested review from brb, pchaigno and a team September 3, 2021 21:32
@maintainer-s-little-helper maintainer-s-little-helper bot assigned brb and pchaigno and unassigned brb Sep 3, 2021
@Weil0ng Weil0ng marked this pull request as draft September 3, 2021 21:41
@Weil0ng Weil0ng marked this pull request as ready for review September 3, 2021 22:16
@Weil0ng Weil0ng mentioned this pull request Sep 3, 2021
Merged
@Weil0ng Weil0ng force-pushed the v2tov1 branch 2 times, most recently from 9a77289 to a0ca618 Compare September 4, 2021 00:10
@Weil0ng Weil0ng mentioned this pull request Sep 4, 2021
Closed
pchaigno
pchaigno requested changes Sep 6, 2021
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

I think this pull request should only be opened against v1.10 given we won't need this code in v1.11.

pkg/service/service.go Outdated Show resolved Hide resolved
@Weil0ng Weil0ng requested a review from a team as a code owner September 6, 2021 20:20
@Weil0ng Weil0ng changed the base branch from master to v1.10 September 6, 2021 20:20
@Weil0ng Weil0ng requested review from a team as code owners September 6, 2021 20:20
@pchaigno pchaigno added kind/backports This PR provides functionality previously merged into master. upgrade-impact This PR has potential upgrade or downgrade impact. and removed release-note/misc This PR makes changes that have no direct user impact. labels Sep 7, 2021
brb
brb reviewed Sep 7, 2021
View reviewed changes
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.

LGTM, a few nits. Also, I think backporters would appreciate if you could provide more context in the commit msg / PR description why this needs to be backported to v1.10.

pkg/service/service.go Outdated Show resolved Hide resolved
pkg/service/service.go Outdated Show resolved Hide resolved
pkg/service/service.go Outdated Show resolved Hide resolved
pkg/service/service.go Outdated Show resolved Hide resolved
pkg/service/service.go Outdated Show resolved Hide resolved
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Sep 7, 2021 •
edited by maintainer-s-little-helper bot

test-me-please

Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with direct routing Tests LoadBalancer Connectivity to endpoint via LB

Failure Output

FAIL: Can not connect to service "http://192.168.1.146" from outside cluster (1/10)

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.16-net-next so I can create a new GitHub issue to track it.

@Weil0ng Weil0ng requested a review from brb September 7, 2021 17:56
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Sep 7, 2021

Travis seems to hit a github infra flakiness (connection timed out while checking out env).

brb
brb requested changes Sep 8, 2021
View reviewed changes
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.

Did another round of reviewing. A few things.

pkg/service/service.go Outdated Show resolved Hide resolved
pkg/service/service.go Outdated Show resolved Hide resolved
pkg/service/service.go Outdated Show resolved Hide resolved
@Weil0ng Weil0ng requested a review from brb September 8, 2021 19:23
brb
brb requested changes Sep 9, 2021
View reviewed changes
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.

OK, getting closer to ready :-) Last comment about DRY.

pkg/service/service.go Outdated Show resolved Hide resolved
@Weil0ng
Populates backend map from V2 backend map
570f8ed 
This is potentially a lossy process, ideally we copy everything from v2
map to v1 map but since v1 map's key type is smaller (16b vs 32b in v2),
for entries with an ID that is larger than 64k, we drop them. This means
existing connection could potentially be interrupted when downgrading
from v2 to v1.

This logic needs to go into 1.10 releases, because we will introduce v2
map in 1.11 and the downgrade logic needs to be in place.

Signed-off-by: Weilong Cui <cuiwl@google.com>
@Weil0ng Weil0ng requested a review from brb September 9, 2021 20:33
@brb
Copy link
Member

brb commented Sep 10, 2021

test-backport-v1.10

@brb
Copy link
Member

brb commented Sep 10, 2021

test-backport-1.10

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Sep 10, 2021

Hit flake: #17365

@Weil0ng Weil0ng added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 13, 2021
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Sep 13, 2021

Only test failure is a known flake tracked in cilium/cilium-cli#367, not related to change.

@kaworu kaworu merged commit e23dc61 into cilium:v1.10 Sep 13, 2021
@joestringer joestringer mentioned this pull request Oct 13, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaworu kaworu kaworu approved these changes

@brb brb brb approved these changes

@pchaigno pchaigno pchaigno approved these changes

Assignees

@Weil0ng Weil0ng

Labels
kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. upgrade-impact This PR has potential upgrade or downgrade impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

V2->V1 bemap downgrade patch in 1.10
10 participants
@Weil0ng @brb @kaworu @pchaigno @nathanjsweet @tklauser @joestringer @tgraf @ldelossa @jrajahalme