[DFSM] Fix race conditions when checking if a live update is supported by a storage change.

In particular, we moved the check that determines if a storage change can be applied with a live update, from a lambda block to specific only_if blocks. This is necessary to let Chef evaluate it when it gets executed, rather than at the beginning of the converge phase.

Also, removed unnecessary state 'SKIPPED_LIVE_UPDATE'
and covered DFSM critical functionalities with spec tests.

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

Author: gmarciani

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

@@ -18,14 +18,6 @@
 DDB_CONFIG_STATUS = {
   # The node successfully deployed the cluster configuration.
   DEPLOYED: "DEPLOYED",
-
-  # The node evaluated the cluster configuration and concluded that a live update should be skipped because:
-  #   1. not supported: the changeset contains changes that cannot be applied with a live update
-  #                   and may instead require a node replacement.
-  #                   Example: mounting a managed EBS.
-  #   2. not required:  the changeset contains changes that could lead to an empty live update.
-  #                   Example: changing the number of static nodes.
-  SKIPPED_LIVE_UPDATE: "SKIPPED_LIVE_UPDATE",
 }.freeze
 
 def save_instance_config_version_to_dynamodb(status)

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

@@ -32,7 +32,7 @@ class SharedStorageChangeInfo
   #                         "EfsSettings": {
   #                           "FileSystemId": "fs-123456789"
   #                         },
-  #                       "currentValue": "-"
+  #                       "currentValue": nil
   #                      }
   def initialize(change)
     old_value = change["currentValue"]
@@ -74,36 +74,26 @@ def to_s
   end
 end
 
-# Checks if the given changes provided by a change-set support live updates.
+# Checks if all the shared storage changes in the change-set (if any) support live updates.
 # With live updates we refer to in-place updates that do not required node replacement.
 # The decision on the support is demanded to SharedStorageChangeInfo.
 # If the changes are nil, empty or they do not contain any shared storage change,
 # we assume that a live update is supported.
-# Example of a change:
-#   {
-#     "parameter": "SharedStorage",
-#     "requestedValue": {
-#       "MountDir": "/opt/shared/efs/managed/1",
-#       "Name": "shared-efs-managed-1",
-#       "StorageType": "Efs",
-#       "EfsSettings": {
-#         "FileSystemId": "fs-123456789"
-#       },
-#     "currentValue": "-"
-#    }
 #
-# @param [List[change]] changes the list fo changes provided by a change-set.
 # @return [Boolean] true if the change supports live updates; false, otherwise.
-def storage_change_supports_live_update?(changes)
+def storage_change_supports_live_update?
+  change_set = JSON.load_file("#{node['cluster']['change_set_path']}")
+  changes = change_set["changeSet"]
+
   if changes.nil? || changes.empty?
-    Chef::Log.info("No change found: live update is supported")
+    Chef::Log.info("Changeset is empty: live update is supported")
     return true
   end
 
   storage_changes = changes.select { |change| change["parameter"] == "SharedStorage" }
 
   if storage_changes.empty?
-    Chef::Log.info("No shared storage change found: live update is supported")
+    Chef::Log.info("Changeset does not contain shared storage changes: live update is supported")
     return true
   end
 
@@ -112,7 +102,7 @@ def storage_change_supports_live_update?(changes)
     change_info = SharedStorageChangeInfo.new(change)
     Chef::Log.info("Generated shared storage change info: #{change_info}")
     supported = change_info.support_live_updates?
-    Chef::Log.info("Change #{change} #{'does not ' unless supported}support live updates")
+    Chef::Log.info("Change #{change} #{'does not ' unless supported}support live update")
     return false unless supported
   end
   Chef::Log.info("All shared storage changes support live update.")

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

@@ -95,45 +95,3 @@ 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']
-
-  Chef::Log.info("Evaluating if a live update is required according to the changeset at #{change_set_path}")
-
-  unless File.exist?(change_set_path)
-    Chef::Log.info("Changeset not found: live update is not required")
-    return false
-  end
-
-  change_set = JSON.load_file("#{node['cluster']['change_set_path']}")
-  changes = change_set["changeSet"]
-
-  Chef::Log.info("Changeset found: evaluating changes #{changes}")
-
-  if changes.nil? || changes.empty?
-    Chef::Log.info("No change found in changeset: live update is not required")
-    return false
-  end
-
-  outcome = case node["cluster"]["node_type"]
-            when 'HeadNode'
-              # The head node supports live updates regardless the content of the changeset,
-              # because we assume here that the CLI permitted only the submission of an update with changes
-              # that can be handled by a live update on the head node.
-              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
-              only_shared_storage_change = changes.all? { |change| change["parameter"] == "SharedStorage" }
-              only_shared_storage_change && storage_change_supports_live_update?(changes)
-            else
-              raise "node_type must be HeadNode, LoginNode or ComputeFleet"
-            end
-
-  Chef::Log.info("Live update required: #{outcome}")
-
-  outcome
-end

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

@@ -15,18 +15,13 @@
 # OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
 # limitations under the License.
 
-unless -> { 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
   block do
     run_context.include_recipe 'aws-parallelcluster-environment::update_shared_storages'
   end
-  only_if { are_mount_or_unmount_required? }
+  only_if { are_mount_or_unmount_required? && storage_change_supports_live_update? }
 end
 
 save_instance_config_version_to_dynamodb(DDB_CONFIG_STATUS[:DEPLOYED])

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

@@ -100,14 +100,9 @@ def get_queues_with_changes(config)
     change_set = JSON.load_file("#{node['cluster']['shared_dir']}/change-set.json")
     changes = change_set["changeSet"]
 
-    if changes.empty?
-      Chef::Log.info("Changeset is empty: queues do not need updates")
-      return queues
-    end
-
     # Changes to the shared storage are applied to all queues,
     # but only changes not supporting live updates will be considered to update the queues.
-    if are_mount_or_unmount_required? && !storage_change_supports_live_update?(changes)
+    if are_mount_or_unmount_required? && !storage_change_supports_live_update?
       queues = get_all_queues(config)
       Chef::Log.info("All queues will be updated in order to update shared storages")
     else

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

@@ -15,18 +15,13 @@
 # OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
 # limitations under the License.
 
-unless -> { 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
   block do
     run_context.include_recipe 'aws-parallelcluster-environment::update_shared_storages'
   end
-  only_if { are_mount_or_unmount_required? }
+  only_if { are_mount_or_unmount_required? && storage_change_supports_live_update? }
 end
 
 save_instance_config_version_to_dynamodb(DDB_CONFIG_STATUS[:DEPLOYED])

cookbooks/aws-parallelcluster-slurm/spec/unit/libraries/storage_spec.rb

@@ -0,0 +1,127 @@
+require 'spec_helper'
+
+describe "aws-parallelcluster-slurm:libraries:storage_change_supports_live_update" do
+  let(:node) do
+    {
+      "cluster" => { "change_set_path" => "/SHARED_DIR/change-set.json" },
+    }
+  end
+
+  let(:mock_shared_storage_change_info) { instance_double(SharedStorageChangeInfo) }
+
+  shared_examples "the correct method" do |changeset, info_outcome, expected_result|
+    it "returns #{expected_result}" do
+      allow(File).to receive(:read).with("/SHARED_DIR/change-set.json").and_return(JSON.dump(changeset))
+      allow(SharedStorageChangeInfo).to receive(:new).and_return(mock_shared_storage_change_info)
+      allow(mock_shared_storage_change_info).to receive(:support_live_updates?).and_return(info_outcome)
+      result = storage_change_supports_live_update?
+      expect(result).to eq(expected_result)
+    end
+  end
+
+  context "when changeset is empty" do
+    changeset = {
+      "changeSet" => [],
+    }
+    include_examples "the correct method", changeset, nil, true
+  end
+
+  context "when changeset does not contain any change for SharedStorage" do
+    changeset = {
+      "changeSet" => [
+        {
+          parameter: "NOT_SharedStorage",
+        },
+      ],
+    }
+    include_examples "the correct method", changeset, nil, true
+  end
+
+  context "when changeset contains a change for SharedStorage" do
+    changeset = {
+      "changeSet" => [
+        {
+          parameter: "SharedStorage",
+        },
+        {
+          parameter: "NOT_SharedStorage",
+        },
+      ],
+    }
+    [true, false].each do |info_outcome|
+      context "and SharedStorageChangeInfo says it is #{'not ' unless info_outcome}supported" do
+        expected_result = info_outcome
+        include_examples "the correct method", changeset, info_outcome, expected_result
+      end
+    end
+  end
+end
+
+def build_storage_change(type, ownership, action)
+  storage_identified = case type
+                       when "Ebs", "FsxOntap", "FsxOpenZfs"
+                         "VolumeId"
+                       when "FileCache"
+                         "FileCacheId"
+                       else
+                         "FileSystemId"
+                       end
+  storage_settings = case ownership
+                     when "external"
+                       {
+                         storage_identified => "STORAGE_ID",
+                       }
+                     when "managed"
+                       {
+                         "SETTING_1" => "VALUE_1",
+                         "SETTING_2" => "VALUE_2",
+                       }
+                     end
+  case action
+  when "mount"
+    current_value = nil
+    requested_value = {
+      "MountDir" => "/opt/shared/#{type}/#{ownership}/1",
+      "Name" => "shared-#{type}-#{ownership}-external-1",
+      "StorageType" => "#{type}",
+      "#{type}Settings" => storage_settings,
+    }
+  when "unmount"
+    current_value = {
+      "MountDir" => "/opt/shared/#{type}/#{ownership}/1",
+      "Name" => "shared-#{type}-#{ownership}-external-1",
+      "StorageType" => "#{type}",
+      "#{type}Settings" => storage_settings,
+    }
+    requested_value = nil
+  else
+    raise "Unrecognized action #{action}. It must be one of : mount, unmount"
+  end
+
+  {
+    "parameter" => "SharedStorage",
+    "currentValue" => current_value,
+    "requestedValue" => requested_value,
+  }
+end
+
+describe "aws-parallelcluster-slurm:libraries:SharedStorageChangeInfo" do
+  shared_examples "behaves correctly" do |change, expected_result|
+    it "support_live_updates? returns the correct value" do
+      result = SharedStorageChangeInfo.new(change).support_live_updates?
+      expect(result).to eq(expected_result)
+    end
+  end
+
+  %w(Ebs Efs FsxLustre FsxOntap FsxOpenZfs FileCache).each do |storage_type|
+    %w(external managed).each do |storage_ownership|
+      %w(mount unmount).each do |storage_action|
+        context "when #{storage_action}ing #{storage_ownership} #{storage_type}" do
+          change = build_storage_change(storage_type, storage_ownership, storage_action)
+          expected_result = %(Efs FsxLustre FsxOntap FsxOpenZfs FileCache).include?(storage_type) && storage_ownership == "external"
+          include_examples "behaves correctly", change, expected_result
+        end
+      end
+    end
+  end
+end

cookbooks/aws-parallelcluster-slurm/spec/unit/libraries/update_spec.rb

@@ -0,0 +1,49 @@
+require 'spec_helper'
+
+describe "aws-parallelcluster-slurm:libraries:are_mount_unmount_required" do
+  let(:node) do
+    {
+      "cluster" => { "shared_dir" => "/SHARED_DIR" },
+    }
+  end
+
+  shared_examples "the correct method" do |changeset, expected_result|
+    it "returns #{expected_result}" do
+      allow(File).to receive(:read).with("/SHARED_DIR/change-set.json").and_return(JSON.dump(changeset))
+      result = are_mount_or_unmount_required?
+      expect(result).to eq(expected_result)
+    end
+  end
+
+  context "when changeset is empty" do
+    changeset = {
+      "changeSet" => [],
+    }
+    include_examples "the correct method", changeset, false
+  end
+
+  context "when changeset does not contain any change with SHARED_STORAGE_UPDATE_POLICY" do
+    changeset = {
+      "changeSet" => [
+        {
+          updatePolicy: "NOT_SHARED_STORAGE_UPDATE_POLICY",
+        },
+      ],
+    }
+    include_examples "the correct method", changeset, false
+  end
+
+  context "when changeset contains at least a change with SHARED_STORAGE_UPDATE_POLICY" do
+    changeset = {
+      "changeSet" => [
+        {
+          updatePolicy: "SHARED_STORAGE_UPDATE_POLICY",
+        },
+        {
+          updatePolicy: "NOT_SHARED_STORAGE_UPDATE_POLICY",
+        },
+      ],
+    }
+    include_examples "the correct method", changeset, true
+  end
+end