puppetlabs / puppetlabs-docker

The Puppet Docker repository
Apache License 2.0
92 stars 309 forks source link

Buggy Docker_Volume options #507

Closed gdlx closed 3 years ago

gdlx commented 5 years ago

What you expected to happen?

Filling Docker_Volume options with a Hash, as defines in the doc: https://forge.puppet.com/puppetlabs/docker#volumes

What happened?

I get the following error:

Error: Execution of '/usr/bin/docker volume create --driver=local --opt={"type"=>"nfs4", "o"=>"addr=diskstation,rw", "device"=>":/volume1/medias"} medias' returned 1: Error response from daemon: create medias: invalid option key: "{\"type\""

The options hash is sent to the docker command as a json strin instead of multiple --opt parameters.

Please note I've already seen #197, there's still an issue.

How to reproduce it?

Define a docker volume with multiple options like this:

  docker_volume { 'medias':
    ensure  => present,
    driver  => 'local',
    options => {
      type   => 'nfs4',
      o      => 'addr=diskstation,rw',
      device => ':/volume1/medias',
    },
  }

Anything else we need to know?

Looking at #197, the only way I've been able to define the volume options, is not in an array, but in 2 arrays, like this:

  docker_volume { 'medias':
    ensure  => present,
    driver  => 'local',
    options => [[
      'type=nfs4',
      'o=addr=diskstation,rw',
      'device=:/volume1/medias',
    ]],
  }

It's working fine, even if the syntax is a bit odd, but each time I run Puppet then I get this notice:

Notice: /Stage[main]/Mediasrv::Docker/Docker_volume[medias]/options: options changed {
  'device' => ':/volume1/medias',
  'o' => 'addr=diskstation,rw',
  'type' => 'nfs4'
} to ['type=nfs4', 'o=addr=diskstation,rw', 'device=:/volume1/medias']

So there is clearly something wrong with the way volume options is handled: The syntax is not the same than in the docs, and the current state doesn't give the same result, leading to a detected difference on each Puppet run.

Versions:

$ puppet --version
`5.4.0`

$ docker version
Client:
 Version:           18.09.7
 API version:       1.39
 Go version:        go1.10.8
 Git commit:        2d0083d
 Built:             Thu Jun 27 17:56:23 2019
 OS/Arch:           linux/amd64
 Experimental:      false

Server: Docker Engine - Community
 Engine:
  Version:          18.09.7
  API version:      1.39 (minimum version 1.12)
  Go version:       go1.10.8
  Git commit:       2d0083d
  Built:            Thu Jun 27 17:23:02 2019
  OS/Arch:          linux/amd64
  Experimental:     false

$ facter os
{
  architecture => "amd64",
  distro => {
    codename => "bionic",
    description => "Ubuntu 18.04.2 LTS",
    id => "Ubuntu",
    release => {
      full => "18.04",
      major => "18.04"
    }
  },
  family => "Debian",
  hardware => "x86_64",
  name => "Ubuntu",
  release => {
    full => "18.04",
    major => "18.04"
  },
  selinux => {
    enabled => false
  }
}

$ puppet module list
/usr/share/puppet/modules (no modules installed)

But here's my Puppetfile.lock:

FORGE
  remote: https://forgeapi.puppetlabs.com
  specs:
    puppetlabs-apt (6.3.0)
      puppetlabs-stdlib (>= 4.16.0, < 6.0.0)
      puppetlabs-translate (>= 1.0.0, < 2.0.0)
    puppetlabs-docker (3.6.0)
      puppetlabs-apt (>= 4.4.1, < 7.0.0)
      puppetlabs-powershell (>= 2.1.4, < 3.0.0)
      puppetlabs-reboot (>= 2.0.0, < 3.0.0)
      puppetlabs-stdlib (>= 4.24.0, < 6.0.0)
      puppetlabs-translate (>= 0.0.1, < 2.0.0)
    puppetlabs-powershell (2.3.0)
    puppetlabs-reboot (2.1.2)
    puppetlabs-stdlib (5.2.0)
    puppetlabs-translate (1.2.0)
    saz-timezone (5.1.1)
      puppetlabs-stdlib (>= 2.6.0, < 6.0.0)
      stm-debconf (>= 2.0.0, < 3.0.0)
    stm-debconf (2.3.0)

DEPENDENCIES
  puppetlabs-apt (= 6.3.0)
  puppetlabs-docker (= 3.5.0)
  puppetlabs-stdlib (= 5.2.0)
  saz-timezone (= 5.1.1)
prometheanfire commented 5 years ago

I'm not sure it's related but running v3.6.0 I am not able to create any docker files, whether by hiera data or manifests.

The following is how I tested via hiera

classes:
  - docker::images
  - docker::volumes
docker::volumes::volumes:
  mayan-edms:
    ensure: present
    driver: local
  mayan-edms-postgres:
    ensure: present
    driver: local

And here is how I tested the classic way

node 'foo' {
  include docker::volumes
  docker_volume { 'mayan-edms2':
    ensure => present
  }
}

I'm probably doing something stupid and missing something but I don't see it docker::volumes run at all with either method. Same version of docker as the OP though

benningm commented 5 years ago

I did the PR 2 month ago to change the documentation from array to hash format. That looked to me what the module expects. But the volume provider seems to have a different logic for creating and modifying the resource. Existing configuration is retrieved by inspect in json format resulting in the configuration to be a hash while the create() is expecting array format.

As @gdlx wrote there is a notice to change the resource on every puppet run. This is because the provider detects the difference but does not support applying changes to the existing resource. Afaik this is also not supported by docker.

May be its best to flatten the hash to an array in case:

--- a/lib/puppet/provider/docker_volume/ruby.rb
+++ b/lib/puppet/provider/docker_volume/ruby.rb
@@ -11,6 +11,7 @@ Puppet::Type.type(:docker_volume).provide(:ruby) do
   def volume_conf
     flags = ['volume', 'create']
     multi_flags = ->(values, format) {
+      values = values.map! { |k,v| "#{k}=#{v}" } if values.is_a? Hash
       filtered = [values].flatten.compact
       filtered.map { |val| format % val }
     }

The provider should also add at least some warning in case of changes he is unable to apply.

gdlx commented 5 years ago

I did the PR 2 month ago to change the documentation from array to hash format. That looked to me what the module expects.

I wish it really becomes the expected format, because it's the most convenient one, but I don't know if there's a technical constraint forcing to use this double nested array format...

carabasdaniel commented 3 years ago

Closing issue as PR was merged