Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buggy Docker_Volume options #507

Closed
gdlx opened this issue Jul 7, 2019 · 4 comments
Closed

Buggy Docker_Volume options #507

gdlx opened this issue Jul 7, 2019 · 4 comments

Comments

@gdlx
Copy link

gdlx commented Jul 7, 2019

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
Copy link

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
Copy link
Contributor

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
Copy link
Author

gdlx commented Jul 18, 2019

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
Copy link
Contributor

Closing issue as PR was merged

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

No branches or pull requests

4 participants