sous-chefs / docker

Development repository for the docker cookbook
https://supermarket.chef.io/cookbooks/docker
Apache License 2.0
1.36k stars 792 forks source link

docker_container does not redeploy when ENV property elements are removed #747

Open rmoriz opened 8 years ago

rmoriz commented 8 years ago

Cookbook version

2.9.6

Chef-client version

12.9.41

Platform Details

Debian 8.3

Scenario:

  1. Initial deployment of a container:

    docker_container 'pg-stable' do
     repo 'postgres'
     tag '9.5.3'
     volumes ['pg-stable-data:/var/lib/postgresql/data']
    
     env %w(
       POSTGRES_USER=username
       WHATEVER=false
       …
     )
     port ['127.0.0.1:15432:5432']
    
     restart_policy 'unless-stopped'
     action :run
    end
  2. Now, let's assume that we have to make changes to the environment and the port-mapping:

    docker_container 'pg-stable' do
     repo 'postgres'
     tag '9.5.3'
     volumes ['pg-stable-data:/var/lib/postgresql/data']
    
     env %w(
       POSTGRES_USER=username
        # change: WHATEVER removed
       …
     )
     port ['127.0.0.1:15432:5432']
    
     restart_policy 'unless-stopped'
     action :run
    end

Unfortunately nothing happens on reconverge. The resource is not able to detect changes (removed elements from env array) and to properly redeploy the container on change.

rmoriz commented 8 years ago

turns out this is also the case with the ":run" action because the comparison (https://github.com/chef-cookbooks/docker/blob/7c3e166a9ff6a208e9462eb178766865998bfc38/libraries/docker_base.rb#L15) only checks for additional elements, ignores removed ENV k/v.

chasebolt commented 7 years ago

this is because of issues we have with a container's Dockerfile adding to the inspect properties.

@someara an idea for this is to grab the image sha from the container, then docker inspect the image sha which will return us the Env, Volumes, Cmd, etc. We can then merge (and overwrite) properties as required to build the correct finalized properties of the container. This will allow us to remove the desired_state: false we have set on properties.

brooksa321 commented 7 years ago

Are there any workarounds for this? It seems this bug removes ALL of the benefits of using Chef to manage your docker containers. For me this is not just an issue with environment variables. It wont redeploy for any changed options.

chasebolt commented 7 years ago

Workaround would be to redeploy the server for now :)

We definitely welcome a PR to get this fixed up. I personally haven't had time to implement my suggested fix, but anyone is welcome to run with implementing it.

On Thu, Dec 29, 2016 at 12:28 PM, brooksa321 notifications@github.com wrote:

Are there any workarounds for this? It seems this bug removes ALL of the benefits of using Chef to manage your docker containers.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chef-cookbooks/docker/issues/747#issuecomment-269688654, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKpSAKiYFJesW3dTfuxmfa5wOkJqOvyks5rNBf_gaJpZM4J42De .

erincerys commented 6 years ago

@rmoriz I tried changing that hunk you referenced to

class UnorderedArray < Array
  def ==(other)
    # If two envs have differences, let == return false
    other.is_a?(Array) && ((self - other) | (other - self)).empty? 
  end
end

but the result was containers being replaced despite no env differences within the context of the attribute passed to docker_container e.g.

  * docker_container[grafana] action run
    - stopping grafana 
    - deleting grafana
    - update grafana
    -   set env to ["IAM_ROLE=utils-container-grafana-role"] (was ["IAM_ROLE=utils-container-grafana-role", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"])
    - starting grafana

Docker sets additional environment variables based on the container's Dockerfile. I assume that is why you set a custom == comparison operation to match on the new env being a subset of the current env.

I was hoping this would have been a simple fix, as this affects me as well. Hopefully this is helpful for others who find interest in contributing.

chasebolt commented 6 years ago

Correct. As you stated Docker can set additional ENV args so comparison becomes difficult. While we can tell if the args you are specifying exist or not, we can’t tell what args should also be present that come from the Dockerfile. If the container is missing one of your args, we can safely say to redeploy it, but that isn’t true if you remove an arg.

It’s a tricky problem and it exists in other container properties as well.

On Thu, Feb 1, 2018 at 19:29 ischoonover notifications@github.com wrote:

@rmoriz https://github.com/rmoriz I tried changing that hunk you referenced to

class UnorderedArray < Array def ==(other)

If two envs have differences, let == return false

other.is_a?(Array) && ((self - other) | (other - self)).empty?

end end

but the result was containers being replaced despite no env differences within the context of the attribute passed to docker_container e.g.

  • docker_container[grafana] action run
    • stopping grafana
    • deleting grafana
    • update grafana
    • set env to ["IAM_ROLE=utils-container-grafana-role"] (was ["IAM_ROLE=utils-container-grafana-role", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"])
    • starting grafana

Docker sets additional environment variables based on the container's Dockerfile. I assume that is why you set a custom == comparison operation to match on the new env being a subset of the current env.

I was hoping this would have been a simple fix, as this affects me as well. Hopefully this is helpful for others who find interest in contributing.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chef-cookbooks/docker/issues/747#issuecomment-362475606, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKpSI9CiHDHZ0H_5fh9i6wSJfIaqPpxks5tQoEfgaJpZM4J42De .

tokozedg commented 4 years ago

Workaround can be to write ENV in a file and use it for container. You can than notify file changes to container redeploy.

But this is quite important bug I agree.

ramereth commented 4 years ago

If someone wants to make a PR which addresses this, we can certainly review and merge it!