[chore] minor cleanup #763
Conversation
jprendes
force-pushed
the
cleanup
branch
2 times, most recently
from
6aa0035 to
f405102
Compare
|
Hold on merging this PR till I finish releasing the shim binaries today! |
| @@ -64,7 +65,7 @@ impl<E: Engine> SandboxInstance for Instance<E> { | |||
| Ok(Self { | |||
| id, | |||
| exit_code: WaitableCell::new(), | |||
| rootdir, | |||
| container: RwLock::new(container), | |||
There was a problem hiding this comment.
question: Why is a lock required for container?
There was a problem hiding this comment.
the operations we do on the container (start, kill, delete...) require mutable access, while we are only receiving an immutable reference. That's why we need the lock.
As to what we were doing before, the container object reads and write the state to disk. We were loading the container from disk. There's nothing enforcing synchronization at the filesystem level, but these operations should indeed be synchronized. With the previous version we could have had two mutable Container instances simultaneously trying to write different states to the same files.
There was a problem hiding this comment.
Is there a scenario where that would happen when this is driven by containerd?
There was a problem hiding this comment.
I don't know. @cpuguy83 , do you know the answer?
In any case, I would expect the lock to be cheaper than loading and parsing the state from disk.
There was a problem hiding this comment.
It looks like there is a pending question here. I agree with @jprendes that this PR is a net gain for the perf, so I am happy to merge this to the main tree.
There was a problem hiding this comment.
Its probably faster in the startup, maybe a bit more memory consumption 🤷
Signed-off-by: Jorge Prendes <[email protected]>
This PR reuses the container returned by
libcontainerinstead of reloading it from disk every time.Note: some of the functions removed here were part of the public API, probably incorrectly so.