Use fewer modprobes#2351
Merged
selansen merged 1 commit intomoby:masterfrom Jul 25, 2019
daym:fewer-modprobes
Merged
Conversation
thaJeztah
reviewed
Mar 26, 2019
| logrus.Warnf("Running modprobe nf_nat failed with message: `%s`, error: %v", strings.TrimSpace(string(out)), err) | ||
| path, err := exec.LookPath("iptables") | ||
| if err != nil { | ||
| return |
Member
There was a problem hiding this comment.
Should this still print a warning perhaps, if iptables cannot be found?
Contributor
Author
There was a problem hiding this comment.
Sure, why not.
Note: detectIptables, where I copied that chunk from, doesn't either.
Member
There was a problem hiding this comment.
Ah, hm, yes, if it wasn't there (not sure why though 🤔 😅) (I'm not a maintainer in this repository, so perhaps there was a reason for that 😅)
Member
Member
thaJeztah
requested changes
Apr 10, 2019
ns/init_linux.go
Outdated
| if out, err := exec.Command("modprobe", "-va", "xfrm_algo").CombinedOutput(); err != nil { | ||
| return fmt.Errorf("Running modprobe xfrm_algo failed with message: `%s`, error: %v", strings.TrimSpace(string(out)), err) | ||
| } | ||
| // Those are automatically loaded when someone opens the socket anyway. |
Member
There was a problem hiding this comment.
This function is not exported, so perhaps it should be removed altogether; looks like checkXfrmSocket() is also no longer called now, so could be removed (do we need to try that socket to force the modules to load?)
perhaps add a comment in the calling function about these being automatically loaded, e.g.;
diff --git a/ns/init_linux.go b/ns/init_linux.go
index 6a56b837..ed78bfa6 100644
--- a/ns/init_linux.go
+++ b/ns/init_linux.go
@@ -74,17 +74,10 @@ func NlHandle() *netlink.Handle {
}
func getSupportedNlFamilies() []int {
- fams := []int{syscall.NETLINK_ROUTE}
- // NETLINK_XFRM test
- if err := loadXfrmModules(); err != nil {
- if checkXfrmSocket() != nil {
- logrus.Warnf("Could not load necessary modules for IPSEC rules: %v", err)
- } else {
- fams = append(fams, syscall.NETLINK_XFRM)
- }
- } else {
- fams = append(fams, syscall.NETLINK_XFRM)
- }
+ // No need to "modprobe" xfrm_user and xfrm_algo, as they are loaded
+ // automatically when a socket is opened
+ fams := []int{syscall.NETLINK_ROUTE, syscall.NETLINK_XFRM}
+
// NETLINK_NETFILTER test
if err := loadNfConntrackModules(); err != nil {
if checkNfSocket() != nil {
@@ -99,21 +92,6 @@ func getSupportedNlFamilies() []int {
return fams
}
-func loadXfrmModules() error {
- // Those are automatically loaded when someone opens the socket anyway.
- return nil
-}
-
-// API check on required xfrm modules (xfrm_user, xfrm_algo)
-func checkXfrmSocket() error {
- fd, err := syscall.Socket(syscall.AF_NETLINK, syscall.SOCK_RAW, syscall.NETLINK_XFRM)
- if err != nil {
- return err
- }
- syscall.Close(fd)
- return nil
-}
-
func loadNfConntrackModules() error {
if out, err := exec.Command("modprobe", "-va", "nf_conntrack").CombinedOutput(); err != nil {
return fmt.Errorf("Running modprobe nf_conntrack failed with message: `%s`, error: %v", strings.TrimSpace(string(out)), err)
Contributor
Author
There was a problem hiding this comment.
Right. But is XFRM supposed to be optional? maybe checkXfrmSocket was supposed to be called still.
Frankly, I hadn't seen the part
if err := loadXfrmModules(); err != nil {
Maybe it would be better to call checkXfrmSocket anyway--and warn on error.
If the XFRM modules are mandatory for this module anyway, I agree with your approach.
Member
There was a problem hiding this comment.
Yes, was suspecting the same, hence my question above;
do we need to try that socket to force the modules to load?
So probably call it unconditionally, and indeed produce an error on failure
Contributor
Author
There was a problem hiding this comment.
I've updated the branch to do that.
Signed-off-by: Danny Milosavljevic <dannym@scratchpost.org>
Member
|
ping @arkodg @kolyshkin PTAL |
Contributor
|
Thanks @daym for working on this PR. |
thaJeztah
added a commit
to thaJeztah/docker
that referenced
this pull request
Jul 30, 2019
full diff: moby/libnetwork@83d30db...09cdcc8 changes included: - moby/libnetwork#2416 Fix hardcoded AF_INET for IPv6 address handling - moby/libnetwork#2411 Macvlan network handles netlabel.Internal wrong - fixes moby/libnetwork#2410 Macvlan network handles netlabel.Internal wrong - moby/libnetwork#2414 Allow network with --config-from to be --internal - fixes moby/libnetwork#2413 Network with --config-from does not honor --internal - moby/libnetwork#2351 Use fewer modprobes - relates to moby#38930 Use fewer modprobes - moby/libnetwork#2415 Support dockerd and system restarts for ipvlan and macvlan networks - carry of moby/libnetwork#2295 phantom ip/mac vlan network after a powercycle - fixes moby/libnetwork#1743 Phantom docker network Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Jul 31, 2019
full diff: moby/libnetwork@83d30db...09cdcc8 changes included: - moby/libnetwork#2416 Fix hardcoded AF_INET for IPv6 address handling - moby/libnetwork#2411 Macvlan network handles netlabel.Internal wrong - fixes moby/libnetwork#2410 Macvlan network handles netlabel.Internal wrong - moby/libnetwork#2414 Allow network with --config-from to be --internal - fixes moby/libnetwork#2413 Network with --config-from does not honor --internal - moby/libnetwork#2351 Use fewer modprobes - relates to moby/moby#38930 Use fewer modprobes - moby/libnetwork#2415 Support dockerd and system restarts for ipvlan and macvlan networks - carry of moby/libnetwork#2295 phantom ip/mac vlan network after a powercycle - fixes moby/libnetwork#1743 Phantom docker network Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 6f234db9fef23c591d8376f96db062e7107b658f Component: engine
thaJeztah
added a commit
to thaJeztah/docker
that referenced
this pull request
Sep 16, 2019
full diff: moby/libnetwork@83d30db...09cdcc8 changes included: - moby/libnetwork#2416 Fix hardcoded AF_INET for IPv6 address handling - moby/libnetwork#2411 Macvlan network handles netlabel.Internal wrong - fixes moby/libnetwork#2410 Macvlan network handles netlabel.Internal wrong - moby/libnetwork#2414 Allow network with --config-from to be --internal - fixes moby/libnetwork#2413 Network with --config-from does not honor --internal - moby/libnetwork#2351 Use fewer modprobes - relates to moby#38930 Use fewer modprobes - moby/libnetwork#2415 Support dockerd and system restarts for ipvlan and macvlan networks - carry of moby/libnetwork#2295 phantom ip/mac vlan network after a powercycle - fixes moby/libnetwork#1743 Phantom docker network Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 6f234db) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Sep 17, 2019
full diff: moby/libnetwork@83d30db...09cdcc8 changes included: - moby/libnetwork#2416 Fix hardcoded AF_INET for IPv6 address handling - moby/libnetwork#2411 Macvlan network handles netlabel.Internal wrong - fixes moby/libnetwork#2410 Macvlan network handles netlabel.Internal wrong - moby/libnetwork#2414 Allow network with --config-from to be --internal - fixes moby/libnetwork#2413 Network with --config-from does not honor --internal - moby/libnetwork#2351 Use fewer modprobes - relates to moby/moby#38930 Use fewer modprobes - moby/libnetwork#2415 Support dockerd and system restarts for ipvlan and macvlan networks - carry of moby/libnetwork#2295 phantom ip/mac vlan network after a powercycle - fixes moby/libnetwork#1743 Phantom docker network Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 6f234db9fef23c591d8376f96db062e7107b658f) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: b6190c2713623ab455d29da4771b684e4eafc63f Component: engine
burnMyDread
pushed a commit
to burnMyDread/moby
that referenced
this pull request
Oct 21, 2019
full diff: moby/libnetwork@83d30db...09cdcc8 changes included: - moby/libnetwork#2416 Fix hardcoded AF_INET for IPv6 address handling - moby/libnetwork#2411 Macvlan network handles netlabel.Internal wrong - fixes moby/libnetwork#2410 Macvlan network handles netlabel.Internal wrong - moby/libnetwork#2414 Allow network with --config-from to be --internal - fixes moby/libnetwork#2413 Network with --config-from does not honor --internal - moby/libnetwork#2351 Use fewer modprobes - relates to moby#38930 Use fewer modprobes - moby/libnetwork#2415 Support dockerd and system restarts for ipvlan and macvlan networks - carry of moby/libnetwork#2295 phantom ip/mac vlan network after a powercycle - fixes moby/libnetwork#1743 Phantom docker network Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: zach <Zachary.Joyner@linux.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It's not customary for user space programs to modprobe stuff in general. Modern distributions will autoload modules (if it is indeed a module and not built-in) on access anyway, so it's also not necessary to manually modprobe stuff.
Therefore, this replaces modprobe by actual accesses of the devices. See also moby/moby#38930