The volumes PVC name is unreasonable after mount a pvc for pipeline

[jinchihe]

Here have a pipeline definition file, part of the file is as following:

Define the pvc_name and mount the pvc by using onprem.mount_pvc.

def my_pipeline(
....
    pvc_name='pipeline-pvc',
    pvc_mount_path='/mnt',
....
):

    my_pipeline = my_pipeline_op(
...
    ).apply(onprem.mount_pvc(pvc_name, 'local-storage', pvc_mount_path))

After compire using dsl-compile. The workflow yaml file is as following:

  volumes:
  - name: local-storage
    persistentVolumeClaim:
      claimName: '{{input.parameters.pvc-name}}'

The clainName value should be workflow.parameters.pvc-name, instead of input.parameters.pvc-name since the volumes is out of templates/container, no inputs parameters, should be workflow parameters.

[elikatsis]

Hello @jinchihe,

Since #879 any field of a ContainerOp may contain parameters.

This helped #801 (we had implemented a similar, less generic approach before it was introduced but removed it since it totally covered our cause), where a PVC is created by a workflow task and its final name is an output parameter.

Such parameter usage was not available before our patch (argoproj/argo-workflows#1238). It actually supports the three types of parameter substitution (global/input/output) in the volumes attribute.

However, we need these fields to get local input parameters, so it functions properly in the case of a PVC creation or a PVC decision via output PipelineParams in a recursion (where the same field gets used multiple times).

That kind of translation for input parameters has always been there though. See for example how this sample is compiled: The url is a global workflow parameter and the task references it as a local input parameter.

Hope this helps!

[Ark-kun]

The clainName value should be workflow.parameters.pvc-name, instead of input.parameters.pvc-name since the volumes is out of templates/container, no inputs parameters, should be workflow parameters.

I agree that it looks pretty strange. But it will start working in Argo 2.3.
Argo 2.3 also adds template-level volumes.

We can try to hack a quick fix.

[jinchihe]

The clainName value should be workflow.parameters.pvc-name, instead of input.parameters.pvc-name since the volumes is out of templates/container, no inputs parameters, should be workflow parameters.

I agree that it looks pretty strange. But it will start working in Argo 2.3.
Argo 2.3 also adds template-level volumes.

We can try to hack a quick fix.

Great! Thanks a lot! @Ark-kun

[jinchihe]

@elikatsis Thanks very much for the replying.

Reviewed #801, seems that will create a new pvc in one step, not handle using a existing pvc?
But in my case, I just want to use the existing pvc (since user have prepared some data on the pvc before running pipeline), I just want to mount the pvc to the pods. And the pvc_name is a global workflow parameter (may be setting from Pipeline UI), when I use onprem.mount_pvc() function to mount that, the volume.persistentVolumeClaim.claimName will be set to input parameters, not global workflow, that's different with this simple since that's out of ContainerOp.

[elikatsis]

@jinchihe
Before Argo v2.3 (and the 1238 patch specifically) there is no parameter substitution in the volumes attribute, so the example wouldn't work at this very moment in pipelines.

I understand the frustration, it is a field out of the ContainerOp. However, it will be attached to the pod created based on that op.

So, an Argo user, being aware of such functionality, will use it properly.
On the other hand, in pipelines the compiler translates it and adds the required input parameters. Also, a pipelines user usually will not look at the argo yaml, since the UI has everything neatly visualized (and we have added a Volumes tab there, so the user may see the mounted volumes).

Also argoproj/argo-workflows#1342 introduces the template-specific volumes, which I believe we should use instead, and makes more sense for this DSL. Since add_volume() is a ContainerOp method it should affect only that template. And there should be some other way to add global volumes.

Edit:

seems that will create a new pvc in one step, not handle using a existing pvc?

Yes, a VolumeOp by default creates a new PVC (or performs any other Argo-Resource-Action)

[jinchihe]

@elikatsis Oh... that's clear to me now. Thanks a lot!

[Ark-kun]

@elikatsis Is is true that in Argo 2.2 (before argoproj/argo-workflows#1238) there is no parameter substitution in volumes at all, so this issue is impossible to fix in Argo 2.2?

[elikatsis]

@Ark-kun To be honest, I hadn't noticed this was actually there in v2.2.1.
So global parameters do get substituted, since the references are added here.

So, a design which would differentiate global parameters in the DSL and substitute them accordingly would work.
For this, it is important to note that global parameters are different than PipelineParams defined inside the pipeline function.

[jessiezcc]

@Ark-kun since we've upgraded to argo 2.3, can this issue be closed?