internal/filesystem consolidation, part 3 (no rename WriteFile, disaster recovery and rollbacks) #4324
Conversation
apostasie
force-pushed
the
2025-06-fs-3
branch
2 times, most recently
from
1d98065 to
f315d4a
Compare
apostasie
force-pushed
the
2025-06-fs-3
branch
3 times, most recently
from
1dd7373 to
de499d0
Compare
apostasie
changed the title
apostasie
changed the title
apostasie
changed the title
|
No merge conflict, but could you rebase just in case |
|
|
||
| import "github.com/containerd/nerdctl/v2/pkg/internal/filesystem" | ||
|
|
||
| func InitFS(path string) error { |
There was a problem hiding this comment.
Needs a explanation comment
|
Rebased and comment added. |
|
Comment seems still missing |
My bad. Added it to the call: https://github.com/containerd/nerdctl/pull/4324/files#diff-202c9b24bcda33888c16a71750bd47809ffd474e126bfd2be11857033c0272f0R149 Will add more to the func declaration then. |
WriteFile now: - no longer relies on Rename, hence no longer changing files inodes - have disaster recovery and rollback Signed-off-by: apostasie <spam_blackhole@farcloser.world>
|
CI failures:
|
Added expanded comment to function declaration (along with comment on function call). |
| err := ensureRecovery(fp) | ||
| assert.NilError(t, err) | ||
|
|
||
| cn, _ := os.ReadFile(fp) |
There was a problem hiding this comment.
Any reason not to check err?
| // Create file | ||
| dir := t.TempDir() | ||
| fp := filepath.Join(dir, "pre-existing-file") | ||
| _ = os.WriteFile(fp, []byte("original content"), 0o600) |
There was a problem hiding this comment.
Any reason not to check err?
There was a problem hiding this comment.
It is indeed a bit lazy - though, given the (test) codepath, if this is failing, presumably everything is failing...
I see you merged.
If this is important, I can update in a follow on.
This is part 3 of internal/FS series.
<!> unlike the previous PRs that were essentially consolidation and enforcing atomic writes, this here is more ambitious, and adds net new features (eg: rollbacks and disaster recovery) - as such, it may be more controversial, so, open to discussion here, as usual <!>
This expands on the previous goal (Atomicity and Durability of
WriteFile) by providing a new version ofWriteFilethat no longer relies on file renames - hence no longer changing inode - making it now 100% compatible withos.WriteFileand usable everywhere (including when files are mounted).Furthermore, it also focuses on enabling Consistency, by introducing:
WriteFilefails, the file is immediately reverted to its previous version (or removed if it did not exist prior)WriteFileis hard interrupted for any reason, the nextReadFileorWriteFilecall will first revert it to its original content (or remove it if it did not exist)WriteFileWithRollbackwill return a rollback function that can revert changesrollbackshould be used in the future to ensure higher-level operations consistency and to facilitate cleanup in case of failures. This should make higher-level operations failures able to revert state / cleanup leftover easier.I do appreciate this is additional complexity, and some of the code is not trivial.
But then:
container.Createand other places that are manually rollbacking in a baroque / ad-hoc formFinally note that this changeset is purely internal to the
filesystempackage and does not change anything in the rest of nerdctl codebase.Let me know your thoughts overall, and if this is desirable or not.