libroach: avoid recycling encrypted WALs

[ajkr]

RocksDB fails recovery if there are any non-empty WALs that have zero
readable entries. So we need to make sure
EncryptedEnv::ReuseWritableFile() cannot produce such files, even if
an inopportune crash happens. We do not know how to achieve this while
still changing the encryption key for the recycled WAL, so for now this
PR works around the problem in EncryptedEnv::ReuseWritableFile() by
faking recycling.

Release note: None

[cockroach-teamcity]

This change is 

[ajkr]

Hm, seems this violates the goal of not reusing keys. I should've read the docs earlier.

[petermattis]

Hm, seems this violates the goal of not reusing keys. I should've read the docs earlier.

I was thinking this weekend that perhaps we should just disable WAL reuse if encryption is enabled. There is a performance hit to doing that, though.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mberhault and @petermattis)

[ajkr]

Thought through it again. I don't really like how RocksDB overwrites our recovery_mode, which permits garbage WALs, with a stricter one that rejects garbage WALs. It is doing that under the assumption that the overwrite does not change behavior, but in fact it does change. Made a post about it: https://www.facebook.com/groups/rocksdb.dev/permalink/2405909486174218/

[ajkr]

Also I did look into writing a zero entry using the new encryption key as we discussed. It worked if the whole WAL were zeroed, but not if there were a portion zeroed followed by some unzeroed bytes.

Works:

$ dd of=./tmp/111111.log if=/dev/zero bs=1024 count=1024
$ ./db_bench_tolerate_corrupt_tail -benchmarks=readrandom -use_existing_db=true -db=./tmp

Doesn't work:

$ dd of=./tmp/222222.log if=/dev/zero bs=1024 count=1024
$ dd conv=notrunc oflag=append of=./tmp/222222.log if=/dev/urandom bs=1024 count=1024
$ ./db_bench_tolerate_corrupt_tail -benchmarks=readrandom -use_existing_db=true -db=./tmp
...
open error: Corruption: checksum mismatch

[petermattis]

Thought through it again. I don't really like how RocksDB overwrites our recovery_mode, which permits garbage WALs, with a stricter one that rejects garbage WALs. It is doing that under the assumption that the overwrite does not change behavior, but in fact it does change. Made a post about it: https://www.facebook.com/groups/rocksdb.dev/permalink/2405909486174218/

That sounds like a good path forward to me. I think you should apply a short-term bandaid here, and disable WAL recycling when encryption is enabled.

[ajkr]

That sounds like a good path forward to me. I think you should apply a short-term bandaid here, and disable WAL recycling when encryption is enabled.

OK, I updated this PR to do that.

[petermattis]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajkr and @mberhault)

---

c-deps/libroach/ccl/encrypted_env_test.cc, line 293 at r1 (raw file):

    {
      std::unique_ptr<rocksdb::WritableFile> res;
      if (i == static_cast<int>(ReuseWritableFileInjectionEnv::Mode::kNone)) {

I think you can use mode instead of i here, to avoid the cast on the right hand side of the expression.

[ajkr]

TFTR.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mberhault and @petermattis)

---

c-deps/libroach/ccl/encrypted_env_test.cc, line 293 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I think you can use mode instead of i here, to avoid the cast on the right hand side of the expression.

Oh right, done.

[ajkr]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)