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: don't merge volume mount on sidecar container if it exists #6

Merged
merged 1 commit into from
Mar 11, 2019

Conversation

bnadim
Copy link
Contributor

@bnadim bnadim commented Mar 6, 2019

What and why?

Adding the same volume mounts to sidecar container and to all containers would end up in an error telling that the volume mounts already exist.

Testing Steps

Using this configuration map we would end up getting the error

---
apiVersion: v1
kind: ConfigMap
metadata:
  name: sidecar-injector-gcsfuse
  namespace: kube-system
  labels:
    app: k8s-sidecar-injector
data:
  gcsfuse: |
    name: gcsfuse
    containers:
    - name: gcsfuse
      image: eu.gcr.io/somerepo/gcsfuse:latest
      imagePullPolicy: IfNotPresent
      securityContext:
        privileged: true
        capabilities:
          add: ["SYS_ADMIN"]
      lifecycle:
        postStart:
          exec:
            command: ["gcsfuse", "-o", "nonempty", "some_bucket", "/home/airflow/gcs/"]
        preStop:
          exec:
            command: ["fusermount", "-zu", "/home/airflow/gcs/"]
      volumeMounts:
      - name: gcsdata
        mountPath: /home/airflow/gcs/
        mountPropagation: Bidirectional
    volumes:
      - name: gcsdata
        emptyDir: {}
    volumeMounts:
      - name: gcsdata
        mountPath: /home/airflow/gcs/
        mountPropagation: HostToContainer

Reviewers

Required reviewers: @byxorna
Request reviews from other people you want to review this PR in the "Reviewers" section on the right.

⚠️ this PR must have at least 2 thumbs from the MAINTAINERS.md of the project before merging!

@yahoocla
Copy link

yahoocla commented Mar 6, 2019

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@bnadim
Copy link
Contributor Author

bnadim commented Mar 6, 2019

i agreed with cla/licenses

@komapa komapa requested a review from byxorna March 6, 2019 20:27
@byxorna
Copy link
Contributor

byxorna commented Mar 6, 2019

@bnadim thanks for the PR! If you click through that link (https://yahoocla.herokuapp.com/), the CLA bot should update this PR with your acceptance. Great catch on this :)

Copy link
Contributor

@byxorna byxorna left a comment

Choose a reason for hiding this comment

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

Awesome catch! Would you mind adding a test case that illustrates this behavior? (i.e. a pod with a VolumeMount "foo" does not have its volume replaced by a sidecar injector volumemount config "foo")

@byxorna
Copy link
Contributor

byxorna commented Mar 6, 2019

cc @iwilltry42 FYI

@iwilltry42
Copy link
Contributor

iwilltry42 commented Mar 7, 2019

Thanks for the PR @bnadim , I actually saw this happening as well (only that for me, the pods wouldn't even be launched, so I couldn't see any error messages). Looks like a stupid autocomplete mistake from my side. Now I can scratch this from my TODO-List, thank you :)

@byxorna , thanks for pinging me 👍

Copy link
Contributor

@iwilltry42 iwilltry42 left a comment

Choose a reason for hiding this comment

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

Looks like VSCode's autocomplete failed me there, thanks for catching that error 👍

@byxorna
Copy link
Contributor

byxorna commented Mar 11, 2019

Hey @bnadim, mind clicking through the linked cla/licenses check and signing the CLA? I would love to merge this change :)

@bnadim
Copy link
Contributor Author

bnadim commented Mar 11, 2019

Hey @bnadim, mind clicking through the linked cla/licenses check and signing the CLA? I would love to merge this change :)

It should be working now

@byxorna
Copy link
Contributor

byxorna commented Mar 11, 2019

@bnadim thank you! merging :)

@byxorna byxorna merged commit 5dee404 into tumblr:master Mar 11, 2019
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.

4 participants