sql, config: make `voter_constraints` inherit from parent by default by aayushshah15 · Pull Request #63079 · cockroachdb/cockroach

aayushshah15 · 2021-04-05T11:13:02Z

Resolves #61316

This commit inverts the definition of the flag used to keep track of
whether voter_constraints were empty or inherited. This allows legacy
(pre-21.1) zone configs to be correctly interpreted by 21.1 nodes.

Release note (backward-incompatible change): the internal representation
of the voter_constraints zone configuration attribute (new in 21.1)
has been altered in a way that's partially incompatible with the
representation used by previous 21.1 betas (and the alphas that include
this attribute). This means that Users who directly set the
voter_constraints attribute to an empty list will lose those
constraints and will have to reset them.

cockroach-teamcity · 2021-04-05T11:13:10Z

This change is

otan · 2021-04-05T11:32:49Z

i think this is backwards incompatible with the betas, which according to @ajstorm is something we care about :\
would it be easier to write the migrations?

ajstorm · 2021-04-05T12:06:50Z

Nice to see this @aayushshah15. Can you talk a bit more about how it's backwards incompatible?

otan · 2021-04-05T12:09:42Z

inherited_voter_constraints has been renamed and re-used as explicitly_set_voter_constraints in the protobuf. anyone who has used voter constraints in the past will have the meaning "swapped".

aayushshah15 · 2021-04-05T14:46:42Z

i think this is backwards incompatible with the betas, which according to @ajstorm is something we care about

~~Is there a source for this on our wiki / somewhere else? I can't find any doc claiming this, but I can find some old slack conversations about how this is more of a best-effort thing.~~

Turns out we do care about this.

I discussed with Nathan and Andrei just now and we felt that, in this particular case, not writing a migration doesn't lead to any issues severe enough (since it just amounts to users essentially losing their existing voter_constraints) to warrant the extra work. Additionally, in this case its unclear exactly what the migration should do, since we're dealing with zone config hierarchies that might already be in an incorrect state.

otan · 2021-04-06T05:10:16Z

if i understand correctly, would the migration be to set all InheritedVoterConstraints to true for every zone config that exists?
i sense some surprises if this is to be the case and the migration is not done, e.g. a user won't be able to migrate to MR if they set gc.ttlseconds on their table unless they discard the zone config (since we dont have CONFIGURE ZONE USING x = INHERIT FROM PARENT)

aayushshah15 · 2021-04-06T05:18:03Z

if i understand correctly, would the migration be to set all InheritedVoterConstraints to true for every zone config that exists?

Nope, this would essentially amount to wiping all voter_constraints on every zone.

I haven't thought through this deeply but any migration would be quite tricky to get right here.

user won't be able to migrate to MR if they set gc.ttlseconds on their table

I don't think I follow, why would gc.ttlseconds matter here?

otan · 2021-04-06T05:20:05Z

It initialises a new zone config but with the default values for voter constraints in v20.2. If a user migrates to v21.1, on the first multi-region add, we'll detect inherit voter constraints = false on the zone config which is not expected and would hence be blocked

aayushshah15 · 2021-04-06T05:28:40Z

@otan I see what you mean. That's what this PR is fixing though, right? If all we're concerned about is the move from 20.2 to 21.1.0 (beta3 and above) then this PR should be all we need.

Could you clarify what you're proposing, just so we're on the same page? What CRDB versions are we proposing to reconcile with a migration? 21.1 beta1 to beta3?

aayushshah15 · 2021-04-06T05:40:36Z

Let me clarify what I'm thinking.

If we care about not breaking existing zone configs for existing 21.1 beta users in addition to not breaking voter_constraints inheritance for 20.2 users, a simple migration for just 20.2 -> 21.1 will not cut it anymore because we've shipped a couple 21.1 betas already.

So, if I'm reading you correctly, your proposal is two sets of migrations: one that deals with 20.2 users moving to "21.1beta3 and above", and another that deals with 21.1beta1 users to "21.1beta3 and above". The latter is the reason for the push-back against this PR, is that correct? Note that this is the migration case that's very tricky cause the users might have already created zone configs with explicitly not inherited voter constraints.

otan · 2021-04-06T08:53:59Z

Hmm, sorry let's try again!

I discussed with Nathan and Andrei just now and we felt that, in this particular case, not writing a migration doesn't lead to any issues severe enough (since it just amounts to users essentially losing their existing voter_constraints) to warrant the extra work.

Somehow connected the wrong dots and thought you meant "we're cancelling this PR and doing nothing". My bad.

The latter is the reason for the push-back against this PR, is that correct?

Yep, that's my pushback on this PR. Anyone who's already in 21.1beta1 (or whatever is out there as of speaking) will have the behaviour surprisingly flip on them with this PR merged if they moved to say v21.1.0.

If we care about not breaking existing zone configs for existing 21.1 beta users in addition to not breaking voter_constraints inheritance for 20.2 users, a simple migration for just 20.2 -> 21.1 will not cut it anymore because we've shipped a couple 21.1 betas already.

Ah yes, that's tricky - didn't think about that.

I guess my question then is can we do something simple to check like

if !zc.InheritParentZoneConfigs && len(voter_constraints) == 0 {
   zc.InheritParentZoneConfigs = true
}

(I would've thought if not inheriting then you must have some voter constraints, but I could be wrong).

Is there a way to check in a migration whether a cluster has ever been a certain version? I would've hope so, but realise I am asking for a lot more than is bargained for. Supporting compat between betas is really gross :\

In either case, both situations suck, but it seems like you have been weighing the options and I chimed in with insufficient context. I guess a release note summarising what could go wrong between betas would be nice to have.

aayushshah15 · 2021-04-06T14:47:20Z

"we're cancelling this PR and doing nothing"

yeah so just to confirm, my / our position was that we would go ahead with this PR since it's a very simple fix for the main issue that we care about, which is correct interpretation of 20.2 zone configs. Regarding the fact that we're mangling with zone configs from 21.1beta1, we would just call this out in a proper release note since it's not worth taking on all this complexity for a tiny set of non-production users (especially when the mitigation is to just discard some existing zone configs and re-add them -- the release note could call this out).

I guess my question then is can we do something simple to check like

I had wondered about this but it would be incorrect because the user could've just explicitly set voter_constraints to an empty list (a good reason to do that would be to avoid inheriting from parent). I'm actually not sure there even is a correct migration we can write here. This kinda brings me to my point below.

Supporting compat between betas is really gross :\

It's just that there's a ton of ways to get this whole ordeal wrong, all for the sake of a vague desire to maintain back-compat on new features between betas (which we don't seem to promise anywhere externally) which seems questionable.

otan · 2021-04-06T20:23:12Z

I'm personally ok with going forwards with this PR, but also impartial to writing a migration that works only for 20.2->21.1 if we're going down this path :)

I guess it's not me the rules should be enforced by :D

ajstorm · 2021-04-06T22:41:33Z

The resolution from the team meeting today was: 1) We need a path for customers to migrate from previous betas to the beta which contains this fix. That migration doesn't need to be handled in the code, but can be prescribed in a release note as a set of steps the customer needs to run. We should test this in-house to ensure that it works, and that we don't get tripped up by any validation scenarios. 2) We need better clarity on what types of backward incompatible changes are acceptable in betas. I'll continue working to ensure that Isaac and Nate define this for future releases.

…

On Tue, Apr 6, 2021 at 4:23 PM Oliver Tan ***@***.***> wrote: I'm personally ok with going forwards with this PR, but also impartial to writing a migration that works only for 20.2->21.1 if we're going down this path :) I guess it's not me the rules should be enforced by :D — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63079 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEMXVOQWJK2RQAA7F6CMQ2TTHNUT5ANCNFSM42MURSUA> .

aayushshah15 · 2021-04-11T04:34:15Z

I've updated the release note with steps for current 21.1 beta users who upgrade to the next beta (here's a script that encapsulates the validation of these steps on my end). PTAL when you get a chance.

ajstorm

I'm probably missing something obvious, but I don't see the updated release notes. @aayushshah15 can you point me to them.

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

aayushshah15 · 2021-04-12T17:16:26Z

I had only updated the PR message, but now I updated the commit message as well.

otan

Changes LGTM, letting a TL sign off for the beta incompatibility though.

Reviewed 3 of 3 files at r2, 12 of 12 files at r3, 12 of 12 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

aayushshah15 · 2021-04-13T06:14:29Z

@nvanbenschoten proposed an alternative approach today, which works out fantastically. The solution in the current revision does not break backwards compatibility with existing multi-region databases. It only breaks back-compat with 21.1 zone configs that explicitly set voter_constraints to an empty list. Even then, the mitigation for that subset of users is very straightforward.

ajstorm

🎉 it's great to see that ingenuity won out here. Nice job team!

Nit: You might want to consider dropping the last two bullets of the release note. I think if we describe who's impacted, we can leave out who's not impacted. Also, you might want to change the first bullet to "Users who directly set the voter_constraints attribute (a new attribute in 21.1) to an empty list in one of the previous 21.1 betas or alphas will lose those constraints and will have to reset them to an empty list". Just a small tweak to clearly indicate that this only impacts previous beta/alpha users.

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

aayushshah15 · 2021-04-13T19:51:23Z

Thanks for the latest suggestions @ajstorm. Updated the note with them.

ajstorm

LGTM. Nit: Now that there's only one item in the list, you might want to remove the colon and just continue the paragraph.

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

nvb

thanks for diligently working through the complexity of this change. I think it ended up in a good spot.

Reviewed 2 of 2 files at r1, 3 of 3 files at r2, 7 of 12 files at r3, 12 of 12 files at r4, 2 of 12 files at r5, 11 of 11 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15)

pkg/config/zonepb/zone.go, line 278 at r6 (raw file):

// explicitly set on this zone or if it is to be inherited from its parent.
func (z *ZoneConfig) InheritedVoterConstraints() bool {
	if len(z.VoterConstraints) == 0 {

nit: this might be easier to read as return len(z.VoterConstraints) == 0 && !z.NullVoterConstraintsIsEmpty

pkg/config/zonepb/zone.go, line 554 at r6 (raw file):

		if !parent.InheritedVoterConstraints() {
			z.VoterConstraints = parent.VoterConstraints
			z.NullVoterConstraintsIsEmpty = true

Should this be z.NullVoterConstraintsIsEmpty = parent.NullVoterConstraintsIsEmpty? I don't think it matters in practice, but it seems like we're copying everything from our parent, so why not this field?

pkg/config/zonepb/zone.proto, line 141 at r6 (raw file):

  // below) because the non-nullable nature of `constraints` and
  // `voter_constraints` means that there is no other way to disambiguate
  // between an unset `constraints` attribute and an empty one.

voter_constraints

pkg/config/zonepb/zone.proto, line 145 at r6 (raw file):

  // NullVoterConstraintsIsEmpty specifies whether the VoterConstraints field
  // was explicitly set to be empty or if it was inherited from its parent.

I'd say again here what you say right above. "This field is needed because the non-nullable nature of voter_constraints means that there is no other way to disambiguate between an unset constraints attribute and an empty one."

pkg/server/diagnostics/reporter.go, line 378 at r6 (raw file):

	}
	dst.VoterConstraints = make([]zonepb.ConstraintsConjunction, len(src.VoterConstraints))
	dst.NullVoterConstraintsIsEmpty = src.NullVoterConstraintsIsEmpty

Should we be doing the same thing for InheritedConstraints up above?

This commit anonymizes voter_constraints info in our server diagnostics reporting. Release note: None

Resolves cockroachdb#61316 This commit changes the definition of the flag used to keep track of whether `voter_constraints` were empty or inherited. This allows legacy (pre-21.1) zone configs to be correctly interpreted by 21.1 nodes. Release note (backward-incompatible change): the internal representation of the `voter_constraints` zone configuration attribute (new in 21.1) has been altered in a way that's partially incompatible with the representation used by previous 21.1 betas (and the alphas that include this attribute). This means that Users who directly set the `voter_constraints` attribute to an empty list _will lose_ those constraints and will have to reset them.

aayushshah15

Thanks for the reviews!

bors r+

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

pkg/config/zonepb/zone.go, line 278 at r6 (raw file):