allow to set a volume as readOnly (apply only in csi driver) (#62)

jagedn · web-flow · commit b7d5ae0a6045 · 2024-07-08T11:57:01.000+02:00
Signed-off-by: Jorge Aguilera &lt;jorge@edn.es&gt;
diff --git a/plugins/nf-nomad/src/main/nextflow/nomad/config/VolumeSpec.groovy b/plugins/nf-nomad/src/main/nextflow/nomad/config/VolumeSpec.groovy
@@ -35,6 +35,7 @@ class VolumeSpec {
     private String name
     private String path
     private boolean workDir = false
+    private boolean readOnly = false
 
     String getType() {
         return type
@@ -52,6 +53,10 @@ class VolumeSpec {
         return path
     }
 
+    boolean getReadOnly(){
+        return readOnly
+    }
+
     VolumeSpec type(String type){
         this.type = type
         this
@@ -72,6 +77,29 @@ class VolumeSpec {
         this
     }
 
+    VolumeSpec readOnly(boolean readOnly){
+        this.readOnly = readOnly
+        this
+    }
+
+    String getAccessMode(){
+        return switch (this.type){
+            case VOLUME_CSI_TYPE->
+                readOnly ?
+                        "multi-node-reader-only"
+                        :
+                        "multi-node-multi-writer";
+            default -> ""
+        }
+    }
+
+    String getAttachmentMode(){
+        return switch (this.type){
+            case VOLUME_CSI_TYPE->"file-system";
+            default -> ""
+        }
+    }
+
     void validate(){
         if( !VOLUME_TYPES.contains(type) ) {
             throw new IllegalArgumentException("Volume type $type is not supported")
@@ -82,5 +110,8 @@ class VolumeSpec {
         if( !this.workDir && !this.path ){
             throw new IllegalArgumentException("Volume path is required in secondary volumes")
         }
+        if( this.workDir && this.readOnly ){
+            throw new IllegalArgumentException("WorkingDir Volume can't be readOnly")
+        }
     }
 }
diff --git a/plugins/nf-nomad/src/main/nextflow/nomad/executor/NomadService.groovy b/plugins/nf-nomad/src/main/nextflow/nomad/executor/NomadService.groovy
@@ -139,15 +139,17 @@ class NomadService implements Closeable{
                     taskGroup.volumes["vol_${idx}".toString()] = new VolumeRequest(
                             type: volumeSpec.type,
                             source: volumeSpec.name,
-                            attachmentMode: "file-system",
-                            accessMode: "multi-node-multi-writer"
+                            attachmentMode: volumeSpec.attachmentMode,
+                            accessMode: volumeSpec.accessMode,
+                            readOnly: volumeSpec.readOnly,
                     )
                 }
 
                 if (volumeSpec && volumeSpec.type == VolumeSpec.VOLUME_HOST_TYPE) {
                     taskGroup.volumes["vol_${idx}".toString()] = new VolumeRequest(
                             type: volumeSpec.type,
                             source: volumeSpec.name,
+                            readOnly: volumeSpec.readOnly,
                     )
                 }
             }
@@ -326,6 +328,9 @@ class NomadService implements Closeable{
     String getClientOfJob(String jobId) {
         try{
             List<AllocationListStub> allocations = jobsApi.getJobAllocations(jobId, config.jobOpts().region, config.jobOpts().namespace, null, null, null, null, null, null, null, null)
+            if( !allocations ){
+                return null
+            }
             AllocationListStub jobAllocation = allocations.first()
             return jobAllocation.nodeName
         }catch (Exception e){
diff --git a/plugins/nf-nomad/src/test/nextflow/nomad/NomadConfigSpec.groovy b/plugins/nf-nomad/src/test/nextflow/nomad/NomadConfigSpec.groovy
@@ -261,20 +261,38 @@ class NomadConfigSpec extends Specification {
         def config3 = new NomadConfig([
                 jobs: [
                         volumes : [
-                                { type "csi" name "test" path '/data'},
+                                { type "csi" name "test" path '/data' readOnly true},
                                 { type "docker" name "test" path '/data'},
                         ],
-                        volume  : { type "host" name "test" },
+                        volume  : { type "csi" name "test" },
                 ]
         ])
 
         then:
         config3.jobOpts.volumeSpec.size()==3
-        config3.jobOpts.volumeSpec[0].type == VolumeSpec.VOLUME_HOST_TYPE
+        config3.jobOpts.volumeSpec[0].type == VolumeSpec.VOLUME_CSI_TYPE
         config3.jobOpts.volumeSpec[1].type == VolumeSpec.VOLUME_CSI_TYPE
         config3.jobOpts.volumeSpec[2].type == VolumeSpec.VOLUME_DOCKER_TYPE
 
-        config.jobOpts.volumeSpec[0].workDir
-        config.jobOpts.volumeSpec.findAll{ it.workDir}.size() == 1
+        config3.jobOpts.volumeSpec[0].workDir
+        config3.jobOpts.volumeSpec.findAll{ it.workDir}.size() == 1
+        config3.jobOpts.volumeSpec[0].accessMode == "multi-node-multi-writer"
+
+        config3.jobOpts.volumeSpec[1].readOnly
+        config3.jobOpts.volumeSpec[1].accessMode == "multi-node-reader-only"
+
+        when:
+        new NomadConfig([
+                jobs: [
+                        volumes : [
+                                { type "csi" name "test" path '/data' readOnly true},
+                                { type "docker" name "test" path '/data'},
+                        ],
+                        volume  : { type "csi" name "test" readOnly true},
+                ]
+        ])
+
+        then:
+        thrown(IllegalArgumentException)
     }
 }
diff --git a/plugins/nf-nomad/src/test/nextflow/nomad/executor/NomadServiceSpec.groovy b/plugins/nf-nomad/src/test/nextflow/nomad/executor/NomadServiceSpec.groovy
@@ -370,7 +370,7 @@ class NomadServiceSpec extends Specification{
         body.Job.TaskGroups[0].Tasks[0].Config.args == args.drop(1)
 
         body.Job.TaskGroups[0].Volumes.size() == 1
-        body.Job.TaskGroups[0].Volumes['vol_0'] == [AccessMode:"multi-node-multi-writer", AttachmentMode:"file-system", Source:"test", Type:"csi"]
+        body.Job.TaskGroups[0].Volumes['vol_0'] == [AccessMode:"multi-node-multi-writer", AttachmentMode:"file-system", Source:"test", Type:"csi", ReadOnly:false]
         body.Job.TaskGroups[0].Tasks[0].VolumeMounts.size() == 1
         body.Job.TaskGroups[0].Tasks[0].VolumeMounts[0] == [Destination:"/a", Volume:"vol_0"]
 
diff --git a/validation/az-nomadlab/2-volumes.config b/validation/az-nomadlab/2-volumes.config
@@ -0,0 +1,23 @@
+plugins {
+    id 'nf-nomad@latest'
+}
+
+process {
+    executor = "nomad"
+}
+
+nomad {
+
+    client {
+     address = "http://10.0.2.6:4646"
+    }
+
+    jobs {
+         deleteOnCompletion = false
+         namespace = "nf"
+         volumes = [
+            { type "csi" name "nextflow-fs-volume" },
+            { type "csi" name "nextflow-fs-volume" path "/var/data" readOnly true}
+	    ]
+    }
+}
diff --git a/validation/run-all.sh b/validation/run-all.sh
@@ -25,6 +25,12 @@ if [ "$SKIPLOCAL" == 0 ]; then
 
   ./run-pipeline.sh -c basic/nextflow.config basic/main.nf
 
+  ./run-pipeline.sh -c directives/nextflow.config directives/main.nf
+
+  ./run-pipeline.sh -c multiple-volumes/2-volumes.config multiple-volumes/main.nf
+
+  ./run-pipeline.sh -c multiple-volumes/3-volumes.config multiple-volumes/main.nf
+
   ./run-pipeline.sh -c basic/nextflow.config nf-core/demo \
     -r dev -profile test,docker \
     --outdir $(pwd)/nomad_temp/scratchdir/out
@@ -46,6 +52,9 @@ if [ "$NFAZURE" == 1 ]; then
   ssh manager@nfazure \
     'cd ~/integration-tests/az-nomadlab; NXF_ASSETS=/projects/assets nextflow run hello -w /projects/ -c nextflow.config'
 
+  ssh manager@nfazure \
+    'cd ~/integration-tests/az-nomadlab; NXF_ASSETS=/projects/assets nextflow run hello -w /projects/ -c 2-volumes.config'
+
   ssh manager@nfazure \
     'cd ~/integration-tests/az-nomadlab; NXF_ASSETS=/projects/assets nextflow run bactopia/bactopia -c nextflow.config -w /projects -profile test,docker --outdir /projects/bactopia/outdir --accession SRX4563634 --coverage 100 --genome_size 2800000 --datasets_cache /projects/bactopia/datasets'
 else

Original file line number	Diff line number	Diff line change
`@@ -139,15 +139,17 @@ class NomadService implements Closeable{`
`139`	`139`	`taskGroup.volumes["vol_${idx}".toString()] = new VolumeRequest(`
`140`	`140`	`type: volumeSpec.type,`
`141`	`141`	`source: volumeSpec.name,`
`142`		`- attachmentMode: "file-system",`
`143`		`- accessMode: "multi-node-multi-writer"`
	`142`	`+ attachmentMode: volumeSpec.attachmentMode,`
	`143`	`+ accessMode: volumeSpec.accessMode,`
	`144`	`+ readOnly: volumeSpec.readOnly,`
`144`	`145`	`)`
`145`	`146`	`}`
`146`	`147`
`147`	`148`	`if (volumeSpec && volumeSpec.type == VolumeSpec.VOLUME_HOST_TYPE) {`
`148`	`149`	`taskGroup.volumes["vol_${idx}".toString()] = new VolumeRequest(`
`149`	`150`	`type: volumeSpec.type,`
`150`	`151`	`source: volumeSpec.name,`
	`152`	`+ readOnly: volumeSpec.readOnly,`
`151`	`153`	`)`
`152`	`154`	`}`
`153`	`155`	`}`
`@@ -326,6 +328,9 @@ class NomadService implements Closeable{`
`326`	`328`	`String getClientOfJob(String jobId) {`
`327`	`329`	`try{`
`328`	`330`	`List<AllocationListStub> allocations = jobsApi.getJobAllocations(jobId, config.jobOpts().region, config.jobOpts().namespace, null, null, null, null, null, null, null, null)`
	`331`	`+ if( !allocations ){`
	`332`	`+ return null`
	`333`	`+ }`
`329`	`334`	`AllocationListStub jobAllocation = allocations.first()`
`330`	`335`	`return jobAllocation.nodeName`
`331`	`336`	`}catch (Exception e){`