Skip to content

Add DefaultAddressPools to docker info#40714

Merged
thaJeztah merged 1 commit into
moby:masterfrom
wangyumu:add-default-address-pools-to-docker-info
Jul 20, 2020
Merged

Add DefaultAddressPools to docker info#40714
thaJeztah merged 1 commit into
moby:masterfrom
wangyumu:add-default-address-pools-to-docker-info

Conversation

@wangyumu

@wangyumu wangyumu commented Mar 18, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

- What I did
This PR will enhance docker info following #40388 :

Things to enhance:
show the current value as part of the output of docker info (so that the user can see the configured value)

Originally posted by @thaJeztah in #40388 (comment)

- How I did it
add DefaultAddressPools field to API "/info"

- How to verify it
Steps to verify:

# cat /etc/docker/daemon.json 
{
  "default-address-pools": [ { "base": "10.123.0.0/16", "size": 24 }, { "base": "10.12.0.0/16", "size": 24 } ]
}
# curl --unix-socket /var/run/docker.sock http:/v1.41/info 2>/dev/null  |  jq ."DefaultAddressPools" 
[
  {
    "size": 24,
    "base": "10.123.0.0/16"
  },
  {
    "size": 24,
    "base": "10.12.0.0/16"
  }
]

- Description for the changelog
Add "Default Address Pools" to docker info output

@wangyumu wangyumu changed the title Add DefaultAddressPool to docker info Add DefaultAddressPools to docker info Mar 18, 2020
@wangyumu wangyumu mentioned this pull request Mar 18, 2020
Merged
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 21, 2020
@wangyumu

wangyumu commented Jun 5, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@thaJeztah how can I help to get this reviewed ?

AkihiroSuda
AkihiroSuda reviewed Jul 16, 2020
View reviewed changes
Comment thread api/types/types.go Outdated

@AkihiroSuda AkihiroSuda Jul 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you mean by temp

@wangyumu wangyumu Jul 17, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I mean this struct only used by Info struct , I see some other comments nearby like PluginsInfo is a temp struct holding Plugins name or ExecStartCheck is a temp struct used by execStart using the same word.

@AkihiroSuda

AkihiroSuda commented Jul 16, 2020

Copy link
Copy Markdown
Member

Please squash commits

cpuguy83
cpuguy83 approved these changes Jul 16, 2020
View reviewed changes

@cpuguy83 cpuguy83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@cpuguy83

cpuguy83 commented Jul 16, 2020

Copy link
Copy Markdown
Member

After squash

thaJeztah
thaJeztah reviewed Jul 16, 2020
View reviewed changes
Comment thread api/types/types.go Outdated
Comment on lines 223 to 227

@thaJeztah thaJeztah Jul 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I actually just realised that for the SwarmKit default-addresspools, we use a regular []string;

Default Address Pool: 10.0.0.0/8

"DefaultAddrPool": [
"10.0.0.0/8"
],

Should we do the same here?

@wangyumu wangyumu Jul 17, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think DefaultAddrPool of SwarmKit is different with DefaultAddressPools here. DefaultAddressPools contains CIDR and pool Size but DefaultAddrPool only contains CIDR, so we can not use regular []string .

"default-address-pools":  [
 { "base": "10.123.0.0/16", "size": 24 },
 { "base": "10.12.0.0/16", "size": 24 } 
]

@thaJeztah thaJeztah Jul 20, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, yes, I recall now that we had some back-and-forth discussion about them being different 👍

@wangyumu
wangyumu force-pushed the add-default-address-pools-to-docker-info branch from 78ee620 to 067b010 Compare July 17, 2020 07:40
@wangyumu

wangyumu commented Jul 17, 2020

Copy link
Copy Markdown
Contributor Author

@AkihiroSuda @cpuguy83 squashed, thanks

@cpuguy83

cpuguy83 commented Jul 17, 2020

Copy link
Copy Markdown
Member

Thanks, one more requeset: Can you make a descriptive commit message?

@wangyumu
wangyumu force-pushed the add-default-address-pools-to-docker-info branch 3 times, most recently from 49fad7b to 193f5aa Compare July 18, 2020 01:04
@wangyumu

wangyumu commented Jul 18, 2020

Copy link
Copy Markdown
Contributor Author

@cpuguy83 done, PTAL, thanks

@wangyumu
wangyumu force-pushed the add-default-address-pools-to-docker-info branch 4 times, most recently from b1a4986 to 840a12a Compare July 18, 2020 15:06
@wangyumu

wangyumu commented Jul 18, 2020

Copy link
Copy Markdown
Contributor Author

soz I missed updating swagger.yaml, now added.

@AkihiroSuda @cpuguy83 @thaJeztah PTAL, thanks

@wangyumu

wangyumu commented Jul 20, 2020

Copy link
Copy Markdown
Contributor Author

Looks like CI failed with flacky test, I tried many times locally but couldn't reprocedure it:

[2020-07-18T15:27:36.893Z] #20 13.94 Downloading 'library/busybox:glibc@sha256:1f81263701cddf6402afe9f33fca0266d9fff379e59b1748f33d3072da71ee85' (1 layers)...

[2020-07-18T15:27:37.463Z] #20 14.44 

[2020-07-18T15:27:38.034Z] #20 ERROR: executor failed running [/bin/sh -c /download-frozen-image-v2.sh /build 	buildpack-deps:buster@sha256:d0abb4b1e5c664828b93e8b6ac84d10bce45ee469999bef88304be04a2709491 	busybox:latest@sha256:95cf004f559831017cdf4628aaf1bb30133677be8702a8c5f2994629f637a209 	busybox:glibc@sha256:1f81263701cddf6402afe9f33fca0266d9fff379e59b1748f33d3072da71ee85 	debian:buster@sha256:46d659005ca1151087efa997f1039ae45a7bf7a2cbbe2d17d3dcbda632a3ee9a 	hello-world:latest@sha256:d58e752213a51785838f9eed2b7a498ffa1cb3aa7f946dda11af39286c3db9a9]: runc did not terminate sucessfully

thaJeztah
thaJeztah approved these changes Jul 20, 2020
View reviewed changes

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah

thaJeztah commented Jul 20, 2020

Copy link
Copy Markdown
Member

Ignoring the CI failure as it's indeed not related

@thaJeztah
thaJeztah merged commit f712444 into moby:master Jul 20, 2020
This was referenced Jul 20, 2020
Merged
Merged
@wangyumu
wangyumu deleted the add-default-address-pools-to-docker-info branch July 21, 2020 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments
@cpuguy83 cpuguy83 cpuguy83 approved these changes
@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

area/api API impact/api impact/changelog status/2-code-review

Projects

None yet

Milestone

20.10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@wangyumu @AkihiroSuda @cpuguy83 @thaJeztah