From a6f368a8f9ff5dd015ca6cb9c547c0e646c1b919 Mon Sep 17 00:00:00 2001 From: Giacomo Marciani Date: Wed, 10 Apr 2024 12:53:58 +0200 Subject: [PATCH] [Storage] Delete the shared directory only if the directory exists and it is empty. We introduce the condition of existence because otherwise, the empty condition may fail when the directory does not exist. Signed-off-by: Giacomo Marciani --- .../resources/efs/partial/_mount_umount.rb | 2 +- .../resources/lustre/partial/_mount_unmount.rb | 2 +- .../resources/volume.rb | 2 +- .../spec/unit/resources/efs_spec.rb | 4 +++- .../spec/unit/resources/lustre_mount_spec.rb | 12 +++++++++--- .../spec/unit/resources/volume_spec.rb | 6 ++++-- 6 files changed, 19 insertions(+), 9 deletions(-) diff --git a/cookbooks/aws-parallelcluster-environment/resources/efs/partial/_mount_umount.rb b/cookbooks/aws-parallelcluster-environment/resources/efs/partial/_mount_umount.rb index 6a871ba99..29149a6a6 100644 --- a/cookbooks/aws-parallelcluster-environment/resources/efs/partial/_mount_umount.rb +++ b/cookbooks/aws-parallelcluster-environment/resources/efs/partial/_mount_umount.rb @@ -120,7 +120,7 @@ mode '1777' recursive false action :delete - only_if { Dir.empty?(efs_shared_dir.to_s) } + only_if { Dir.exist?(efs_shared_dir.to_s) && Dir.empty?(efs_shared_dir.to_s) } end end end diff --git a/cookbooks/aws-parallelcluster-environment/resources/lustre/partial/_mount_unmount.rb b/cookbooks/aws-parallelcluster-environment/resources/lustre/partial/_mount_unmount.rb index f7fe72cea..58ee34ee8 100644 --- a/cookbooks/aws-parallelcluster-environment/resources/lustre/partial/_mount_unmount.rb +++ b/cookbooks/aws-parallelcluster-environment/resources/lustre/partial/_mount_unmount.rb @@ -92,7 +92,7 @@ mode '1777' recursive false action :delete - only_if { Dir.empty?(fsx.shared_dir) } + only_if { Dir.exist?(fsx.shared_dir) && Dir.empty?(fsx.shared_dir) } end end end diff --git a/cookbooks/aws-parallelcluster-environment/resources/volume.rb b/cookbooks/aws-parallelcluster-environment/resources/volume.rb index e9eefdb67..4f166a39e 100644 --- a/cookbooks/aws-parallelcluster-environment/resources/volume.rb +++ b/cookbooks/aws-parallelcluster-environment/resources/volume.rb @@ -114,7 +114,7 @@ directory shared_dir do recursive false action :delete - only_if { Dir.empty?(shared_dir) } + only_if { Dir.exist?(shared_dir) && Dir.empty?(shared_dir) } end end diff --git a/cookbooks/aws-parallelcluster-environment/spec/unit/resources/efs_spec.rb b/cookbooks/aws-parallelcluster-environment/spec/unit/resources/efs_spec.rb index e77be715a..b9ffbf38e 100644 --- a/cookbooks/aws-parallelcluster-environment/spec/unit/resources/efs_spec.rb +++ b/cookbooks/aws-parallelcluster-environment/spec/unit/resources/efs_spec.rb @@ -355,7 +355,9 @@ def mock_already_installed(package, expected_version, installed) before do stub_command("mount | grep ' /shared_dir_1 '").and_return(false) stub_command("mount | grep ' /shared_dir_2 '").and_return(true) + allow(Dir).to receive(:exist?).with("/shared_dir_1").and_return(true) allow(Dir).to receive(:empty?).with("/shared_dir_1").and_return(true) + allow(Dir).to receive(:exist?).with("/shared_dir_2").and_return(true) allow(Dir).to receive(:empty?).with("/shared_dir_2").and_return(false) end @@ -382,7 +384,7 @@ def mock_already_installed(package, expected_version, installed) end end - it "deletes shared dir only if empty" do + it "deletes shared dir only if it exists and it is empty" do is_expected.to delete_directory('/shared_dir_1') .with(recursive: false) diff --git a/cookbooks/aws-parallelcluster-environment/spec/unit/resources/lustre_mount_spec.rb b/cookbooks/aws-parallelcluster-environment/spec/unit/resources/lustre_mount_spec.rb index d02d3c585..75db5fa1e 100644 --- a/cookbooks/aws-parallelcluster-environment/spec/unit/resources/lustre_mount_spec.rb +++ b/cookbooks/aws-parallelcluster-environment/spec/unit/resources/lustre_mount_spec.rb @@ -362,7 +362,9 @@ before do stub_command("mount | grep ' /shared_dir_1 '").and_return(false) stub_command("mount | grep ' /shared_dir_2 '").and_return(true) + allow(Dir).to receive(:exist?).with("/shared_dir_1").and_return(true) allow(Dir).to receive(:empty?).with("/shared_dir_1").and_return(true) + allow(Dir).to receive(:exist?).with("/shared_dir_2").and_return(true) allow(Dir).to receive(:empty?).with("/shared_dir_2").and_return(false) end @@ -386,7 +388,7 @@ .with(pattern: "lustre_id_2.fsx.REGION.amazonaws.com@tcp:/mount_name_2 *") end - it 'deletes shared dir only if empty' do + it 'deletes shared dir only if it exists and it is empty' do is_expected.to delete_directory('/shared_dir_1') .with(recursive: false) is_expected.not_to delete_directory('/shared_dir_2') @@ -417,7 +419,9 @@ before do stub_command("mount | grep ' /shared_dir_1 '").and_return(false) stub_command("mount | grep ' /shared_dir_2 '").and_return(true) + allow(Dir).to receive(:exist?).with("/shared_dir_1").and_return(true) allow(Dir).to receive(:empty?).with("/shared_dir_1").and_return(true) + allow(Dir).to receive(:exist?).with("/shared_dir_2").and_return(true) allow(Dir).to receive(:empty?).with("/shared_dir_2").and_return(false) end @@ -441,7 +445,7 @@ .with(pattern: "ontap_id_2.fsx.REGION.amazonaws.com:/junction_path_2 *") end - it 'deletes shared dir only if empty' do + it 'deletes shared dir only if it exists and it is empty' do is_expected.to delete_directory('/shared_dir_1') .with(recursive: false) is_expected.not_to delete_directory('/shared_dir_2') @@ -472,7 +476,9 @@ before do stub_command("mount | grep ' /filecache_dir_1 '").and_return(false) stub_command("mount | grep ' /filecache_dir_2 '").and_return(true) + allow(Dir).to receive(:exist?).with("/filecache_dir_1").and_return(true) allow(Dir).to receive(:empty?).with("/filecache_dir_1").and_return(true) + allow(Dir).to receive(:exist?).with("/filecache_dir_2").and_return(true) allow(Dir).to receive(:empty?).with("/filecache_dir_2").and_return(false) end @@ -496,7 +502,7 @@ .with(pattern: "filecache_dns_name_2@tcp:/filecache_mount_name_2 *") end - it 'deletes shared dir only if empty' do + it 'deletes shared dir only if it exists and it is empty' do is_expected.to delete_directory('/filecache_dir_1') .with(recursive: false) is_expected.not_to delete_directory('/filecache_dir_2') diff --git a/cookbooks/aws-parallelcluster-environment/spec/unit/resources/volume_spec.rb b/cookbooks/aws-parallelcluster-environment/spec/unit/resources/volume_spec.rb index 958aae8fe..03e9ac468 100644 --- a/cookbooks/aws-parallelcluster-environment/spec/unit/resources/volume_spec.rb +++ b/cookbooks/aws-parallelcluster-environment/spec/unit/resources/volume_spec.rb @@ -134,6 +134,7 @@ before do stub_command("mount | grep ' /SHARED_DIR '").and_return(false) + allow(Dir).to receive(:exist?).with("/SHARED_DIR").and_return(true) allow(Dir).to receive(:empty?).with("/SHARED_DIR").and_return(is_dir_empty) end @@ -147,7 +148,7 @@ .with(pattern: " /SHARED_DIR ") end - it "deletes shared dir only if empty" do + it "deletes shared dir only if it exists and it is empty" do if is_dir_empty is_expected.to delete_directory('/SHARED_DIR') .with(recursive: false) @@ -174,6 +175,7 @@ before do stub_command("mount | grep ' /SHARED_DIR '").and_return(true) + allow(Dir).to receive(:exist?).with("/SHARED_DIR").and_return(true) allow(Dir).to receive(:empty?).with("/SHARED_DIR").and_return(is_dir_empty) end @@ -189,7 +191,7 @@ .with(pattern: " /SHARED_DIR ") end - it "deletes shared dir only if empty" do + it "deletes shared dir only if it exists and it is empty" do if is_dir_empty is_expected.to delete_directory('/SHARED_DIR') .with(recursive: false)