Skip to content

Separate IPAM config/operational data in nw inspect #26880

Closed
aboch wants to merge 2 commits intomoby:masterfrom
aboch:ip
Closed

Separate IPAM config/operational data in nw inspect #26880
aboch wants to merge 2 commits intomoby:masterfrom
aboch:ip

Conversation

@aboch
Copy link
Copy Markdown
Contributor

@aboch aboch commented Sep 24, 2016 •
edited
Loading

Currently IPAM.Config entry in docker network inspect output is being filled with the network ipam configuration data if present, otherwise with the ipam operationional data.

It was decided to finally separate config from operational state.
This means now the IPAM field in the network object will contain Config as well as Data field.

An excerpt from a docker networ inspect would look like this:

 "IPAM": {
            "Driver": "default",
            "Options": {},
            "Config": [
                {
                    "Subnet": "5.5.0.0/16",
                    "IPRange": "5.5.2.0/24",
                    "Gateway": "5.5.254.254",
                    "AuxiliaryAddresses": {
                        "my_router": "5.5.2.2",
                        "my_switch": "5.5.2.1"
                    }
                },
                {
                    "Subnet": "2001:db8:abcd:1234::/64",
                    "IPRange": "2001:db8:abcd:1234:5678::/80"
                }
            ],
            "Data": [
                {
                    "Subnet": "5.5.0.0/16",
                    "Gateway": "5.5.254.254"
                },
                {
                    "Subnet": "2001:db8:abcd:1234::/64",
                    "Gateway": "2001:db8:abcd:1234:5678::"
                }
            ]
        },

Fixes #26799
Fixes #26933

Signed-off-by: Alessandro Boch aboch@docker.com

@aboch aboch mentioned this pull request Sep 26, 2016
Open
@mrjana
Copy link
Copy Markdown
Contributor

mrjana commented Sep 27, 2016

@aboch How do I know which one of those IPAMConfigs is actually config and which one is operation data? Isn't it better to separate Config and Operational data into two different slices and show them? Right now it will show up in as IPAM.Config which is not true.

@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Sep 27, 2016

@mrjana I initially thought of that, but from the issues about the network inspect it seems people are looking for the operational data in the current output, and this PR fixes that.
In fact currently for all the network created with no ipam config, we report the pure ipam data.

Given we fail the network creation if we cannot satisfy the config data, I am not sure whether there is a value in showing both.
Consider the output may get pretty verbose if we list both config and operational, for then finding that both fields would have pretty much the same content most of the time.

I though of changing Config to Data to properly identify what we are returning, but that is an API change which I would refrain to do now. And would retain the status quo where user expects to find the ipam operational data in that field.

@mrjana
Copy link
Copy Markdown
Contributor

mrjana commented Sep 28, 2016

@aboch Since we are talking about API representation it is fine if it is verbose as long as it provides unambiguous information. For example if somebody is writing migration code where they are reading the network state from one daemon to another, if they use the api in the current form then they can't recreate the network correctly because there is no way of knowing whether this is is user provided config or system generated operational data. I think we should follow the swarmkit api model of separating Spec from Object. User provided configuration is Spec which should never be modified by the system and the system generated state is in Object.

Since this is not going to be released in a patch release I think we can make the necessary API changes and support both versions based on API versioning design.

@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Sep 28, 2016

@mrjana
Yes, the migration is already broken, given today there is no way to know if what we read was a user config.

Ok then, let's reuse this PR to also take care of the config/operational separation.

@mrjana
Copy link
Copy Markdown
Contributor

mrjana commented Sep 28, 2016

@aboch Thanks. Yes I was suggesting to make use of this PR to fix it properly since we are changing this part of the API behavior.

@aboch aboch changed the title buildIpamResources to only consider operational data Separate IPAM config/operational data in nw inspect Sep 28, 2016
@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Sep 28, 2016

@mrjana Took care of your comment, added the new IPAM.Data field and now IPAM.Config really represents the user configuration only.

PTAL when you get a chance.

@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Sep 28, 2016

experimental to pass requires docker-py changes

@aboch aboch mentioned this pull request Sep 29, 2016
Merged
@aboch aboch mentioned this pull request Nov 1, 2016
Merged
@sanimej sanimej mentioned this pull request Nov 3, 2016
Merged
@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Nov 3, 2016

Finally all green. @mrjana PTAL.

@aboch aboch mentioned this pull request Nov 4, 2016
Merged
@mrjana
Copy link
Copy Markdown
Contributor

mrjana commented Nov 16, 2016

LGTM

@aboch aboch mentioned this pull request Nov 18, 2016
Open
@thaJeztah thaJeztah mentioned this pull request Nov 25, 2016
Closed
@mavenugo mavenugo self-assigned this Nov 28, 2016
cpuguy83
cpuguy83 requested changes Dec 22, 2016
View reviewed changes
api/types/network/network.go Outdated
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 Dec 22, 2016

Choose a reason for hiding this comment

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

Data doesn't seem like a great name... but I'm blanking on a good one atm.

Copy link
Copy Markdown
Contributor Author

@aboch aboch Dec 22, 2016

Choose a reason for hiding this comment

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

Operational, OperationalData, OperationalState ?
I am not good at names either. We need to somehow convey this info is the ipam operational state.
Hopefully somebody can propose a good one.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 22, 2016

@aboch would it be an idea to use the same concept as the service API; i.e. have a Spec field at the top-level, and anything inside that is the "provided" configuration

@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Dec 22, 2016

I think this change needs to be done in the swagger spec now?

@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Dec 22, 2016

@thaJeztah
This change is backward compatible for the tools which already check the Config field.
I am not sure if we want to change the network inspect o/p structure now.

@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Dec 22, 2016

@cpuguy83 Thanls. I will take care of the swagger specs

@thaJeztah thaJeztah mentioned this pull request Jan 3, 2017
Open
aboch added 2 commits January 24, 2017 16:03
@aboch
Separate IPAM config/operational state in nw inspect
1b61e1d 
Signed-off-by: Alessandro Boch <aboch@docker.com>
@aboch
Bump docker-py to version 2.0.2
fdb6518 
Signed-off-by: Alessandro Boch <aboch@docker.com>
@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Jan 31, 2017

ping @aboch
Is this still a thing or should we close it?
Thanks!

@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Jan 31, 2017

It's still a thing, but do not have time to invastgate/fix the docker-py test failures now.

@icecrime
Copy link
Copy Markdown
Contributor

icecrime commented Feb 28, 2017

Let's reopen when the PR is complete then. Thanks.

@icecrime icecrime closed this Feb 28, 2017
@aboch aboch deleted the ip branch November 8, 2017 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 requested changes

Assignees

@mavenugo mavenugo

Labels

status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@aboch @mrjana @thaJeztah @cpuguy83 @LK4D4 @icecrime @mavenugo @GordonTheTurtle