Use ECS properties instead of container properties

[jorgee]

Closes #5096

Ports the @tom-seqera implementation in #5391 to AWS SDK v2 implementation.

With the RegisterJobDefinitionModel and ContainerPropertiesModel introduced by @pditommaso, these changes shouldn't affect the xpack as the interface it extends remains untouched.

[netlify]

✅ Deploy Preview for nextflow-docs-staging canceled.

Name	Link
🔨 Latest commit	06b3a07
🔍 Latest deploy log	https://app.netlify.com/projects/nextflow-docs-staging/deploys/6889d5f01bf0580008cd631a

[pditommaso]

Nice, let's test using [e2e prod] in the commit message

[pditommaso]

@jorgee aws tests are failing

[jorgee]

It seems the tests are failing because of the job definition reuse. The old definition is using containerProperties and the new one is trying to override with ecsProperties. If I am not wrong, the job definition is reused if container name and nf-token is the same, isn't it? So, I think we should include something else to the token to ensure both are using the same container or ecs properties.

[jorgee]

Token was basically hashing the result of containerProperties.toString. I have added a fake property ecsMode=true in the new version. If there is a job definition with same values but with the previous version, the token will be different and it will generate a new revision. Now, the job is failing because of CI user role permissions as in the other CI pipelines.

[pditommaso]

Launching some e2e tests

[pditommaso]

@jorgee would not this impact also the xpack plugin for amazon?

[jorgee]

@jorgee would not this impact also the xpack plugin for amazon?

The xpack is overriding the configJobDefRequest method that is not changed. The changes are internal in the RegisterJobDefinitionModel.toBatchRequest method where it sets the ecs properties. But let me do a test to see if I miss something.

[jorgee]

When testing with xpack I have got this error. Not sure yet if it is related to this change or happening also with the master.

java.lang.ClassCastException: class software.amazon.awssdk.services.batch.model.MountPoint cannot be cast to class software.amazon.awssdk.services.batch.model.MountPoint (software.amazon.awssdk.services.batch.model.MountPoint is in unnamed module of loader org.pf4j.PluginClassLoader @4ea17147; software.amazon.awssdk.services.batch.model.MountPoint is in unnamed module of loader org.pf4j.PluginClassLoader @9a20cbd)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)
        at software.amazon.awssdk.services.batch.model.MountPointsCopier.copy(MountPointsCopier.java:32)
        at software.amazon.awssdk.services.batch.model.ContainerProperties$BuilderImpl.mountPoints(ContainerProperties.java:2578)
        at nextflow.cloud.aws.batch.model.ContainerPropertiesModel.toBatchContainerProperties(ContainerPropertiesModel.groovy:254)

[jorgee]

It was an xpack gradle problem. Working after applying the fix.

[pditommaso]

Why now check first getTaskProperties(job) and then job?.container()?.taskArn() ?

[jorgee]

I got it from Tom's PR #5391. I think it is needed because of the reuse of job definitions. If the job was defined with ECS, the information is inside TaskProperties. If it is reusing an old definition with container properties, the taskProperties are empty and you need to get in the Container. It is the same for other job properties.

Maybe, with the change I did to do not reuse jobs definitions with container properties, it is not needed (54ee92b). However, I am not 100% sure because the hash is only applied when container properties are defined, and there could be cases were old definitions are still reused. Maybe the best is just add a comment to explain it.

[pditommaso]

Not sure to follow, if it's an improvement it should go into a separate PR and keep this as a mere SDK upgrade

[jorgee]

It is not an improvement. When using ECS properties in the task definition (the goal of the PR), some properties that were obtained before from the container are now inside TaskProperties. This is why we check them first in getTaskProperties(job), it is the expected place with the new definition. We have a fallback to the old container properties because of the job definition reusage. If a job submission is reused with the old container properties, the task properties will be empty and we should get them from the container properties as before.

[pditommaso]

ok, triggered e2e tests

[jorgee]

It seems that something is broken with the e2e test triggering condition. It has not triggered the e2e tests.

[jorgee]

The problem was the git commit message was returning a string with escapes due to %q option in
echo "commit_message=$(printf "%q" "$COMMIT_MESSAGE")" | head -n 1 >> $GITHUB_OUTPUT
The logs of the get_commit_message show the escaped string:

GitHub event=pull_request
Commit message=\[e2e\ prod\]\ Merge\ branch\ \'master\'\ into\ migrate-ecs-properties

I have changed %q to %s to generate a plain string.