Fix VM network clone IP calculation and consolidate cleanup #6461
Conversation
bduffany
force-pushed
the
fix-vm-networking
branch
3 times, most recently
from
eb98f8d
to
fb57b6d
Compare
bduffany
changed the title
bduffany
force-pushed
the
fix-vm-networking
branch
from
fb57b6d
to
b64b3b9
Compare
| // this func that needs to be explicitly cleaned up, we append a cleanup | ||
| // task to this list. Cleanup work is done in the reverse order in which | ||
| // it's added to this list (i.e., it's a stack). | ||
| var cleanupStack []func(ctx context.Context) error |
There was a problem hiding this comment.
I find this logic a bit complex to follow. For example it takes some mental work to figure out the order the cleanup tasks are executing (i.e. "if I want to cleanup the route before I unlock the net idx, when should I add it to the slice?") I would personally find it a bit more readable to use booleans
Ex.
if shouldCleanupRoute {}
if shouldCleanupRule {}
There was a problem hiding this comment.
I did it this way because it keeps the cleanup logic close to the creation logic, which gives me more peace of mind, similar to how I expect to see a defer Unlock() after a Lock(). Also, similar to defer, it provides a natural "reverse-dependency-ordering" execution order - if A depends on B then that means A will get created before B, which means A's cleanup func will also be registered before B's, which means it'll execute after B's, which is what we want.
Overall I feel like this makes the code more robust, but let me know if you feel strongly about the readability aspect. I feel like with booleans, it's another potential source of bugs, because we have to explicitly keep track of the dependencies between the resources whereas the stack naturally does the cleanup in reverse dependency order.
| // If we return an error from this func then we need to clean up any | ||
| // resources that were created before returning. | ||
| defer func() { | ||
| if err != nil { |
There was a problem hiding this comment.
When err is shadowed - i.e. hostEndpointNet, err := hostNetAllocator.Get(vmIdx) - does this still work correctly?
There was a problem hiding this comment.
Yeah - in that example, err is not being declared in a new scope (shadowed), it's being re-assigned in the top-level function body scope. The declaration happens in the named return value.
I was on the fence about whether to do this vs using a wrapper func but I think ultimately the wrapper func is more error prone because we have to remember to return cleanup, err instead of return nil, err everywhere in this func.
There was a problem hiding this comment.
Ah okay. I wasn't sure if a new err variable was being created, but it seems like it won't, and only the value will get reassigned (https://go.dev/ref/spec#Short_variable_declarations)
| // If we return an error from this func then we need to clean up any | ||
| // resources that were created before returning. | ||
| defer func() { | ||
| if err != nil { |
There was a problem hiding this comment.
Ah okay. I wasn't sure if a new err variable was being created, but it seems like it won't, and only the value will get reassigned (https://go.dev/ref/spec#Short_variable_declarations)
Instead of using
vmIdxfor the clone IP, we want to use thenetIdxwhich we have locked. ThenetIdxis different from thevmIdxin the case where thevmIdxis already in use.This also restructures the cleanup work that needs to be done after calling
SetupVethPair. Because the clone IP depends on thenetIdxand not thevmIdx, some of the current cleanup work that is done infirecracker.gowould now need to know about thenetIdx. But this feels a bit like implementation details leaking out. So, in order to avoid exposing thenetIdxand having to plumb it around, this PR makes it so that the cleanup func already returned bySetupVethPairnow also does this cleanup work. This makes the networking API a bit simpler since more of the cleanup work is done automatically.Related issues: N/A