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

charles-dyfis-net
Copy link
Contributor

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?

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 Show resolved Hide resolved
fi
done

"do_$mode" options "${args[@]}"
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...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.

Copy link
Member

@infinisil infinisil Nov 19, 2018

Choose a reason for hiding this comment

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

@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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Some minor stuff

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 Show resolved Hide resolved
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

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
Copy link

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
Copy link

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

@charles-dyfis-net
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
Copy link
Contributor Author

@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).

@7c6f434c
Copy link
Member

My vision is now a blur and I cannot firmly determine whether I am reading the current version of the wrapper script or rewinding my memory of a previous version…

@7c6f434c
Copy link
Member

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

@charles-dyfis-net
Copy link
Contributor Author

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
Copy link
Member

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

@7c6f434c
Copy link
Member

@GrahamcOfBorg eval

@charles-dyfis-net
Copy link
Contributor Author

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
Copy link
Member

7c6f434c commented Nov 26, 2018 via email

@charles-dyfis-net
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

7c6f434c commented Nov 26, 2018 via email

@charles-dyfis-net
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
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
Copy link
Member

The test looks nice in my opinion

@charles-dyfis-net
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
Copy link
Member

@charles-dyfis-net Yes please

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
Copy link
Contributor Author

@infinisil Review changes squashed in; thanks!

@infinisil
Copy link
Member

Nice work, thanks!

@infinisil infinisil merged commit 4afae70 into NixOS:master Dec 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants