fix: ensure containerd-related directories removed on failed bootstrap/join-cluster.
#863
Conversation
aznashwan
force-pushed
the
containerd-bootstrap-cleanup
branch
5 times, most recently
from
1f725fc
to
773eb93
Compare
…ap/join-cluster`. Signed-off-by: Nashwan Azhari <[email protected]>
aznashwan
force-pushed
the
containerd-bootstrap-cleanup
branch
from
773eb93
to
fa16a12
Compare
| continue | ||
| } | ||
|
|
||
| // Ensure lockfile's contents is the one we expect: |
There was a problem hiding this comment.
What does this check ensure for us? Is it to check someone else did not create the locks?
There was a problem hiding this comment.
I admit I have no concrete usecase where this check would ever be relevant, it just "felt right" to double-check in case someone changed their contents from what the k8s-snap expects.
Note that, as far as I can tell, the k8s-snap does NOT use the paths written within the lockfiles (considering they're hardcoded/derived within the k8s-snap itself, it has no need for them).
The only place I've seen the actual file contents read is in the remove hook of the snap.
I could gladly remove it if it's "too redundant" or switch the k8s-snap to always clean up the paths which are written within the file, instead of what the k8s-snap expects that file lock to be for...
| } else { | ||
| if err := os.RemoveAll(dirpath); err != nil { | ||
| log.Info("WARN: failed to remove containerd data directory", "directory", dirpath, "error", err) | ||
| continue // Avoid removing the lockfile path. |
There was a problem hiding this comment.
Isn't cleanup done on a best effort basis? Do we check the lockfile later on at bootstrap time?
There was a problem hiding this comment.
It is, but IMO it would have been "incorrect" for this code to remove the lockfile path if it had failed to remove the actual path it was pointing too. (IMO either both should be deleted; or none)
Do we check the lockfile later on at bootstrap time?
AFAIK the LockFilesDir() is used only by k8sd in this file when setting up the containerd dirs, and to mark a a node as worker (contents doesn't matter for marking workers, only the file existing does).
These contents of the lockfiles are only read by the remove hook of the k8s-snap (it also neglects to delete the lockfiles after it's done though, unsure whether that's intentional or not...).
Signed-off-by: Nashwan Azhari <[email protected]>
Signed-off-by: Nashwan Azhari <[email protected]>
|
@berkayoz note that it just hit me that because of the new While I believe it's better to leave that test for NIGHTLY, I've manually run the test on a 22.04 VM and I have. I needed to add this small fix (I had forgotten to actually call Can vouch that the added integration test seems to work when run manually now BUT it does sometimes exhibit the |
No description provided.