Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

network: While updating routes, do not delete routes with proto "kernel"#624

Merged
egernst merged 2 commits intokata-containers:masterfrom
amshinde:skip-deleting-kernel-proto-routes
Aug 9, 2019
Merged

network: While updating routes, do not delete routes with proto "kernel"#624
egernst merged 2 commits intokata-containers:masterfrom
amshinde:skip-deleting-kernel-proto-routes

Conversation

@amshinde
Copy link
Copy Markdown
Member

@amshinde amshinde commented Aug 6, 2019

Routes with proto kernel are automatically added by the kernel
when an interface is added with ip assigned as a subnet address.
With github.com/kata-containers/runtime#1936, these routes will no
longer be sent from the runtime.

Depends-on: github.com/kata-containers/runtime#1936

Fixes: #623

Signed-off-by: Archana Shinde archana.m.shinde@intel.com

Copy link
Copy Markdown
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

lgtm
(note, also fixed in the runtime I believe...)

@amshinde amshinde requested a review from mcastelino August 6, 2019 15:49
amshinde added a commit to amshinde/kata-runtime that referenced this pull request Aug 6, 2019
Routes with proto "kernel" are routes that are automatically added
by the kernel.
It is a route added automatically when you assign an address to an
interface which is not /32.
With this commit, these routes are ignored. The guest kernel
would add these routes on the guest side. A corresponding commit on the
agent side would no longer delete these routes while updating them.

Without this commit, netlink gives an error complaining that a route
already exists when you try to add a route with the same dest subnet.

Something like:
dest: 192.168.1.0/24 device:net1 source:192.168.1.217 scope:253
dest: 192.168.1.0/24 device:net2 source:192.168.1.218 scope:253

Depends-on: github.com/kata-containers/agent#624

Fixes: kata-containers#1811

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde amshinde force-pushed the skip-deleting-kernel-proto-routes branch 2 times, most recently from 873e887 to fe9721a Compare August 6, 2019 17:38
amshinde added a commit to amshinde/kata-runtime that referenced this pull request Aug 6, 2019
Routes with proto "kernel" are routes that are automatically added
by the kernel.
It is a route added automatically when you assign an address to an
interface which is not /32.
With this commit, these routes are ignored. The guest kernel
would add these routes on the guest side. A corresponding commit on the
agent side would no longer delete these routes while updating them.

Without this commit, netlink gives an error complaining that a route
already exists when you try to add a route with the same dest subnet.

Something like:
dest: 192.168.1.0/24 device:net1 source:192.168.1.217 scope:253
dest: 192.168.1.0/24 device:net2 source:192.168.1.218 scope:253

Depends-on: github.com/kata-containers/agent#624

Fixes: kata-containers#1811

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde amshinde requested a review from egernst August 6, 2019 21:46
Routes with proto kernel are automatically added by the kernel
when an interface is added with ip assigned as a subnet address.
With github.com/kata-containers/runtime#1936, these routes will no
longer be sent from the runtime.

Depends-on: github.com/kata-containers/runtime#1936

Fixes: kata-containers#623

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde amshinde force-pushed the skip-deleting-kernel-proto-routes branch from fe9721a to 2537235 Compare August 9, 2019 01:03
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 9, 2019

Codecov Report

Merging #624 into master will increase coverage by 0.1%.
The diff coverage is 66.66%.

@@            Coverage Diff            @@
##           master     #624     +/-   ##
=========================================
+ Coverage   61.37%   61.47%   +0.1%     
=========================================
  Files          17       17             
  Lines        2506     2523     +17     
=========================================
+ Hits         1538     1551     +13     
- Misses        822      824      +2     
- Partials      146      148      +2

This is not recommended, however we should not error out in
this scenario.

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde
Copy link
Copy Markdown
Member Author

amshinde commented Aug 9, 2019

/test

@amshinde
Copy link
Copy Markdown
Member Author

amshinde commented Aug 9, 2019

PR updated to allow replacing routes.
@egernst PTAL.

Copy link
Copy Markdown
Member

@egernst egernst left a comment

Choose a reason for hiding this comment

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

thanks for the unit tests, too.

@egernst egernst merged commit a78e8cf into kata-containers:master Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Do not delete routes with proto as "kernel"

4 participants