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.

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

daemon.NewDaemon(): fix network feature detection on first start #43689

Merged
merged 1 commit into from Jun 3, 2022

Conversation

thaJeztah
Copy link
Member

Commit 483aa62 (#43130) introduced a regression, causing spurious warnings to be shown when starting a daemon for the first time after a fresh install:

docker info
...
WARNING: IPv4 forwarding is disabled
WARNING: bridge-nf-call-iptables is disabled
WARNING: bridge-nf-call-ip6tables is disabled

The information shown is incorrect, as checking the corresponding options on the system, shows that these options are available:

cat /proc/sys/net/ipv4/ip_forward
1
cat /proc/sys/net/bridge/bridge-nf-call-iptables
1
cat /proc/sys/net/bridge/bridge-nf-call-ip6tables
1

The reason this is failing is because the daemon itself reconfigures those options during networking initialization in configureIPForwarding();

const (
ipv4ForwardConf = "/proc/sys/net/ipv4/ip_forward"
ipv4ForwardConfPerm = 0644
)
func configureIPForwarding(enable bool) error {
var val byte
if enable {
val = '1'
}
return os.WriteFile(ipv4ForwardConf, []byte{val, '\n'}, ipv4ForwardConfPerm)
}

Network initialization happens in the daemon.restore() function within daemon.NewDaemon():

moby/daemon/daemon.go

Lines 475 to 478 in cf45952

// Note that we cannot initialize the network controller earlier, as it
// needs to know if there's active sandboxes (running containers).
if err = daemon.initNetworkController(activeSandboxes); err != nil {
return fmt.Errorf("Error initializing network controller: %v", err)

However, 483aa62 moved detection of features earlier in the daemon.NewDaemon() function, and collects the system information (d.RawSysInfo()) before we enter daemon.restore();

moby/daemon/daemon.go

Lines 1008 to 1011 in cf45952

sysInfo := d.RawSysInfo()
for _, w := range sysInfo.Warnings {
logrus.Warn(w)
}

For optimization (collecting the system information comes at a cost), those results are cached on the daemon, and will only be performed once (using a sync.Once).

This patch:

  • introduces a getSysInfo() utility, which collects system information without caching the results
  • uses getSysInfo() to collect the preliminary information needed at that point in the daemon's lifecycle.
  • moves printing warnings to the end of daemon.NewDaemon(), after all information can be read correctly.

- A picture of a cute animal (not mandatory but encouraged)

2825418679_024de324b4_b

@thaJeztah
Copy link
Member Author

Ah, dang, or course it helps if you stage all files

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   daemon/daemon_windows.go

Commit 483aa62 introduced a regression, causing
spurious warnings to be shown when starting a daemon for the first time after
a fresh install:

    docker info
    ...
    WARNING: IPv4 forwarding is disabled
    WARNING: bridge-nf-call-iptables is disabled
    WARNING: bridge-nf-call-ip6tables is disabled

The information shown is incorrect, as checking the corresponding options on
the system, shows that these options are available:

    cat /proc/sys/net/ipv4/ip_forward
    1
    cat /proc/sys/net/bridge/bridge-nf-call-iptables
    1
    cat /proc/sys/net/bridge/bridge-nf-call-ip6tables
    1

The reason this is failing is because the daemon itself reconfigures those
options during networking initialization in `configureIPForwarding()`;
https://github.com/moby/moby/blob/cf4595265e7703e1e9745a30f1dd265acbc075d3/libnetwork/drivers/bridge/setup_ip_forwarding.go#L14-L25

Network initialization happens in the `daemon.restore()` function within `daemon.NewDaemon()`:
https://github.com/moby/moby/blob/cf4595265e7703e1e9745a30f1dd265acbc075d3/daemon/daemon.go#L475-L478

However, 483aa62 moved detection of features
earlier in the `daemon.NewDaemon()` function, and collects the system information
(`d.RawSysInfo()`) before we enter `daemon.restore()`;
https://github.com/moby/moby/blob/cf4595265e7703e1e9745a30f1dd265acbc075d3/daemon/daemon.go#L1008-L1011

For optimization (collecting the system information comes at a cost), those
results are cached on the daemon, and will only be performed once (using a
`sync.Once`).

This patch:

- introduces a `getSysInfo()` utility, which collects system information without
  caching the results
- uses `getSysInfo()` to collect the preliminary information needed at that
  point in the daemon's lifecycle.
- moves printing warnings to the end of `daemon.NewDaemon()`, after all information
  can be read correctly.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

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

:shipit:

@thaJeztah
Copy link
Member Author

Thanks! Bringing this one in 👍

1 similar comment
@thaJeztah
Copy link
Member Author

Thanks! Bringing this one in 👍

@thaJeztah thaJeztah merged commit 38633e7 into moby:master Jun 3, 2022
@thaJeztah thaJeztah deleted the fix_incorrect_warnings branch June 3, 2022 17:30
@skepticoitusInteruptus
Copy link

Good job, @thaJeztah 🥇

"…causing spurious warnings to be shown…"

Given that #43674 uses similar wording in its description, I'm hoping this fixes that issue's spurious "WARNING: Your kernel does not support swap limit capabilities or the cgroup is not mounted. Memory limited without swap." too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants