create and canonicalize account_paths #32037

apfitzge · 2023-06-08T23:00:45Z

Problem

We stopped canonicalizing validator account_paths in #30645.
This causes issues if users specify account_paths which are symlinks, when we start to clean our account directories. The directory that the cleanup process deletes & recreates should be at the followed path, not the location of the link.

Summary of Changes

canonicalize the account_paths like we've always done.

Fixes #

apfitzge · 2023-06-08T23:05:22Z

validator/src/main.rs

+    let account_paths: Vec<PathBuf> = account_paths
+        .into_iter()
+        .map(|account_path| {
+            match std::fs::create_dir_all(&account_path)


Creation happens before canonicalization; this is intentional.

std::fs::canonicalize will fail if the path does not exist. So we have 4 cases:

user passes a path which does not exist -> we create it, canonicalization just makes it absolute.

user passes a path which does exist -> "creating" does nothing, canonicalization just makes it absolute.

user passes a broken symlink -> canonicalization fails, validator crashes

user passes a good symlink -> create does nothing, canonicalization gives us the followed link

To be clear I just re-implemented the logic which was removed in the previous PR

should have commented after I create the function. Same comments applied to the create_and_canonicalize_directories fn

apfitzge · 2023-06-08T23:24:21Z

runtime/src/snapshot_utils.rs

@@ -422,6 +422,21 @@ pub enum VerifySlotDeltasError {
    BadSlotHistory,
 }

+/// Creates directories if they do not exist, and canonicalizes the paths.
+pub fn create_and_canonicalize_directories(directories: &[PathBuf]) -> Result<Vec<PathBuf>> {


wasn't super happy about this returning a SnapshotError, since it's only somewhat related to snapshots. Somewhat of a hack because I want to keep info about which path caused the error which afaik isn't easy to do w/ IoError

apfitzge · 2023-06-08T23:25:40Z

validator/src/main.rs

@@ -1413,20 +1420,10 @@ pub fn main() {
    validator_config.account_snapshot_paths = account_snapshot_paths;

    validator_config.account_shrink_paths = account_shrink_paths.map(|paths| {
-        paths


we were doing the same thing in 2 places, just moved it to a function so we can also use this in ledger-tool when I get to checking that out.

apfitzge · 2023-06-08T23:27:19Z

Leaving ledger-tool as a separate task/PR - this PR just tries to get us back to the state we had wrt canonicalization pre-#30645

codecov · 2023-06-09T00:06:17Z

Codecov Report

Merging #32037 (286a1ad) into master (8596e00) will increase coverage by 0.0%.
The diff coverage is 0.0%.

@@           Coverage Diff           @@
##           master   #32037   +/-   ##
=======================================
  Coverage    81.8%    81.9%           
=======================================
  Files         763      763           
  Lines      207671   207682   +11     
=======================================
+ Hits       170073   170113   +40     
+ Misses      37598    37569   -29

validator/src/main.rs

t-nelson

brooksprumo · 2023-06-09T13:26:54Z

validator/src/main.rs

@@ -963,8 +964,7 @@ pub fn main() {
        .map(BlockstoreRecoveryMode::from);

    // Canonicalize ledger path to avoid issues with symlink creation
-    let _ = fs::create_dir_all(&ledger_path);


Interesting that we would ignore errors here...

Seems correct to not ignore those errors in the new code.

History: #7877
Probably just the simplest/quickest way to get it created & not have an unwrap?

🤔 maybe some concerns about permissions? It's possible we have a ledger_dir w/ permissions to write inside the dir, but not permissions to write in the parent dir. To me it was not obvious what the result of create_dir_all would be just from reading the docs (it's Ok if ledger_path exists).

Agree it seems better to not ignore the errors.

validator/src/main.rs

Co-authored-by: Brooks <brooks@prumo.org>

apfitzge · 2023-06-09T15:37:49Z

validator/src/main.rs

+        .pop()
+        .unwrap();


fn returns a Vec, so we gotta pop it out.
We created slice above and already checked the Result is Ok, unwrap is safe.

note's like this are better as code comments (in the future)

brooksprumo

lgtm

brooksprumo · 2023-06-09T15:42:29Z

This one needs to be backported to v1.16, yes?

steviez · 2023-06-09T15:46:30Z

validator/src/main.rs

-    });
+    let ledger_path = create_and_canonicalize_directories(&[ledger_path])
+        .unwrap_or_else(|err| {
+            eprintln!("Unable to access ledger path: {err}");


Thinking about making the logs easy to grep, it would be nice if the message contains Error. Since this is an eprintln!() instead of an error!(), what do you think about prepending Error to the existing for:

"Error: unable to access ledger path: {err}"

steviez

As for my last comment about including Error, we have a mix of these in the validator/src/main.rs; I'd say things are good to go as-is and maybe I'll do a cleanup to make all instances of eprintln!() and exit() specifically include the word Error

apfitzge · 2023-06-09T15:49:47Z

This one needs to be backported to v1.16, yes?

Added 1.16 label, now waiting on CI to merge.

Co-authored-by: Brooks <brooks@prumo.org> (cherry picked from commit 4a566ec)

…2048) create and canonicalize account_paths (#32037) Co-authored-by: Brooks <brooks@prumo.org> (cherry picked from commit 4a566ec) Co-authored-by: Andrew Fitzgerald <apfitzge@gmail.com>

create and canonicalize account_paths

3efbf08

apfitzge commented Jun 8, 2023

View reviewed changes

apfitzge added 2 commits June 8, 2023 23:18

create_and_canonicalize_directories

4157433

remove unneccessary <Vec<_>>

151f3c0

apfitzge commented Jun 8, 2023

View reviewed changes

apfitzge requested review from t-nelson, brooksprumo and steviez June 8, 2023 23:26

steviez reviewed Jun 9, 2023

View reviewed changes

validator/src/main.rs Show resolved Hide resolved

ledger_path, I choose you!

04e8f94

t-nelson previously approved these changes Jun 9, 2023

View reviewed changes

brooksprumo reviewed Jun 9, 2023

View reviewed changes

Use err Display instead of Debug

954cc4b

Co-authored-by: Brooks <brooks@prumo.org>

apfitzge dismissed t-nelson’s stale review via 954cc4b June 9, 2023 15:35

Pull ledger_path out of Vec

286a1ad

apfitzge commented Jun 9, 2023

View reviewed changes

apfitzge requested a review from brooksprumo June 9, 2023 15:38

brooksprumo approved these changes Jun 9, 2023

View reviewed changes

steviez reviewed Jun 9, 2023

View reviewed changes

steviez approved these changes Jun 9, 2023

View reviewed changes

apfitzge added the v1.16 PRs that should be backported to v1.16 label Jun 9, 2023

apfitzge merged commit 4a566ec into solana-labs:master Jun 9, 2023
4 checks passed

apfitzge deleted the gotta_canonicalize_em_all branch June 9, 2023 17:04

mergify bot pushed a commit that referenced this pull request Jun 9, 2023

create and canonicalize account_paths (#32037)

ccbd627

Co-authored-by: Brooks <brooks@prumo.org> (cherry picked from commit 4a566ec)

mergify bot mentioned this pull request Jun 9, 2023

v1.16: create and canonicalize account_paths (backport of #32037) #32048

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create and canonicalize account_paths #32037

create and canonicalize account_paths #32037

apfitzge commented Jun 8, 2023

apfitzge Jun 8, 2023

apfitzge Jun 8, 2023

apfitzge Jun 8, 2023

apfitzge Jun 8, 2023

apfitzge Jun 8, 2023

apfitzge commented Jun 8, 2023

codecov bot commented Jun 9, 2023 •

edited

Loading

t-nelson left a comment

brooksprumo Jun 9, 2023

apfitzge Jun 9, 2023

apfitzge Jun 9, 2023

t-nelson Jun 9, 2023

brooksprumo left a comment

brooksprumo commented Jun 9, 2023

steviez Jun 9, 2023

steviez left a comment

apfitzge commented Jun 9, 2023

create and canonicalize account_paths #32037

create and canonicalize account_paths #32037

Conversation

apfitzge commented Jun 8, 2023

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge commented Jun 8, 2023

codecov bot commented Jun 9, 2023 • edited Loading

Codecov Report

t-nelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brooksprumo left a comment

Choose a reason for hiding this comment

brooksprumo commented Jun 9, 2023

Choose a reason for hiding this comment

steviez left a comment

Choose a reason for hiding this comment

apfitzge commented Jun 9, 2023

codecov bot commented Jun 9, 2023 •

edited

Loading