config/storage: per-replica constraints, improved locality handling

This creates a new allowable format for zone config constraints, which
lets the user apply different sets of constraints to different numbers
of replicas within a zone. See the included tests for what the new format
looks like.

Fixes cockroachdb#19985

It also improves the handling of localities that are more or less full
than one another for some reason outside the system's control (e.g. if
one has more nodes than the others or if some constraints have required
more ranges to be in one). In such cases, this does a better job of
balancing ranges amongst the nodes that are more or less full because we
compare them amongst each other now rather than comparing them to the
StoreList averages of all the stores in the cluster.

Fixes cockroachdb#20751

This also deprecates positive (non-required, non-prohibited) constraints.
New positive constraints cannot be set, and existing positive
constraints will be ignored.

Release note (cli change): Replication zone constraints can now be
specified on a per-replica basis, meaning you can configure some
replicas in a zone's ranges to follow one set of constraints and other
replicas to follow other constraints.

Release note (backwards-incompatible change): Positive constraints in
replication zone configs no longer work. They have never been
documented or officially supported, but will no longer be allowed at
all. Any existing positive constraints will be ignored.

pkg/ccl/cliccl/cli_test.go

@@ -118,15 +118,15 @@ func Example_cclzone() {
 	// gc:
 	//   ttlseconds: 90000
 	// num_replicas: 1
-	// constraints: [us-east-1a, ssd]
+	// constraints: [+us-east-1a, +ssd]
 	// zone get db.t.p0
 	// db.t@primary
 	// range_min_bytes: 1048576
 	// range_max_bytes: 67108864
 	// gc:
 	//   ttlseconds: 90000
 	// num_replicas: 1
-	// constraints: [us-east-1a, ssd]
+	// constraints: [+us-east-1a, +ssd]
 	// zone get db.t
 	// .default
 	// range_min_bytes: 1048576
@@ -149,23 +149,23 @@ func Example_cclzone() {
 	// gc:
 	//   ttlseconds: 90000
 	// num_replicas: 3
-	// constraints: [us-east-1a, ssd]
+	// constraints: [+us-east-1a, +ssd]
 	// zone get db.t.p1
 	// db.t.p1
 	// range_min_bytes: 1048576
 	// range_max_bytes: 134217728
 	// gc:
 	//   ttlseconds: 90000
 	// num_replicas: 3
-	// constraints: [us-east-1a, ssd]
+	// constraints: [+us-east-1a, +ssd]
 	// zone get db.t.p0
 	// db.t@primary
 	// range_min_bytes: 1048576
 	// range_max_bytes: 67108864
 	// gc:
 	//   ttlseconds: 90000
 	// num_replicas: 1
-	// constraints: [us-east-1a, ssd]
+	// constraints: [+us-east-1a, +ssd]
 	// zone ls
 	// .default
 	// .liveness
@@ -190,7 +190,7 @@ func Example_cclzone() {
 	// gc:
 	//   ttlseconds: 90000
 	// num_replicas: 3
-	// constraints: [us-east-1a, ssd]
+	// constraints: [+us-east-1a, +ssd]
 	// zone ls
 	// .default
 	// .liveness

pkg/ccl/partitionccl/partition_test.go

@@ -33,6 +33,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/pkg/errors"
+	yaml "gopkg.in/yaml.v2"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -169,16 +170,11 @@ func (t *partitioningTest) parse() error {
 			}
 		}
 
-		for _, constraintStr := range strings.Split(constraints, `,`) {
-			if constraintStr == "" {
-				continue
-			}
-			var c config.Constraint
-			if err := c.FromString(constraintStr); err != nil {
-				return errors.Wrapf(err, "parsing constraint: %s", constraintStr)
-			}
-			subzone.Config.Constraints.Constraints = append(subzone.Config.Constraints.Constraints, c)
+		var parsedConstraints config.ConstraintsList
+		if err := yaml.UnmarshalStrict([]byte("["+constraints+"]"), &parsedConstraints); err != nil {
+			return errors.Wrapf(err, "parsing constraints: %s", constraints)
 		}
+		subzone.Config.Constraints = ([]config.Constraints)(parsedConstraints)
 
 		t.parsed.subzones = append(t.parsed.subzones, subzone)
 	}

pkg/cli/cli_test.go

@@ -473,7 +473,7 @@ func Example_zone() {
 	// gc:
 	//   ttlseconds: 90000
 	// num_replicas: 1
-	// constraints: [us-east-1a, ssd]
+	// constraints: [+us-east-1a, +ssd]
 	// zone ls
 	// .default
 	// .liveness
@@ -505,7 +505,7 @@ func Example_zone() {
 	// gc:
 	//   ttlseconds: 90000
 	// num_replicas: 1
-	// constraints: [us-east-1a, ssd]
+	// constraints: [+us-east-1a, +ssd]
 	// zone set system.descriptor --file=./testdata/zone_attrs.yaml
 	// pq: cannot set zone configs for system config tables; try setting your config on the entire "system" database instead
 	// zone set system.namespace --file=./testdata/zone_attrs.yaml
@@ -518,15 +518,15 @@ func Example_zone() {
 	// gc:
 	//   ttlseconds: 90000
 	// num_replicas: 3
-	// constraints: [us-east-1a, ssd]
+	// constraints: [+us-east-1a, +ssd]
 	// zone get system
 	// system
 	// range_min_bytes: 1048576
 	// range_max_bytes: 134217728
 	// gc:
 	//   ttlseconds: 90000
 	// num_replicas: 3
-	// constraints: [us-east-1a, ssd]
+	// constraints: [+us-east-1a, +ssd]
 	// zone rm system
 	// CONFIGURE ZONE 1
 	// zone ls

pkg/cli/testdata/zone_attrs.yaml

@@ -1,2 +1,2 @@
 num_replicas: 1
-constraints: [us-east-1a,ssd]
+constraints: [+us-east-1a,+ssd]

pkg/config/zone.go

@@ -25,7 +25,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/pkg/errors"
-	yaml "gopkg.in/yaml.v2"
 )
 
 // Several ranges outside of the SQL keyspace are given special names so they
@@ -226,6 +225,9 @@ func (c Constraint) String() string {
 
 // FromString populates the constraint from the constraint shorthand notation.
 func (c *Constraint) FromString(short string) error {
+	if len(short) == 0 {
+		return fmt.Errorf("the empty string is not a valid constraint")
+	}
 	switch short[0] {
 	case '+':
 		c.Type = Constraint_REQUIRED
@@ -234,7 +236,7 @@ func (c *Constraint) FromString(short string) error {
 		c.Type = Constraint_PROHIBITED
 		short = short[1:]
 	default:
-		c.Type = Constraint_POSITIVE
+		c.Type = Constraint_DEPRECATED_POSITIVE
 	}
 	parts := strings.Split(short, "=")
 	if len(parts) == 1 {
@@ -248,34 +250,6 @@ func (c *Constraint) FromString(short string) error {
 	return nil
 }
 
-var _ yaml.Marshaler = Constraints{}
-var _ yaml.Unmarshaler = &Constraints{}
-
-// MarshalYAML implements yaml.Marshaler.
-func (c Constraints) MarshalYAML() (interface{}, error) {
-	short := make([]string, len(c.Constraints))
-	for i, c := range c.Constraints {
-		short[i] = c.String()
-	}
-	return short, nil
-}
-
-// UnmarshalYAML implements yaml.Unmarshaler.
-func (c *Constraints) UnmarshalYAML(unmarshal func(interface{}) error) error {
-	var shortConstraints []string
-	if err := unmarshal(&shortConstraints); err != nil {
-		return err
-	}
-	constraints := make([]Constraint, len(shortConstraints))
-	for i, short := range shortConstraints {
-		if err := constraints[i].FromString(short); err != nil {
-			return err
-		}
-	}
-	c.Constraints = constraints
-	return nil
-}
-
 // minRangeMaxBytes is the minimum value for range max bytes.
 const minRangeMaxBytes = 64 << 10 // 64 KB
 
@@ -321,8 +295,8 @@ func TestingSetDefaultZoneConfig(cfg ZoneConfig) func() {
 	}
 }
 
-// Validate returns an error if the ZoneConfig specifies a known-dangerous
-// configuration.
+// Validate returns an error if the ZoneConfig specifies a known-dangerous or
+// disallowed configuration.
 func (z *ZoneConfig) Validate() error {
 	for _, s := range z.Subzones {
 		if err := s.Config.Validate(); err != nil {
@@ -348,6 +322,43 @@ func (z *ZoneConfig) Validate() error {
 		return fmt.Errorf("RangeMinBytes %d is greater than or equal to RangeMaxBytes %d",
 			z.RangeMinBytes, z.RangeMaxBytes)
 	}
+
+	for _, constraints := range z.Constraints {
+		for _, constraint := range constraints.Constraints {
+			if constraint.Type == Constraint_DEPRECATED_POSITIVE {
+				return fmt.Errorf("constraints must either be required (prefixed with a '+') or " +
+					"prohibited (prefixed with a '-')")
+			}
+		}
+	}
+
+	// We only need to further validate constraints if per-replica constraints
+	// are in use. The old style of constraints that apply to all replicas don't
+	// require validation.
+	if len(z.Constraints) > 1 || (len(z.Constraints) == 1 && z.Constraints[0].NumReplicas != 0) {
+		var numConstrainedRepls int64
+		for _, constraints := range z.Constraints {
+			if constraints.NumReplicas <= 0 {
+				return fmt.Errorf("constraints must apply to at least one replica")
+			}
+			numConstrainedRepls += int64(constraints.NumReplicas)
+			for _, constraint := range constraints.Constraints {
+				if constraint.Type != Constraint_REQUIRED && constraints.NumReplicas != z.NumReplicas {
+					return fmt.Errorf(
+						"only required constraints (prefixed with a '+') can be applied to a subset of replicas")
+				}
+				if strings.Contains(constraint.Key, ":") || strings.Contains(constraint.Value, ":") {
+					return fmt.Errorf("the ':' character is not allowed in constraint keys or values")
+				}
+			}
+		}
+		// TODO(a-robinson): Relax this constraint, as discussed on #22412.
+		if numConstrainedRepls != int64(z.NumReplicas) {
+			return fmt.Errorf(
+				"the number of replicas specified in constraints (%d) does not equal the number of replicas configured for the zone (%d)",
+				numConstrainedRepls, z.NumReplicas)
+		}
+	}
 	return nil
 }