storage: sideloading bypasses RocksDB env for some operations

[tbg]

I noticed in #31732 (comment) that the sideloaded storage does not perform the TruncateTo operation through the RocksDB env:

cockroach/pkg/storage/replica_sideload_disk.go

Lines 135 to 149 in fa7fb35

	return err
	}
	var deleted int
	for _, match := range matches {
	base := filepath.Base(match)
	if len(base) < 1 || base[0] != 'i' {
	continue
	}
	base = base[1:]
	upToDot := strings.SplitN(base, ".", 2)
	i, err := strconv.ParseUint(upToDot[0], 10, 64)
	if err != nil {
	return errors.Wrapf(err, "while parsing %q during TruncateTo", match)
	}
	if i >= index {

This may or may not be a problem, but it's definitely suspicious. Note how Clear just above the snippet does go through extra trouble to use the env.

Perhaps this all works as intended, depending on how the RocksDB env is implemented. It does seem to work when encryption is on, probably because the env really just does the same thing writing directly to the FS would do. With encryption, is that still true, i.e. are files still passed through to the filesystem into the same location (mod scrambling the contents)? I suspect it might be, but then why does Clear go the extra mile through the env?

This really seems like something needs to be investigated and cleaned up.

Note that there's one case where things are definitely broken, namely the one in which you use an in-memory RocksDB instance. In that case, TruncateTo will snoop around on disk, but the env will keep everything in memory (i.e. TruncateTo becomes a no-op and garbage will pile up in-mem).

[petermattis]

@mberhault Also related to encryption at rest. It is believed that this leaks non-encrypted data to disk.

[tbg]

It is believed that this leaks non-encrypted data to disk.

Hmm, not sure about that. The files are written through the env:

cockroach/pkg/storage/syncing_write.go

Lines 90 to 114 in 2aba151

	f, err := eng.OpenFile(filename)
	if err != nil {
	if strings.Contains(err.Error(), "No such file or directory") {
	return os.ErrNotExist
	}
	return err
	}
	for i := int64(0); i < int64(len(data)); i += chunkSize {
	end := i + chunkSize
	if l := int64(len(data)); end > l {
	end = l
	}
	chunk := data[i:end]
	// rate limit
	limitBulkIOWrite(ctx, limiter, len(chunk))
	err = f.Append(chunk)
	if err == nil && sync {
	err = f.Sync()
	}
	if err != nil {
	break
	}

@mberhault also tells me in #19783 that the encrypted files have the same size as the unencrypted version, so in my eyes this becomes a cleanup rather than a technical necessity.

[tbg]

@petermattis could storage take a look at this? Concretely, whether we're leaking unencrypted data to disk and if not, if we feel good about the status quo.

[petermattis]

I think @jbowens possibly already fixed this in 20.2. @jbowens can you see if there is anything left to do here?

[jbowens]

@jbowens can you see if there is anything left to do here?

I think we're all good. Closing.