kv,migration: rm code handling legacy raft truncated state

[irfansharif]

Fixes #66544.
Fixes #66834.

We fully migrated away from the replicated truncated state
and the old range applied state keys (RaftAppliedIndexLegacyKey,
LeaseAppliedIndexLegacyKey, and RangeStatsLegacyKey) in #58088. We're
now guaranteed to never see the legacy truncated+applied state
representations, making this code (finally) safe to delete.

Release justification: cluster version cleanup
Release note: None

---

This turned out to be pretty hairy and invasive. I'll wait for a few CI
runs and inspect the diffs here a bit more closely. Given the surface
area here, I'm curious if we feel this is worth getting in during
stability. Some questions to consider:

• The various legacy keys that were removed were removed in their
  entirety. Our CLI to scan over store keys will no longer recognize these
  keys. Is that ok?
• I don't think we need any checks to make sure we're not re-using some
  of these keys in the future -- but I'd like other opinions.
• Looks like we re-purposed the LocalRaftTruncatedStateLegacySuffix for
  the unreplicated truncated key as well, so just renamed it more
  appropriately.
• v21.1 nodes have to interop with v20.2 nodes where the legacy
  representation was possible, and it identified such cases through
  booleans in proto messages (SnapshotRequest.Header.UseUnreplicatedTruncatedState
  for example). This means that we still need to explicitly set them in
  v21.2 code. Crucially we'll stop reading these fields in v21.2 code,
  letting us remove these fields entirely in v22.1.
• The treatment above does not extend to ReplicaState.UsingAppliedStateKey;
  that field was only being used to trigger an idempotent migration to
  start using the newer applied state key representation, not as a way
  to express that the replica should expect the legacy representation.
  Since #58088 has already migrated every range in the system, we no
  longer need to trigger anything, and this field can be removed.

[cockroach-teamcity]

This change is 

[ajwerner]

The various legacy keys that were removed were removed in their entirety. Our CLI to scan over store keys will no longer recognize these keys. Is that ok?

It doesn't seem that important to me to delete all of these keys. I could see deleting all of the kvserver code and some of the constructors but leaving the keys around for debugging. I'm curious what others think about that.

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

---

pkg/cli/debug_check_store.go, line 255 at r1 (raw file):

				}
				getReplicaInfo(rangeID).committedIndex = hs.Commit
			case bytes.Equal(suffix, keys.LocalRaftTruncatedStateLegacySuffix):

I think you could leave this code in perpetuity if you wanted.

---

pkg/kv/kvserver/raft.proto, line 195 at r1 (raw file):

    //
    // TODO(irfansharif): Remove this in v22.1.
    bool legacy_unreplicated_truncated_state = 8;

nit: I'd call this Deprecated rather than Legacy as it made me think the state was legacy rather than the field. Maybe that's just me.

[ajwerner]

Reviewed 3 of 10 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @irfansharif, @nvanbenschoten, and @tbg)

---

pkg/kv/kvserver/stateloader/stateloader.go, line 166 at r4 (raw file):

) (enginepb.RangeAppliedState, error) {
	var as enginepb.RangeAppliedState
	_, err := storage.MVCCGetProto(ctx, reader, rsl.RangeAppliedStateKey(), hlc.Timestamp{}, &as,

Is it good to return a zero value here silently? before we would check for nil at least and do something. Now we might get a zero value and no error.

[irfansharif]

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

---

pkg/kv/kvserver/stateloader/stateloader.go, line 166 at r4 (raw file):

Previously, ajwerner wrote…

Is it good to return a zero value here silently? before we would check for nil at least and do something. Now we might get a zero value and no error.

If I'm reading it all correctly, this is exactly what we're doing today. StateLoader.Load would previously read from LoadRangeAppliedState, and if the key (RangeAppliedStateKey) was not found, fall back to using LoadAppliedIndex + LoadMVCCStats. Each of those helpers would in turn first read from the RangeAppliedStateKey first, and if nothing is found, fall back to reading from the legacy keys. If those legacy keys were not found we would return the zero values.

Given that things have been fully migrated, we're not expecting to find anything under the legacy keys; if nothing exists under RangeAppliedStateKey, we'd then already be returning the zero values.

---

Does that seem sound? I'm not sure what else to do if the the key is not found.

[tbg]

I'm curious if we feel this is worth getting in during stability
Thanks for getting this out!
I think we should consinder merging this only after we have a beta out (looks like we're inching closer there). Let me know if that has any downside I'm not considering. The changes look good, very satisfying. I have requested some additional clarifications to bad comments (I can say that, I wrote them originally), but there's nothing in this PR that I'd consider problematic.

The various legacy keys that were removed were removed in their
entirety. Our CLI to scan over store keys will no longer recognize these
keys. Is that ok?

I think that's ok. I can also see Andrew's argument (it's cheap to keep them so we should just in case) but also a <=21.1 cli is easy to come by if you need one. We have so many keys and it's an area of confusion to many, so if we can strip down the list we should. Also, this is already what this PR does so adding it back in and rearranging seems like busy-work for you that I'd like to avoid.

I don't think we need any checks to make sure we're not re-using some
of these keys in the future -- but I'd like other opinions.

Agreed.

Reviewed 33 of 41 files at r1, 10 of 10 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @irfansharif, and @nvanbenschoten)

---

pkg/kv/kvserver/replica_command.go, line 2443 at r1 (raw file):

	}

	canAvoidSendingLog := snap.State.TruncatedState.Index < snap.State.RaftAppliedIndex

#66834 (comment)

Let's make this true (which also allows us to close #66843 if you also remove the actual ability to send the log). If it is false, we have a way worse problem.

It's fine if you want to postpone this, but it's so tempting.

---

pkg/kv/kvserver/replica_raft.go, line 1794 at r1 (raw file):

// truncated state. This isn't 100% trivial. The truncated state is
// unreplicated, and effectively determines the first index of the log. We make
// use of this fact by not sending the raft log in snapshots. Updates to the

nit: now "this fact" seems to refer to the truncated state determining where the log starts. But it should refer to the "unreplicated" bit. How about

// The truncated state of a replica determines where its Raft log starts (by giving the last index that already deleted). It is unreplicated, i.e. can differ between replicas despite being at the same applied index. The main way in which this happens is through snapshots, which contain no log (i.e. the truncated index will equal the applied index).

---

pkg/kv/kvserver/replica_raft.go, line 1791 at r4 (raw file):

}

// handleTruncatedStateBelowRaft is called when a Raft command updates the

Can you rename this to handleTruncatedStateBelowRaftPreApply? Took me a minute to remember that this is pre-apply. I was also confused about what's in the incoming ReadWriter. This is called from runPreApplyTriggersAfterStagingWriteBatch, so for a bit I thought that the ReadWriter would already have a TruncatedState write applied to it, and we may have to undo it (if it's incompatible with the local log). But that is not the case; the truncation is a purely replicated side effect (i.e. it doesn't write to the truncated state key, which makes sense, since that is an unreplicated key). If this confused me it's a good idea to write it down explicitly that this is how it works.

---

pkg/kv/kvserver/replica_raft.go, line 1840 at r4 (raw file):

	}

	if truncStatePostApply.Index < newTruncatedState.Index {

Doesn't this method need to be simplified further? We have in scope:

• oldTruncatedState (receives b.state.TruncatedState)
• newTruncatedState (receives res.State.TruncatedState, i.e. what the raft leader intends to truncate to)
• truncStatePostApply (loaded from the batch, but since a truncation doesn't write to the batch, this should equal oldTruncatedState?)

Let's rename newTruncatedState to suggestedTruncatedState and I think we should also be able to eliminate truncStatePostApply and the corresponding LoadRaftTruncatedState (though we could keep it for an assertion for a while)

---

pkg/kv/kvserver/stateloader/stateloader.go, line 166 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

If I'm reading it all correctly, this is exactly what we're doing today. StateLoader.Load would previously read from LoadRangeAppliedState, and if the key (RangeAppliedStateKey) was not found, fall back to using LoadAppliedIndex + LoadMVCCStats. Each of those helpers would in turn first read from the RangeAppliedStateKey first, and if nothing is found, fall back to reading from the legacy keys. If those legacy keys were not found we would return the zero values.

Given that things have been fully migrated, we're not expecting to find anything under the legacy keys; if nothing exists under RangeAppliedStateKey, we'd then already be returning the zero values.

---

Does that seem sound? I'm not sure what else to do if the the key is not found.

This seems consistent with what we do elsewhere, but I think it always needs to exist, so we might want to make this return an error if it does not.

---

pkg/kv/kvserver/stateloader/stateloader.go, line 172 at r4 (raw file):

// LoadAppliedIndex returns the Raft applied index and the lease applied index.
func (rsl StateLoader) LoadAppliedIndex(

nit: can't this method be removed? Why not go through the AppliedState? I think there are only two callers here.

[ajwerner]

I'm cool with removing the keys.

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

[irfansharif]

I'll just remove the keys -- it's super unlikely we'd ever run into them in practice, and yea, older binaries are easy to come about. Thanks for reviewing!

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

---

pkg/cli/debug_check_store.go, line 255 at r1 (raw file):

Previously, ajwerner wrote…

I think you could leave this code in perpetuity if you wanted.

Leaving it as is per discussion above.

---

pkg/kv/kvserver/replica_command.go, line 2443 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

#66834 (comment)

Let's make this true (which also allows us to close #66843 if you also remove the actual ability to send the log). If it is false, we have a way worse problem.

It's fine if you want to postpone this, but it's so tempting.

Done.

---

pkg/kv/kvserver/replica_raft.go, line 1794 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: now "this fact" seems to refer to the truncated state determining where the log starts. But it should refer to the "unreplicated" bit. How about

// The truncated state of a replica determines where its Raft log starts (by giving the last index that already deleted). It is unreplicated, i.e. can differ between replicas despite being at the same applied index. The main way in which this happens is through snapshots, which contain no log (i.e. the truncated index will equal the applied index).

Done.

---

pkg/kv/kvserver/replica_raft.go, line 1791 at r4 (raw file):

Can you rename this to handleTruncatedStateBelowRaftPreApply

Done.

I'm not entirely sure I followed the second part of your review. I've tried rewording this comment block to something that makes a bit more sense to me. PTAL.

---

pkg/kv/kvserver/replica_raft.go, line 1840 at r4 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Doesn't this method need to be simplified further? We have in scope:

• oldTruncatedState (receives b.state.TruncatedState)
• newTruncatedState (receives res.State.TruncatedState, i.e. what the raft leader intends to truncate to)
• truncStatePostApply (loaded from the batch, but since a truncation doesn't write to the batch, this should equal oldTruncatedState?)

Let's rename newTruncatedState to suggestedTruncatedState and I think we should also be able to eliminate truncStatePostApply and the corresponding LoadRaftTruncatedState (though we could keep it for an assertion for a while)

Done.

---

pkg/kv/kvserver/stateloader/stateloader.go, line 166 at r4 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This seems consistent with what we do elsewhere, but I think it always needs to exist, so we might want to make this return an error if it does not.

I was actually seeing in CI that it was possible for the key to not exist, and that we were relying on the zero values (and no errors). I could work through where exactly that happens now and improve this signature, but it seems relatively non-important. (Also like Tobi mentioned, we use this pattern elsewhere).

---

pkg/kv/kvserver/stateloader/stateloader.go, line 172 at r4 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: can't this method be removed? Why not go through the AppliedState? I think there are only two callers here.

Good catch, done.

[irfansharif]

Added the GA-blocker + backport-21.2 labels. I think they're correct but could be wrong. It'd be nice to get this into 21.2, that'll let us clean up a few of these hold-over proto fields early next cycle.

[tbg]

Let's keep this to master only and not target 21.2. I'm all for removing proto fields, but whether we do that in a month or in seven months is relatively unimportant. The change here isn't trivial, and they also make any problems in the migrations a lot more dicey.

I think there might be a place or two where you need to adjust when we can remove the fields.

Reviewed 10 of 10 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @nvanbenschoten)

---

pkg/kv/kvserver/replica_command.go, line 2443 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Done.

Oops, meant #66834 in my last comment.

---

pkg/kv/kvserver/replica_raft.go, line 1793 at r5 (raw file):

, i.e. a command that includes a suggested truncated state.

I want to make sure that we understand from the comment that the truncation is not "mandated" by the command. We could just do nothing and everything would be correct. The raft command is basically just saying "hey, I (the raft leader) am suggesting this truncation, see if you like it".

---

pkg/kv/kvserver/replica_raft.go, line 1808 at r5 (raw file):
Add this (bonus points if you can say it more elegantly):

Note also that we crucially rely on truncations happening in the apply loop. This makes sure that a truncation can not remove log entries yet to be applied to the Replica state machine. Since a truncation only ever removes committed log indexes, and after chosing the index gets proposed, it will itself be assigned an index ahead of the highest index it could possibly remove. By the time the truncation is then handled, the state machine has thus applied all entries the truncation could possibly affect.

This was the source of my confusion in #70109, and it's an invariant we are likely to accidentally break if we ever do a decoupled apply loop or improve raft log truncations.

---

pkg/kv/kvserver/replica_raft.go, line 1838 at r5 (raw file):

		unsafeKey := prefixBuf.RaftLogKey(idx)
		if err := readWriter.ClearUnversioned(unsafeKey); err != nil {
			return false, errors.Wrapf(err, "unable to clear truncated Raft entries for %+v", suggestedTruncatedState)

mention the idx here.

---

pkg/kv/kvserver/replica_raft_truncation_test.go, line 39 at r5 (raw file):

	ctx := context.Background()
	var prevTruncatedState roachpb.RaftTruncatedState

Oops, this should be declared inside of Walk, right? Also, can we "just" load the proto from eng right before handleTruncatedStateBelowRaftPreApply and avoid this out-of-band tracking?

[irfansharif]

Sounds good. Begone replicated truncated state!

bors r+

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

[craig]

Build succeeded:

• GitHub CI (Cockroach)