fix: Fix issue with volume mount #194

peiniliu · 2021-04-12T10:30:08Z

What changes were proposed in this pull request?

fix Issue #193

Why are the changes needed?

fix Issue #193

Does this PR introduce any user-facing change?

No

How was this patch tested?

volume_test.py

CLAassistant · 2021-04-12T10:30:15Z

All committers have signed the CLA.

peiniliu · 2021-04-12T10:38:24Z

#193

terrytangyuan

Thanks for submitting the PR. Looks like you are still working on it so I changed it to WIP. Let me know when it's ready for review.

peiniliu · 2021-04-12T14:30:45Z

Hi,
I think you could start reviewing.
following these examples, I find the volume is usually defined inside the workflow.
volume empty
volume existing
volume pvc

Changes I did in couler,
(1) deleted the automatic volume definition inside container.py
(2) add get_volumeMounts_name function to get the volumeMount name of a container
(3) inside the workflow, check if the volume exists before adding the template.dic. If the volume not exists, add empty_dir

I don't know if it is somehow fits your idea, also if you have any comments on this.

terrytangyuan · 2021-04-15T17:57:10Z

couler/core/templates/container.py

@@ -70,6 +70,12 @@ def __init__(
        self.node_selector = node_selector
        self.volumes = volumes

+    def get_volumeMounts_name(self):


Use consistent function names

terrytangyuan · 2021-04-15T17:59:16Z

couler/core/templates/workflow.py

@@ -158,6 +166,22 @@ def to_dict(self):
                            "Unsupported signature for cluster spec: %s" % sig
                        )
            ts.append(template_dict)
+            #check volumes


Could you add some tests to make sure the existing and new use cases are covered?

terrytangyuan · 2021-04-15T17:59:30Z

couler/core/templates/workflow.py

@@ -201,6 +225,7 @@ def to_dict(self):
        else:
            d["spec"] = workflow_spec

+        print(d)


Remove this?

terrytangyuan · 2021-04-15T17:59:38Z

couler/core/templates/workflow.py

+                        }))
+
+        if self.volumes:
+            print(self.volumes)


terrytangyuan · 2021-04-15T17:59:54Z

couler/core/templates/container.py

-                template["volumes"].append(
-                    {"name": volume_mount.name, "emptyDir": {}}
-                )
+#        # Volume


Remove commented code if no longer needed

terrytangyuan · 2021-04-15T18:00:50Z

couler/tests/volume_test.py

@@ -0,0 +1,17 @@
+import os


This looks like an integration test so perhaps move it to integration tests folder and make sure to test this on CI as well similar to others.

peiniliu · 2021-05-01T10:43:56Z

Hi Terry,

Sorry for the late response, Thanks for your comments!
I have fixed all the comments you provided and also added an integration test. Let me know if more things need to be improved.

Best,
Peini

integration_tests/volume_example.py

couler/core/templates/workflow.py

terrytangyuan · 2021-05-18T18:57:07Z

Please address unsolved comments and fix failed CI builds. Thanks.

peiniliu · 2021-05-18T21:27:21Z

Hi Terry,

I have fixed the comments, the unit test from my side passed. CI failed because of the exit code == 1 in my test, which is not happening in my env, maybe I miss some CI configuration in couler?

Best,

Peini

terrytangyuan · 2021-05-19T00:59:00Z

The sanity check failed https://github.com/couler-proj/couler/runs/2614334892

Please check out the instruction here on how to fix it: https://github.com/couler-proj/couler/blob/master/CONTRIBUTING.md#run-sanity-checks

terrytangyuan · 2021-05-20T00:53:20Z

couler/tests/test_data/output_golden_1.yaml

@@ -51,3 +48,6 @@ spec:
          - cowsay
        args:
          - "{{inputs.parameters.para-consumer-0}}"
+  volumes:


This change doesn't look correct? It should be part of the template? (similar in other YAML files)

terrytangyuan · 2021-05-20T00:53:45Z

go.sum

@@ -741,6 +748,10 @@ golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20201112073958-5cba982894dd h1:5CtCZbICpIOFdgO940moixOPjc0178IU44m4EjOO5IY=
 golang.org/x/sys v0.0.0-20201112073958-5cba982894dd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20210330210617-4fbd30eecc44/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=


I wonder what introduced these changes.

Hi，
I think it because I was trying to run integrated tests locally. But I think my go env(proto3) has some version problems.
I have removed these changes.

terrytangyuan · 2021-05-20T00:54:35Z

integration_tests/volume_example.py

+            ],
+            volume_mounts=[VolumeMount("apppath", "/tmp")],
+        )
+        # 3) Add an exit handler that runs when the workflow succeeds.


Simplify this example by removing the exit handler

terrytangyuan · 2021-05-20T00:55:23Z

integration_tests/volume_example.py

+                ' vol_found=`mount | grep /tmp` && \
+            if [[ -n $vol_found ]]; \
+            then echo "Volume mounted and found"; \
+            else echo "Not found"; fi '


How do we know if this is correct? Should we be exiting 0 and non-zero here instead of just echo-ing here?

yes, I removed the handler, and add exit -1 instead of not found.

terrytangyuan · 2021-05-20T00:56:07Z

@peiniliu Thanks for fixing the tests. I previous overlooked some of the details and just left some additional (minor) comments. Please take another look.

terrytangyuan

Thank you!

volume couler-proj#193

3edcb8c

terrytangyuan reviewed Apr 12, 2021

View reviewed changes

terrytangyuan changed the title ~~volume #193~~ [WIP] Fix issue with volume mount Apr 12, 2021

terrytangyuan reviewed Apr 15, 2021

View reviewed changes

peiniliu added 2 commits May 1, 2021 12:28

add intergration test

1a7e440

change if

6e7bdb1

terrytangyuan reviewed May 18, 2021

View reviewed changes

integration_tests/volume_example.py Show resolved Hide resolved

couler/core/templates/workflow.py Outdated Show resolved Hide resolved

terrytangyuan mentioned this pull request May 18, 2021

container volumeMounts can not mount exsist PVC. #193

Closed

peiniliu added 2 commits May 18, 2021 22:31

fix comments

697f4bc

fetch latest couler, fix conflict

b86a2e5

peiniliu added 4 commits May 19, 2021 10:11

run sanity-checks

b378173

sanity isort error

5702e1b

fix unit test

ebfa62f

remove go

9525d78

terrytangyuan reviewed May 20, 2021

View reviewed changes

terrytangyuan changed the title ~~[WIP] Fix issue with volume mount~~ fix: Fix issue with volume mount May 20, 2021

fix comments

081e0db

terrytangyuan approved these changes May 22, 2021

View reviewed changes

terrytangyuan merged commit 5c10a2c into couler-proj:master May 22, 2021

peiniliu deleted the containerbug branch May 23, 2021 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix issue with volume mount #194

fix: Fix issue with volume mount #194

peiniliu commented Apr 12, 2021

CLAassistant commented Apr 12, 2021 •

edited

peiniliu commented Apr 12, 2021

terrytangyuan left a comment •

edited

peiniliu commented Apr 12, 2021

terrytangyuan Apr 15, 2021

terrytangyuan Apr 15, 2021

terrytangyuan Apr 15, 2021

terrytangyuan Apr 15, 2021

terrytangyuan Apr 15, 2021

terrytangyuan Apr 15, 2021

peiniliu commented May 1, 2021

terrytangyuan commented May 18, 2021

peiniliu commented May 18, 2021

terrytangyuan commented May 19, 2021

terrytangyuan May 20, 2021

terrytangyuan May 20, 2021

peiniliu May 20, 2021

terrytangyuan May 20, 2021

terrytangyuan May 20, 2021

peiniliu May 20, 2021

terrytangyuan commented May 20, 2021

terrytangyuan left a comment

fix: Fix issue with volume mount #194

fix: Fix issue with volume mount #194

Conversation

peiniliu commented Apr 12, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

CLAassistant commented Apr 12, 2021 • edited

peiniliu commented Apr 12, 2021

terrytangyuan left a comment • edited

Choose a reason for hiding this comment

peiniliu commented Apr 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peiniliu commented May 1, 2021

terrytangyuan commented May 18, 2021

peiniliu commented May 18, 2021

terrytangyuan commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrytangyuan commented May 20, 2021

terrytangyuan left a comment

Choose a reason for hiding this comment

CLAassistant commented Apr 12, 2021 •

edited

terrytangyuan left a comment •

edited