Add switches to the host init script to toggle legacy behaviour - #3139
Add switches to the host init script to toggle legacy behaviour#3139dxapd wants to merge 1 commit into
Conversation
Can we do this without having to do this at init? Can we interrogate nftables and see if our tables and chains exist, and if not, install them then? |
that can theoretically be done for setup, but for teardown, we'd need to do something similar to refcounting to reason out that it's safe to tear down all of the resources. i don't think that machinery is worth it in this case, given that the service stopping is a pretty natural hook to clean up the tables and chains. and then (IMO) once we have that teardown path, it's nice and simple to just expand it a bit to have a simple setup routine to go with it. which i guess boils down to - it's possible, but imo this is simpler and we're handed the lifecycle for free by debian itself |
Ideally, we shouldn't need the init script at all. I don't know if we need to fully refcount; for example, if I remember correctly the existing implementation checks the bridge interface's associated addresses and only tears the bridge down if it's the last. I might not be fully having all the context at hand with nftables though. If that's a bigger lift, nonetheless we should at least init dynamically and teardown statically and revisit stateless teardown later. |
|
i don't think i agree about this case; there is a handful of things that we will need to keep the init script around for (unless there is a plan to move off of the debian package that i didn't know about), so these hook points will always be there for the taking and even beyond that, it's just very simple to reason about (imo) "run the setup routine when the program is installed, then run the teardown routine when it is removed" vs. depending on each instance at runtime to have some machinery to construct its own environment statelessly. for the teardown case, there's two ways I'm familiar of:
for the init case, sure, it's possible to make that idempotent. just make the expected environment exist at init time. but is that needed, given that we have the ideal hook points available already, and we're already using one of them? dunno, this one is less defensible to me than my reasoning for teardown above, but i do think it makes it very easy to understand how the assumptions are set up. either cvdalloc is installed or it isn't. unless of course there are pitfalls to the way debian handles it that i don't know about. that aside, i do feel more strongly about teardown being run at uninstall time, just as a matter of hygiene. we're uninstalling the application, so we make sure we've cleaned house. i'd want that to be there regardless, i think. so it's not really about how much of a lift it is; i just think it's cleaner, as well as easier to understand what assumptions the allocator is operating under like this. but perhaps there's an angle to this i haven't thought about yet. |
|
Adding to @3405691582's points, part of the motivation of cvdalloc in the first place was to make the debian packages less intrusive to have installed, to make them more palatable for debian upstream. The average public user that installs the packages may be interested in launching a Cuttlefish device on occasion, but is less likely to be launching one every day. It's nicer to them to have fewer runtime system resources allocated while they're not actively engaging with our software. |
yes, i understand that, but it's not clear to me what the line between intrusive and standard is, especially since a single nftables table won't clutter any command outputs like static resources does. that said! all of this discussion got me thinking, and I'm quite optimistic that making a fresh nftables table for every instance might actually be super clean and let us delete setup and teardown modes. i think that would resolve all of our concerns simultaneously since there would be no special running modes, no state to leak during uninstall, and the logic would also be dead simple... I'll look into whether or not it's possible |
add two new flags for toggling static resource initialization as well as running cvdalloc's init (which is to be implemented in the final PR in this series).
due to the way nftables works, cvdalloc also needs to do a small amount of install-time init (namely, creating its tables and chains). that code is better off living inside the cvdalloc binary, so we have the init script, which is our one hook point into install and uninstall, invoke it at exactly those times.
this change, as is, should be a no-op. the code enclosed within the
if [ "${allocate_static_resources}" = "1" ]; thenbranches is the code we hope to delete one day, but until that day it will remain.