Skip to content

merge default address pool flags with daemon config#40711

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
wangyumu:fix-merge-flags-address-pools
Jul 23, 2020
Merged

merge default address pool flags with daemon config#40711
cpuguy83 merged 1 commit intomoby:masterfrom
wangyumu:fix-merge-flags-address-pools

Conversation

@wangyumu
Copy link
Copy Markdown
Contributor

@wangyumu wangyumu commented Mar 18, 2020 •
edited
Loading

- What I did
Fixes #40388

this PR will fix the bug that cause dockerd ignores the --default-address-pool option

- How I did it

We use mergo.Merge to merge the options from command-line and daemon.json, but Mergo won't merge unexported (private) fields (see https://github.com/imdario/mergo#usage), that's why default-address-pool were ignored.

So I just change values to Values of PoolsOpt struct.

- How to verify it
Steps to verify:

# cat empty.json
{}
# dockerd --default-address-pool 'base=10.123.0.0/16,size=24' --config-file empty.json
# docker network create n1 > /dev/null && docker network inspect n1 --format '{{json .}}' | jq .IPAM.Config[0].Subnet
"10.123.1.0/24"

it successfully allocate from the default pool set on command-line

@wangyumu wangyumu force-pushed the fix-merge-flags-address-pools branch from 9309c5f to bd3e4f9 Compare July 21, 2020 11:24
fix address pool flags merge moby#40388
c8008bf 
Signed-off-by: Wang Yumu <37442693@qq.com>
@wangyumu
Copy link
Copy Markdown
Contributor Author

wangyumu commented Jul 21, 2020

@thaJeztah PTAL, thanks.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jul 21, 2020

Mergo won't merge unexported (private) fields

Ah, hm, good call

Do you think it would be possible to have a test for this? With #40388 merged, I expect it to be easier to call the API, and verify that the excepted settings were loaded 🤔 wdyt?

There are some existing tests in

moby/daemon/config/config_test.go

Line 41 in 3af8d48

func TestParseClusterAdvertiseSettings(t *testing.T) {
, so it could either be added to one of the existing test (haven't looked if there's a suitable test for that), or a new test to be added

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jul 21, 2020

Or otherwise in https://github.com/moby/moby/blob/3af8d484b132a492bd6f159e454560b7d7691fa9/integration/ 🤔

@wangyumu wangyumu force-pushed the fix-merge-flags-address-pools branch from bd3e4f9 to 1239ba5 Compare July 22, 2020 07:35
@wangyumu
Copy link
Copy Markdown
Contributor Author

wangyumu commented Jul 22, 2020

@thaJeztah good idea, I added a new test: TestDaemonConfigurationMergeFlagsWitchConfigFile

Test before this fix:
=== RUN   TestDaemonConfigurationMergeFlagsWitchConfigFile
    config_test.go:178: expect result not empty, but got []
--- FAIL: TestDaemonConfigurationMergeFlagsWitchConfigFile (0.00s)

Test after this fix:
=== RUN   TestDaemonConfigurationMergeFlagsWitchConfigFile
--- PASS: TestDaemonConfigurationMergeFlagsWitchConfigFile (0.00s)
PASS

thaJeztah
thaJeztah requested changes Jul 22, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! Left some suggestions inline 🤗

daemon/config/config_test.go Outdated
Comment on lines 177 to 179
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jul 22, 2020

Choose a reason for hiding this comment

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

Probably good to check that the value is what we expected. Something like;

Suggested change
if config.DefaultAddressPools.Value() == nil || len(config.DefaultAddressPools.Value()) == 0 {
t.Fatalf("expect result not empty, but got %v", config.DefaultAddressPools.Value())
}
expected := []*ipamutils.NetworkToSplit{{Base: "10.123.0.0/16", Size: 24}}
assert.DeepEqual(t, config.DefaultAddressPools.Value(), expected)

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jul 22, 2020

Choose a reason for hiding this comment

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

I had the branch checked out locally, so perhaps easier to copy/paste here what I wrote up quickly with my suggestions;

// Test for #40711
func TestDaemonConfigurationMergeDefaultAddressPools(t *testing.T) {
	emptyConfigFile := fs.NewFile(t, "config", fs.WithContent(`{}`))
	defer emptyConfigFile.Remove()
	configFile := fs.NewFile(t, "config", fs.WithContent(`{"default-address-pools":[{"base": "10.123.0.0/16", "size": 24 }]}`))
	defer configFile.Remove()

	expected := []*ipamutils.NetworkToSplit{{Base: "10.123.0.0/16", Size: 24}}

	t.Run("empty config file", func(t *testing.T) {
		var conf = Config{}
		flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
		flags.Var(&conf.NetworkConfig.DefaultAddressPools, "default-address-pool", "")
		flags.Set("default-address-pool", "base=10.123.0.0/16,size=24")

		config, err := MergeDaemonConfigurations(&conf, flags, emptyConfigFile.Path())
		assert.NilError(t, err)
		assert.DeepEqual(t, config.DefaultAddressPools.Value(), expected)
	})

	t.Run("config file", func(t *testing.T) {
		var conf = Config{}
		flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
		flags.Var(&conf.NetworkConfig.DefaultAddressPools, "default-address-pool", "")

		config, err := MergeDaemonConfigurations(&conf, flags, configFile.Path())
		assert.NilError(t, err)
		assert.DeepEqual(t, config.DefaultAddressPools.Value(), expected)
	})

	t.Run("with conflicting options", func(t *testing.T) {
		var conf = Config{}
		flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
		flags.Var(&conf.NetworkConfig.DefaultAddressPools, "default-address-pool", "")
		flags.Set("default-address-pool", "base=10.123.0.0/16,size=24")

		_, err := MergeDaemonConfigurations(&conf, flags, configFile.Path())
		assert.ErrorContains(t, err, "the following directives are specified both as a flag and in the configuration file")
		assert.ErrorContains(t, err, "default-address-pools")
	})
}

Let me know if you think that looks good (and feel free to copy to your PR)

Copy link
Copy Markdown
Contributor Author

@wangyumu wangyumu Jul 22, 2020

Choose a reason for hiding this comment

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

It looks wonderful, really appreciate your work, I will copy it. 👍

@wangyumu wangyumu force-pushed the fix-merge-flags-address-pools branch 3 times, most recently from 8102107 to c8008bf Compare July 22, 2020 12:53
thaJeztah
thaJeztah approved these changes Jul 22, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM if green; thanks so much!

@thaJeztah thaJeztah added this to the 20.03.0 milestone Jul 22, 2020
cpuguy83
cpuguy83 approved these changes Jul 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 cpuguy83 merged commit a770dc1 into moby:master Jul 23, 2020
@thaJeztah thaJeztah mentioned this pull request Nov 1, 2020
Closed
@jessesimpson36 jessesimpson36 mentioned this pull request Jan 7, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

area/daemon Core Engine impact/changelog kind/bugfix PR's that fix bugs process/cherry-picked status/2-code-review

Projects

None yet

Milestone

20.10.0

Development

Successfully merging this pull request may close these issues.

dockerd ignores the --default-address-pool option

5 participants

@wangyumu @thaJeztah @tonistiigi @cpuguy83 @tiborvass