Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

irmin-pack: fix silent mode for integrity checks #2179

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

icristescu
Copy link
Contributor

This is a fix for #2170.

The user can ask for silent integrity checks by not providing a ppf argument. But there was a bug, the progress bars in integrity checks were not using the same ppf as the one set by the user.

Manually tested this using dune exec -- ./test/irmin-tezos/irmin_fsck.exe integrity-check ....

@icristescu icristescu changed the title irmin-pack: fix ppf for integrity checks irmin-pack: fix silent mode for integrity checks Jan 25, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2023

Codecov Report

Merging #2179 (abb524d) into main (ff45092) will increase coverage by 0.00%.
The diff coverage is 50.00%.

❗ Current head abb524d differs from pull request most recent head b964da8. Consider uploading reports for the commit b964da8 to get more accurate results

@@           Coverage Diff           @@
##             main    #2179   +/-   ##
=======================================
  Coverage   68.07%   68.07%           
=======================================
  Files         134      134           
  Lines       16161    16164    +3     
=======================================
+ Hits        11001    11004    +3     
  Misses       5160     5160           
Impacted Files Coverage Δ
src/irmin-pack/inode.ml 78.96% <0.00%> (ø)
src/irmin-pack/unix/async.ml 56.60% <40.00%> (+2.75%) ⬆️
src/irmin-pack/unix/checks.ml 23.28% <66.66%> (+0.22%) ⬆️
src/irmin-pack/unix/utils.ml 92.30% <100.00%> (+0.30%) ⬆️
src/irmin-fs/unix/irmin_fs_unix.ml 67.74% <0.00%> (-0.65%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@metanivek
Copy link
Member

Thanks for fixing this! It's nice we don't have to introduce a new parameter.

Since NL is currently using Integrity_check.run (https://gitlab.com/tezos/tezos/-/blob/master/src/bin_node/node_storage_command.ml#L127), I think we need to add the ppf as an optional parameter to this function as well. Currently run hard codes the formatter:

let* result =
Store.integrity_check ~ppf:Format.err_formatter ~auto_repair ~heads repo
in

We could then update

Lwt_main.run (run ~root ~auto_repair ~always ~heads))
to pass ~ppf:Fmt.err_formatter.

@icristescu
Copy link
Contributor Author

Since NL is currently using Integrity_check.run (https://gitlab.com/tezos/tezos/-/blob/master/src/bin_node/node_storage_command.ml#L127), I think we need to add the ppf as an optional parameter to this function as well.

Thanks for the suggestion, done.

Copy link
Member

@metanivek metanivek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

@metanivek metanivek merged commit 73f33c1 into mirage:main Jan 31, 2023
@irmaTS irmaTS added the tezos-support Support for bugs related to Tezos label Feb 24, 2023
metanivek added a commit to metanivek/opam-repository that referenced this pull request Apr 21, 2023
…min-pack, irmin-pack-tools, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.7.0)

CHANGES:

### Added

- **irmin**
  - Add `Conf.pp` and `Conf.equal` to print and compare configuration values
    (mirage/irmin#2227, @samoht)
  - Add a `clear` optional arguments to all function that adds a new commit:
    `Commit.v`, `set`, `set_tree`, `remove`, `test_and_set`,
    `test_and_set_tree`, `test_set_and_get`, `test_set_and_get_tree`, `merge`,
    `merge_tree` and `with_tree`. This new argument allows to control whether
    the tree caches are cleared up after objects are exported to disk during
    the commit. (mirage/irmin#2225, @samoht)

- **irmin-pack**
  - Add configuration option, `lower_root`, to specify a path for archiving data
    during a GC. (mirage/irmin#2177, @metanivek)
  - Add `is_split_allowed` to check if a store allows split. (mirage/irmin#2175, @metanivek)
  - Add `add_volume` to allow creating new empty volume in lower layer. (mirage/irmin#2188,
    @metanivek)
  - Add a `behaviour` function to the GC to check wether the GC will archive or
    delete data. (mirage/irmin#2190, @Firobe)
  - Add a migration on `open_rw` to move the data to the `lower_root` if
    the configuration was enabled (mirage/irmin#2205, @art-w)

### Changed

- **irmin**
  - Expose type equality for `Schema.Info` to avoid defining the `info` function
    multiple times when using similar stores (mirage/irmin#2189, mirage/irmin#2193, @samoht)
- **irmin-pack**
  - GC now changes its behaviour depending on the presence of a lower layer.
    (mirage/irmin#2190, @Firobe)
  - Split now raises an exception if it is not allowed. It is not allowed on
    stores that do not allow GC. (mirage/irmin#2175, @metanivek)
  - GC now supports stores imported V1/V2 stores, in presence of a lower layer
    only. (mirage/irmin#2190, @art-w, @Firobe)
  - Upgrade on-disk format to version 5. (mirage/irmin#2184, @metanivek)
  - Archive to lower volume does not copy orphaned commits. (mirage/irmin#2215, @art-w)

### Fixed
- **irmin-pack**
  - Unhandled exceptions in GC worker process are now reported as a failure
    (mirage/irmin#2163, @metanivek)
  - Fix the silent mode for the integrity checks. (mirage/irmin#2179, @icristescu)
  - Fix file descriptor leak caused by `mmap`. (mirage/irmin#2232, @art-w)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tezos-support Support for bugs related to Tezos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants