[Node] Install aws-parallelcluster-node from S3 in all regions#3211
[Node] Install aws-parallelcluster-node from S3 in all regions#3211himani2411 wants to merge 2 commits into
Conversation
The node package was installed from S3 only in us-iso regions; everywhere else it fell back to 'pip install aws-parallelcluster-node' from PyPI, breaking air-gapped/proxied build-image in commercial and govcloud. Set custom_node_package to the official S3 node URL in all regions by default, routing through the existing custom_parallelcluster_node S3 install path. Reuse the existing install_python_from_internet attribute as the opt-out: set it to install from PyPI instead. A user-provided custom_node_package still takes precedence. node_spec: default path now asserts S3 install; added a context covering the install_python_from_internet PyPI fallback. Note: requires the node package to be present at <s3_url>/parallelcluster/<version>/node/ in all partitions
| default['cluster']['install_node_from_internet'] = false | ||
|
|
||
| # Official ParallelCluster Node Package | ||
| default['cluster']['custom_node_package'] = "#{node['cluster']['s3_url']}/parallelcluster/#{node['cluster']['parallelcluster-node-version']}/node/aws-parallelcluster-node-#{node['cluster']['parallelcluster-node-version']}.tgz" |
There was a problem hiding this comment.
NOTE FOR REVIEWER: Addition of a default attribute will always make is_custom_node? to be always true; which leads to node package being installed in cluster finalize recipe
Setting a default value for an attribute is now making the user-intent unclear.
So every pcluster create and update command will lead to node package being installed on eevry node; the only solution is to add a new attribute or go to the original node.override if-else condition we have set; do you agree?
There was a problem hiding this comment.
If the cost of simplifying code and making the user intent clear is to add a new chef attribute, then we should do that
There was a problem hiding this comment.
As discussed will change how we identify the node is custom (the URL should differ from the default we set) rather than adding a new attribute.
| end | ||
|
|
||
| bash "install custom aws-parallelcluster-node" do | ||
| bash "install aws-parallelcluster-node" do |
There was a problem hiding this comment.
Why removing "custom" since the resource it is actually installing the custom node?
There was a problem hiding this comment.
Modified to add the source from which we are installing node package
| # Install the official node package from S3 in all regions (air-gapped/proxied | ||
| # support), unless the user opts into installing from PyPI via | ||
| # install_python_from_internet. A user-provided custom_node_package always wins. | ||
| if !is_custom_node? && !node['cluster']['install_python_from_internet'] |
There was a problem hiding this comment.
When do we expect to use install_python_from_internet?
There was a problem hiding this comment.
The ['cluster']['install_python_from_internet'] is a generalized attribute which was to install latest python and any python related dependencies from internet.
| default['cluster']['install_node_from_internet'] = false | ||
|
|
||
| # Official ParallelCluster Node Package | ||
| default['cluster']['custom_node_package'] = "#{node['cluster']['s3_url']}/parallelcluster/#{node['cluster']['parallelcluster-node-version']}/node/aws-parallelcluster-node-#{node['cluster']['parallelcluster-node-version']}.tgz" |
There was a problem hiding this comment.
If the cost of simplifying code and making the user intent clear is to add a new chef attribute, then we should do that
We now ship a default custom_node_package (the official S3 package) which made is_custom_node? permanently true and caused finalize/update to reinstall the node package on every cluster create/update. Compare the effective custom_node_package against the full default-precedence value (node.default[...]); only a value the customer changed counts as custom. Log the decision via Chef::Log.info. Add helper unit tests covering the default, nil, empty, customer-supplied, and same-base/different-path cases.
| default['cluster']['install_node_from_internet'] = false | ||
|
|
||
| # Official ParallelCluster Node Package | ||
| default['cluster']['custom_node_package'] = "#{node['cluster']['s3_url']}/parallelcluster/#{node['cluster']['parallelcluster-node-version']}/node/aws-parallelcluster-node-#{node['cluster']['parallelcluster-node-version']}.tgz" |
There was a problem hiding this comment.
[Minor] Not necessary to handle in this PR, but it would be good to rename the attribute in a follow up PR.
The attribute custom_node_package used to make sense because we were using it only to specify a custom package. Now we are instead using it also to specify the official package, so it should be renamed to node_package.
| def is_custom_node? | ||
| custom_node_package = node['cluster']['custom_node_package'] | ||
| !custom_node_package.nil? && !custom_node_package.empty? | ||
| return false if custom_node_package.nil? || custom_node_package.empty? |
There was a problem hiding this comment.
what is the scenario where it is nil?/empty? My understanding is that it must always have a value (either the official package or the custom package). If nil/empty the cookbook would break even before this check.
I think the logic should be: return True unless node['cluster']['custom_node_package'] is equal to the default value.
Description of changes
The node package was installed from S3 only in us-iso regions; everywhere else it install from PyPI ('pip install aws-parallelcluster-node'), breaking air-gapped/proxied build-image in commercial and govcloud.
Set custom_node_package to the official S3 node URL in all regions by default, routing through the existing custom_parallelcluster_node S3 install path. Reuse the existing install_python_from_internet attribute as the opt-out: set it to install from PyPI instead. A user-provided custom_node_package still takes precedence.
node_spec: default path now asserts S3 install; added a context covering the install_python_from_internet PyPI fallback.
Note: requires the node package to be present at
<s3_url>/parallelcluster//node/ in all partitions
Tests
References
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.