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

bees: init at 0.6.1; nixos/modules: services.bees init #48423

Merged
merged 3 commits into from Dec 2, 2018

Conversation

Projects
None yet
6 participants
@charles-dyfis-net
Copy link
Contributor

charles-dyfis-net commented Oct 14, 2018

Motivation for this change

Introduce an extent-layer (as opposed to the existing file-level) deduplication
system for btrfs. This provides a means of finding similarities within
non-identical files, when they contain identical, aligned blocks.

Also introduces a nixos module for configuring this service.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS N/A: not portable beyond Linux
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md. Feedback would be appreciated: Does the wrapper derivation need metadata?

Show resolved Hide resolved nixos/modules/services/misc/bees.nix Outdated
Show resolved Hide resolved nixos/modules/services/misc/bees.nix Outdated
Show resolved Hide resolved nixos/modules/services/misc/bees.nix Outdated
Show resolved Hide resolved nixos/modules/services/misc/bees.nix
Show resolved Hide resolved nixos/modules/services/misc/bees.nix Outdated
Show resolved Hide resolved nixos/modules/services/misc/bees.nix Outdated
Show resolved Hide resolved nixos/modules/services/misc/bees.nix
Show resolved Hide resolved pkgs/tools/filesystems/bees/default.nix Outdated
Show resolved Hide resolved pkgs/tools/filesystems/bees/default.nix
pkgs/tools/filesystems/bees/bees-service-wrapper Outdated
fi
done

"do_$mode" options "${args[@]}"

This comment has been minimized.

@Infinisil

Infinisil Nov 19, 2018

Contributor

I don't really feel qualified to review this wrapper. Do you have maybe a link to the upstream wrapper? How much does it differ?

This comment has been minimized.

@charles-dyfis-net

charles-dyfis-net Nov 19, 2018

Author Contributor

It's pretty much a completely independent reimplementation; upstream is at https://github.com/Zygo/bees/blob/v0.6.1/scripts/beesd.in

This comment has been minimized.

@charles-dyfis-net

charles-dyfis-net Nov 19, 2018

Author Contributor

...one thing that is an open issue (with the module as well, not just the wrapper) is that it looks like bees no longer requires a power-of-2 multiplier, but now permits hash table sizes that are any multiple of 16MB.

This comment has been minimized.

@Infinisil

Infinisil Nov 19, 2018

Contributor

@charles-dyfis-net Ah, well in that case it's even easier: types.addCheck types.int (n: mod n 16 == 0) (for the nix type)

This comment has been minimized.

@charles-dyfis-net

charles-dyfis-net Nov 19, 2018

Author Contributor

Excellent. I think I've addressed everything, now, except for the matter of finding someone more comfortable reviewing the rewritten wrapper.

@charles-dyfis-net charles-dyfis-net force-pushed the charles-dyfis-net:bees branch 4 times, most recently Nov 19, 2018

@Infinisil
Copy link
Contributor

Infinisil left a comment

Some minor stuff

Show resolved Hide resolved pkgs/tools/filesystems/bees/default.nix Outdated
Show resolved Hide resolved nixos/modules/services/misc/bees.nix Outdated
Show resolved Hide resolved nixos/modules/services/misc/bees.nix Outdated
Show resolved Hide resolved nixos/modules/services/misc/bees.nix Outdated
Show resolved Hide resolved nixos/modules/services/misc/bees.nix Outdated

@charles-dyfis-net charles-dyfis-net force-pushed the charles-dyfis-net:bees branch 4 times, most recently Nov 20, 2018

@Infinisil
Copy link
Contributor

Infinisil left a comment

Looks good to me! If we don't find a reviewer soon and you feel confident with your wrapper, I won't hold back on merging this anyways.

@charles-dyfis-net charles-dyfis-net changed the title bees: init at 0.6; nixos/modules: services.bees init bees: init at 0.6.1; nixos/modules: services.bees init Nov 20, 2018

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Nov 20, 2018

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/volunteers-to-review-a-shell-wrapper-using-modern-bash-4-x-features/1477/1

1 similar comment
@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Nov 20, 2018

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/volunteers-to-review-a-shell-wrapper-using-modern-bash-4-x-features/1477/1

Show resolved Hide resolved pkgs/tools/filesystems/bees/bees-service-wrapper Outdated
Show resolved Hide resolved pkgs/tools/filesystems/bees/bees-service-wrapper Outdated
Show resolved Hide resolved pkgs/tools/filesystems/bees/bees-service-wrapper Outdated
Show resolved Hide resolved pkgs/tools/filesystems/bees/bees-service-wrapper Outdated
Show resolved Hide resolved pkgs/tools/filesystems/bees/bees-service-wrapper Outdated
@charles-dyfis-net

This comment has been minimized.

Copy link
Contributor Author

charles-dyfis-net commented Nov 20, 2018

I'm currently testing a revision that addresses the review notes above (and also takes an initial attempt at compatibility with config files written for the upstream bees service wrapper). It'll be amended and force-pushed to this PR after any changes needed to pass smoke-test have taken place, but for those curious in the interim, see 8330090bfb119e7248eb1ea6ec7ac65fa65572ca

@charles-dyfis-net charles-dyfis-net force-pushed the charles-dyfis-net:bees branch Nov 20, 2018

@charles-dyfis-net

This comment has been minimized.

Copy link
Contributor Author

charles-dyfis-net commented Nov 20, 2018

@7c6f434c, thank you for the feedback. I've tried to address it. (The latest revision also tries to be acceptable to bees upstream, parsing the config file format supported by the existing wrapper; that adds a fair bit of complexity, but hopefully the payoff is that as of bees 0.7 we'll be able to take that code out of Nix altogether).

@charles-dyfis-net charles-dyfis-net force-pushed the charles-dyfis-net:bees branch Nov 20, 2018

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 23, 2018

I have a suspicion that the eval failure of ofborg is not caused by the code in the current PR.

@charles-dyfis-net charles-dyfis-net force-pushed the charles-dyfis-net:bees branch Nov 23, 2018

@charles-dyfis-net

This comment has been minimized.

Copy link
Contributor Author

charles-dyfis-net commented Nov 23, 2018

I was going to ask your advice on running that down -- I don't know ofborg's tests well enough how to locally run the steps where the failure took place.

Rebasing onto current master, in case that helps.

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 23, 2018

Will trigger a re-eval when ofBorg fix is deployed (discussing it on IRC and in ofBorg repository now)

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 23, 2018

@charles-dyfis-net charles-dyfis-net force-pushed the charles-dyfis-net:bees branch 2 times, most recently to 3ea797b Nov 25, 2018

@charles-dyfis-net

This comment has been minimized.

Copy link
Contributor Author

charles-dyfis-net commented Nov 26, 2018

I do think a NixOS test for this is possible. TBD whether runtime will be reasonable, even with a tiny test filesystem, particularly for reliably detecting failures. Might be a win in terms of perf if we call it a success as soon as any deduplication takes place, rather than waiting for the test content to be fully deduplicated.

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 26, 2018

@charles-dyfis-net

This comment has been minimized.

Copy link
Contributor Author

charles-dyfis-net commented Nov 26, 2018

The limits placed by configuration are implicit rather than explicit, and based on limitations on resource usage permitted. We can test that the resource limits are honored, separate from trying to quantify exactly how much deduplication is done.

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 26, 2018

Well, I meant a brief check that it doesn't suddenly deduplicate another partition it was never told to touch.

(tomorrow I have a flight, so sorry for no attempt so far to re-read the script as a whole…)

@charles-dyfis-net

This comment has been minimized.

Copy link
Contributor Author

charles-dyfis-net commented Nov 26, 2018

Gotcha. Since it's one instance per filesystem, with the script mounting a distinct copy of that filesystem and passing the path to the mount point as an argument, it's hard to see how that bug could happen. We can certainly check that it's the right filesystem that was mounted.

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 26, 2018

@charles-dyfis-net

This comment has been minimized.

Copy link
Contributor Author

charles-dyfis-net commented Nov 27, 2018

It's presently a dirt-simple/minimal test that only verifies that deduplication takes place (and makes assumptions about timing/performance without any leniency/flexibility) -- but 923a3e4 proves the concept that we can test this without unreasonable delay/resource usage/etc.

@charles-dyfis-net charles-dyfis-net force-pushed the charles-dyfis-net:bees branch from 923a3e4 Nov 27, 2018

@charles-dyfis-net

This comment has been minimized.

Copy link
Contributor Author

charles-dyfis-net commented Nov 27, 2018

@7c6f434c, ...the proof-of-concept test is now replaced with something that's maybe more like what you were looking for?

@Infinisil, I made another module feature addition (allowing log levels to be specified as strings rather than numeric values only). Kept that as a separate commit -- 782dba9127bc66a737a98c865cd3bcf25b6162ea -- for ease-of-review.

@charles-dyfis-net

This comment has been minimized.

Copy link
Contributor Author

charles-dyfis-net commented Nov 28, 2018

@kakra, as someone looking at this from the bees project's perspective, are there more things you'd like to see in the system test included in this module (which is to say, as of this writing, the code in 196b48c6cd23f54cf49a04f0004127172c012154)?

@kakra

This comment has been minimized.

Copy link

kakra commented Nov 28, 2018

@charles-dyfis-net At a quick glance I like the testing approach but I didn't go into detail. But my first impression is that we should maybe see similar tests directly in make test of bees itself... But at this stage, please progress. I won't have time currently to work on such a feature in bees itself.

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Nov 28, 2018

The test looks nice in my opinion

@charles-dyfis-net

This comment has been minimized.

Copy link
Contributor Author

charles-dyfis-net commented Nov 28, 2018

Should I squash in the review-related fixes pre-merge? (Left to my own devices, I'd have this as three commits -- one for the package, one for the module, one for the test; in that order -- thus, each fulfilling the dependencies for the next).

@Infinisil

This comment has been minimized.

Copy link
Contributor

Infinisil commented Nov 30, 2018

@charles-dyfis-net Yes please

charles-dyfis-net added some commits Oct 14, 2018

bees: init at 0.6.1
Introduce an extent-layer (as opposed to the existing file-level) deduplication
system for btrfs. This provides a means of finding similarities within
non-identical files, when they contain identical, aligned blocks.

@charles-dyfis-net charles-dyfis-net force-pushed the charles-dyfis-net:bees branch to f50bfe2 Nov 30, 2018

@charles-dyfis-net

This comment has been minimized.

Copy link
Contributor Author

charles-dyfis-net commented Nov 30, 2018

@Infinisil Review changes squashed in; thanks!

@Infinisil

This comment has been minimized.

Copy link
Contributor

Infinisil commented Dec 2, 2018

Nice work, thanks!

@Infinisil Infinisil merged commit 4afae70 into NixOS:master Dec 2, 2018

9 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.