Auto recovery from out of space errors

[anand1976]

Summary:
This commit implements automatic recovery from a Status::NoSpace() error
during background operations such as write callback, flush and
compaction. The broad design is as follows -

1. Compaction errors are treated as soft errors and don't put the
   database in read-only mode. A compaction is delayed until enough free
   disk space is available to accomodate the compaction outputs, which is
   estimated based on the input size. This means that users can continue to
   write, and we rely on the WriteController to delay or stop writes if the
   compaction debt becomes too high due to persistent low disk space
   condition
2. Errors during write callback and flush are treated as hard errors,
   i.e the database is put in read-only mode and goes back to read-write
   only fater certain recovery actions are taken.
3. Both types of recovery rely on the SstFileManagerImpl to poll for
   sufficient disk space. We assume that there is a 1-1 mapping between an
   SFM and the underlying OS storage container. For cases where multiple
   DBs are hosted on a single storage container, the user is expected to
   allocate a single SFM instance and use the same one for all the DBs. If
   no SFM is specified by the user, DBImpl::Open() will allocate one, but
   this will be one per DB and each DB will recover independently. The
   recovery implemented by SFM is as follows -
   a) On the first occurance of an out of space error during compaction,
   subsequent
   compactions will be delayed until the disk free space check indicates
   enough available space. The required space is computed as the sum of
   input sizes.
   b) The free space check requirement will be removed once the amount of
   free space is greater than the size reserved by in progress
   compactions when the first error occured
   c) If the out of space error is a hard error, a background thread in
   SFM will poll for sufficient headroom before triggering the recovery
   of the database and putting it in write-only mode. The headroom is
   calculated as the sum of the write_buffer_size of all the DB instances
   associated with the SFM
4. EventListener callbacks will be called at the start and completion of
   automatic recovery. Users can disable the auto recov ery in the start
   callback, and later initiate it manually by calling DB::Resume()

Todo:

1. More extensive testing
2. Add disk full condition to db_stress (follow-on PR)

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:

[anand1976]

Setting the WIP tag to account for the remaining testing. The implementation is largely complete, I think, pending feedback/comments.

[sagar0]

It might be good to be more specific with the reason here.

[anand1976]

Let me rethink this. I was using TableFileCreationReason to signal SstFileManagerImpl::OnAddFile that the file was added due to compaction (with kMisc as sort of the default), but it doesn't necessarily mean the file was just created. It could be called during DB::Open, when its just listing all the files. So maybe TableFileCreationReason is not appropriate. Instead, I can have an explicit parameter in OnAddFile to specify whether the file being added was created by compaction.

[siying]

I'm still reviewing. This is more complicated than I though.

[siying]

Oh wow, this is complicated unit test technique...

[anand1976]

This is just temporary. I'm not going to merge this file. One of the tests is crashing only in Travis, so this is trying to print the backtrace.

[siying]

Can you explain why we need exclusive and non-exclusive mode, and can't make everything exclusive?

[anand1976]

That's in fact the idea - to have only one kind of recovery going on (either auto or manual). I think the implementation is probably confusing. The idea is that a user that doesn't want auto recovery would turn it off in the OnErrorRecoveryBegin event callback, and then call DB::Resume(). The exclusive parameter here is to indicate whether the call is from manual recovery or not. I should change the name to be more explicit.

[siying]

Can you write a comment describing what the function is doing?

[siying]

If we set a flag here, will the race condition between recovery and shutdown be easier to resolve?

[anand1976]

That's a interesting suggestion. We could set a new flag here, and in DBImpl::CloseHelper, call error_handler_.CancelErrorRecovery before CancelAllBackgroundWork. That way, we would not have both trying to schedule memtable flushes. Let me look into this a bit more.

[siying]

Is all what it does in shutdown=false case these lines?

   while (bg_bottom_compaction_scheduled_ || bg_compaction_scheduled_ ||
          bg_flush_scheduled_) {
     bg_cv_.Wait();
 }

If that is the case, is it easier to make these lines a helper function, rather than fitting into CancelAllBackgroundWork()? Otherwise, it doesn't feel cancelling any background work.

[anand1976]

That's a good point. It is effectively just waiting for background work to be completed. This can be moved to a helper and reused.

[siying]

Is there a way to start it only when out-of-space happened?

[anand1976]

Yes. I think that's a good idea. That way, if an out of space error never happens, the behavior is exactly the same as it is now.

[siying]

Can you comment more about what it means?

[anand1976]

Its referring to the case where some DB instances experienced soft errors (i.e compaction failure) and some experienced hard errors (i.e flush/WAL write failures), we would estimate required free space as if all instances experienced hard errors. For hard errors, the free space threshold is reserved_disk_buffer_, which is basically the sum of write_buffer_size of all instances. For soft errors, the threshold is free_space_trigger_, which is the value of cur_compactions_reserved_size_ at the time of error.

[siying]

This is a fairly complicated logic. I wonder whether we can simplify it, trading off some feature supports.

[anand1976]

There are a couple of things I added here that increased the complexity - 1. The in_progress_files_size_ was added in order to account for a compaction that has generated some outputs that are already counted in cur_compactions_reserved_size_, and 2. Since compaction_buffer_size_ is a user provided parameter during SstFileManager construction, choose a default if they didn't set it.

I can move these down below to the NoSpace case in order to avoid impacting the normal case. Maybe #2 can also be removed, depending on how conservative we want to be when estimating headroom after getting out of space error.

[siying]

I have a worry using this one.
For example, if a user runs universal compaction, and has 200 shards, and it is out-of-memory. Eventually, full compaction of all the shards will reserve the size equivalent to the whole shard size, and fail. The compaction trigger would be equal to the total size of the 200 shards. Then users need to drop the disk space to lower than 50% to make it recover. This is going to be much more than what is needed. Is it a valid worry?

[anand1976]

The cur_compactions_reserved_size_ is the space reserved by currently running compactions. In your example, would the user tune max_background_jobs to limit the number of concurrent compactions? If they set it to a high number, they will always run the risk of doubling the space used. This check may, in fact benefit that case, as it does not stop background work but rather just slows it down. If EnoughRoomForCompaction returns false, the compaction is retried after some time. So it would end up throttling concurrent compactions to some lower number that can actually be accommodated by the available disk space.

[siying]

When an SST file manager manages multiple DBs, can we recover them slowly, rather than enable them all at one shot? I'm worry about the sudden space increase may complicate things.

[siying]

Maybe call it shutdown_initiated_ directly?

[siying]

Maybe I'm picky but if possible can we place it together with those bool variables, for better padding?

[siying]

Is it possible that, before the mutex is acquired again, the same DB recovered, but quickly failed again. Since this DB is still en error_handler_list_, it is not inserted again, but we poped it up in line 294, so the error handle of this failure DB is out of track?

[anand1976]

That's a good catch. I think the safest way for now is to check the DB's error status again after acquiring the mutex, and leave it in the list if there is an error.

[facebook-github-bot]

anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[yiwu-arbug]

@anand1976 Hi! Seems the abs_time_us here should be uncomment? Otherwise db_bench will wait forever on error.

[rohansuri]

Hi @anand1976 / @siying, I had some questions around this change. Thanks in advance!

• The check here shouldn't have size_added_by_compaction since it is already included in needed_headroom before in the variable initialization?

• It seems we never pass compaction=true to SstFileManagerImpl::OnAddFile which means the logic around in_progress_files_size_ is never executed. Any reason why it is turned off?

• Finally, in what scenario will an already tracked file by SstFileManagerImpl be added again? And why do subtract its size from cur_compactions_reserved_size_ in that case?