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
Conversation
7f8d1dd
to
f52a290
Compare
fi | ||
done | ||
|
||
"do_$mode" options "${args[@]}" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
0739e5d
to
46e64d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor stuff
44a733e
to
736680c
Compare
There was a problem hiding this 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.
This pull request has been mentioned on Nix community. There might be relevant details there: |
1 similar comment
This pull request has been mentioned on Nix community. There might be relevant details there: |
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 |
736680c
to
4f4a1c8
Compare
@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). |
4f4a1c8
to
daa9df0
Compare
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… |
I have a suspicion that the eval failure of ofborg is not caused by the code in the current PR. |
76444b1
to
1080751
Compare
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. |
Will trigger a re-eval when ofBorg fix is deployed (discussing it on IRC and in ofBorg repository now) |
@GrahamcOfBorg eval |
4b5eaea
to
3ea797b
Compare
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. |
Might be a win in terms of perf if we call it a success as soon as any deduplication takes place
As long as we don't want to check it doesn't deduplicate more than it should according to configuration,
|
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. |
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…) |
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. |
Well, it would be a check that the correct FS is being mounted, and that upstream didn't quietly change the configuration logic completely.
|
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. |
923a3e4
to
196b48c
Compare
@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. |
@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)? |
@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 |
The test looks nice in my opinion |
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). |
@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.
7fa9a9c
to
f50bfe2
Compare
@infinisil Review changes squashed in; thanks! |
Nice work, thanks! |
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
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)