kvserver: don't use marshaled proto for liveness CPut

[andreimatei]

Fixes #38308

Updates to the liveness records are done as CPuts. Before this patch,
the CPuts' expected value was the re-marshaled proto with the previous
version. As #38308 explains, that's a bad practice since it prevents the
proto's encoding to change in any way (e.g. fields can't be removed).
Instead, this patch moves to keeping track of the raw bytes that have
been read from the DB, besides the unmarshalled liveness protos. The raw
bytes are used as the expected values.

Release note: None

[cockroach-teamcity]

This change is 

[bdarnell]

Reviewed 10 of 10 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @nvanbenschoten, and @tbg)

---

pkg/kv/kvserver/node_liveness.go, line 182 at r1 (raw file):

// update to the liveness record: the raw value will act as the expected value.
// This way the proto's encoding can change without the CPut failing.
type LivenessRecordEx struct {

nit: just LivenessRecord?

---

pkg/kv/kvserver/node_liveness.go, line 324 at r1 (raw file):

			return false, errors.Wrap(err, "invalid liveness record")
		}
		//if (oldLiveness == storagepb.Liveness{}) {

Remove or uncomment this? (remove, I think)

---

pkg/kv/kvserver/node_liveness.go, line 777 at r1 (raw file):

// the database has for this liveness record in addition to the decoded liveness
// proto.
func (nl *NodeLiveness) GetLivenessEx(nodeID roachpb.NodeID) (LivenessRecordEx, error) {

Do we really need two different versions of these methods? I'd rather just use the version that includes the raw value everywhere.

---

pkg/kv/kvserver/node_liveness.go, line 950 at r1 (raw file):

			log.Fatalf(ctx, "failed to marshall proto: %s", err)
		}
		// Make a copy of the expected value. We can't pass oldRaw because b.CPut()

Really? Eww.

[tbg]

Reviewed 10 of 10 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, and @nvanbenschoten)

---

pkg/kv/kvserver/node_liveness.go, line 408 at r1 (raw file):

// liveness record. It has both the new and the old version of the proto.
type livenessUpdate struct {
	new storagepb.Liveness

how about "updated"? Using keywords (new) tickles my OCD. It also tends to throw off syntax highlighters, as it does in reviewable.

---

pkg/kv/kvserver/node_liveness.go, line 419 at r1 (raw file):

	// if they don't correspond, the CPut is considered to have failed.
	//
	// When ignoreCache is set, oldLRaw needs to be set as well.

oldRaw

---

pkg/kv/kvserver/node_liveness.go, line 777 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Do we really need two different versions of these methods? I'd rather just use the version that includes the raw value everywhere.

+1

---

pkg/kv/kvserver/node_liveness.go, line 814 at r1 (raw file):

// another successful heartbeat, and a second increment to come in
// after that)
func (nl *NodeLiveness) IncrementEpoch(ctx context.Context, liveness storagepb.Liveness) error {

shouldn't this one be the augmented one with the raw bytes?

---

pkg/kv/kvserver/node_liveness.go, line 950 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Really? Eww.

Is it this?

cockroach/pkg/kv/batch.go

Line 456 in f80ec9c

ev.ClearChecksum()

That seems like bad code we would want to go away. I wonder why we need to clear the checksum - doesn't the checksum match expVal?

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, @nvanbenschoten, and @tbg)

---

pkg/kv/kvserver/node_liveness.go, line 814 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

shouldn't this one be the augmented one with the raw bytes?

That's what I wanted initially, but the thing is that this is called from the lease handling code, which has a storagepb.LeaseStatus. That guy is a proto (for no good reason) and currently doesn't include the raw bytes. I've considered changing that, but... I dunno. There's a bunch of stuff to change, and it's not really necessary given that this code doesn't set ignoreCache - so we check the cache anyway, which has the raw bytes.
Thoughts?

[irfansharif]

(Unsolicited) LGTM. Was looking around changing the liveness proto for random reasons and stumbled by this.

Reviewed 3 of 10 files at r1, 7 of 7 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, @nvanbenschoten, and @tbg)

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @nvanbenschoten, @petermattis, and @tbg)

---

pkg/kv/kvserver/node_liveness.go, line 182 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

nit: just LivenessRecord?

done

---

pkg/kv/kvserver/node_liveness.go, line 324 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Remove or uncomment this? (remove, I think)

done

---

pkg/kv/kvserver/node_liveness.go, line 408 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

how about "updated"? Using keywords (new) tickles my OCD. It also tends to throw off syntax highlighters, as it does in reviewable.

Done.

---

pkg/kv/kvserver/node_liveness.go, line 419 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

oldRaw

Done.

---

pkg/kv/kvserver/node_liveness.go, line 777 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

+1

Done.

---

pkg/kv/kvserver/node_liveness.go, line 950 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Is it this?

cockroach/pkg/kv/batch.go

Line 456 in f80ec9c

ev.ClearChecksum()

That seems like bad code we would want to go away. I wonder why we need to clear the checksum - doesn't the checksum match expVal?

Yeah, this has bothered me greatly too. I spent some time digging.
So Batch.CPut() takes an expected Value. The value has a checksum, which includes the key corresponding to the respective key-value pair. So, if you read val from k1 and then try to used it as the expected value for key k2, the checksum needs to change.
At the moment, as we've seen, the CPut code wipes the checksum that was given to it and recomputes it. I've tried to change it to deal with already-populated checksums by asserting that they're correct for the key currently being written - so then the contract would be that if you pass a value with a filled-in checksum to cput as the expected val, then it must have been read from the key you're currently trying to write. The caller would have the option to clear the checksum, in which case the CPut code will compute a good one.

This hit a problem because the liveness record values we're using for these CPuts often come from gossip, and gossip, alas, uses different damn keys when it broadcasts the liveness records (different from the keys where the records are in the db). So, in order to keep these gossiped liveness records immutable, we'd have to recompute their checksums when a node receives them. Which is doable, but pretty gross too.

So I'm taking opinions about what the right thing to do here is. Is a *roachpb.Value even the right type for the expected val in the CPut interface, or should it be simply []byte (with no checksum)? The *roachpb.Value has the advantage that it has a nice interface for encoding any type into it.

@petermattis I'm also curious if you have any insight into why gossip uses its own keys, different from the database keys (i.e. stuff in gossip/keys.go).

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @nvanbenschoten, @petermattis, and @tbg)

---

pkg/kv/kvserver/node_liveness.go, line 950 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Yeah, this has bothered me greatly too. I spent some time digging.
So Batch.CPut() takes an expected Value. The value has a checksum, which includes the key corresponding to the respective key-value pair. So, if you read val from k1 and then try to used it as the expected value for key k2, the checksum needs to change.
At the moment, as we've seen, the CPut code wipes the checksum that was given to it and recomputes it. I've tried to change it to deal with already-populated checksums by asserting that they're correct for the key currently being written - so then the contract would be that if you pass a value with a filled-in checksum to cput as the expected val, then it must have been read from the key you're currently trying to write. The caller would have the option to clear the checksum, in which case the CPut code will compute a good one.

This hit a problem because the liveness record values we're using for these CPuts often come from gossip, and gossip, alas, uses different damn keys when it broadcasts the liveness records (different from the keys where the records are in the db). So, in order to keep these gossiped liveness records immutable, we'd have to recompute their checksums when a node receives them. Which is doable, but pretty gross too.

So I'm taking opinions about what the right thing to do here is. Is a *roachpb.Value even the right type for the expected val in the CPut interface, or should it be simply []byte (with no checksum)? The *roachpb.Value has the advantage that it has a nice interface for encoding any type into it.

@petermattis I'm also curious if you have any insight into why gossip uses its own keys, different from the database keys (i.e. stuff in gossip/keys.go).

There's also the option of having the CPut code clear the checksum without a fuss if it's not correct for the key.

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @nvanbenschoten, @petermattis, and @tbg)

---

pkg/kv/kvserver/node_liveness.go, line 950 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

There's also the option of having the CPut code clear the checksum without a fuss if it's not correct for the key.

I've discussed this offline with @bdarnell; I'm going to change the CPut interface to simply take a read-only []byte for the expected val, so that we don't need to worry about checksums any more.

[irfansharif]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, @irfansharif, @nvanbenschoten, @petermattis, and @tbg)

---

pkg/kv/kvserver/node_liveness.go, line 871 at r4 (raw file):

// cache.
func (nl *NodeLiveness) updateLiveness(
	ctx context.Context, update livenessUpdate, handleCondFailed func(actual LivenessRecord) error,

Does this handleCondFailed callback need a LivenessRecord type, or could it to with a vanilla storagepb.Liveness? I don't think the callers use the raw bytes.

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, @irfansharif, @nvanbenschoten, @petermattis, and @tbg)

---

pkg/kv/kvserver/node_liveness.go, line 950 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I've discussed this offline with @bdarnell; I'm going to change the CPut interface to simply take a read-only []byte for the expected val, so that we don't need to worry about checksums any more.

The change changing the CPut API is #50408. Let's agree on that one and come back.

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @irfansharif, @nvanbenschoten, @petermattis, and @tbg)

---

pkg/kv/kvserver/node_liveness.go, line 871 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Does this handleCondFailed callback need a LivenessRecord type, or could it to with a vanilla storagepb.Liveness? I don't think the callers use the raw bytes.

they do; the callback calls maybeUpdate() (not new).
I've tried to make the callbacks not call maybeUpdate(), and instead return the actual from updateLiveness() (since that caller needs to sometimes call maybeUpdate() anyway), but it gets a bit gnarly because the callback can return errors too, and then it becomes unclear when the caller of updateLiveness should call maybeUpdate and when it shouldn't. So I've left updateLiveness() return an empty LivenessRecord when handleCondFailed() is called.

[andreimatei]

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @irfansharif, @nvanbenschoten, @petermattis, and @tbg)

[craig]

Build succeeded

• GitHub CI (Cockroach)