V2 API support for win-overlay CNI #725
Conversation
selansen
changed the title
selansen
force-pushed
the
v2api-suuport-win-overlay
branch
from
b733a9f to
5c5c440
Compare
|
Fixes #713 |
selansen
force-pushed
the
v2api-suuport-win-overlay
branch
3 times, most recently
from
71b88c5 to
f30799f
Compare
selansen
changed the title
|
@dcbw @JacobTanenbaum This is up for review. PTAL |
|
Want feedback from Microsoft networking team: @daschott @Keith-Mange PTAL |
There was a problem hiding this comment.
Nice work. I can see that AddHcnEndpoint is calling AddNamespaceEndpoint and the CNI ADD call looks good to me.
For the deletion, I believe it is missing an attempt to detach from namespace. Deleting the endpoint without detaching from namespace is fine as a fallback if the namespace is not found, but ideally we should try to detach from namespace first and attempt to clean up properly.
| } | ||
|
|
||
| networkName := n.Name | ||
| hnsNetwork, err := hcsshim.GetHNSNetworkByName(networkName) |
There was a problem hiding this comment.
nit Feels like we should be able to avoid making a query to get network representation through HNS APIs since the result from HCN APIs should be sufficient... Or is there a reason we are getting the same network using both HCN and HNS?
There was a problem hiding this comment.
To be clear, this is a nit-pick and not blocking.
There was a problem hiding this comment.
@daschott I was running into an issue where I was not able to get the exact gateway IP from the HCN network object. The way win-bridge CNI gets the gateway IP address is returning me a "different IP" when I tried to use it in win-overlay. That is the only reason I am still using the HNS network object. I already discussed it with @mansikulkarni96 and will take it as a separate improvement PR. I need to dig and find the right way to get the default IP.
If you are aware of getting the correct gateway from the HCN network object, pls do let me know.
| epInfo.NetworkName = n.Name | ||
| epInfo.EndpointName = hns.ConstructEndpointName(args.ContainerID, args.Netns, epInfo.NetworkName) | ||
|
|
||
| if n.IPAM.Type != "" { |
There was a problem hiding this comment.
tip: FYI If desirable, we could expand this to fallback to HNS to provision an IP address and MAC address for you, instead of requiring on an IPAM plugin. This is just an FYI, it is also a perfectly valid decision to require IPAM.
There was a problem hiding this comment.
To be clear, this is not blocking.
mansikulkarni96
force-pushed
the
v2api-suuport-win-overlay
branch
3 times, most recently
from
3d95ada to
75a5394
Compare
|
Thanks for updating the delete call. There are some minor improvements that could be done w.r.t. querying HNS, but that isn't blocking and can come in a future PR. LGTM |
This PR bring V2 API support into win-overlay CNI. With the current V1 API, only docker runtime works for win-overlay. By bringing new changes, we should be able to use containerd as the runtime.Below are the key points regarding this implementation. 1. Clear seperation for V1 & V2 API support 2. New cni.conf sample that works for win-overlay Signed-off-by: selansen <esiva@redhat.com> Signed-off-by: mansikulkarni96 <mankulka@redhat.com>
mansikulkarni96
force-pushed
the
v2api-suuport-win-overlay
branch
from
75a5394 to
8b8825b
Compare
|
Folks, could you cut a new release so this change becomes available? Thanks! |
|
Included in release v1.2.0 |
This PR bring V2 API support into win-overlay CNI. With the current V1
API, only docker runtime works for win-overlay. By bringing new changes, we
should be able to use containerd as the runtime.Below are the key
points regarding this implementation.
1. Clear seperation for V1 & V2 API support in cmdAdd
2. New cni.conf sample that works for win-overlay
3. cmdDel handles HCN API changes for V2 API version
Signed-off-by: selansen esiva@redhat.com