(BKR-1189) Fix port mapping #3
Conversation
|
Can you make a new release asap please ? I really need these latest PRs ! |
|
I'll try to get this merged and cut off a new version today. Sorry for the delay |
| @@ -102,7 +113,7 @@ def provision | |||
| fix_ssh(container) if @options[:provision] == false | |||
|
|
|||
| @logger.debug("Starting container #{container.id}") | |||
| container.start({"PublishAllPorts" => true, "Privileged" => true}) | |||
There was a problem hiding this comment.
What is the a reason you moved these parameters to container_opts? Is there a change in behavior by doing so, or is it just a refactor?
There was a problem hiding this comment.
If any options are passed with container.start then those are the only options that are used. So originally any options provided by container_opts were not being used at all and therefore consolidating all the options in container_opts
There was a problem hiding this comment.
container_opts is the parameter used in ::Docker::Container.create(container_opts) on line 104 of this PR, so it is definitely still being used. I'm just wondering if the options parameter provided there are read in the same way as they are when provide to start.
There was a problem hiding this comment.
I'm still a bit unclear why removing the parameters has this effect; the start method only makes an api call with parameters if they are provided, and doesn't seem to do any sort of reading of currently set parameters from the logic seen here.
There was a problem hiding this comment.
looking at their API they dont have any options that are provided with start (i understand that the code still support taking options on start). but looks like creating container is where the options are passed
i also tried a combination of keeping the options provided with start as is and passing other options with create but all the options passed with create is being overwritten 😞
Originally on restarting a container, docker would change the port mapping for port 22. And beaker would still be using the old port to ssh and would not be able to ssh. Mapping a port when creating a container would keep that as a static port and woudn't be changed on restart. Also added a restart policy. Docker containers apparently don't restart, you would have to run docker start again to restart them. Having this policy would allow them to restart when its rebooted from within. Originally we were passing Privileged and RestartPolicy with container.start. I moved those options such that they are passed with options to create the container. Apparantely, passing options with container.start overrides the options given at ::Docker::Container.create.
rishijavia
force-pushed
the
fix-port-mapping
branch
from
047407f
to
5d815fb
Compare
|
I'm fine merging this, even though I still have some unanswered questions about what is happening inside the docker-api gem. Thanks to @rishijavia's work, we can just do a new release if there are any issues. |
| @@ -70,6 +70,17 @@ def provision | |||
| container_opts = { | |||
| 'Image' => image_name, | |||
| 'Hostname' => host.name, | |||
| 'name' => host.name, | |||
There was a problem hiding this comment.
I think this introduced a regression. The SUT containers are now named the same as their hostname, whereas before they had randomly generated names. When running two instances of the same nodeset simultaneously, or two separate nodesets with overlapping hostnames, beaker fails with the following error message:
An error occurred while loading ./spec/acceptance/class_spec.rb.
Failure/Error: require 'beaker-rspec'
Docker::Error::ConflictError:
Conflict. The container name "/centos-6-x64" is already in use by container "5d843a8281173c6d3048432301fffc625301816eeba9c945822525a8a195001f". You have to remove (or rename) that container to be able to reuse that name.
The port mapping is for port 22 that is used to ssh. This maps a static random 4 digit port when the container is created so a random port is not assigned.
When a random port is assigned it would change it when the container is restarted and beaker would still be using old port and would not be able to ssh.