Add `--halt-after-import` flag to CLI #1835

connormullett · 2022-08-17T17:33:31Z

Summary of changes
Changes introduced in this pull request:

Add ability to import a snapshot and immediately exit after completion
Small improvements to RocksDb configuration ergonomics with set_auto_disable_compactions
Add log level from string for RocksDb for testing and debugging purposes

Reference issue to close (if applicable)

Other information and links

elmattic · 2022-08-17T18:11:08Z

forest/src/cli/client.rs

@@ -23,6 +23,7 @@ pub struct Client {
    pub snapshot: bool,
    pub snapshot_height: Option<i64>,
    pub snapshot_path: Option<String>,
+    pub exit_after_import: bool,


Would be great to use the same wording as in Lotus which is --halt-after-import.

https://lotus.filecoin.io/lotus/manage/chain-management/#restoring-a-custom-snapshot

Most of Forest cli args are already similar to theirs.

Oh, I didn't know this was a thing. I'll rename it

lemmih · 2022-08-19T08:55:31Z

forest/src/daemon.rs

+
+        if config.client.halt_after_import {
+            info!("Forest finish shutdown");
+            process::exit(0);


@elmattic Do you think it'll cause problems with RocksDB if we call exit(0) here? Do we need to properly shutdown the database?

Sure we should avoid this and this is possible to do (from my experience, the main source of resource leaks are situated after the import).

But more problematic with this PR is that the code exits before import_chain in case of user --halt-after-importing, resulting in a no-op. We need to do it after the import_chain.

Plus we also need to run this code:

match chain_store.heaviest_tipset().await { ...

This is for checking that the user is not importing too old snapshots.

If you do it just after:

// Halt if config.client.halt_after_import { info!("Forest finished import"); return; }

this should terminate forest properly. Also make sure you check rocksdb LOG file to verify that it closed properly the db (you need to enable rocksdb debug log level).

I think we could also promote this log debug!("Imported snapshot in: {}s", ... to an info! one.

This is useful information to know for troubleshooting, and it won't really bloat our logs.

elmattic · 2022-08-19T10:11:43Z

forest/src/cli/mod.rs

@@ -143,6 +143,8 @@ pub struct CliOpts {
    pub height: Option<i64>,
    #[structopt(long, help = "Import a snapshot from a local CAR file or url")]
    pub import_snapshot: Option<String>,
+    #[structopt(long)]
+    pub halt_after_import: bool,


Please add some doc comment help for this new flag. Consider refactoring to use doc comments for the other flags too (this will gives us documentation like IDE tooltip and spell checking for free).

@lemmih do you think such self-describing flags need help comments?

Yes. The help message could even add more details such as "Halt with exit code 0 after successfully importing a snapshot".

elmattic · 2022-08-22T14:17:57Z

forest/src/cli/mod.rs

@@ -143,6 +143,11 @@ pub struct CliOpts {
    pub height: Option<i64>,
    #[structopt(long, help = "Import a snapshot from a local CAR file or url")]
    pub import_snapshot: Option<String>,
+    #[structopt(
+        long,
+        help = "Halt with exit code 0 after successfully importing a snapshot"


You can put this into a doc comment like this:

/// Halt with exit code 0 after successfully importing a snapshot #[structopt(long)] pub halt_after_import: bool,

elmattic · 2022-08-22T16:38:07Z

node/db/src/rocks_config.rs

            compression_type: Some("lz4".into()),
            enable_statistics: false,
+            log_level: "debug".into(),


Don't activate debug log by default here. What do you think of warn?

That makes more sense for a default, yeah

lemmih · 2022-08-23T08:32:51Z

node/db/src/rocks.rs

+            db_opts.set_disable_auto_compactions(false);
+        } else {
+            db_opts.set_disable_auto_compactions(true);


This appears to be unrelated to the --halt-after-import flag. Could you explain the RocksDB changes in the PR description?

You're right, this was part of testing to see if RocksDb was being dropped after import. I've left some of the configuration logic in as it was part of @elmattic 's work

lemmih

Tentative approval. The RocksDB changes are probably fine but I think they're unrelated to the original goal of this PR. Please describe what you've changed and why in the PR description.

* add flag to cliopts * and options to config * move exit logig to sync_from_snapshot * rename to match lotus cli * changes for testing; move halt to after import chain * verify that rocksdb is dropped * requested changes and remove testing logs * move comment * default rocks log level debug -> warn * fix workflow errors

connormullett added 3 commits August 17, 2022 13:27

add flag to cliopts

4080e47

Merge branch 'main' into connor/import-and-exit-flag

0330ea1

and options to config

2510da2

elmattic reviewed Aug 17, 2022

View reviewed changes

connormullett added 2 commits August 17, 2022 15:13

move exit logig to sync_from_snapshot

3b8c3db

rename to match lotus cli

01fbdd3

connormullett marked this pull request as ready for review August 18, 2022 15:31

connormullett requested review from lemmih, LesnyRumcajs, jdjaustin and tyshko5 as code owners August 18, 2022 15:31

lemmih changed the title ~~Add --import-snapshot-and-exit flag to CLI~~ Add --halt-after-import flag to CLI Aug 19, 2022

lemmih reviewed Aug 19, 2022

View reviewed changes

elmattic reviewed Aug 19, 2022

View reviewed changes

changes for testing; move halt to after import chain

b59bce4

elmattic reviewed Aug 22, 2022

View reviewed changes

connormullett added 3 commits August 22, 2022 11:35

verify that rocksdb is dropped

c75fd32

requested changes and remove testing logs

4c6a1ba

move comment

ebec6f5

elmattic reviewed Aug 22, 2022

View reviewed changes

connormullett added 3 commits August 22, 2022 14:53

default rocks log level debug -> warn

f65827f

Merge branch 'main' into connor/import-and-exit-flag

9bacd9d

fix workflow errors

512395f

connormullett requested review from lemmih and elmattic August 22, 2022 19:31

lemmih reviewed Aug 23, 2022

View reviewed changes

lemmih approved these changes Aug 23, 2022

View reviewed changes

jdjaustin approved these changes Aug 23, 2022

View reviewed changes

connormullett merged commit 314baef into main Aug 23, 2022

connormullett deleted the connor/import-and-exit-flag branch August 23, 2022 16:51

elmattic mentioned this pull request Aug 29, 2022

Misleading RocksDb configuration #1791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--halt-after-import` flag to CLI #1835

Add `--halt-after-import` flag to CLI #1835

connormullett commented Aug 17, 2022 •

edited

Loading

elmattic Aug 17, 2022

connormullett Aug 17, 2022

lemmih Aug 19, 2022

elmattic Aug 19, 2022 •

edited

Loading

elmattic Aug 19, 2022

elmattic Aug 19, 2022 •

edited

Loading

elmattic Aug 19, 2022 •

edited

Loading

lemmih Aug 19, 2022

elmattic Aug 22, 2022

elmattic Aug 22, 2022

connormullett Aug 22, 2022

lemmih Aug 23, 2022

connormullett Aug 23, 2022

lemmih left a comment

Add --halt-after-import flag to CLI #1835

Add --halt-after-import flag to CLI #1835

Conversation

connormullett commented Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmattic Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmattic Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

elmattic Aug 19, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemmih left a comment

Choose a reason for hiding this comment

Add `--halt-after-import` flag to CLI #1835

Add `--halt-after-import` flag to CLI #1835

connormullett commented Aug 17, 2022 •

edited

Loading

elmattic Aug 19, 2022 •

edited

Loading

elmattic Aug 19, 2022 •

edited

Loading

elmattic Aug 19, 2022 •

edited

Loading