From 533bb8c684beedba673975ec68b7d393d04b4042 Mon Sep 17 00:00:00 2001 From: Victor Allombert Date: Mon, 24 Oct 2022 09:57:10 +0200 Subject: [PATCH 1/4] Store/snasphot: fix cleanup while cancelling during export --- src/lib_store/unix/snapshots.ml | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/lib_store/unix/snapshots.ml b/src/lib_store/unix/snapshots.ml index a5c6cb9a65d5..c83de199ecb8 100644 --- a/src/lib_store/unix/snapshots.ml +++ b/src/lib_store/unix/snapshots.ml @@ -1797,6 +1797,8 @@ module Tar_exporter : EXPORTER = struct return_unit let cleaner ?to_clean t = + let open Lwt_syntax in + let* () = Event.(emit cleaning_after_failure ()) in let paths = match to_clean with | Some paths -> paths @@ -2491,11 +2493,7 @@ module Make_snapshot_exporter (Exporter : EXPORTER) : Snapshot_exporter = struct Lwt.return_unit) in let* metadata = - protect - ~on_error:(fun errors -> - let*! () = Exporter.cleaner snapshot_exporter in - Lwt.return (Error errors)) - (fun () -> + protect (fun () -> let* ( export_mode, export_block, pred_block, -- GitLab From f21fad12f6e074aeab9f40a3d85d3fc69cc5b380 Mon Sep 17 00:00:00 2001 From: Victor Allombert Date: Mon, 24 Oct 2022 15:52:57 +0200 Subject: [PATCH 2/4] Store: clean temporary files before exporting snapshots --- src/lib_store/unix/snapshots.ml | 32 ++++++++++++++++++-------- src/lib_store/unix/snapshots.mli | 2 +- src/lib_store/unix/snapshots_events.ml | 9 ++++++++ 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/src/lib_store/unix/snapshots.ml b/src/lib_store/unix/snapshots.ml index c83de199ecb8..5ef5af218b7f 100644 --- a/src/lib_store/unix/snapshots.ml +++ b/src/lib_store/unix/snapshots.ml @@ -77,7 +77,7 @@ type error += | Target_block_validation_failed of Block_hash.t * string | Directory_already_exists of string | Empty_floating_store - | Cannot_create_tmp_export_directory of string + | Cannot_remove_tmp_export_directory of string | Inconsistent_version_import of {expected : int list; got : int} | Inconsistent_chain_import of { expected : Distributed_db_version.Name.t; @@ -440,18 +440,19 @@ let () = (fun () -> Empty_floating_store) ; register_error_kind `Permanent - ~id:"snapshots.cannot_create_tmp_export_directory" - ~title:"Cannot create temporary export directory" + ~id:"snapshots.cannot_remove_tmp_export_directory" + ~title:"Cannot remove temporary export directory" ~description:"Cannot create temporary directory for exporting snapshot." ~pp:(fun ppf msg -> Format.fprintf ppf "Cannot export snapshot: the temporary snapshot directory already \ - exists. Please remove %s and restart the snapshot export." + exists and cannot be removed. Please remove %s and restart the \ + snapshot export." msg) (obj1 (req "message" string)) - (function Cannot_create_tmp_export_directory str -> Some str | _ -> None) - (fun str -> Cannot_create_tmp_export_directory str) ; + (function Cannot_remove_tmp_export_directory str -> Some str | _ -> None) + (fun str -> Cannot_remove_tmp_export_directory str) ; register_error_kind `Permanent ~id:"snapshots.inconsistent_version_import" @@ -769,9 +770,22 @@ let default_snapshot_filename metadata = else default_name let ensure_valid_tmp_snapshot_path snapshot_tmp_dir = - fail_when - (Sys.file_exists (Naming.dir_path snapshot_tmp_dir)) - (Cannot_create_tmp_export_directory (Naming.dir_path snapshot_tmp_dir)) + let open Lwt_result_syntax in + let path = Naming.dir_path snapshot_tmp_dir in + let exists = Sys.file_exists path in + if exists then + Lwt.catch + (fun () -> + let*! () = Event.(emit cleaning_tmp_export_directory) path in + let*! () = Lwt_utils_unix.remove_dir path in + return_unit) + (function + | _ -> + fail_when + exists + (Cannot_remove_tmp_export_directory + (Naming.dir_path snapshot_tmp_dir))) + else return_unit let ensure_valid_export_path = let open Lwt_result_syntax in diff --git a/src/lib_store/unix/snapshots.mli b/src/lib_store/unix/snapshots.mli index ab0a00270ffe..2642e94fd20c 100644 --- a/src/lib_store/unix/snapshots.mli +++ b/src/lib_store/unix/snapshots.mli @@ -131,7 +131,7 @@ type error += | Target_block_validation_failed of Block_hash.t * string | Directory_already_exists of string | Empty_floating_store - | Cannot_create_tmp_export_directory of string + | Cannot_remove_tmp_export_directory of string | Inconsistent_chain_import of { expected : Distributed_db_version.Name.t; got : Distributed_db_version.Name.t; diff --git a/src/lib_store/unix/snapshots_events.ml b/src/lib_store/unix/snapshots_events.ml index 826f374250b0..994ebc65c574 100644 --- a/src/lib_store/unix/snapshots_events.ml +++ b/src/lib_store/unix/snapshots_events.ml @@ -54,6 +54,15 @@ module Event = struct ~pp1:Format.pp_print_string ("filename", Data_encoding.string) + let cleaning_tmp_export_directory = + declare_1 + ~section + ~level:Notice + ~name:"cleaning_tmp_export_directory" + ~msg:"cleaning leftover export directory '{filename}'" + ~pp1:Format.pp_print_string + ("filename", Data_encoding.string) + let import_info = let option_pp ~default pp fmt = function | None -> Format.fprintf fmt "%s" default -- GitLab From 0916518bfdb92eedc576241be6c472e735be4c55 Mon Sep 17 00:00:00 2001 From: Victor Allombert Date: Wed, 26 Oct 2022 16:15:20 +0200 Subject: [PATCH 3/4] Store/snasphot: fix cleanup while cancelling during import --- src/bin_node/node_snapshot_command.ml | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/bin_node/node_snapshot_command.ml b/src/bin_node/node_snapshot_command.ml index de19c9d02d9e..fc6d0a22af18 100644 --- a/src/bin_node/node_snapshot_command.ml +++ b/src/bin_node/node_snapshot_command.ml @@ -202,14 +202,19 @@ module Term = struct let* snapshot_path = check_snapshot_path snapshot_path in let dir_cleaner () = let*! () = Event.(emit cleaning_up_after_failure) data_dir in - if existing_data_dir then - (* Remove only context and store if the import directory - was previously existing. *) - let*! () = - Lwt_utils_unix.remove_dir (Node_data_version.store_dir data_dir) - in - Lwt_utils_unix.remove_dir (Node_data_version.context_dir data_dir) - else Lwt_utils_unix.remove_dir data_dir + let*! () = + if existing_data_dir then + (* Remove only context and store if the import directory + was previously existing. *) + let*! () = + Lwt_utils_unix.remove_dir (Node_data_version.store_dir data_dir) + in + Lwt_utils_unix.remove_dir (Node_data_version.context_dir data_dir) + else Lwt_utils_unix.remove_dir data_dir + in + let lock_file = Node_data_version.lock_file data_dir in + let*! lock_file_exists = Lwt_unix.file_exists lock_file in + if lock_file_exists then Lwt_unix.unlink lock_file else Lwt.return_unit in let* () = Node_data_version.ensure_data_dir ~mode:Is_bare data_dir in (* Lock only on snapshot import *) -- GitLab From 6855cdd6900d9c1fbd6133bae374c01c5414a0c7 Mon Sep 17 00:00:00 2001 From: Victor Allombert Date: Mon, 24 Oct 2022 15:58:04 +0200 Subject: [PATCH 4/4] Changelog: clean tmp/lock files while importing/exporting snapshots --- CHANGES.rst | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index 7cfe8a59e18c..5699b4540d68 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -31,6 +31,18 @@ Node can still be used to override the data directory location from the configuration file, whether it is specified from the command-line or not. +- Fixed a bug that caused the ``snapshot import`` command to fail when + used on data directories configured with an explicit number + additional cycles. + +- Fixed an issue that could left a temporary directory if a snapshot + export was cancelled. Additionally, a cleanup now ensures the + absence of leftovers temporary directories when exporting a + snapshot. + +- Fixed an issue that could left a lock file if a snapshot import was + cancelled. + Client ------ -- GitLab