From b1542155d7ab029386d4ea9b7ead80992a966635 Mon Sep 17 00:00:00 2001 From: Lucas Roesler Date: Sat, 23 Jun 2018 11:18:56 +0200 Subject: [PATCH] Refactor label parsing to set OF labels last **What** - Ensure that internal OF labels are set after user labels to ensure that users do no override these internal values - Refactor the getMinReplicaCount to work with the labels pointer, this helps simplify the control flow in the handler methods - Add constants for the label keys and update all references to those keys throughout Closes #209 Signed-off-by: Lucas Roesler --- handlers/delete.go | 2 +- handlers/deploy.go | 38 ++--------------- handlers/labels.go | 55 +++++++++++++++++++++++++ handlers/lables_test.go | 91 +++++++++++++++++++++++++++++++++++++++++ handlers/reader.go | 2 +- handlers/update.go | 17 ++------ 6 files changed, 155 insertions(+), 50 deletions(-) create mode 100644 handlers/labels.go create mode 100644 handlers/lables_test.go diff --git a/handlers/delete.go b/handlers/delete.go index fc05075aa..54da6e0b1 100644 --- a/handlers/delete.go +++ b/handlers/delete.go @@ -64,7 +64,7 @@ func MakeDeleteHandler(functionNamespace string, clientset *kubernetes.Clientset func isFunction(deployment *v1beta1.Deployment) bool { if deployment != nil { - if _, found := deployment.Labels["faas_function"]; found { + if _, found := deployment.Labels[OFFunctionNameLabel]; found { return true } } diff --git a/handlers/deploy.go b/handlers/deploy.go index 27af35667..d9ac1d77b 100644 --- a/handlers/deploy.go +++ b/handlers/deploy.go @@ -12,7 +12,6 @@ import ( "os" "path/filepath" "regexp" - "strconv" "strings" "github.com/openfaas/faas/gateway/requests" @@ -28,9 +27,6 @@ import ( // watchdogPort for the OpenFaaS function watchdog const watchdogPort = 8080 -// initialReplicasCount how many replicas to start of creating for a function -const initialReplicasCount = 1 - // ValidateDeployRequest validates that the service name is valid for Kubernetes func ValidateDeployRequest(request *requests.CreateFunctionRequest) error { // Regex for RFC-1123 validation: @@ -164,22 +160,9 @@ func makeDeploymentSpec(request requests.CreateFunctionRequest, existingSecrets FailureThreshold: 3, } - initialReplicas := int32p(initialReplicasCount) - labels := map[string]string{ - "faas_function": request.Service, - } - - if request.Labels != nil { - if min := getMinReplicaCount(*request.Labels); min != nil { - initialReplicas = min - } - for k, v := range *request.Labels { - labels[k] = v - } - } - + initialReplicas := getMinReplicaCount(request.Labels) + labels := parseLabels(request.Service, request.Labels) nodeSelector := createSelector(request.Constraints) - resources, resourceErr := createResources(request) if resourceErr != nil { @@ -209,7 +192,7 @@ func makeDeploymentSpec(request requests.CreateFunctionRequest, existingSecrets Spec: v1beta1.DeploymentSpec{ Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{ - "faas_function": request.Service, + OFFunctionNameLabel: request.Service, }, }, Replicas: initialReplicas, @@ -282,7 +265,7 @@ func makeServiceSpec(request requests.CreateFunctionRequest) *v1.Service { Spec: v1.ServiceSpec{ Type: v1.ServiceTypeClusterIP, Selector: map[string]string{ - "faas_function": request.Service, + OFFunctionNameLabel: request.Service, }, Ports: []v1.ServicePort{ { @@ -396,19 +379,6 @@ func createResources(request requests.CreateFunctionRequest) (*apiv1.ResourceReq return resources, nil } -func getMinReplicaCount(labels map[string]string) *int32 { - if value, exists := labels["com.openfaas.scale.min"]; exists { - minReplicas, err := strconv.Atoi(value) - if err == nil && minReplicas > 0 { - return int32p(int32(minReplicas)) - } - - log.Println(err) - } - - return nil -} - // configureReadOnlyRootFilesystem will create or update the required settings and mounts to ensure // that the ReadOnlyRootFilesystem setting works as expected, meaning: // 1. when ReadOnlyRootFilesystem is true, the security context of the container will have ReadOnlyRootFilesystem also diff --git a/handlers/labels.go b/handlers/labels.go new file mode 100644 index 000000000..3adbab7a9 --- /dev/null +++ b/handlers/labels.go @@ -0,0 +1,55 @@ +package handlers + +import ( + "log" + "strconv" +) + +const ( + // initialReplicasCount how many replicas to start of creating for a function, this is + // also used as the default return value for getMinReplicaCount + initialReplicasCount = 1 + + // OFFunctionNameLabel is the label key used by OpenFaaS to store the function name + // on the resources managed by OpenFaaS for that function. This key is also used to + // denote that a resource is a "Function" + OFFunctionNameLabel = "faas_function" + // OFFunctionMinReplicaCount is a label that user's can set and will be passed to Kubernetes + // as the Deployment replicas value. + OFFunctionMinReplicaCount = "com.openfaas.scale.min" +) + +// parseLabels will copy the user request labels and ensure that any required internal labels +// are set appropriately. +func parseLabels(functionName string, requestLables *map[string]string) map[string]string { + labels := map[string]string{} + if requestLables != nil { + for k, v := range *requestLables { + labels[k] = v + } + } + + labels[OFFunctionNameLabel] = functionName + + return labels +} + +// getMinReplicaCount extracts the functions minimum replica count from the user's +// request labels. If the value is not found, this will return the default value, 1. +func getMinReplicaCount(labels *map[string]string) *int32 { + if labels == nil { + return int32p(initialReplicasCount) + } + + l := *labels + if value, exists := l[OFFunctionMinReplicaCount]; exists { + minReplicas, err := strconv.Atoi(value) + if err == nil && minReplicas > 0 { + return int32p(int32(minReplicas)) + } + + log.Println(err) + } + + return int32p(initialReplicasCount) +} diff --git a/handlers/lables_test.go b/handlers/lables_test.go new file mode 100644 index 000000000..043678543 --- /dev/null +++ b/handlers/lables_test.go @@ -0,0 +1,91 @@ +package handlers + +import "testing" + +func Test_getMinReplicaCount(t *testing.T) { + scenarios := []struct { + name string + labels *map[string]string + output int + }{ + { + name: "nil map returns default", + labels: nil, + output: initialReplicasCount, + }, + { + name: "empty map returns default", + labels: &map[string]string{}, + output: initialReplicasCount, + }, + { + name: "empty map returns default", + labels: &map[string]string{OFFunctionMinReplicaCount: "2"}, + output: 2, + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + output := getMinReplicaCount(s.labels) + if output == nil { + t.Errorf("getMinReplicaCount should not return nil pointer") + } + + value := int(*output) + if value != s.output { + t.Errorf("expected: %d, got %d", s.output, value) + } + }) + } +} + +func Test_parseLabels(t *testing.T) { + scenarios := []struct { + name string + labels *map[string]string + functionName string + output map[string]string + }{ + { + name: "nil map returns just the function name", + labels: nil, + functionName: "testFunc", + output: map[string]string{OFFunctionNameLabel: "testFunc"}, + }, + { + name: "empty map returns just the function name", + labels: &map[string]string{}, + functionName: "testFunc", + output: map[string]string{OFFunctionNameLabel: "testFunc"}, + }, + { + name: "non-empty map does not overwrite the function name label", + labels: &map[string]string{OFFunctionNameLabel: "anotherValue", "customLabel": "test"}, + functionName: "testFunc", + output: map[string]string{OFFunctionNameLabel: "testFunc", "customLabel": "test"}, + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + output := parseLabels(s.functionName, s.labels) + if output == nil { + t.Errorf("parseLabels should not return nil map") + } + + outputFuncName := output[OFFunctionNameLabel] + if outputFuncName != s.functionName { + t.Errorf("parseLabels should always set the function name: expected %s, got %s", s.functionName, outputFuncName) + } + + for key, value := range output { + expectedValue := s.output[key] + if value != expectedValue { + t.Errorf("Incorrect output label for %s, expected: %s, got %s", key, expectedValue, value) + } + } + + }) + } +} diff --git a/handlers/reader.go b/handlers/reader.go index 89db7f107..7d2aa1812 100644 --- a/handlers/reader.go +++ b/handlers/reader.go @@ -39,7 +39,7 @@ func getServiceList(functionNamespace string, clientset *kubernetes.Clientset) ( functions := []requests.Function{} listOpts := metav1.ListOptions{ - LabelSelector: "faas_function", + LabelSelector: OFFunctionNameLabel, } res, err := clientset.ExtensionsV1beta1().Deployments(functionNamespace).List(listOpts) diff --git a/handlers/update.go b/handlers/update.go index c7313f33c..abaecc878 100644 --- a/handlers/update.go +++ b/handlers/update.go @@ -70,23 +70,12 @@ func updateDeploymentSpec( deployment.Spec.Template.Spec.NodeSelector = createSelector(request.Constraints) - labels := map[string]string{ - "faas_function": request.Service, - "uid": fmt.Sprintf("%d", time.Now().Nanosecond()), - } - - if request.Labels != nil { - if min := getMinReplicaCount(*request.Labels); min != nil { - deployment.Spec.Replicas = min - } - - for k, v := range *request.Labels { - labels[k] = v - } - } + labels := parseLabels(request.Service, request.Labels) + labels["uid"] = fmt.Sprintf("%d", time.Now().Nanosecond()) deployment.Labels = labels deployment.Spec.Template.ObjectMeta.Labels = labels + deployment.Spec.Replicas = getMinReplicaCount(request.Labels) deployment.Annotations = annotations deployment.Spec.Template.Annotations = annotations