Skip to content

Change Sandbox.SetKey() in case of shared osl.Sandbox#528

Closed
aboch wants to merge 1 commit intomoby:masterfrom
aboch:phil
Closed

Change Sandbox.SetKey() in case of shared osl.Sandbox#528
aboch wants to merge 1 commit intomoby:masterfrom
aboch:phil

Conversation

@aboch
Copy link
Contributor

@aboch aboch commented Sep 15, 2015

  • In case of useDefaultSandbox, if SetKey finds
    an existing osl.Sandbox it is not an error
    condition and no more processing should be done

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

@aboch aboch mentioned this pull request Sep 15, 2015
Merged
@mavenugo
Copy link
Contributor

mavenugo commented Sep 16, 2015

@aboch with this change, though it is not failing to create container in host mode, there is no connectivity. infact the namespace is missing all the host interfaces. this fix needs a rework.
Also, please test the changes integrating with docker.
ping @mrjana

@estesp
Copy link
Contributor

estesp commented Sep 16, 2015

As @mavenugo said, there is a continuing problem with host networking not having the network interfaces/connectivity. Container mode is actually working with this change, but not --net=host. Actually, if it helps, in --net=host mode the /proc/self/ns/net is most definitely not the same as the host, but in --net=container:<somecontainer> it does actually match the net namespace of the other container.

@mavenugo
Copy link
Contributor

mavenugo commented Sep 16, 2015

@estesp thats correct. --net=container mode is handled differently and should work without any extra changes. But I forgot to handle the --net=host case in my original change (as it was originally intended for userns support). In any case, this PR needs some rework to handle this scenario.

@aboch
Change Sandbox.SetKey() in case of shared osl.Sandbox
00f31cb 
- In case of `useDefaultSandbox`, if SetKey finds
  an existing osl.Sandbox it is not an error
  condition and different processing should be done

Signed-off-by: Alessandro Boch <aboch@docker.com>
@aboch
Copy link
Contributor Author

aboch commented Sep 16, 2015

@estesp @mavenugo I updated the new diffs. I will test them within docker later, no time now.

mavenugo
mavenugo reviewed Sep 16, 2015
View reviewed changes
sandbox.go
Copy link
Contributor

@mavenugo mavenugo Sep 16, 2015

Choose a reason for hiding this comment

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

i believe you meant !sb.config.useDefaultSandBox

@mavenugo
Copy link
Contributor

mavenugo commented Sep 16, 2015

@aboch even after fixing the above comment, it is still not working. :(

@mavenugo
Copy link
Contributor

mavenugo commented Sep 16, 2015

@aboch @estesp PTAL moby/moby#16305 (comment). I dont think we need this patch any more. We can close it if you agree.

@aboch
Copy link
Contributor Author

aboch commented Sep 16, 2015

Thanks @mavenugo Closing this

@aboch aboch closed this Sep 16, 2015
@aboch aboch deleted the phil branch September 29, 2015 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aboch @mavenugo @estesp