nixos/vpp: init module #347797
nixos/vpp: init module #347797
Conversation
github-actions
bot
added
6.topic: nixos
romner-set
force-pushed
the
vpp-service
branch
2 times, most recently
from
3cad8ad to
e0dc519
Compare
github-actions
bot
removed
8.has: documentation
romner-set
force-pushed
the
vpp-service
branch
2 times, most recently
from
8a3d670 to
35dd879
Compare
ofborg
bot
added
8.has: package (new)
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4668 |
romner-set
force-pushed
the
vpp-service
branch
from
35dd879 to
b38f26f
Compare
h7x4
added
8.has: module (new)
romner-set
force-pushed
the
vpp-service
branch
4 times, most recently
from
ef5b42f to
c93df1d
Compare
github-actions
bot
removed
the
8.has: maintainer-list (update)
romner-set
force-pushed
the
vpp-service
branch
from
c93df1d to
714f7cf
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4895 |
romner-set
force-pushed
the
vpp-service
branch
2 times, most recently
from
9da97c6 to
047ca1c
Compare
ofborg
bot
added
the
2.status: merge conflict
romner-set
force-pushed
the
vpp-service
branch
from
047ca1c to
1f33c2e
Compare
romner-set
removed
the
2.status: merge conflict
romner-set
force-pushed
the
vpp-service
branch
from
1f33c2e to
339293e
Compare
|
|
||
| # make sure VPP initialized correctly | ||
| # the sleep is necessary since startupConfig executes *after* the service starts | ||
| router.sleep(2) |
There was a problem hiding this comment.
This could easily fail on hydra. Is it not enough to wait for the socket above?
There was a problem hiding this comment.
VPP reads startupConfig right after actually starting up, the 4 commands should execute near-instantaneously so I don't think it'll be an issue with 2s. It does feel like a bit of a hack, but I can't come up with any better solution.
| gid = config.services.vpp.instances.\${name}.group; | ||
| }; | ||
| ''; | ||
| example = lib.literalExpression '' |
There was a problem hiding this comment.
We should try to condense this, too.
There was a problem hiding this comment.
Removed a bunch of unnecessary options, is this short enough? I'd prefer keeping the comment in there since I think it's pretty useful.
| autoSetup = mkOption { | ||
| default = false; | ||
| example = true; | ||
| description = '' | ||
| Whether to automatically setup hugepages for use with FD.io's Vector Packet Processor. | ||
|
|
||
| If any programs other than VPP use hugepages or you want to use 1GB hugepages, it's recommended | ||
| to keep this `false` and set them up manually using {option}`boot.kernel.sysctl` or {option}`boot.kernelParams`. | ||
| ''; | ||
| }; |
There was a problem hiding this comment.
For example redis sets this up. Would this work together to make things easy for end users?
There was a problem hiding this comment.
That I don't know, depends on how exactly VPP and redis use explicit hugepages. The autosetup is only here since VPP requires hugepages to even start up properly, and using it is significantly easier than having to add all the sysctls for a trivial deployment. More complicated setups like having 2 services that use hugepages are out of scope for this option & indicated as such in the description.
romner-set
force-pushed
the
vpp-service
branch
from
339293e to
3cf1645
Compare
|
@TheRealGramdalf @SuperSandro2000 Thanks for the reviews! Integrated most of the suggestions, commented on the rest. |
|
No problem! I still have some thoughts but I'm busy for a while, I'll have to take a look at the end of today or possibly tomorrow. |
There was a problem hiding this comment.
Mostly some more nitpicks about with, I think the descriptions are much more concise now. I would be content to leave the descriptions as is for now and see how they look rendered on search/in the manual, making further adjustments if necessary - I'm not sure how or if you could render those locally for testing.
I'm mostly trying to make things as clean as possible, since having readable code makes it much easier to debug and wrap your head around, since most repeated data is redundant - if I have an option with type = types.str, the types. doesn't convey any more valuable information. And since it's using inherit, it's still easy enough to tell where str came from since it's defined explicitly at the top of the file.
|
Sorry it took me a while to get to this review, I've been fairly busy recently. Given that, don't feel the need to wait for further review/hold up this PR on my account. |
romner-set
force-pushed
the
vpp-service
branch
from
3cf1645 to
8893101
Compare
romner-set
force-pushed
the
vpp-service
branch
2 times, most recently
from
115d59b to
1c707ec
Compare
romner-set
force-pushed
the
vpp-service
branch
from
1c707ec to
d505396
Compare
Description of changes
Module & associated tests for #346281.
Includes an RFC42
settingsoption using a custom parser based on znc's and support for multiple instances similar to authelia's module. The test is pretty basic and verifies routing functionality between 2 hosts with VPP acting as the router.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.