use create_and_canonicalize_directory for snapshots dir

[yihau]

Problem

we don't create "snapshots" dir for users before we canonicalize.

Summary of Changes

do something similar with ledger path:

agave/validator/src/main.rs

Lines 997 to 1004 in df27fb3

	let ledger_path = create_and_canonicalize_directories([&ledger_path])
	.unwrap_or_else(|err| {
	eprintln!(
	"Unable to access ledger path '{}': {err}",
	ledger_path.display(),
	);
	exit(1);
	})

[mergify]

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

[steviez]

Funny enough, I remember reporting this on Discord and doing nothing about it so kudos for making the change.

The change seems fine to me, but Brooks is in the know about all of the snapshot / accountsdb paths so I think we need his ship-it

[brooksprumo]

Here's what I'm thinking as the fix:

diff --git a/validator/src/main.rs b/validator/src/main.rs
index bee65e487b..0ae88b94f9 100644
--- a/validator/src/main.rs
+++ b/validator/src/main.rs
@@ -24,7 +24,10 @@ use {
             AccountsIndexConfig, IndexLimitMb, ScanFilter,
         },
         partitioned_rewards::TestPartitionedEpochRewards,
-        utils::{create_all_accounts_run_and_snapshot_dirs, create_and_canonicalize_directories},
+        utils::{
+            create_all_accounts_run_and_snapshot_dirs, create_and_canonicalize_directories,
+            create_and_canonicalize_directory,
+        },
     },
     solana_clap_utils::input_parsers::{keypair_of, keypairs_of, pubkey_of, value_of, values_of},
     solana_core::{
@@ -1671,9 +1674,9 @@ pub fn main() {
     } else {
         &ledger_path
     };
-    let snapshots_dir = fs::canonicalize(snapshots_dir).unwrap_or_else(|err| {
+    let snapshots_dir = create_and_canonicalize_directory(snapshots_dir).unwrap_or_else(|err| {
         eprintln!(
-            "Failed to canonicalize snapshots path '{}': {err}",
+            "Failed to create snapshots directory '{}': {err}",
             snapshots_dir.display(),
         );
         exit(1);

So we only need to change the one fs::create_dir_all() and update the error text.

[brooksprumo]

(Assuming the change still fixes the problem 😸)

[mergify]

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

[steviez]

Just tried it out and can confirm that passing a directory that did not exist resulted in the directory getting created.

So, with our validator script, this means you can just do rm -rf ledger-snapshots again and let the script figure out to remove --no-snapshot-fetch if you want to force snapshot download.

LGTM

[yihau]

thank you @steviez for checking. yeah, I checked before I pushed but it’s great to see your double confirmation!

[yihau]

@brooksprumo if I don't miss anything, v2.0 doesn't affect by this. maybe we can bp to 2.1 first 🤔

[steviez]

@brooksprumo if I don't miss anything, v2.0 doesn't affect by this. maybe we can bp to 2.1 first 🤔

In genereal, yes, we should BP to v2.1 first and test it out before considering a v2.0 BP. Given that we used to create this directory (not sure when it changed), I'd be in the favor of the BP. Technically, we are re-establishing pre-existing behavior

[brooksprumo]

v2.0 doesn't affect by this

Got it. I looked at the v2.0 code, but I must've done it too quickly and I read it wrong. I see now that v2.0 doesn't canonicalize the snapshots dir, so that's why v2.0 doesn't have this issue.