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

remove duplicate volumes #558

Merged
merged 2 commits into from
Dec 18, 2018
Merged

remove duplicate volumes #558

merged 2 commits into from
Dec 18, 2018

Conversation

gaoning777
Copy link
Contributor

@gaoning777 gaoning777 commented Dec 17, 2018

This change is Reviewable

@gaoning777
Copy link
Contributor Author

/cc IronPan

@k8s-ci-robot k8s-ci-robot requested a review from IronPan December 18, 2018 00:00
@Ark-kun
Copy link
Contributor

Ark-kun commented Dec 18, 2018

This is not a fully correct solution.

Fully correct solution would be to dedup based on the volume itself, not the name. Then, if we have name conflicts, we should rename the volume+volumeMount (we have to do that when processing a task (ContainerOp), because later we won't know which volume is used by which volumeMount).

One solution to this is to have a method like task.add_mounted_volume(volume, volume_mount) which will automatically set the volume name to volume structure hash. That way different volumes will get different names and same volumes will get same names.

@gaoning777
Copy link
Contributor Author

Maybe addvolume should be pipeline level thing because it gets converted to workflow level volumes in argo yaml.

@Ark-kun
Copy link
Contributor

Ark-kun commented Dec 18, 2018

Maybe addvolume should be pipeline level thing because it gets converted to workflow level volumes in argo yaml.

It's complicated because in Kubernetes volumes are specified at the Pod level. Argo has many properties that can be set both at workflow and at template levels. Maybe some day volumes will also be available in template. argoproj/argo-workflows#1094

I find it convenient to be able to specify resource requirements for the tasks at the time of creation without splitting that between two places.

@gaoning777
Copy link
Contributor Author

gaoning777 commented Dec 18, 2018

There is no pod level volumes, is there. AFAIK, there is pod level volume mount.
Correct me if I'm wrong.

@Ark-kun
Copy link
Contributor

Ark-kun commented Dec 18, 2018

There is no pod level volumes, is there. AFAIK, there is pod level volume mount.

volumes are Pod-level
volumeMounts are Container-level

See https://kubernetes.io/docs/tasks/configure-pod-container/configure-volume-storage/

apiVersion: v1
kind: Pod
metadata:
  name: redis
spec:
  containers:
  - name: redis
    image: redis
    volumeMounts:
    - name: redis-storage
      mountPath: /data/redis
  volumes:
  - name: redis-storage
    emptyDir: {}

@gaoning777
Copy link
Contributor Author

gaoning777 commented Dec 18, 2018

Discussed offline.
Make sense. Added a todo to add the support later.

@Ark-kun
Copy link
Contributor

Ark-kun commented Dec 18, 2018

/lgtm
Issue: #494

@Ark-kun
Copy link
Contributor

Ark-kun commented Dec 18, 2018

/lgtm

@Ark-kun
Copy link
Contributor

Ark-kun commented Dec 18, 2018

/test kubeflow-pipeline-build-image

@gaoning777
Copy link
Contributor Author

/test kubeflow-pipeline-e2e-test

@gaoning777
Copy link
Contributor Author

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaoning777

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gaoning777
Copy link
Contributor Author

/test kubeflow-pipeline-build-image

@k8s-ci-robot k8s-ci-robot merged commit b313e40 into kubeflow:master Dec 18, 2018
@gaoning777 gaoning777 deleted the remove-duplicate-volumes branch December 19, 2018 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants