Set AWS container properties from container options #2471

manuelesimi · 2021-12-01T19:31:26Z

This contribution addresses the requirements from #2282 and #2413.

With this change, the container properties of a job definition registered by Nextflow in AWS Batch are set starting from the container options defined in the Nextflow task.

Example

Given the following container options:

process sayHelloFirst {

  containerOptions '--env MYVAR2=foo2 --env MYVAR3=foo3 --memory-swap 240000 --memory-swappiness 20 --shm-size 16000000 --ulimit nofile=1280:2560 --ulimit nproc=16:32 --privileged'

}

the job is registered with the following properties on AWS Batch:

Do note that:

this mapping does not include properties already set by other means in Nextflow (like volumes, mount points, cpus, memory, etc.)
not all the docker run options can be mapped into AWS. See AWS doc.

…tainer properties. Signed-off-by: Manuele Simi <manuele.simi@gmail.com>

Signed-off-by: Manuele Simi <manuele.simi@gmail.com>

manuelesimi · 2021-12-02T16:34:22Z

I just pushed a fix for tmpfs mounts. They are now correctly set in the container properties:

process sayHelloFirst {
  containerOptions '--tmpfs /run:rw,noexec,nosuid,size=64 --tmpfs /app:ro,size=128'
  ...
}

The size must be in MiB.

plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AWSContainerOptionsMapper.groovy

pditommaso

Hi Manuale, this is welcome. Actually, I had gave a try some time ago but gave up when I realised container options should be managed at Job Definition level (what a design mistake ..)

My main comment is that this is a good opportunity to manage the containerOption as a map. Look at my commit that I've just pushed

5e9c1cc

manuelesimi · 2021-12-02T17:03:33Z

Great work (and fast!).
I noticed you pushed your change for the map options to another branch. How should I proceed? Merger yours in this one, wait until you merge yours into master and then merge master back into this, or else?

pditommaso · 2021-12-02T17:11:23Z

I would say to merge my branch in your PR, then you can continue with that

…container_properties

manuelesimi · 2021-12-02T17:30:01Z

@pditommaso There is a problem with your parser. The Map doesn't consider options that can be repeated multiple times (like --env or --tmpfs). Each option should be mapped to a List of values.

manuelesimi · 2021-12-02T18:57:26Z

Also, '=' cannot be used as separator in the QuoteStringTokenizer. There are options that use '=' in their values. E.g. --env MY_VAR=value.

pditommaso · 2021-12-02T19:16:15Z

It's getting complicated. Should repeatable options be supported? my original plan was only to support a subset of them.

manuelesimi · 2021-12-02T19:27:45Z

It's getting complicated. Should repeatable options be supported? my original plan was only to support a subset of them.

Parsing command line options is never easy. Unfortunately ulimit is one of the repeatable options that I expect users will require.

I made a change that seems to work:
From '--env VAR_FOO -e VAR_FOO2=value2 --env VAR_FOO3=value3 --user nf-user', I get:

Would that be OK with you?

What's probably more concerning is that we should support only the format "option value" (which is the one documented in the docker run help), not "option=value". So a value with an equal char is not separated (like in the screenshot above).

pditommaso · 2021-12-02T19:32:50Z

Is basically a Map<String,List> ?

manuelesimi · 2021-12-02T19:33:08Z

Is basically a Map<String,List> ?

yes, it is.

pditommaso · 2021-12-02T19:37:29Z

Maybe we should model it as a List of pairs, or even a custom holder class

manuelesimi · 2021-12-02T19:39:29Z

I'd go for a custom holder where we can add the lookup methods we will need.

pditommaso · 2021-12-02T19:41:39Z

👍

manuelesimi · 2021-12-02T20:07:11Z

Question:

The method Map getContainerOptionsMap() in TaskConfig returns a Map. Hence, we have the following options:

we change the method signature to return our custom holder class. As far as I see, it is used only here so it would have a low impact.
our custom holder extends a Map (but then we need to cast back to the holder's class to access its methods, which I'd really dislike)
we stick to a Map<String,List>

Which one would you favor?

pditommaso · 2021-12-02T20:08:41Z

I would say 1.

manuelesimi · 2021-12-02T20:08:54Z

Also a combination of 1. and 2. would work well.

manuelesimi · 2021-12-02T20:10:17Z

I would say 1.

Sounds good.

pditommaso · 2021-12-02T20:10:26Z

As you wish 😄

…ptions. Signed-off-by: Manuele Simi <manuele.simi@gmail.com>

modules/nf-commons/src/main/nextflow/util/CmdLineHelper.groovy

modules/nf-commons/src/main/nextflow/util/CmdLineOptionMap.groovy

plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AwsContainerOptionsMapper.groovy

plugins/nf-amazon/src/test/nextflow/cloud/aws/batch/AwsBatchTaskHandlerTest.groovy

plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AwsBatchTaskHandler.groovy

pditommaso · 2021-12-10T18:41:47Z

Just realised that AwsContainerOptionsMapper was tested via indirectly the TaskHandler. Think it would be better to simplify this testing directly the class ie.

 when:
 def map = CmdLineHelper.parseGnuArgs('--ulimit nofile=1280:2560 --ulimit nproc=16:32')
 def properties = AwsContainerOptionsMapper.createContainerOpts(map) 
 then:
 properties.getUlimits().size() == 2
 properties.getUlimits().get(0).toString() == '{HardLimit: 2560,Name: nofile,SoftLimit: 1280}'
 properties.getUlimits().get(1).toString() == '{HardLimit: 32,Name: nproc,SoftLimit: 16}'

nextflow/plugins/nf-amazon/src/test/nextflow/cloud/aws/batch/AwsContainerOptionsMapperTest.groovy

Lines 12 to 156 in 80dbfa8

    
           def 'should set env vars'() { 
        
               given: 
        
               def task = Mock(TaskRun) 
        
               task.getName() >> 'batch-task' 
        
               task.getConfig() >> new TaskConfig(containerOptions: '--env VAR_FOO -e VAR_FOO2=value2 --env VAR_FOO3=value3') 
        
               def handler = Spy(AwsBatchTaskHandler) 
        
               handler.task >> task 
        
               when: 
        
               def job = handler.makeJobDefRequest('repo/any_image:latest') 
        
               then: 
        
               1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') } 
        
               def environment = job.getContainerProperties().getEnvironment() 
        
               environment.size() == 3 
        
               environment.get(0).toString() == '{Name: VAR_FOO,}' 
        
               environment.get(1).toString() == '{Name: VAR_FOO3,Value: value3}' 
        
               environment.get(2).toString() == '{Name: VAR_FOO2,Value: value2}' 
        
           } 
        
           def 'should set ulimits'() { 
        
               given: 
        
               def task = Mock(TaskRun) 
        
               task.getName() >> 'batch-task' 
        
               task.getConfig() >> new TaskConfig(containerOptions: '--ulimit nofile=1280:2560 --ulimit nproc=16:32') 
        
               def handler = Spy(AwsBatchTaskHandler) 
        
               handler.task >> task 
        
               when: 
        
               def job = handler.makeJobDefRequest('repo/any_image:latest') 
        
               then: 
        
               1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') } 
        
               def properties = job.getContainerProperties() 
        
               properties.getUlimits().size() == 2 
        
               properties.getUlimits().get(0).toString() == '{HardLimit: 2560,Name: nofile,SoftLimit: 1280}' 
        
               properties.getUlimits().get(1).toString() == '{HardLimit: 32,Name: nproc,SoftLimit: 16}' 
        
           } 
        
           def 'should set privileged, readonly and user'() { 
        
               given: 
        
               def task = Mock(TaskRun) 
        
               task.getName() >> 'batch-task' 
        
               task.getConfig() >> new TaskConfig(containerOptions: '--privileged --read-only --user nf-user') 
        
               def handler = Spy(AwsBatchTaskHandler) 
        
               handler.task >> task 
        
               when: 
        
               def job = handler.makeJobDefRequest('repo/any_image:latest') 
        
               then: 
        
               1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') } 
        
               job.getContainerProperties().getPrivileged() == true 
        
               job.getContainerProperties().getReadonlyRootFilesystem() == true 
        
               job.getContainerProperties().getUser() == 'nf-user' 
        
           } 
        
           def 'should set tmpfs linux params'() { 
        
               given: 
        
               def task = Mock(TaskRun) 
        
               task.getName() >> 'batch-task' 
        
               task.getConfig() >> new TaskConfig(containerOptions: '--tmpfs /run:rw,noexec,nosuid,size=64 --tmpfs /app:ro,size=128') 
        
               def handler = Spy(AwsBatchTaskHandler) 
        
               handler.task >> task 
        
               when: 
        
               def job = handler.makeJobDefRequest('repo/any_image:latest') 
        
               then: 
        
               1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') } 
        
               def params = job.getContainerProperties().getLinuxParameters() 
        
               params.getTmpfs().size() == 2 
        
               params.getTmpfs().get(0).toString() == '{ContainerPath: /run,Size: 64,MountOptions: [rw, noexec, nosuid]}' 
        
               params.getTmpfs().get(1).toString() == '{ContainerPath: /app,Size: 128,MountOptions: [ro]}' 
        
           } 
        
           def 'should set memory linux params'() { 
        
               given: 
        
               def task = Mock(TaskRun) 
        
               task.getName() >> 'batch-task' 
        
               task.getConfig() >> new TaskConfig(containerOptions: '--memory-swappiness 90 --memory-swap 2048 --shm-size 1024 ') 
        
               def handler = Spy(AwsBatchTaskHandler) 
        
               handler.task >> task 
        
               when: 
        
               def job = handler.makeJobDefRequest('repo/any_image:latest') 
        
               then: 
        
               1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') } 
        
               def params = job.getContainerProperties().getLinuxParameters() 
        
               params.getSharedMemorySize() == 1024 
        
               params.getMaxSwap() == 2048 
        
               params.getSwappiness() == 90 
        
           } 
        
           def 'should set exactly one param'() { 
        
               given: 
        
               def task = Mock(TaskRun) 
        
               task.getName() >> 'batch-task' 
        
               task.getConfig() >> new TaskConfig(containerOptions: '--privileged') 
        
               def handler = Spy(AwsBatchTaskHandler) 
        
               handler.task >> task 
        
               when: 
        
               def job = handler.makeJobDefRequest('repo/any_image:latest') 
        
               then: 
        
               1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') } 
        
               def properties = job.getContainerProperties() 
        
               properties.getLinuxParameters() == null 
        
               properties.getUlimits() == null 
        
               properties.getPrivileged() == true 
        
               properties.getReadonlyRootFilesystem() == null 
        
               properties.getUser() == null 
        
           } 
        
           def 'should set no params'() { 
        
               given: 
        
               def task = Mock(TaskRun) 
        
               task.getName() >> 'batch-task' 
        
               task.getConfig() >> new TaskConfig(containerOptions: '') 
        
               def handler = Spy(AwsBatchTaskHandler) 
        
               handler.task >> task 
        
               when: 
        
               def job = handler.makeJobDefRequest('repo/any_image:latest') 
        
               then: 
        
               1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') } 
        
               def properties = job.getContainerProperties() 
        
               properties.getLinuxParameters() == null 
        
               properties.getUlimits() == null 
        
               properties.getPrivileged() == null 
        
               properties.getReadonlyRootFilesystem() == null 
        
               properties.getUser() == null 
        
           }

manuelesimi · 2021-12-10T19:16:37Z

Just realised that AwsContainerOptionsMapper was tested via indirectly the TaskHandler. Think it would be better to simplify this testing directly the class ie.
 when:
 def map = CmdLineHelper.parseGnuArgs('--ulimit nofile=1280:2560 --ulimit nproc=16:32')
 def properties = AwsContainerOptionsMapper.createContainerOpts(map) 
 then:
 properties.getUlimits().size() == 2
 properties.getUlimits().get(0).toString() == '{HardLimit: 2560,Name: nofile,SoftLimit: 1280}'
 properties.getUlimits().get(1).toString() == '{HardLimit: 32,Name: nproc,SoftLimit: 16}'
nextflow/plugins/nf-amazon/src/test/nextflow/cloud/aws/batch/AwsContainerOptionsMapperTest.groovy

Lines 12 to 156 in 80dbfa8

def 'should set env vars'() {

given:

def task = Mock(TaskRun)

task.getName() >> 'batch-task'

task.getConfig() >> new TaskConfig(containerOptions: '--env VAR_FOO -e VAR_FOO2=value2 --env VAR_FOO3=value3')

def handler = Spy(AwsBatchTaskHandler)

handler.task >> task

when:

def job = handler.makeJobDefRequest('repo/any_image:latest')

then:

1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') }

def environment = job.getContainerProperties().getEnvironment()

environment.size() == 3

environment.get(0).toString() == '{Name: VAR_FOO,}'

environment.get(1).toString() == '{Name: VAR_FOO3,Value: value3}'

environment.get(2).toString() == '{Name: VAR_FOO2,Value: value2}'

}

def 'should set ulimits'() {

given:

def task = Mock(TaskRun)

task.getName() >> 'batch-task'

task.getConfig() >> new TaskConfig(containerOptions: '--ulimit nofile=1280:2560 --ulimit nproc=16:32')

def handler = Spy(AwsBatchTaskHandler)

handler.task >> task

when:

def job = handler.makeJobDefRequest('repo/any_image:latest')

then:

1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') }

def properties = job.getContainerProperties()

properties.getUlimits().size() == 2

properties.getUlimits().get(0).toString() == '{HardLimit: 2560,Name: nofile,SoftLimit: 1280}'

properties.getUlimits().get(1).toString() == '{HardLimit: 32,Name: nproc,SoftLimit: 16}'

}

def 'should set privileged, readonly and user'() {

given:

def task = Mock(TaskRun)

task.getName() >> 'batch-task'

task.getConfig() >> new TaskConfig(containerOptions: '--privileged --read-only --user nf-user')

def handler = Spy(AwsBatchTaskHandler)

handler.task >> task

when:

def job = handler.makeJobDefRequest('repo/any_image:latest')

then:

1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') }

job.getContainerProperties().getPrivileged() == true

job.getContainerProperties().getReadonlyRootFilesystem() == true

job.getContainerProperties().getUser() == 'nf-user'

}

def 'should set tmpfs linux params'() {

given:

def task = Mock(TaskRun)

task.getName() >> 'batch-task'

task.getConfig() >> new TaskConfig(containerOptions: '--tmpfs /run:rw,noexec,nosuid,size=64 --tmpfs /app:ro,size=128')

def handler = Spy(AwsBatchTaskHandler)

handler.task >> task

when:

def job = handler.makeJobDefRequest('repo/any_image:latest')

then:

1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') }

def params = job.getContainerProperties().getLinuxParameters()

params.getTmpfs().size() == 2

params.getTmpfs().get(0).toString() == '{ContainerPath: /run,Size: 64,MountOptions: [rw, noexec, nosuid]}'

params.getTmpfs().get(1).toString() == '{ContainerPath: /app,Size: 128,MountOptions: [ro]}'

}

def 'should set memory linux params'() {

given:

def task = Mock(TaskRun)

task.getName() >> 'batch-task'

task.getConfig() >> new TaskConfig(containerOptions: '--memory-swappiness 90 --memory-swap 2048 --shm-size 1024 ')

def handler = Spy(AwsBatchTaskHandler)

handler.task >> task

when:

def job = handler.makeJobDefRequest('repo/any_image:latest')

then:

1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') }

def params = job.getContainerProperties().getLinuxParameters()

params.getSharedMemorySize() == 1024

params.getMaxSwap() == 2048

params.getSwappiness() == 90

}

def 'should set exactly one param'() {

given:

def task = Mock(TaskRun)

task.getName() >> 'batch-task'

task.getConfig() >> new TaskConfig(containerOptions: '--privileged')

def handler = Spy(AwsBatchTaskHandler)

handler.task >> task

when:

def job = handler.makeJobDefRequest('repo/any_image:latest')

then:

1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') }

def properties = job.getContainerProperties()

properties.getLinuxParameters() == null

properties.getUlimits() == null

properties.getPrivileged() == true

properties.getReadonlyRootFilesystem() == null

properties.getUser() == null

}

def 'should set no params'() {

given:

def task = Mock(TaskRun)

task.getName() >> 'batch-task'

task.getConfig() >> new TaskConfig(containerOptions: '')

def handler = Spy(AwsBatchTaskHandler)

handler.task >> task

when:

def job = handler.makeJobDefRequest('repo/any_image:latest')

then:

1 * handler.getAwsOptions() >> { new AwsOptions(cliPath: '/bin/aws') }

def properties = job.getContainerProperties()

properties.getLinuxParameters() == null

properties.getUlimits() == null

properties.getPrivileged() == null

properties.getReadonlyRootFilesystem() == null

properties.getUser() == null

}

It is also tested there. When you asked for a test, I thought you wanted a dedicated test class. I agree and will adjust the tests as you suggested.

Signed-off-by: Manuele Simi <manuele.simi@gmail.com>

manuelesimi · 2021-12-10T20:00:35Z

Just realised that AwsContainerOptionsMapper was tested via indirectly the TaskHandler. Think it would be better to simplify this testing directly the class ie.
 when:
 def map = CmdLineHelper.parseGnuArgs('--ulimit nofile=1280:2560 --ulimit nproc=16:32')
 def properties = AwsContainerOptionsMapper.createContainerOpts(map) 
 then:
 properties.getUlimits().size() == 2
 properties.getUlimits().get(0).toString() == '{HardLimit: 2560,Name: nofile,SoftLimit: 1280}'
 properties.getUlimits().get(1).toString() == '{HardLimit: 32,Name: nproc,SoftLimit: 16}'
It is also tested there. When you asked for a test, I thought you wanted a dedicated test class. I agree and will adjust the tests as you suggested.

done, see d1d9da0

manuelesimi · 2021-12-10T20:02:41Z

I think I've addressed your latest comments. Pls, let me know if I missed something. Thanks!

manuelesimi · 2021-12-10T20:15:06Z

Hold on, there is a test failing. I know why. I can fix it later.

Signed-off-by: Manuele Simi <manuele.simi@gmail.com>

manuelesimi · 2021-12-11T02:11:07Z

I changed two GoogleLifeSciencesTaskHandlerTest tests that use the new config map. I hope I didn't misinterpret them, but basically I'd expect that when the entrypoint is not set in the map, the default entrypoint is returned. See 96c3fd4

...-google/src/test/nextflow/cloud/google/lifesciences/GoogleLifeSciencesTaskHandlerTest.groovy

modules/nf-commons/src/main/nextflow/util/CmdLineOptionMap.groovy

plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AwsContainerOptionsMapper.groovy

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>

pditommaso

OK, Sunday PR approval! thanks for this, much appreciated!

manuelesimi · 2021-12-12T20:45:10Z

Terrific! This was much needed for our AWS projects.
Thanks for all the help and patience along .

pditommaso · 2021-12-22T08:39:41Z

Released along with version 21.12.1-edge

This commit adds the support for `containerOptions` directive for AWS Batch jobs This is useful to fine control the container execution properties using the same the option used for the Docker container runtime. Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> Co-authored-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>

pditommaso and others added 2 commits November 30, 2021 17:52

Update changelog

c664e63

Map the container options of the task into the equivalent AWS job con…

b9008c8

…tainer properties. Signed-off-by: Manuele Simi <manuele.simi@gmail.com>

manuelesimi requested a review from pditommaso December 1, 2021 19:38

Adjust format.

dad83bf

Signed-off-by: Manuele Simi <manuele.simi@gmail.com>

manuelesimi added the executor/aws-batch label Dec 2, 2021

manuelesimi marked this pull request as draft December 2, 2021 15:21

Set size and options for tmpfs mounts.

9dce577

Signed-off-by: Manuele Simi <manuele.simi@gmail.com>

manuelesimi marked this pull request as ready for review December 2, 2021 16:34

pditommaso reviewed Dec 2, 2021

View reviewed changes

plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AWSContainerOptionsMapper.groovy Outdated Show resolved Hide resolved

Add containerOptions map [ci fast]

5e9c1cc

pditommaso reviewed Dec 2, 2021

View reviewed changes

Merge remote-tracking branch 'origin/aws-container-options' into AWS/…

f2ef534

…container_properties

Use a custom handler as command line option map. Support repeatable o…

5e9ae82

…ptions. Signed-off-by: Manuele Simi <manuele.simi@gmail.com>