cilium: rename aws/eni/routing to pkg/datapath/linux/routing by bpineau · Pull Request #11452 · cilium/cilium

bpineau · 2020-05-10T21:57:48Z

Per discussion in #11268 : the routing rules we use here are in
no way specific to AWS ENI; they are for also used to setup
Azure IPAM routes.

While at it, also garbage collect routes on endpoint deletion
when running Azure IPAM mode.

Signed-off-by: Benjamin Pineau benjamin.pineau@datadoghq.com

Also garbage collect Azure IPAM routes on endpoint removal

maintainer-s-little-helper · 2020-05-10T21:57:50Z

Please set the appropriate release note label.

coveralls · 2020-05-10T22:27:07Z

Coverage increased (+0.02%) to 37.035% when pulling f8f1f0d on DataDog:shared-eni-routing into 969600f on cilium:master.

christarazi

Thanks for following up! A few small comments to address for me. Also #11328 is pending, but should be merged soon which I'd like to get in before this one.

christarazi · 2020-05-10T22:57:42Z

We should update this comment

christarazi · 2020-05-10T22:59:49Z

Suggested change

// are setting the Src because interfacesingress rules don't have

// are setting the Src because ingress rules don't have

Nit

christarazi · 2020-05-10T23:01:54Z

Suggested change

}).Debug("Deleting endpoint interface rules")

}).Debug("Deleting endpoint routing rules")

I like "routing" better here as "interface rules" isn't very informative

christarazi · 2020-05-10T23:02:16Z

Suggested change

errs = append(errs, fmt.Errorf("unable to delete endpoint interface rules: %s", err))

errs = append(errs, fmt.Errorf("unable to delete endpoint routing rules: %s", err))

christarazi · 2020-05-10T23:15:55Z

Added backport label as I'd like to backport this to 1.7 since it'll be a branch we support for a while and this PR has changed code which was recently backported, so it'll be good to keep the divergence to a minimum.

maintainer-s-little-helper · 2020-05-11T09:40:48Z

Please set the appropriate release note label.

tklauser · 2020-05-11T12:33:23Z

This needs a rebase now that #11328 is merged.

christarazi

Awesome!

joestringer

Thanks for following up on this.

Per discussion in #11268 : the routing rules we use here are in
no way specific to AWS ENI; they are for also used to setup
Azure IPAM routes.

While at it, also garbage collect routes on endpoint deletion
when running Azure IPAM mode.

I was surprised that the PR named "rename ..." made functional changes, same with the commit naming. Please consider using the biggest change as the title for the PR and commit to make it more clear for anyone searching through the PRs or commits later.

In a similar vein, in future please consider splitting functional changes into a separate commit from the refactoring changes. If we end up wanting to trace back and git-bisect the tree, it's easier to understand what changed when commits are smaller and when the changes are incremental with either functional or cosmetic changes. As is, when I glance through git log, I would have a tendency to skip over this commit because it sounds like it doesn't make any functional changes. Commits like this break this assumption which can be painful when attempting to find the cause of an issue later on via git logs.

The changes themselves look fine. Given that you'll need to do a rebase anyway, do you mind fixing up the commit title?

bpineau · 2020-05-13T07:22:25Z

@joestringer duly noted. I rebased and updated commit title as suggested.

christarazi · 2020-05-14T02:57:37Z

test-me-please

gandro · 2020-05-14T11:28:57Z

retest-runtime

gandro · 2020-05-14T11:29:05Z

retest-net-next

Also garbage collect routes on endpoint deletion when running Azure IPAM mode (the same way we do on AWS instances). And rename aws/eni/routing to pkg/datapath/linux/routing: per discussion in cilium#11268 : the routing rules we use here are in no way specific to AWS ENI; they are for also used to setup Azure IPAM routes. Signed-off-by: Benjamin Pineau <benjamin.pineau@datadoghq.com>

joestringer · 2020-05-14T23:10:36Z

test-me-please

Due to the refactor from #11452, the device name was renamed from "enirouting-test" to "linuxrouting-test" which is 17 characters long, exceeding the max device name size (`IFNAMSIZ`) of 16. [1] This commit renames the device name to be under 16 characters. Fixes #11547 Fixes #11452 [1]: https://elixir.bootlin.com/linux/v5.6/source/include/uapi/linux/if.h#L33 Signed-off-by: Chris Tarazi <chris@isovalent.com>

[ upstream commit 2141eca ] Due to the refactor from #11452, the device name was renamed from "enirouting-test" to "linuxrouting-test" which is 17 characters long, exceeding the max device name size (`IFNAMSIZ`) of 16. [1] This commit renames the device name to be under 16 characters. Fixes #11547 Fixes #11452 [1]: https://elixir.bootlin.com/linux/v5.6/source/include/uapi/linux/if.h#L33 Signed-off-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

bpineau requested review from a team May 10, 2020 21:57

bpineau requested a review from a team as a code owner May 10, 2020 21:57

bpineau requested a review from a team May 10, 2020 21:57

maintainer-s-little-helper Bot added the dont-merge/needs-release-note label May 10, 2020

christarazi requested changes May 10, 2020

View reviewed changes

christarazi added the needs-backport/1.7 label May 10, 2020

bpineau force-pushed the shared-eni-routing branch from 53a7787 to 65fb484 Compare May 11, 2020 06:23

aanm removed the dont-merge/needs-release-note label May 11, 2020

maintainer-s-little-helper Bot added the dont-merge/needs-release-note label May 11, 2020

aanm added the release-note/misc This PR makes changes that have no direct user impact. label May 11, 2020

maintainer-s-little-helper Bot removed the dont-merge/needs-release-note label May 11, 2020

tklauser added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. and removed conflicts-with-pr labels May 11, 2020

aanm approved these changes May 11, 2020

View reviewed changes

christarazi approved these changes May 11, 2020

View reviewed changes

joestringer requested changes May 12, 2020

View reviewed changes

bpineau force-pushed the shared-eni-routing branch from 65fb484 to 26c406a Compare May 13, 2020 06:55

bpineau requested a review from a team as a code owner May 13, 2020 06:55

joestringer approved these changes May 14, 2020

View reviewed changes

christarazi removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label May 14, 2020

christarazi approved these changes May 14, 2020

View reviewed changes

christarazi added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label May 14, 2020

bpineau force-pushed the shared-eni-routing branch from 26c406a to f8f1f0d Compare May 14, 2020 18:25

joestringer removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label May 14, 2020

aanm merged commit fac1802 into cilium:master May 15, 2020

christarazi mentioned this pull request May 15, 2020

Shorten dummy device name in linuxrouting tests #11555

Merged

jrajahalme mentioned this pull request May 20, 2020

v1.7 backports 2020-05-20 #11630

Merged

jrajahalme added backport-pending/1.7 and removed needs-backport/1.7 labels May 20, 2020

joestringer added backport-done/1.7 and removed backport-pending/1.7 labels Jun 3, 2020

	// are setting the Src because interfacesingress rules don't have
	// are setting the Src because ingress rules don't have

	}).Debug("Deleting endpoint interface rules")
	}).Debug("Deleting endpoint routing rules")

	errs = append(errs, fmt.Errorf("unable to delete endpoint interface rules: %s", err))
	errs = append(errs, fmt.Errorf("unable to delete endpoint routing rules: %s", err))

Conversation

bpineau commented May 10, 2020

Uh oh!

maintainer-s-little-helper Bot commented May 10, 2020

Uh oh!

coveralls commented May 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christarazi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christarazi May 10, 2020

Choose a reason for hiding this comment

Uh oh!

christarazi May 10, 2020

Choose a reason for hiding this comment

Uh oh!

christarazi May 10, 2020

Choose a reason for hiding this comment

Uh oh!

christarazi May 10, 2020

Choose a reason for hiding this comment

Uh oh!

christarazi commented May 10, 2020

Uh oh!

maintainer-s-little-helper Bot commented May 11, 2020

Uh oh!

tklauser commented May 11, 2020

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

joestringer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpineau commented May 13, 2020

Uh oh!

christarazi commented May 14, 2020

Uh oh!

gandro commented May 14, 2020

Uh oh!

gandro commented May 14, 2020

Uh oh!

joestringer commented May 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

coveralls commented May 10, 2020 •

edited

Loading

christarazi left a comment •

edited

Loading

joestringer left a comment •

edited

Loading