Delete empty shard folders when DB is deleted

[danydrouin]

Deleting a DB will currently only remove the index data files in the shard.. but doesn't actually delete the folders which may lead to an inodes leak in certain file systems (such as Spectrum scale).

after recursing the files, the folder should also be deleted.

[rnewson]

I think it's safe but we've never tried cleaning up directories as we go. I worry about what happens if something is removing an index and its newly empty parent directories at the same time as a request to create an index that overlaps with one of those directories. can that happen? I think yes. cleanup is one 'thread' and so we can discount collisions of multiple cleanups (which likely wouldn't be a problem anyway), but index creation is done by the managerservice, so delete and create can happen concurrently. if it does, does the index creation spuriously fail because cleanup deleted a directory we just made (but was empty at the time)? I think so, and we must guard against that if so.

[theburge]

@rnewson Yes, the raciness is a bother here (as it is for the "CouchDB DB recovery" code that renames files and resides in IndexCleanupService -- i.e., a separate actor than that which would create directories, etc.).

One much more insidious possibility is that we unlink a directory that another bit of the code has open and is perhaps about to use with openat or similar, such that newly created files are immediately unreachable. If that did occur, it would be pretty serious because the LRU would then refer to something that didn't exist in filesystem terms.

[danydrouin]

Hi @rnewson / @theburge, my changes dont impact database recovery scenario since that uses Rename event.

What i noticed is creating a new DB will also call the same cleanup. It does nothing since none of the folders exist.. no index created yet. https://github.com/apache/couchdb/blob/main/src/dreyfus/src/dreyfus_index_manager.erl#L123-L125

If i create a db and index, then delete this DB and recreate it with an index, it will result in a new shard folder. Same DB name was used but it's uniquely identified with a suffix in the shard folder.

// first create db and index
[actor:2] INFO clouseau.cleanup - Removing shards/40000000-5fffffff/test1.1661353443
// delete db
[actor:4] INFO clouseau.cleanup - Removing shards/40000000-5fffffff/test1.1661353443

// second create db and index create calls cleanup)
[actor:3] INFO clouseau.cleanup - Removing shards/40000000-5fffffff/test1.1661353706

I believe my change is safe. But the ultimate test would be to get it tested. If I can get a build going of clouseau, I could include the jar in a new couchdb docker image and test it on my env.

I'm not setup locally to build clouseau im afraid :(

[theburge]

@danydrouin

Same DB name was used but it's uniquely identified with a suffix in the shard folder.

That's generally true, but not necessarily in all cases. For our purposes (as those running a service using this code and with additional tools, etc.), we cannot necessarily rely on it for reasoning about the safety of the change.

In any case, the recursivelyDelete() function is called in two cases: database deletion and removal of unused indexes. It's the latter case that really opens up the possibility of races, because a user updating their indexes programmatically might trigger the cleanup and then create new indexes rapidly in sequence (from their perspective, although it may end up being resequenced in actual execution).

But the ultimate test would be to get it tested.

If only. :) Proving it works in some number of trials doesn't prove freedom from races.

For a potential race condition we'd need to either successfully explore all possible interleavings (challenging!) or reason it through (with enough confidence that we can decide one way or another).

[danydrouin]

@theburge what if i change the recursivelyDelete() to only delete the folders for the database deletion case CleanupPathMsg. Keep it intact for the removal of unused indexes which happens via CleanupDbMsg.

Would that be safer?

[theburge]

@danydrouin

@theburge what if i change the recursivelyDelete() to only delete the folders for the database deletion case CleanupPathMsg. Keep it intact for the removal of unused indexes which happens via CleanupDbMsg.

Would that be safer?

Yeah, that would be safer. It shouldn't be possible to hit any race window there in normal usage. :)

@rnewson Any thoughts on/objection to that change?

[rnewson]

That sounds smart to me.

[danydrouin]

Please review latest commits.. changes are in.

[danydrouin]

@theburge can we get a new release build with these changes? thanks!

[theburge]

@danydrouin Sorry for the delay, here we go: https://github.com/cloudant-labs/clouseau/releases/tag/2.21.1