-
Notifications
You must be signed in to change notification settings - Fork 114
Conversation
The go netlink package has added more netlink socket protocols than we need. Specify one to avoid failure due to unsupported protocols. Fixes: kata-containers#209 Signed-off-by: Peng Tao <bergwolf@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #210 +/- ##
==========================================
+ Coverage 34.7% 35.37% +0.67%
==========================================
Files 11 11
Lines 1533 1631 +98
==========================================
+ Hits 532 577 +45
- Misses 929 969 +40
- Partials 72 85 +13
Continue to review full report at Codecov.
|
LGTM. I tested it out on my x86 machine and the kata container is getting created with the hyperstart kernel using the initrd approach. sudo docker run -ti --runtime kata-runtime busybox sh |
@bergwolf does this change is to remove XFRM support from the kernel? |
@devimc actually it was |
@grahamwhaley PTAL. |
@bergwolf ok, then IP_SET is not needed, right? |
@devimc could you give more context about what you're talking about ? I am asking because I don't understand the relation between your comments and this change. |
@sboeuf this change impacts to the kernel, when a new netlink handler is created three netlink families are used, NETLINK_ROUTE, NETLINK_XFRM and NETLINK_NETFILTER, that means kernel need to have enabled these three families, if this patch is merged, then we can rid of NETLINK_XFRM and NETLINK_NETFILTER, since we don't need them |
@bergwolf do you know what config do we need to enable NETLINK_ROUTE ? |
@devimc thanks for the explanation here. So because this change makes only use of |
@sboeuf yes |
@devimc Ok then I think we could open an issue for that, as I expect that we will merge this PR very soon. This way we can continue the discussion. |
@sboeuf kata-containers/linux#5 has not been merged, so I'll update the PR |
@devimc We are apparently not depending on |
@bergwolf good question, @mcastelino @amshinde WDYT ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@mcastelino We may end up needing iptables for supporting docker compose in the future. |
@mcastelino in runv, we use iptables to setup port mapping and to support docker CNM. @ALL whether we need NETLINK_XFRM and NETLINK_NETFILTER is not relevant to this patch. Can we go ahead and merge it and have another issue to discuss kernel config options? |
I am all for merging this. @egernst @grahamwhaley PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change
lgtm
the discussion about kernel configs can happen elsewhere indeed.
codecov is not too happy - I think mainly because there is not huge test coverage for this file - but, these changes don't really make it any worse, so should not be a hard block on a merge (but, it would be nice if we could sneak some more tests in at the same time ;-) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
agreed @grahamwhaley. Let's merge ignoring the codecov failure (which actually reports |
The go netlink package has added more netlink socket protocols
than we need. Specify one to avoid failure due to unsupported
protocols.
Fixes: #209