Move node bootstrap files from /tmp to /opt/parallelcluster/tmp#7481
Conversation
hanwen-cluster
force-pushed
the
developjun30
branch
from
0234e8f to
f91d33c
Compare
| all: "| tee -a /var/log/cloud-init-output.log | logger -t user-data -s 2>/dev/ttyS0" | ||
| write_files: | ||
| - path: /tmp/dna.json | ||
| - path: /opt/parallelcluster/tmp/dna.json |
There was a problem hiding this comment.
Can we convert this into a variable which can be replaced by Fn::Sub as this keeps being repeated in all user_data files?
There was a problem hiding this comment.
The pattern in this file uses paths with literal strings. So I'd recommend to keep the code as is
There was a problem hiding this comment.
The concern isn't the style of a single literal — it's that the same path is now repeated ~7× within this file and across all three user_data files (compute/head/login) plus cluster_stack.py. That's the duplication I'd like to remove: one path change (or a typo in one copy) has to be kept in sync everywhere, and a single wrong copy silently breaks bootstrap. If this change was already in-place it would have made this PR easier and smaller.
On "the pattern uses literal strings" — that's true for paths that appear once or are smaller like /tmp was, but this one doesn't; and we already use Fn::Sub variables throughout these files (${DnaJson}, ${Timeout}, ${AWS::Region}…), so a ${PclusterTmpDir} var fits the existing pattern rather than breaking it. It also has a concrete advantage a shell variable can't offer: Fn::Sub substitutes into the write_files: - path: keys too (cloud-init YAML, where a shell var wouldn't interpolate), so the same token covers
both the YAML paths and the script body.
Cleanest form: define the path once as a CLI constant, feed it into the Fn::Sub mapping (get_common_user_data_env, so all three node types get it), and reference the same constant in cluster_stack.py's cfn-init files keys. Then the path is defined
in exactly one place for all four files. Given we're moving this path specifically to make bootstrap robust, having it defined once seems worth it — can we do that rather than N literal copies?
| # [B108:hardcoded_tmp_directory] Probable insecure usage of temp file/directory. | ||
| "/tmp/dna.json": { # nosec B108 | ||
| # Staged under the ParallelCluster-managed directory (not /tmp) so bootstrap works | ||
| # on STIG/CIS-hardened AMIs that mount /tmp with noexec (PCLUSTER-3858). cfn-init |
There was a problem hiding this comment.
Please remove internal Task IDs and remove the mention of STIG/CIS-hardened AMIs as we are not supporting them yet.
| [ -f /etc/parallelcluster/pcluster_cookbook_environment.sh ] && . /etc/parallelcluster/pcluster_cookbook_environment.sh | ||
|
|
||
| cd /tmp | ||
| mkdir -p -m 0755 /opt/parallelcluster/tmp |
There was a problem hiding this comment.
Also why are we creating this directory again if its already created as part of Build Image?
There was a problem hiding this comment.
Done. Now the only place the directory is created is in cookbook "directories.rb"
| vendor_cookbook | ||
| fi | ||
| cd /tmp | ||
| mkdir -p -m 0755 /opt/parallelcluster/tmp |
There was a problem hiding this comment.
[Blocking] Why are we creating this directory in user-data and not in cookbook? Do we not plan on retaining it?
There was a problem hiding this comment.
We are making sure the directory exists in multiple places. This is defensive program
There was a problem hiding this comment.
My response is aws/aws-parallelcluster-cookbook#3221 (comment)
hanwen-cluster
force-pushed
the
developjun30
branch
from
f91d33c to
e08b40b
Compare
Some custom AMIs mount /tmp with noexec, and create-cluster wrote and consumed bootstrap files under /tmp. Move them under /opt/parallelcluster/tmp instead so create-cluster works on such AMIs.
hanwen-cluster
force-pushed
the
developjun30
branch
from
35beee6 to
de30f05
Compare
2 participants
Description of changes
Tests
pcluster build-imageandpcluster create-clustersucceedednoexecon/tmpafter Nvidia and efs-utils are moved to repo based installationReferences
Checklist
developadd the branch name as prefix in the PR title (e.g.[release-3.6]).Please review the guidelines for contributing and Pull Request Instructions.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.