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

Conventions: add convention around chaining interfaces #427

Merged
merged 1 commit into from
Jul 5, 2017

Conversation

squeed
Copy link
Member

@squeed squeed commented Apr 11, 2017

This is a change to CONVENTIONS that states, essentially, that plugins SHOULD be chained if at all possible. The intention is to support the widest-possible set of users.

@mcastelino
Copy link
Contributor

@squeed does this mean in the case of #420 I should retain the veth functionality in the bridge plugin. Hence we do not need a special veth plugin, but just need a tap plugin?

Also when you have bridge+tap chained, who is responsible for invoking the IPAM plugin? Also in that case should the plugin chain be inverted. i.e. call tap followed by bridge?

@squeed
Copy link
Member Author

squeed commented Apr 19, 2017

@mcastelino Great questions
1 - We should probably make a veth plugin for completeness, but in the interest of both optimizing the most common case and preserving existing behavior, the bridge plugin should create a veth automatically
2 - The first plugin is always responsible for invoking ipam.

Make sense? Thanks!

@mcastelino
Copy link
Contributor

@squeed makes sense. I will update #420 to continue to support veth setup in the bridge plugin. That will also allow the current unit tests to run successfully.

However even in this case I am still not sure how the IPAM can be assumed to called by the first plugin.
Consider the following case where we want to chain tap with the bridge plugin. As you see below the bridge plugin will continue to need IPAM specification in addition to the tap plugin. This will be needed as the veth component within the bridge will need the IPAM plugin.

If we update the spec to call out that the IPAM plugin can be repeated when chaining plugins, and how that should function should help clarify the relationship between the IPAM and invoking plugin.

{
        "cniVersion": "0.3.0",
        "name": "mynet",
        "plugins": [
        {
                "type": "tap",
                "ipam": {
                        "type": "host-local",
                        "subnet": "10.22.0.0/16",
                        "routes": [
                                { "dst": "0.0.0.0/0" }
                        ]
                }
        },
        {
                "type": "bridge",
                "bridge": "cni0",
                "isGateway": true,
                "ipMasq": true,
                "ipam": {
                        "type": "host-local",
                        "subnet": "10.22.0.0/16",
                        "routes": [
                                { "dst": "0.0.0.0/0" }
                        ]
                }
        }
        ]
}

@mcastelino
Copy link
Contributor

@squeed Longer term we should consider the IPAM plugin to be chained vs embedded into the veth/tap plugin. This is required in the case where the IPAM is of type DHCP. Here the IPAM can only be invoked once the interface is connected to the bridge. So invoking the IPAM as part of the veth plugin will not work in the case where the chain is veth->bridge.

So the chain that would work is veth -> bridge -> ipam.
Here the new result information in version 0.3.0 passed through the plugin invocation sequence really helps.

I will rework my existing veth->bridge implementation to prototype the veth->bridge->ipam sequence and check if there are any gaps.

Also in the case of a VM based runtime and tap interfaces, the DHCP request needs to be initiated from within the VM. Hence splitting out the IPAM would make sense in that case too.

@dcbw
Copy link
Member

dcbw commented Apr 29, 2017

@mcastelino agreed on the chaining behavior of veth -> bridge -> ipam; we discussed that in the maintainers meeting this week. If you can do the rework that would be awesome. Or is #420 already reworked in that way?

@dcbw
Copy link
Member

dcbw commented May 31, 2017

LGTM

1 similar comment
@bboreham
Copy link
Contributor

bboreham commented Jul 5, 2017

LGTM

Copy link
Member

@tomdee tomdee left a comment

Choose a reason for hiding this comment

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

LGTM

@rosenhouse rosenhouse merged commit a2da8f8 into containernetworking:master Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants