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

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
 }