[DFSM] Fix major gaps in the update workflow.

In particular:
  1. If the change-set does not require a live update on compute and login nodes, they save a DDB record with status SKIPPED_LIVE_UPDATE and exists from the update without performing and storage change.
  2. If the change-set requires a live update on compute and login nodes, they perform the storage change and save a DDB record with status DEPLOYED.
  3. The cluster readiness check performed by the head node considers successful all nodes having either status DEPLOYED or SKIPPED_LIVE_UPDATE.
  4. Cluster readiness check now emits a more readable log about retrieved DDB items.

Signed-off-by: Giacomo Marciani <[email protected]>

Author: gmarciani

cookbooks/aws-parallelcluster-slurm/files/default/head_node_checks/check_cluster_ready.py

@@ -108,7 +108,7 @@ def check_deployed_config_version(cluster_name: str, table_name: str, expected_c
         logger.info("Found batch of %s cluster node(s): %s", n_instance_ids, instance_ids)
 
         items = get_cluster_config_records(table_name, instance_ids, region)
-        logger.info("Retrieved %s DDB item(s): %s", len(items), items)
+        logger.info("Retrieved %s DDB item(s):\n\t%s", len(items), "\n\t".join([str(i) for i in items]))
 
         missing, incomplete, wrong = _check_cluster_config_items(instance_ids, items, expected_config_version)
 

cookbooks/aws-parallelcluster-slurm/libraries/dynamo.rb

@@ -14,13 +14,24 @@
 # Saves on the DynamoDB table 'parallelcluster-$CLUSTERNAME' the correspondence between
 # the instance id and the cluster config version deployed on that instance.
 # This is done to understand what is the deployed cluster config in each node, for example, during a CFN Stack update.
-def save_instance_config_version_to_dynamodb
+
+
+DDB_CONFIG_STATUS = {
+  DEPLOYED: "DEPLOYED",
+  SKIPPED_LIVE_UPDATE: "SKIPPED_LIVE_UPDATE"
+}
+
+def save_instance_config_version_to_dynamodb(status)
   unless on_docker? || kitchen_test? && !node['interact_with_ddb']
     # TODO: the table name should be read from a dedicated node attribute,
     #   but it is currently missing in the dna.json for compute nodes.
     table_name = "parallelcluster-#{node['cluster']['cluster_name']}"
     item_id = "CLUSTER_CONFIG.#{node['ec2']['instance_id']}"
-    item_data = "{\"cluster_config_version\": {\"S\": \"#{node['cluster']['cluster_config_version']}\"}, \"lastUpdateTime\": {\"S\": \"#{Time.now.utc}\"}}"
+    node_type = node['cluster']['node_type']
+    config_version = node['cluster']['cluster_config_version']
+    last_update_time = Time.now.utc
+
+    item_data = "{\"cluster_config_version\": {\"S\": \"#{config_version}\"}, \"status\": {\"S\": \"#{status}\"}, \"node_type\": {\"S\": \"#{node_type}\"}, \"lastUpdateTime\": {\"S\": \"#{last_update_time}\"}}"
     item = "{\"Id\": {\"S\": \"#{item_id}\"}, \"Data\": {\"M\": #{item_data}}}"
 
     execute "Save cluster config version to DynamoDB" do

cookbooks/aws-parallelcluster-slurm/libraries/storage.rb

@@ -104,7 +104,7 @@ def storage_change_supports_live_update?(changes)
 
   if storage_changes.empty?
     Chef::Log.info("No shared storage change found: assuming live update is supported")
-    return true
+    return false
   end
 
   storage_changes.each do |change|

cookbooks/aws-parallelcluster-slurm/libraries/update.rb

@@ -95,3 +95,26 @@ def is_login_nodes_removed?
   previous_config = YAML.safe_load(File.read(node['cluster']['previous_cluster_config_path']))
   previous_config.dig("LoginNodes") and !config.dig("LoginNodes")
 end
+
+def is_live_update_required?
+  require 'json'
+
+  change_set_path = node['cluster']['change_set_path']
+
+  return false unless File.exist?(change_set_path)
+
+  change_set = JSON.load_file("#{node['cluster']['change_set_path']}")
+  changes = change_set["changeSet"]
+
+  case node["cluster"]["node_type"]
+  when 'HeadNode'
+    # The head node supports live updates regardless the content of the changeset.
+    return true
+  when 'ComputeFleet','LoginNode'
+    # The compute and login nodes support live updates only in specific cases:
+    #   * changeset  contains only shared storage changes that support live updates
+    return storage_change_supports_live_update?(changes)
+  else
+    raise "node_type must be HeadNode, LoginNode or ComputeFleet"
+  end
+end

cookbooks/aws-parallelcluster-slurm/recipes/finalize/finalize_compute.rb

@@ -60,4 +60,4 @@
   only_if { is_static_node?(node.run_state['slurm_compute_nodename']) }
 end
 
-save_instance_config_version_to_dynamodb
+save_instance_config_version_to_dynamodb(DDB_CONFIG_STATUS[:DEPLOYED])

cookbooks/aws-parallelcluster-slurm/recipes/finalize/finalize_login_node.rb

@@ -15,4 +15,4 @@
 # OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
 # limitations under the License.
 
-save_instance_config_version_to_dynamodb
+save_instance_config_version_to_dynamodb(DDB_CONFIG_STATUS[:DEPLOYED])

cookbooks/aws-parallelcluster-slurm/recipes/update/update_compute.rb

@@ -15,13 +15,18 @@
 # OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
 # limitations under the License.
 
+unless lambda { is_live_update_required? }.call
+  save_instance_config_version_to_dynamodb(DDB_CONFIG_STATUS[:SKIPPED_LIVE_UPDATE])
+  return
+end
+
 # TODO: Move the only_if decision to the update_shared_storage recipe for better definition of responsibilities
-# and to facilitate unit testing.
+#  and to facilitate unit testing.
 ruby_block "update_shared_storages" do
   block do
     run_context.include_recipe 'aws-parallelcluster-environment::update_shared_storages'
   end
   only_if { are_mount_or_unmount_required? }
 end
 
-save_instance_config_version_to_dynamodb
+save_instance_config_version_to_dynamodb(DDB_CONFIG_STATUS[:DEPLOYED])

cookbooks/aws-parallelcluster-slurm/recipes/update/update_login_node.rb

@@ -15,6 +15,11 @@
 # OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
 # limitations under the License.
 
+unless lambda { is_live_update_required? }.call
+  save_instance_config_version_to_dynamodb(DDB_CONFIG_STATUS[:SKIPPED_LIVE_UPDATE])
+  return
+end
+
 # TODO: Move the only_if decision to the update_shared_storage recipe for better definition of responsibilities
 #  and to facilitate unit testing.
 ruby_block "update_shared_storages" do
@@ -24,4 +29,4 @@
   only_if { are_mount_or_unmount_required? }
 end
 
-save_instance_config_version_to_dynamodb
+save_instance_config_version_to_dynamodb(DDB_CONFIG_STATUS[:DEPLOYED])

cookbooks/aws-parallelcluster-slurm/spec/unit/recipes/finalize_compute_spec.rb

@@ -15,6 +15,7 @@
 
 describe 'aws-parallelcluster-slurm::finalize_compute' do
   for_all_oses do |platform, version|
+    node_type = "ComputeFleet"
     cookbook_venv_path = "MOCK_COOKBOOK_VENV_PATH"
     cluster_name = "MOCK_CLUSTER_NAME"
     region = "MOCK_REGION"
@@ -32,8 +33,7 @@
           allow(Time).to receive(:now).and_return(Time.parse(time_now))
           RSpec::Mocks.configuration.allow_message_expectations_on_nil = true
 
-          node.override['cluster']['node_type'] = 'ComputeFleet'
-          node.override['interact_with_ddb'] = true
+          node.override['cluster']['node_type'] = node_type
           node.override['cluster']['cluster_name'] = cluster_name
           node.override['cluster']['region'] = region
           node.override['ec2']['instance_id'] = instance_id
@@ -42,10 +42,11 @@
         runner.converge(described_recipe)
       end
 
-      it 'saves the cluster config version to dynamodb' do
+      status = "DEPLOYED"
+      it "saves the cluster config version to dynamodb with status #{status}" do
         expected_command = "#{cookbook_venv_path}/bin/aws dynamodb put-item" \
           " --table-name parallelcluster-#{cluster_name}"\
-          " --item '{\"Id\": {\"S\": \"CLUSTER_CONFIG.#{instance_id}\"}, \"Data\": {\"M\": {\"cluster_config_version\": {\"S\": \"#{cluster_config_version}\"}, \"lastUpdateTime\": {\"S\": \"#{time_now}\"}}}}'" \
+          " --item '{\"Id\": {\"S\": \"CLUSTER_CONFIG.#{instance_id}\"}, \"Data\": {\"M\": {\"cluster_config_version\": {\"S\": \"#{cluster_config_version}\"}, \"status\": {\"S\": \"#{status}\"}, \"node_type\": {\"S\": \"#{node_type}\"}, \"lastUpdateTime\": {\"S\": \"#{time_now}\"}}}}'" \
           " --region #{region}"
         is_expected.to run_execute("Save cluster config version to DynamoDB").with(
           command: expected_command,
@@ -62,11 +63,8 @@
             allow_any_instance_of(Object).to receive(:is_static_node?).and_return(false)
             RSpec::Mocks.configuration.allow_message_expectations_on_nil = true
 
-            node.override['cluster']['node_type'] = 'ComputeFleet'
             node.override['kitchen'] = true
             node.override['interact_with_ddb'] = false
-            node.override['ec2']['instance_id'] = instance_id
-            node.override['cluster']['cluster_config_version'] = cluster_config_version
           end
           runner.converge(described_recipe)
         end

cookbooks/aws-parallelcluster-slurm/spec/unit/recipes/finalize_login_node_spec.rb

@@ -15,6 +15,7 @@
 
 describe 'aws-parallelcluster-slurm::finalize_login_node' do
   for_all_oses do |platform, version|
+    node_type = "LoginNode"
     cookbook_venv_path = "MOCK_COOKBOOK_VENV_PATH"
     cluster_name = "MOCK_CLUSTER_NAME"
     region = "MOCK_REGION"
@@ -32,8 +33,7 @@
           allow(Time).to receive(:now).and_return(Time.parse(time_now))
           RSpec::Mocks.configuration.allow_message_expectations_on_nil = true
 
-          node.override['cluster']['node_type'] = 'LoginNode'
-          node.override['interact_with_ddb'] = true
+          node.override['cluster']['node_type'] = node_type
           node.override['cluster']['cluster_name'] = cluster_name
           node.override['cluster']['region'] = region
           node.override['ec2']['instance_id'] = instance_id
@@ -42,10 +42,11 @@
         runner.converge(described_recipe)
       end
 
-      it 'saves the cluster config version to dynamodb' do
+      status = "DEPLOYED"
+      it "saves the cluster config version to dynamodb with status #{status}" do
         expected_command = "#{cookbook_venv_path}/bin/aws dynamodb put-item" \
           " --table-name parallelcluster-#{cluster_name}"\
-          " --item '{\"Id\": {\"S\": \"CLUSTER_CONFIG.#{instance_id}\"}, \"Data\": {\"M\": {\"cluster_config_version\": {\"S\": \"#{cluster_config_version}\"}, \"lastUpdateTime\": {\"S\": \"#{time_now}\"}}}}'" \
+          " --item '{\"Id\": {\"S\": \"CLUSTER_CONFIG.#{instance_id}\"}, \"Data\": {\"M\": {\"cluster_config_version\": {\"S\": \"#{cluster_config_version}\"}, \"status\": {\"S\": \"#{status}\"}, \"node_type\": {\"S\": \"#{node_type}\"}, \"lastUpdateTime\": {\"S\": \"#{time_now}\"}}}}'" \
           " --region #{region}"
         is_expected.to run_execute("Save cluster config version to DynamoDB").with(
           command: expected_command,
@@ -62,11 +63,8 @@
             allow_any_instance_of(Object).to receive(:is_static_node?).and_return(false)
             RSpec::Mocks.configuration.allow_message_expectations_on_nil = true
 
-            node.override['cluster']['node_type'] = 'LoginNode'
             node.override['kitchen'] = true
             node.override['interact_with_ddb'] = false
-            node.override['ec2']['instance_id'] = instance_id
-            node.override['cluster']['cluster_config_version'] = cluster_config_version
           end
           runner.converge(described_recipe)
         end