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

fix(controller): Mount volumes defined in script templates. Closes #1722 #2377

Merged
merged 2 commits into from
Mar 6, 2020

Conversation

changhc
Copy link
Contributor

@changhc changhc commented Mar 6, 2020

Description:

Closes #1722.

The controller has a behavior that when a template has an artifact whose path overlaps with any of the volumes to be mounted in this template, it will not load the artifact into the default artifact directory but to the volume to be mounted. It should mount the volume to the init container as well before doing so. However, when we check for volumes that should be mounted to the init container, we miss out volumes defined in script templates, and thus artifacts cannot be loaded properly only for this type of template. To be safe, I also checked other types of templates, and I think we've covered all cases, i.e., container templates and script templates.

Updated E2E to cover this case and tested manually with the following template (basically fetched from the issue):

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: artifact-passing-
spec:
  entrypoint: artifact-example
  volumes:
    - name: aws
  templates:
  - name: artifact-example
    steps:
    - - name: generate-artifact
        template: execute

  - name: execute
    inputs:
      artifacts:
      - name: config
        path: /mnt/vol/.aws/config
        mode: 0777
        raw:
          data: |
            [profile default]
            role_arn = arn:abc
            credential_source = Ec2InstanceMetadata
    script:
      volumeMounts:
      - mountPath: /mnt/vol/.aws
        name: aws
      image: alpine:latest
      env:
      - name: AWS_DEFAULT_REGION
        value: us-west-2
      - name: AWS_CONFIG_FILE
        value: /mnt/vol/.aws/config
      command: [sh]
      source: |
        echo "Hello World"

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I have written unit and/or e2e tests for my change. PRs without these are unlike to be merged.
  • Optional. I've added My organization is added to the README.
  • I've signed the CLA and required builds are green.

@changhc changhc changed the title fix (controller): Mount volumes defined in script templates. Closes #1722 fix(controller): Mount volumes defined in script templates. Closes #1722 Mar 6, 2020
@codecov
Copy link

codecov bot commented Mar 6, 2020

Codecov Report

Merging #2377 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2377      +/-   ##
==========================================
+ Coverage   13.28%   13.32%   +0.04%     
==========================================
  Files          72       72              
  Lines       24653    24657       +4     
==========================================
+ Hits         3274     3285      +11     
+ Misses      20966    20961       -5     
+ Partials      413      411       -2
Impacted Files Coverage Δ
workflow/controller/workflowpod.go 72.22% <100%> (+1.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9743831...cdde8d9. Read the comment docs.

Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for this fix!

@simster7 simster7 merged commit 42200fa into argoproj:master Mar 6, 2020
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

Successfully merging this pull request may close these issues.

When using script template input artifact is not able to save in volume mount
2 participants