Crash resiliency: deleting incomplete layers doesn’t reliably happen

[mtrmac]

Consider this sequential sequence of events, with the overlay graph driver.

• The user initiates pull of an image which contains 2 layers, parentLayer and childLayer
• While creating parentLayer, the WIP layer object is recorded in layers.json with incompleteFlag.
• Afterwards, during ApplyDiff, the pull process is forcibly killed (so that it can’t do its own cleanup).
• Result: layers.json contain a record of the layer, with incompleteFlag; the overlay graph driver contains an incomplete/inconsistent layer, but a $parentLayer/link file and a l/$link symbolic link exist. This is all as expected.

---

• The user initiates a pull of the same image again.
• (Just like the first time), the pull first checks for pre-existing layers in storage, via Store.Layer(parentLayer). This locks the layerStore read-only first. Thus, the first layerStore.ReloadIfChanged does trigger a layerStore.Load(), but that does not clean up incomplete layers. But layerStore.lockFile.lw was updated to match the lock file contents.
• Consequently, the record of the incomplete layer continues to exist, and Store.Layer reports that parentLayer exists.
• Pull proceeds, assuming that parentLayer exists, and starts creating childLayer.
• While creating childLayer, the layerStore is locked read-write, but because nothing has changed on disk and layerStore.lockFile.lw matches (within the same process), layerStore.ReloadIfChanged does nothing, and does not enter layerStore.Load() and the “delete incomplete layers” code is not reached. Consequently, parentLayer continues to exist in incomplete state.
• This allows creation of childLayer to succeed. $childLayer/lower is created, and includes the short link from parentLayer/link.
• Result: The whole pull is reported as successful. The image, though, contains an incomplete layer, with incomplete/inconsistent contents.

---

• Next, the user does something that doesn’t start with a read-only lock of layerStore. That finally triggers layerStore.Load to delete incomplete layers — and now parentLayer is deleted, resulting in a broken parent link from childLayer to parentLayer.
• For example, podman run theSameImage works for this purpose. That deletes the layer and fails with Error: layer not known (with a currently unclear call stack).

---

• One more podman run theSameImage causes the missing layer to be noticed, with

ERRO[0000] Image theSameImage exists in local storage but may be corrupted: layer not known 

• … and that triggers a re-pull.
• This re-pull correctly detects that parentLayer is missing, and creates it afresh, with a new $parentLayer/link value.
• But, childLayer is not missing, and the previous one is just reused. $childLayer/lower continues to contain the old $parentLayer/link value.
• Finally, when trying to actually use childLayer, this manifests in

WARN[0093] Can't read link "/var/lib/containers/storage/overlay/l/UDGNJ5CR2MQ2QQDGYYK2W4WCBR" because it does not exist. A storage corruption might have occurred, attempting to recreate the missing symlinks. It might be best wipe the storage to avoid further errors due to storage corruption. 
Error: readlink /var/lib/containers/storage/overlay/l/UDGNJ5CR2MQ2QQDGYYK2W4WCBR: no such file or directory

[mtrmac]

@nalind @vrothberg This is a locking design issue I’m not immediately sure how to handle.

A cheap kludge would be to have the read-only layerStore.Load pretend that incomplete layers don’t exist at all, but that could only make diagnosing such situations even harder.

Maybe have layerStore maintain a cleanupNotDone bit of state, so that the first write-locked use of layerStore does trigger a cleanup?

[vrothberg]

Maybe have layerStore maintain a cleanupNotDone bit of state, so that the first write-locked use of layerStore does trigger a cleanup?

It sounds like a new state would help. But I only parsed your summary and did not take a look at code. I'd easily need a full workday to catch up on this issue.

[mtrmac]

…cleanupNotDone…

One concern with this approach is that a single process would still see a layer disappear during its lifetime. Notably a pull would first see a layer present in TryReusingBlob, but trying to commit a child layer on top would, later, fail.

That’s not, in principle, a new concern — layers can always disappear while the store is unlocked. But it would now be more likely to be user-noticeable.

Alternatively, should every process start by taking a read-write lock and cleaning up in-progress layers, even if the operation is purely read-only? That seems vaguely reminiscent of earlier read-write/read-only discussions like #473 (which, to be explicit, I haven’t now re-read).

[mtrmac]

#1145 doesn’t fix this.