Merge cockroachdb#124804 cockroachdb#124806

124804: raft: re-enable config change safety r=nvanbenschoten a=pav-kv

Config changes in this `raft` implementation require a safety constraint: the leader must not append a config change if it hasn't applied all config changes in its log.

The `DisableConfChangeValidation` flag disables this check under the assumption that the state machine layer provides the equivalent guarantee. However, it is hard to argue that this is true in split leaseholder/leader scenarios.

This commit re-enables this check, to bring the safety back. The other two state-machine-level checks concerned with entering and leaving joint configs can still be disabled.

Related to cockroachdb#105797, etcd-io/raft#81, cockroachdb#106515 (cockroachdb#106515 (comment))

Epic: none
Release note: none

124806: release: push PRs to main repo r=celiala a=rail

Previously, we pushed PRs using the forked repos. For auto-merge to work, we need to push these PRs back to the main repo, so `GITHUB_TOKEN` has enough privileges to merge the PRs.

This PR adds an ability to push the PR branches to the origin repo. Note: the push user has to be given `write` permissions to the repos.

Epic: none
Release note: None

Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
Co-authored-by: Rail Aliiev <rail@iqchoice.com>

pkg/cmd/release/update_versions.go

@@ -92,6 +92,10 @@ func init() {
 type prRepo struct {
 	owner string
 	repo  string
+	// pushToOrigin tells if the PR branch should be pushed to the origin repo.
+	// This is required for the repos, where auto-merge is enabled in order to
+	// grant proper permissions to the corresponding GitHub Actions.
+	pushToOrigin bool
 	// what branch should be used as the PR base
 	branch         string
 	commitMessage  string
@@ -113,10 +117,15 @@ func (r prRepo) checkoutDir() string {
 }
 
 func (r prRepo) pushURL() string {
+	pushOwner := r.githubUsername
+	if r.pushToOrigin {
+		pushOwner = r.owner
+	}
+
 	if token := os.Getenv("GH_TOKEN"); token != "" {
-		return fmt.Sprintf("https://%s:%s@github.com/%s/%s", r.githubUsername, token, r.githubUsername, r.repo)
+		return fmt.Sprintf("https://%s:%s@github.com/%s/%s", r.githubUsername, token, pushOwner, r.repo)
 	}
-	return fmt.Sprintf("git@github.com:%s/%s.git", r.githubUsername, r.repo)
+	return fmt.Sprintf("git@github.com:%s/%s.git", pushOwner, r.repo)
 }
 
 func (r prRepo) clone() error {
@@ -455,6 +464,7 @@ func generateRepoList(
 		reposToWorkOn = append(reposToWorkOn, prRepo{
 			owner:          owner,
 			repo:           prefix + "homebrew-tap",
+			pushToOrigin:   true,
 			branch:         "master",
 			githubUsername: "cockroach-teamcity",
 			prBranch:       fmt.Sprintf("update-versions-%s-%s", releasedVersion.Original(), randomString(4)),
@@ -473,6 +483,7 @@ func generateRepoList(
 		reposToWorkOn = append(reposToWorkOn, prRepo{
 			owner:          owner,
 			repo:           prefix + "helm-charts",
+			pushToOrigin:   true,
 			branch:         "master",
 			githubUsername: "cockroach-teamcity",
 			prBranch:       fmt.Sprintf("update-versions-%s-%s", releasedVersion.Original(), randomString(4)),

pkg/raft/raft.go

@@ -1269,7 +1269,13 @@ func stepLeader(r *raft, m pb.Message) error {
 				cc = ccc
 			}
 			if cc != nil {
+				// Per the "Apply" invariant in the config change safety argument[^1],
+				// the leader must not append a config change if it hasn't applied all
+				// config changes in its log.
+				//
+				// [^1]: https://github.com/etcd-io/etcd/issues/7625#issuecomment-489232411
 				alreadyPending := r.pendingConfIndex > r.raftLog.applied
+
 				alreadyJoint := len(r.trk.Config.Voters[1]) > 0
 				wantsLeaveJoint := len(cc.AsV2().Changes) == 0
 
@@ -1282,7 +1288,11 @@ func stepLeader(r *raft, m pb.Message) error {
 					failedCheck = "not in joint state; refusing empty conf change"
 				}
 
-				if failedCheck != "" && !r.disableConfChangeValidation {
+				// Allow disabling config change constraints that are guaranteed by the
+				// upper state machine layer (incorrect ones will apply as no-ops).
+				//
+				// NB: !alreadyPending requirement is always respected, for safety.
+				if alreadyPending || (failedCheck != "" && !r.disableConfChangeValidation) {
 					r.logger.Infof("%x ignoring conf change %v at config %s: %s", r.id, cc, r.trk.Config, failedCheck)
 					m.Entries[i] = pb.Entry{Type: pb.EntryNormal}
 				} else {

pkg/raft/testdata/confchange_disable_validation.txt

@@ -32,45 +32,47 @@ l2 l3
 ----
 ok
 
-# Entries both get appended.
-process-ready 1
-----
-Ready MustSync=true:
-Entries:
-1/4 EntryNormal "foo"
-1/5 EntryConfChangeV2 l2 l3
-
-# Dummy entry comes up for application.
-process-ready 1
-----
-Ready MustSync=false:
-HardState Term:1 Vote:1 Commit:5
-CommittedEntries:
-1/4 EntryNormal "foo"
-
-# Propose new config change. Note how it isn't rejected,
-# which is due to DisableConfChangeValidation=true.
-propose-conf-change 1
-----
-ok
-
-# Turn on autopilot: the first config change applies, the
-# second one gets committed and also applies.
-stabilize
+# Entries both get appended and applied.
+stabilize 1
 ----
 > 1 handling Ready
   Ready MustSync=true:
   Entries:
-  1/6 EntryConfChangeV2
+  1/4 EntryNormal "foo"
+  1/5 EntryConfChangeV2 l2 l3
+> 1 handling Ready
+  Ready MustSync=false:
+  HardState Term:1 Vote:1 Commit:5
+  CommittedEntries:
+  1/4 EntryNormal "foo"
+> 1 handling Ready
+  Ready MustSync=false:
   CommittedEntries:
   1/5 EntryConfChangeV2 l2 l3
   INFO 1 switched to configuration voters=(1)&&(1) learners=(2 3)
 > 1 handling Ready
   Ready MustSync=false:
-  HardState Term:1 Vote:1 Commit:6
-  CommittedEntries:
-  1/6 EntryConfChangeV2
   Messages:
-  1->2 MsgApp Term:1 Log:1/5 Commit:5 Entries:[1/6 EntryConfChangeV2]
-  1->3 MsgApp Term:1 Log:1/5 Commit:5 Entries:[1/6 EntryConfChangeV2]
-  INFO 1 switched to configuration voters=(1) learners=(2 3)
+  1->2 MsgApp Term:1 Log:1/4 Commit:5 Entries:[1/5 EntryConfChangeV2 l2 l3]
+  1->3 MsgApp Term:1 Log:1/4 Commit:5 Entries:[1/5 EntryConfChangeV2 l2 l3]
+
+# Propose new config change. Note how it isn't rejected,
+# which is due to DisableConfChangeValidation=true.
+propose-conf-change 1
+l4
+----
+ok
+
+# The new config change is appended to the log.
+process-ready 1
+----
+Ready MustSync=true:
+Entries:
+1/6 EntryConfChangeV2 l4
+
+# If we process-ready on node 1 now, the second config change will come up for
+# application, and the node will panic with "config is already joint". The state
+# machine must ensure not to apply it.
+#
+# TODO(pav-kv): support no-op command application in tests, or support asserting
+# that the node panics.

pkg/raft/testdata/confchange_drop_if_unapplied.txt

@@ -0,0 +1,59 @@
+# This test verifies that a config change is not proposed if the leader has
+# unapplied config changes. This ensures a safety requirement stated in
+# https://github.com/etcd-io/etcd/issues/7625#issuecomment-489232411
+
+# The check should be performed even if conf change validation is disabled.
+add-nodes 1 voters=(1) index=2 disable-conf-change-validation=true
+----
+INFO 1 switched to configuration voters=(1)
+INFO 1 became follower at term 0
+INFO newRaft 1 [peers: [1], term: 0, commit: 2, applied: 2, lastindex: 2, lastterm: 1]
+
+campaign 1
+----
+INFO 1 is starting a new election at term 0
+INFO 1 became candidate at term 1
+
+stabilize log-level=none
+----
+ok
+
+# Propose one config change. It should be accepted.
+propose-conf-change 1 transition=explicit
+l2 l3
+----
+ok
+
+# The first config change gets appended.
+process-ready 1
+----
+Ready MustSync=true:
+Entries:
+1/4 EntryConfChangeV2 l2 l3
+
+# Propose another config change. It should be rejected, because the first config
+# change hasn't applied on the leader yet.
+propose-conf-change 1
+l4
+----
+INFO 1 ignoring conf change {ConfChangeTransitionAuto [{ConfChangeAddLearnerNode 4}] []} at config voters=(1): possible unapplied conf change at index 4 (applied to 3)
+
+# The new config change is appended to the log as an empty entry.
+stabilize 1
+----
+> 1 handling Ready
+  Ready MustSync=true:
+  HardState Term:1 Vote:1 Commit:4
+  Entries:
+  1/5 EntryNormal ""
+  CommittedEntries:
+  1/4 EntryConfChangeV2 l2 l3
+  INFO 1 switched to configuration voters=(1)&&(1) learners=(2 3)
+> 1 handling Ready
+  Ready MustSync=false:
+  HardState Term:1 Vote:1 Commit:5
+  CommittedEntries:
+  1/5 EntryNormal ""
+  Messages:
+  1->2 MsgApp Term:1 Log:1/4 Commit:4 Entries:[1/5 EntryNormal ""]
+  1->3 MsgApp Term:1 Log:1/4 Commit:4 Entries:[1/5 EntryNormal ""]