Skip to content

Conversation

@plooploops
Copy link
Member

What this PR does / why we need it:
Hot attach should happen in the join, not in create endpoint.
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
#235
Special notes for your reviewer:
cnm testing

Release note:

@plooploops
Copy link
Member Author

@sharmasushant Let me know if this works. I had checked with @madhanrm and we believe this should be the flow.

@sharmasushant
Copy link
Contributor

@plooploops Will check... thanks for the PR.

@madhanrm
Copy link
Contributor

@plooploops Make sure you don't break CNI. CNI needs hotadd as part of endpoint creation (Single call). Its just CNM that needs creation separately & hot add separately.

@plooploops
Copy link
Member Author

Let me see if I preserve that then, thanks for the tip @madhanrm

@plooploops
Copy link
Member Author

plooploops commented Dec 7, 2018

@madhanrm @sharmasushant

I had a chance to verify the CNI side, as far as we know, with @jsturtevant @cnadolny @techbunny. Test with acs-engine + windows containers using Microsoft/iis:latest, and able to ping from head node + additional VM in VNET, and we get 200 by the private IP for the container.

Also verified on CNM side with @techbunny. Looks like we can get the static IP as it's assigned to NIC for VM. We're also able to reach the container separately from another VM in VNET using the private ip (10.0.0.33 for example).

@madhanrm
Copy link
Contributor

madhanrm commented Jan 2, 2019

/lgtm

Copy link
Member

@ashvindeodhar ashvindeodhar left a comment

Choose a reason for hiding this comment

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

In this PR, it’s assumed that if the containerID is empty it must be coming from CNM and as a result we should skip hotAttach and rely on getting join later on. We may miss on real issues where the containerID is empty in case of CNI ( should never happen but with this PR we won’t even log the real error )
Instead I think we could add a flag in epInfo indicating if the hotAttach is required. CNM code making the call to createEndpoint will set this flag to false. CNI would set this to true. There would be no ambiguity as to who is making the call when it comes to newEndpointImpl

@ashvindeodhar
Copy link
Member

Closing this pull request as #332 is merged after discussing with Andy Gee.

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.

4 participants