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

community.docker.docker_image_build: outputs[] is not recognized #1001

Closed
Kejzin opened this issue Nov 29, 2024 · 13 comments · Fixed by #1006
Closed

community.docker.docker_image_build: outputs[] is not recognized #1001

Kejzin opened this issue Nov 29, 2024 · 13 comments · Fixed by #1006
Labels
docker-plain plain Docker (no swarm, no compose, no stack) question Further information is requested

Comments

@Kejzin
Copy link

Kejzin commented Nov 29, 2024

SUMMARY

outputs[].name has no effect on final build

ISSUE TYPE
  • Bug Report
COMPONENT NAME

community.docker.docker_image_build

ANSIBLE VERSION
ansible [core 2.16.6]
  config file = ps-automation/config/ansible.cfg
  configured module search path = ['config/library/modules', '/usr/share/ansible']
  ansible python module location = /home/.local/lib/python3.10/site-packages/ansible
  ansible collection location = /homeWorkspaces/ITFCP-2394-chainguard_alpine/fps-automation/config/collections
  executable location = /usr/bin/ansible
  python version = 3.10.12 (main, Nov  6 2024, 20:22:13) [GCC 11.4.0] (/usr/bin/python3)
  jinja version = 3.1.4
  libyaml = True
COLLECTION VERSION
# /home/config/collections/ansible_collections
Collection       Version
---------------- -------
community.docker 4.1.0  
CONFIGURATION
ANSIBLE_NOCOWS(<home>/config/ansible.cfg) = True
ANSIBLE_PIPELINING(<home>/config/ansible.cfg) = True
CACHE_PLUGIN(<home>/config/ansible.cfg) = pickle
CACHE_PLUGIN_CONNECTION(<home>/config/ansible.cfg) = ./ansible_facts
CACHE_PLUGIN_TIMEOUT(<home>/config/ansible.cfg) = 3600
COLLECTIONS_PATHS(<home>/config/ansible.cfg) = ['<home>/config/collections']
CONFIG_FILE() = <home>/config/ansible.cfg
DEFAULT_FORCE_HANDLERS(<home>/config/ansible.cfg) = True
DEFAULT_FORKS(<home>/config/ansible.cfg) = 10
DEFAULT_GATHERING(<home>/config/ansible.cfg) = explicit
DEFAULT_INVENTORY_PLUGIN_PATH(<home>/config/ansible.cfg) = ['<home>/config/library/inventories', '/usr/share/ansible/plugins/inventory']
DEFAULT_MANAGED_STR(<home>/config/ansible.cfg) = WARNING: this file is maintained by Ansible
DEFAULT_MODULE_PATH(<home>/config/ansible.cfg) = ['<home>/config/library/modules', '/usr/share/ansible']
DEFAULT_NO_TARGET_SYSLOG(<home>/config/ansible.cfg) = True
DEFAULT_POLL_INTERVAL(<home>/config/ansible.cfg) = 5
DEFAULT_REMOTE_USER(<home>/config/ansible.cfg) = ubuntu
DEFAULT_ROLES_PATH(<home>/config/ansible.cfg) = ['<home>/config/roles']
DEFAULT_STDOUT_CALLBACK(<home>/config/ansible.cfg) = community.general.yaml
DEFAULT_TEST_PLUGIN_PATH(<home>/config/ansible.cfg) = ['<home>/config/library/plugins/test', '<home>.ansible/plugins/test', '/usr/share/ansible/plugins/test']
DEFAULT_TIMEOUT(<home>/config/ansible.cfg) = 30
DEPRECATION_WARNINGS(<home>/config/ansible.cfg) = True
DIFF_ALWAYS(<home>/config/ansible.cfg) = True
DISPLAY_SKIPPED_HOSTS(<home>/config/ansible.cfg) = True
EDITOR(env: EDITOR) = vim
INJECT_FACTS_AS_VARS(<home>/config/ansible.cfg) = False
INTERPRETER_PYTHON(<home>/config/ansible.cfg) = auto_silent
INVENTORY_ENABLED(<home>/config/ansible.cfg) = ['aws_ec2', 'aws_rds', 'yaml']
INVENTORY_IGNORE_EXTS(<home>/config/ansible.cfg) = ['~', '.orig', '.bak', '.cfg', '.ini', '.retry', '.pyc', '.pyo']
PAGER(env: PAGER) = less
RETRY_FILES_ENABLED(<home>/config/ansible.cfg) = False
RUN_VARS_PLUGINS(<home>/config/ansible.cfg) = demand
OS / ENVIRONMENT

ubuntu 22.04 on windows WSL

STEPS TO REPRODUCE
- name: 'Build multiarch image'
  community.docker.docker_image_build:
    dockerfile: Dockerfile
    name: '{{ repository }}/{{ image_name }}:{{ version_default }}'
    rebuild: 'always'
    nocache: true
    outputs:
      - name: '{{ repository }}/{{ image_name }}:{{ version }}'
        push: true
        type: image
      - name: '{{ repository }}/{{ image_name }}:latest'
        push: true
        type: image
    path: '{{ role_path }}/files/'
    platform: '{{ platforms }}'
EXPECTED RESULTS

Image with '{{ repository }}/{{ image_name }}:{{ version }}' and '{{ repository }}/{{ image_name }}:latest' are pushed to repository

ACTUAL RESULTS

Only Image '{{ repository }}/{{ image_name }}:{{ version_default }}' is pushed.

verbatim?
@felixfontein
Copy link
Collaborator

The module simply runs docker buildx build --progress plain --tag {{ repository }}/{{ image_name }}:{{ version_default }} --file {{ role_path }}/files/Dockerfile --no-cache --platform {{ platforms }} --output type=image,name={{ repository }}/{{ image_name }}:{{ version }},push=true --output type=image,name={{ repository }}/{{ image_name }}:latest,push=true -- {{ role_path }}/files/ (with --platform repeated for every platform in platforms, if that list has more than one entry). Can you try that command directly and see what happens? If one of the --output flags is ignored, then this could be a bug in Docker Buildx.

@felixfontein felixfontein added question Further information is requested docker-plain plain Docker (no swarm, no compose, no stack) labels Nov 30, 2024
@Kejzin
Copy link
Author

Kejzin commented Dec 2, 2024

@felixfontein nice suggestion, I've run some test nad it seems like this :
docker buildx build --progress plain --file Dockerfile --no-cache --platform amd64 --output type=image,name=test_output:1.0,store=true --output type=image,name=test_output:latest,store=true -- ./

works as expected, but this:

docker buildx build --progress plain --tag test:default --file Dockerfile --no-cache --platform amd64 --output type=docker,name=test_output:1.0,store=true --output type=docker,name=test_output:latest,store=true -- ./

produces only test:default docker image.

So to me it seems like docker buildx ignore --output flags when --tag flas is passed.

As far as I understand a code of the library, here is said:

name=dict(type='str', required=True),
that 'name' is required, and later it's translated to --tag, so it's impossible to ommit that.

I tried to propose PR where it's something like "required if not output specified" but I couldn't find any docuemntation about required_if and mutualy_exclusive params, like there:

.

And I can't propose something that I'm confident is working :(

Maybe making name optional will be a solution? Or mutualy exclusive with output params?

It does seem like intended buildx workflow as I can't see any issues about it and all documentation reffers to --outputs with --tag ommited.

@felixfontein
Copy link
Collaborator

Screwing around with the module's inputs is not the right solution. We have to figure out what to pass to docker buildx build to achieve the correct results. Maybe the --tag parameter has to be converted to another --output parameter.

@felixfontein
Copy link
Collaborator

While browsing in the buildx code I found: https://github.com/docker/buildx/blob/master/build/opt.go#L189, I traced this back to docker/buildx@a932d52. I'm not sure whether this is actually on the relevant code path, I have to check in more detail when I have more time.

@Kejzin
Copy link
Author

Kejzin commented Dec 3, 2024

Thanks for that insight. I see that in first link that you attached the attribute 'tag' is mapped to opt.Exports[i].Attrs["name"]. But do you think it's relevant to our case? It seems to me like internal buildx stuff that shall not be visible from outside.

I've tried something like that:
docker buildx build --name test-docker-build:1.0 --progress plain --file Dockerfile --no-cache --platform amd64 -- ./ unknown flag: --name
And as it tells there is no mapping between name and tag form user perspective.

Why do you think altering module inputs is not an option? For me it seem most intuitive.

We map this:

- name: 'Build multiarch image'
  community.docker.docker_image_build:
    dockerfile: Dockerfile
    name: 'some_name:1.0'

docker buildx build --tag some_name:1.0

So if buildx doesn't require --tag parameter ( with outputs) why shall we?

If we keep that 'name' required, if I want to get two tags - 1.0 and latest, I have only two options:

 - name: 'Build multiarch image'
  delegate_to: localhost
  community.docker.docker_image_build:
    dockerfile: Dockerfile
    name: '<repostitory>/test_image:1.0'
    nocache: true # Must be true to ensure tags are set correctly on the image manifest for multi architecture builds
    outputs:
      - push: true # that will push the default name form what I can understand from current behaviour
      - name: '<repository>/test_iamge:latest'
        push: true
        type: image

- name: 'Build multiarch image'
  delegate_to: localhost
  community.docker.docker_image_build:
    dockerfile: Dockerfile
    name: '<repostitory>/dummy_name:not_use'
    nocache: true # Must be true to ensure tags are set correctly on the image manifest for multi architecture builds
    outputs:
      - push: false # that will push the default name form what I can understand from current behaviour
      - name: '<repository>/test_iamge:latest'
        push: true
        type: image
      - name: '<repository>/test_iamge:1.0'
        push: true
        type: image

Which you can see is quite odd.

For comprasion, this one:

 - name: 'Build multiarch image'
  delegate_to: localhost
  community.docker.docker_image_build:
    dockerfile: Dockerfile
    nocache: true # Must be true to ensure tags are set correctly on the image manifest for multi architecture builds
    outputs:
      - name: '<repository>/test_iamge:latest'
        push: true
        type: image
      - name: '<repository>/test_iamge:1.0'
        push: true
        type: image

looks super clean.

So IMO, we can map the name parameter to outputs instead of --tag and it will allow to function it properly but is quite confusing. Making name optional when outputs are defined feels more intuitive.

What do you think?

@Kejzin
Copy link
Author

Kejzin commented Dec 3, 2024

I didn't have time right now to read contributer guide, I may do PR later so it's easier to talk, but I see that wit the chagne to file plugins/modules/docker_image_build.py:385 to
args.extend(['--output type=docker,name=', '%s:%s' % (self.name, self.tag)])
We may achieve what we were talking about. But once again, it will fell quite odd to me, especially that there is no parameters for output type and push within 'main' parameters, there are only in 'outputs'

@felixfontein
Copy link
Collaborator

Thanks for that insight. I see that in first link that you attached the attribute 'tag' is mapped to opt.Exports[i].Attrs["name"]. But do you think it's relevant to our case? It seems to me like internal buildx stuff that shall not be visible from outside.

I added a lot of debug output to buildx and was able to verify that it's exactly the reason why only test:default is exported and not test_output:1.0 and test_output:latest. Commenting that line out makes docker buildx build --progress plain --tag test:default --file Dockerfile --no-cache --platform amd64 --output type=docker,name=test_output:1.0,store=true --output type=docker,name=test_output:latest,store=true -- ./ export test_output:1.0 and test_output:latest. But then test:default is no longer exported.

When running docker buildx build --progress plain --tag test:default --file Dockerfile --no-cache --platform amd64 --output type=docker,name=test_output:1.0,store=true --output type=docker,name=test_output:latest,store=true -- ./ , https://github.com/docker/buildx/blob/master/build/opt.go#L167 adds --output type=image,name=<tag> - and at this point the "output names are overwritten by --tag values" comes into play, because it actually only adds --output type=image, the name=<tag> part is then added in https://github.com/docker/buildx/blob/master/build/opt.go#L189.

Why this was implemented this way I have no idea. I traced the behavior back to docker/buildx@4b0c046; back then it was only possible to specify --output at most once, and if you didn't specify --output, --tag seems to have had no effect 🤷

In any case, the solution seems to be to leave --tag <name:tag> away and add --output type=image,name=<name:tag> instead. But as you said, then you cannot specify more options, which isn't great either. Maybe that --output option should only be added if no other --output with type=image and name=<name:tag> is there. Then users can control that way whether to push or not.

Making type configurable doesn't make sense, since the module assumes that the build output is always tagged as specified in name/tag. Though, alternatively, we could make name no longer required, but then force the user to provide rebuild=always (since checking for idempotency is no longer possible), and not add --output.

I have to think more about this...

@Kejzin
Copy link
Author

Kejzin commented Dec 4, 2024

I've checked after looking at the code you've send, that:
docker buildx build --tag test-docker-build:tag1 --tag test-docker-build:tag2 --progress plain --file Dockerfile --no-cache --platform amd64 -- ./ works also perfectly fine. Meaning, docker buildx is fine with having multiple --tag parameter.

However in ansible if you create role like that:

---
- name: 'Build multiarch image'
  delegate_to: localhost
  community.docker.docker_image_build:
    dockerfile: Dockerfile
    name: 'test-multiname:1.0'
    name: 'test-multiname-2:1.0'
    rebuild: 'always'
    outputs:
      - type: docker
      - type: docker
    nocache: true # Must be true to ensure tags are set correctly on the image manifest for multi architecture builds
    path: '{{ role_path }}/files/'
    platform: amd64

There is an warning:

TASK [Build test docker container image] *****************************************************************************************************************************************************************************************************************************
[WARNING]: While constructing a mapping from <HOME>/config/roles/docker_test_build/tasks/build.yml, line 5, column 5, found a duplicate dict key (name). Using last defined value only.

Therefore we can't reprocude it 1:1 I guess.

I think that this:

Maybe that --output option should only be added if no other --output with type=image and name=name:tag is there. Then users can control that way whether to push or not

Is the best solution. Actually in that scenario, there is no need to change --tag to --output. Logick can be "use --tag name:tag only if there is no --output specified".

I personally think that leaving outputs and staying only with --tag is big step backward as specifing multiple ouptuts is super handy.

In more complex scenario you can specify for ex. 3 outputs to push to 3 different repos (ex. dev, qa, prod) . Without outputs you'll need to define role/task 3 times.

Let's think about this but I think that it is a good solution.

@Kejzin
Copy link
Author

Kejzin commented Dec 6, 2024

Either way it would be nice to do any plan for it :D I need to decide whether to develop workaround for this bug/missing feature or we can fix it relatively soon

@felixfontein
Copy link
Collaborator

#1006 fixes the problem in community.docker 4.2.0, and #1007 documents it in community.docker 3.x.y.

@Kejzin
Copy link
Author

Kejzin commented Dec 11, 2024

Hi @felixfontein , those are cool changes. I've tested it and I've managed to get outputs almost work with this:

- name: 'Build multiarch image'
  delegate_to: localhost
  community.docker.docker_image_build:
    dockerfile: Dockerfile
    name: 'dumb-name'
    rebuild: 'always'
    outputs:
      - name: '<repo_url>/test/ansible-docker:1.0'
        type: 'image'
        push: true
      - name: '<repo_url>/test/ansible-docker:2.0'
        type: 'image'
        push: true
    nocache: true # Must be true to ensure tags are set correctly on the image manifest for multi architecture builds
    path: '{{ role_path }}/files/'
    platform: [amd64, arm64]

As expected it's a bit weird to have to define root.name if that s not used but in this config both image tags are build.

However, it does weird thing when pushing. In this configuration, I expect two tags "ansible-docker:1.0" and "ansile-docker:2.0" to be pushed as Image Index, and then without a tag actual images that relates to that.

It seems to be achievable with:

docker buildx build --progress plain --file Dockerfile --no-cache --platform amd64,arm64 --output type=image,name=<repo_url>/test/ansible-docker:buildx,push=true --output type=image,name=<repo_url>test/ansible-docker:buildx.1,push=true -- ./

But with current proposition, 1:0 is Image index, 2:0 is an image, and second Image Index is left untagged. There is still something off :(

What do you think?

@Kejzin
Copy link
Author

Kejzin commented Dec 13, 2024

After consideration I've come to conclusion that what is achievable with multiple output is not really what is needed there (at least from my perspective)

Previous solution produce multiple image manifests (thought image is exactly the same), and tags it with two tags.

What I want is to have an single image (Image Index) tagged twice, what can be achieved by

docker buildx build --progress plain --file Dockerfile --platform amd64,arm64 --output type=image,name="<repo_url>/test/ansible-docker:buildx,<repo_url>test/ansible-docker:buildx.1",push=true -- ./

For that to work you need to modify this:
image

Into

if output['name'] is not None:
    subargs.append(f'"name={output['name']}"')

otherwise "," is treaded as separate argument and only one name will be taken (last one)

I think that would be best solution as it produces one image(ImageIndex) with multiple tags, what is desired usually. Also similar behaviour is fully supported by buildx

@felixfontein
Copy link
Collaborator

I adjusted the PR to now accept a list of strings for outputs[].name, and to correctly quote arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker-plain plain Docker (no swarm, no compose, no stack) question Further information is requested
Projects
None yet
2 participants