refactor and remove <= and >=

Signed-off-by: Victor Vieux <vieux@docker.com>

contrib/demo.sh

@@ -43,16 +43,16 @@ docker run -d -p 80:80 nginx
 # clean up cluster
 docker rm -f `docker ps -aq`
 
-docker run -d -e "constraint:operatingsystem=fedora*" redis
+docker run -d -e "constraint:operatingsystem==fedora*" redis
 docker ps
 
-docker run -d -e constraint:storagedriver=devicemapper redis
+docker run -d -e constraint:storagedriver==devicemapper redis
 docker ps
 
-docker run -d -e constraint:storagedriver=aufs redis
+docker run -d -e constraint:storagedriver==aufs redis
 docker ps
 
-docker run -d -e constraint:node=fedora-1 redis
+docker run -d -e constraint:node==fedora-1 redis
 docker ps
 
 # clean up cluster

scheduler/filter/README.md

@@ -157,8 +157,7 @@ A `value` must be one of the following:
 * A globbing pattern, i.e., `abc*`.
 * A regular expression in the form of `/regexp/`. We support the Go's regular expression syntax.
 
-Current `swarm` supports affinity/constraint operators as the following: `==`, `!=`, `>=` and `<=`.
+Current `swarm` supports affinity/constraint operators as the following: `==` and `!=`.
-Relative comparisons, `>=` and `<=` are supported, but limited to `string` comparison only.
 
 For example,
 * `constraint:name==node1` will match nodes named with `node1`.
@@ -169,8 +168,6 @@ For example,
 * `constraint:node!=/node-[01]-id/` will match all nodes, except those with ids `node-0-id` and `node-1-id`.
 * `constraint:name!=/foo\[bar\]/` will match all nodes, except those with name `foo[bar]`. You can see the use of escape characters here.
 * `constraint:name==/(?i)node1/` will match all nodes named with `node1` case-insensitive. So 'NoDe1' or 'NODE1' will also matched.
-* `constraint:kernel>=3.0` will match all nodes with label `kernel` greater than or equal to "3.0". This is the string, not numeric, comparison.
-* `constraint:group<=3` will match all nodes with `group`  less than or equal to "3". This is also the string, not numeric, comparison.
 
 ## Port Filter
 

scheduler/filter/affinity.go

@@ -13,26 +13,20 @@ type AffinityFilter struct {
 }
 
 func (f *AffinityFilter) Filter(config *dockerclient.ContainerConfig, nodes []*cluster.Node) ([]*cluster.Node, error) {
-	affinities, err := extractEnv("affinity", config.Env)
+	affinities, err := parseExprs("affinity", config.Env)
 	if err != nil {
 		return nil, err
 	}
 
-	for k, v := range affinities {
+	for _, affinity := range affinities {
-		log.Debugf("matching affinity: %s%s%s", k, v[0], v[1])
+		log.Debugf("matching affinity: %s%s%s", affinity.key, OPERATORS[affinity.operator], affinity.value)
 
 		candidates := []*cluster.Node{}
 		for _, node := range nodes {
-			switch k {
+			switch affinity.key {
 			case "container":
 				for _, container := range node.Containers() {
-					matchResult := false
+					if affinity.Match(container.Id, container.Names[0]) {
-					if v[0] != "!=" {
-						matchResult = match(v, container.Id) || match(v, container.Names[0])
-					} else if v[0] == "!=" {
-						matchResult = match(v, container.Id) && match(v, container.Names[0])
-					}
-					if matchResult {
 						candidates = append(candidates, node)
 						break
 					}
@@ -40,12 +34,12 @@ func (f *AffinityFilter) Filter(config *dockerclient.ContainerConfig, nodes []*c
 			case "image":
 			done:
 				for _, image := range node.Images() {
-					if match(v, image.Id) {
+					if affinity.Match(image.Id) {
 						candidates = append(candidates, node)
 						break
 					}
 					for _, tag := range image.RepoTags {
-						if match(v, tag) {
+						if affinity.Match(tag) {
 							candidates = append(candidates, node)
 							break done
 						}
@@ -54,7 +48,7 @@ func (f *AffinityFilter) Filter(config *dockerclient.ContainerConfig, nodes []*c
 			}
 		}
 		if len(candidates) == 0 {
-			return nil, fmt.Errorf("unable to find a node that satisfies %s%s%s", k, v[0], v[1])
+			return nil, fmt.Errorf("unable to find a node that satisfies %s%s%s", affinity.key, OPERATORS[affinity.operator], affinity.value)
 		}
 		nodes = candidates
 	}

scheduler/filter/constraint.go

@@ -13,38 +13,32 @@ type ConstraintFilter struct {
 }
 
 func (f *ConstraintFilter) Filter(config *dockerclient.ContainerConfig, nodes []*cluster.Node) ([]*cluster.Node, error) {
-	constraints, err := extractEnv("constraint", config.Env)
+	constraints, err := parseExprs("constraint", config.Env)
 	if err != nil {
 		return nil, err
 	}
 
-	for k, v := range constraints {
+	for _, constraint := range constraints {
-		log.Debugf("matching constraint: %s %s %s", k, v[0], v[1])
+		log.Debugf("matching constraint: %s %s %s", constraint.key, OPERATORS[constraint.operator], constraint.value)
 
 		candidates := []*cluster.Node{}
 		for _, node := range nodes {
-			switch k {
+			switch constraint.key {
 			case "node":
 				// "node" label is a special case pinning a container to a specific node.
-				matchResult := false
+				if constraint.Match(node.ID, node.Name) {
-				if v[0] != "!=" {
-					matchResult = match(v, node.ID) || match(v, node.Name)
-				} else if v[0] == "!=" {
-					matchResult = match(v, node.ID) && match(v, node.Name)
-				}
-				if matchResult {
 					candidates = append(candidates, node)
 				}
 			default:
-				if label, ok := node.Labels[k]; ok {
+				if label, ok := node.Labels[constraint.key]; ok {
-					if match(v, label) {
+					if constraint.Match(label) {
 						candidates = append(candidates, node)
 					}
 				}
 			}
 		}
 		if len(candidates) == 0 {
-			return nil, fmt.Errorf("unable to find a node that satisfies %s%s%s", k, v[0], v[1])
+			return nil, fmt.Errorf("unable to find a node that satisfies %s%s%s", constraint.key, OPERATORS[constraint.operator], constraint.value)
 		}
 		nodes = candidates
 	}

scheduler/filter/constraint_test.go

@@ -240,6 +240,7 @@ func TestFilterRegExpCaseInsensitive(t *testing.T) {
 }
 
 func TestFilterWithRelativeComparisons(t *testing.T) {
+	t.Skip()
 	var (
 		f      = ConstraintFilter{}
 		nodes  = testFixtures()

scheduler/filter/expr.go

@@ -0,0 +1,106 @@
+package filter
+
+import (
+	"fmt"
+	"regexp"
+	"strings"
+
+	log "github.com/Sirupsen/logrus"
+)
+
+const (
+	EQ = iota
+	NOTEQ
+)
+
+var OPERATORS = []string{"==", "!="}
+
+type expr struct {
+	key      string
+	operator int
+	value    string
+}
+
+func parseExprs(key string, env []string) ([]expr, error) {
+	exprs := []expr{}
+	for _, e := range env {
+		if strings.HasPrefix(e, key+":") {
+			entry := strings.TrimPrefix(e, key+":")
+			found := false
+			for i, op := range OPERATORS {
+				if strings.Contains(entry, op) {
+					// split with the op
+					parts := strings.SplitN(entry, op, 2)
+
+					// validate key
+					// allow alpha-numeric
+					matched, err := regexp.MatchString(`^(?i)[a-z_][a-z0-9\-_]+$`, parts[0])
+					if err != nil {
+						return nil, err
+					}
+					if matched == false {
+						return nil, fmt.Errorf("Key '%s' is invalid", parts[0])
+					}
+
+					if len(parts) == 2 {
+
+						// validate value
+						// allow leading = in case of using ==
+						// allow * for globbing
+						// allow regexp
+						matched, err := regexp.MatchString(`^(?i)[=!\/]?[a-z0-9:\-_\.\*/\(\)\?\+\[\]\\\^\$]+$`, parts[1])
+						if err != nil {
+							return nil, err
+						}
+						if matched == false {
+							return nil, fmt.Errorf("Value '%s' is invalid", parts[1])
+						}
+						exprs = append(exprs, expr{key: strings.ToLower(parts[0]), operator: i, value: parts[1]})
+					} else {
+						exprs = append(exprs, expr{key: strings.ToLower(parts[0]), operator: i})
+					}
+
+					found = true
+					break // found an op, move to next entry
+				}
+			}
+			if !found {
+				return nil, fmt.Errorf("One of operator ==, != is expected")
+			}
+		}
+	}
+	return exprs, nil
+}
+
+func (e *expr) Match(whats ...string) bool {
+	var (
+		pattern string
+		match   bool
+		err     error
+	)
+
+	if e.value[0] == '/' && e.value[len(e.value)-1] == '/' {
+		// regexp
+		pattern = e.value[1 : len(e.value)-1]
+	} else {
+		// simple match, create the regex for globbing (ex: ub*t* -> ^ub.*t.*$) and match.
+		pattern = "^" + strings.Replace(e.value, "*", ".*", -1) + "$"
+	}
+
+	for _, what := range whats {
+		if match, err = regexp.MatchString(pattern, what); match {
+			break
+		} else if err != nil {
+			log.Error(err)
+		}
+	}
+
+	switch e.operator {
+	case EQ:
+		return match
+	case NOTEQ:
+		return !match
+	}
+
+	return false
+}

scheduler/filter/expr_test.go

@@ -0,0 +1,59 @@
+package filter
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestParseExprs(t *testing.T) {
+	// Cannot use the leading digit for key
+	_, err := parseExprs("constraint", []string{"constraint:1node"})
+	assert.Error(t, err)
+
+	// Cannot use space in key
+	_, err = parseExprs("constraint", []string{"constraint:node ==node1"})
+	assert.Error(t, err)
+
+	// Cannot use dot in key
+	_, err = parseExprs("constraint", []string{"constraint:no.de==node1"})
+	assert.Error(t, err)
+
+	// Cannot use * in key
+	_, err = parseExprs("constraint", []string{"constraint:no*de==node1"})
+	assert.Error(t, err)
+
+	// Allow leading underscore
+	_, err = parseExprs("constraint", []string{"constraint:_node==_node1"})
+	assert.NoError(t, err)
+
+	// Allow globbing
+	_, err = parseExprs("constraint", []string{"constraint:node==*node*"})
+	assert.NoError(t, err)
+
+	// Allow regexp in value
+	_, err = parseExprs("constraint", []string{"constraint:node==/(?i)^[a-b]+c*$/"})
+	assert.NoError(t, err)
+}
+
+func TestMatch(t *testing.T) {
+	e := expr{operator: EQ, value: "foo"}
+	assert.True(t, e.Match("foo"))
+	assert.False(t, e.Match("bar"))
+	assert.True(t, e.Match("foo", "bar"))
+
+	e = expr{operator: NOTEQ, value: "foo"}
+	assert.False(t, e.Match("foo"))
+	assert.True(t, e.Match("bar"))
+	assert.False(t, e.Match("foo", "bar"))
+
+	e = expr{operator: EQ, value: "f*o"}
+	assert.True(t, e.Match("foo"))
+	assert.True(t, e.Match("fuo"))
+	assert.True(t, e.Match("foo", "fuo", "bar"))
+
+	e = expr{operator: NOTEQ, value: "f*o"}
+	assert.False(t, e.Match("foo"))
+	assert.False(t, e.Match("fuo"))
+	assert.False(t, e.Match("foo", "fuo", "bar"))
+}

scheduler/filter/utils.go

@@ -1,100 +0,0 @@
-package filter
-
-import (
-	"fmt"
-	"regexp"
-	"strings"
-
-	log "github.com/Sirupsen/logrus"
-)
-
-type opWithValue []string
-
-func extractEnv(key string, env []string) (map[string]opWithValue, error) {
-	ops := []string{"==", "!=", ">=", "<="}
-	values := make(map[string]opWithValue)
-	for _, e := range env {
-		if strings.HasPrefix(e, key+":") {
-			entry := strings.TrimPrefix(e, key+":")
-			found := false
-			for _, op := range ops {
-				if strings.Contains(entry, op) {
-					// split with the op
-					parts := strings.SplitN(entry, op, 2)
-
-					// validate key
-					// allow alpha-numeric
-					matched, err := regexp.MatchString(`^(?i)[a-z_][a-z0-9\-_]+$`, parts[0])
-					if err != nil {
-						return nil, err
-					}
-					if matched == false {
-						return nil, fmt.Errorf("Key '%s' is invalid", parts[0])
-					}
-
-					if len(parts) == 2 {
-
-						// validate value
-						// allow leading = in case of using ==
-						// allow * for globbing
-						// allow regexp
-						matched, err := regexp.MatchString(`^(?i)[=!\/]?[a-z0-9:\-_\.\*/\(\)\?\+\[\]\\\^\$]+$`, parts[1])
-						if err != nil {
-							return nil, err
-						}
-						if matched == false {
-							return nil, fmt.Errorf("Value '%s' is invalid", parts[1])
-						}
-						values[strings.ToLower(parts[0])] = opWithValue{op, parts[1]}
-					} else {
-						values[strings.ToLower(parts[0])] = opWithValue{op, ""}
-					}
-
-					found = true
-					break // found an op, move to next entry
-				}
-			}
-			if !found {
-				return nil, fmt.Errorf("One of operator ==, !=, >=, <= is expected")
-			}
-		}
-	}
-	return values, nil
-}
-
-// Create the regex for globbing (ex: ub*t* -> ^ub.*t.*$) and match.
-// If useRegex is true, the pattern will be used directly
-func internalMatch(pattern, s string) bool {
-	regex := pattern
-	useRegex := false
-	if strings.HasPrefix(pattern, "/") && strings.HasSuffix(pattern, "/") {
-		log.Debugf("regex detected")
-		regex = strings.TrimPrefix(strings.TrimSuffix(pattern, "/"), "/")
-		useRegex = true
-	}
-
-	if !useRegex {
-		regex = "^" + strings.Replace(pattern, "*", ".*", -1) + "$"
-	}
-	matched, err := regexp.MatchString(regex, s)
-	if err != nil {
-		log.Error(err)
-	}
-	return matched
-}
-
-func match(val opWithValue, what string) bool {
-	op, v := val[0], val[1]
-	if op == ">=" && what >= v {
-		return true
-	} else if op == "<=" && what <= v {
-		return true
-	} else {
-		matchResult := internalMatch(v, what)
-		if (op == "!=" && !matchResult) || (op == "==" && matchResult) {
-			return true
-		}
-	}
-
-	return false
-}

scheduler/filter/utils_test.go

@@ -1,38 +0,0 @@
-package filter
-
-import (
-	"github.com/stretchr/testify/assert"
-	"testing"
-)
-
-func TestValidatingEnvExtraction(t *testing.T) {
-	err := error(nil)
-
-	// Cannot use the leading digit for key
-	_, err = extractEnv("constraint", []string{"constraint:1node"})
-	assert.Error(t, err)
-
-	// Cannot use space in key
-	_, err = extractEnv("constraint", []string{"constraint:node ==node1"})
-	assert.Error(t, err)
-
-	// Cannot use dot in key
-	_, err = extractEnv("constraint", []string{"constraint:no.de==node1"})
-	assert.Error(t, err)
-
-	// Cannot use * in key
-	_, err = extractEnv("constraint", []string{"constraint:no*de==node1"})
-	assert.Error(t, err)
-
-	// Allow leading underscore
-	_, err = extractEnv("constraint", []string{"constraint:_node==_node1"})
-	assert.NoError(t, err)
-
-	// Allow globbing
-	_, err = extractEnv("constraint", []string{"constraint:node==*node*"})
-	assert.NoError(t, err)
-
-	// Allow regexp in value
-	_, err = extractEnv("constraint", []string{"constraint:node==/(?i)^[a-b]+c*$/"})
-	assert.NoError(t, err)
-}