webhook: add support for multiple FPGAs per container

cmd/fpga_admissionwebhook/fpga_admissionwebhook_test.go

@@ -210,7 +210,7 @@ func TestMutatePods(t *testing.T) {
 			},
 			mode:             orchestrated,
 			expectedResponse: true,
-			expectedPatchOps: 4,
+			expectedPatchOps: 5,
 		},
 		{
 			name: "handle error after wrong getPatchOps()",
@@ -250,8 +250,8 @@ func TestMutatePods(t *testing.T) {
 			if err != nil {
 				t.Errorf("Test case '%s': got unparsable patch '%s'", tcase.name, resp.Patch)
 			} else if len(ops.([]interface{})) != tcase.expectedPatchOps {
-				t.Errorf("Test case '%s': got wrong number of operations in the patch. Expected %d, but got %d",
-					tcase.name, tcase.expectedPatchOps, len(ops.([]interface{})))
+				t.Errorf("Test case '%s': got wrong number of operations in the patch. Expected %d, but got %d\n%s",
+					tcase.name, tcase.expectedPatchOps, len(ops.([]interface{})), string(resp.Patch))
 			}
 		}
 	}

cmd/fpga_admissionwebhook/patcher.go

@@ -15,11 +15,12 @@
 package main
 
 import (
-	"encoding/json"
+	"bytes"
 	"fmt"
 	"regexp"
 	"strings"
 	"sync"
+	"text/template"
 
 	"github.com/pkg/errors"
 
@@ -39,16 +40,28 @@ const (
                 "path": "/spec/containers/%d/resources/%s/%s",
                 "value": %s
         }`
-	envAddOp = `{
+	resourceRemoveOp = `{
+                "op": "remove",
+                "path": "/spec/containers/%d/resources/%s/%s"
+        }`
+	resourceAddOp = `{
                 "op": "add",
-                "path": "/spec/containers/%d/env",
-                "value": [{
-                        "name": "FPGA_REGION",
-                        "value": "%s"
-                }, {
-                        "name": "FPGA_AFU",
-                        "value": "%s"
-                } %s]
+                "path": "/spec/containers/%d/resources/%s/%s",
+                "value": "%d"
+        }`
+	envAddOpTpl = `{
+                "op": "add",
+                "path": "/spec/containers/{{- .ContainerIdx -}}/env",
+                "value": [
+                     {{- $first := true -}}
+                     {{- range $key, $value := .EnvVars -}}
+                       {{- if not $first -}},{{- end -}}{
+                          "name": "{{$key}}",
+                          "value": "{{$value}}"
+                       }
+                       {{- $first = false -}}
+                     {{- end -}}
+                ]
         }`
 )
 
@@ -169,10 +182,41 @@ func (p *patcher) translateFpgaResourceName(oldname corev1.ResourceName) (string
 	return "", errors.Errorf("Unknown FPGA resource: %s", rname)
 }
 
+func (p *patcher) checkResourceRequests(container corev1.Container) error {
+	for resourceName, resourceQuantity := range container.Resources.Requests {
+		interfaceID, _, err := p.parseResourceName(string(resourceName))
+		if err != nil {
+			return err
+		}
+
+		if interfaceID == "" {
+			// Skip non-FPGA resources
+			continue
+		}
+		if container.Resources.Limits[resourceName] != resourceQuantity {
+			return errors.Errorf("'limits' and 'requests' for %s must be equal", string(resourceName))
+		}
+	}
+
+	return nil
+}
+
 func (p *patcher) getPatchOpsOrchestrated(containerIdx int, container corev1.Container) ([]string, error) {
 	var ops []string
 
-	mutated := false
+	for _, v := range container.Env {
+		if strings.HasPrefix(v.Name, "FPGA_REGION") || strings.HasPrefix(v.Name, "FPGA_AFU") {
+			return nil, errors.Errorf("The environment variable '%s' is not allowed", v.Name)
+		}
+	}
+
+	if err := p.checkResourceRequests(container); err != nil {
+		return nil, err
+	}
+
+	regions := make(map[string]int64)
+	envVars := make(map[string]string)
+	counter := 0
 	for resourceName, resourceQuantity := range container.Resources.Limits {
 		interfaceID, afuID, err := p.parseResourceName(string(resourceName))
 		if err != nil {
@@ -180,47 +224,52 @@ func (p *patcher) getPatchOpsOrchestrated(containerIdx int, container corev1.Con
 		}
 
 		if interfaceID == "" && afuID == "" {
+			// Skip non-FPGA resources
 			continue
 		}
 
-		if mutated {
-			return nil, errors.Errorf("Only one FPGA resource per container is supported in '%s' mode", orchestrated)
+		if container.Resources.Requests[resourceName] != resourceQuantity {
+			return nil, errors.Errorf("'limits' and 'requests' for %s must be equal", string(resourceName))
 		}
 
-		op := fmt.Sprintf(resourceReplaceOp, containerIdx, "limits", rfc6901Escaper.Replace(string(resourceName)),
-			containerIdx, "limits", rfc6901Escaper.Replace(namespace+"/region-"+interfaceID), resourceQuantity.String())
+		quantity, ok := resourceQuantity.AsInt64()
+		if !ok {
+			return nil, errors.New("Resource quantity isn't of integral type")
+		}
+		regions[interfaceID] = regions[interfaceID] + quantity
+
+		for i := int64(0); i < quantity; i++ {
+			counter++
+			envVars[fmt.Sprintf("FPGA_REGION_%d", counter)] = interfaceID
+			envVars[fmt.Sprintf("FPGA_AFU_%d", counter)] = afuID
+		}
+
+		ops = append(ops, fmt.Sprintf(resourceRemoveOp, containerIdx, "limits", rfc6901Escaper.Replace(string(resourceName))))
+		ops = append(ops, fmt.Sprintf(resourceRemoveOp, containerIdx, "requests", rfc6901Escaper.Replace(string(resourceName))))
+	}
+
+	for interfaceID, quantity := range regions {
+		op := fmt.Sprintf(resourceAddOp, containerIdx, "limits", rfc6901Escaper.Replace(namespace+"/region-"+interfaceID), quantity)
 		ops = append(ops, op)
-
-		if afuID != "" {
-			oldVars, err := getEnvVars(container)
-			if err != nil {
-				return nil, err
-			}
-			op = fmt.Sprintf(envAddOp, containerIdx, interfaceID, afuID, oldVars)
+		op = fmt.Sprintf(resourceAddOp, containerIdx, "requests", rfc6901Escaper.Replace(namespace+"/region-"+interfaceID), quantity)
 		ops = append(ops, op)
 	}
-		mutated = true
-	}
 
-	mutated = false
-	for resourceName, resourceQuantity := range container.Resources.Requests {
-		interfaceID, _, err := p.parseResourceName(string(resourceName))
-		if err != nil {
-			return nil, err
+	if len(envVars) > 0 {
+		for _, envvar := range container.Env {
+			envVars[envvar.Name] = envvar.Value
 		}
-
-		if interfaceID == "" {
-			continue
+		data := struct {
+			ContainerIdx int
+			EnvVars      map[string]string
+		}{
+			ContainerIdx: containerIdx,
+			EnvVars:      envVars,
 		}
-
-		if mutated {
-			return nil, errors.Errorf("Only one FPGA resource per container is supported in '%s' mode", orchestrated)
-		}
-
-		op := fmt.Sprintf(resourceReplaceOp, containerIdx, "requests", rfc6901Escaper.Replace(string(resourceName)),
-			containerIdx, "requests", rfc6901Escaper.Replace(namespace+"/region-"+interfaceID), resourceQuantity.String())
-		ops = append(ops, op)
-		mutated = true
+		t := template.Must(template.New("add_operation").Parse(envAddOpTpl))
+		buf := new(bytes.Buffer)
+		t.Execute(buf, data)
+		ops = append(ops, buf.String())
 	}
 
 	return ops, nil
@@ -259,23 +308,3 @@ func (p *patcher) parseResourceName(input string) (string, string, error) {
 
 	return interfaceID, afuID, nil
 }
-
-func getEnvVars(container corev1.Container) (string, error) {
-	var jsonstrings []string
-	for _, envvar := range container.Env {
-		if envvar.Name == "FPGA_REGION" || envvar.Name == "FPGA_AFU" {
-			return "", errors.Errorf("The env var '%s' is not allowed", envvar.Name)
-		}
-		jsonbytes, err := json.Marshal(envvar)
-		if err != nil {
-			return "", err
-		}
-		jsonstrings = append(jsonstrings, string(jsonbytes))
-	}
-
-	if len(jsonstrings) == 0 {
-		return "", nil
-	}
-
-	return fmt.Sprintf(", %s", strings.Join(jsonstrings, ",")), nil
-}

cmd/fpga_admissionwebhook/patcher_test.go

@@ -229,6 +229,12 @@ func TestGetPatchOpsOrchestrated(t *testing.T) {
 						"cpu": resource.MustParse("1"),
 					},
 				},
+				Env: []corev1.EnvVar{
+					{
+						Name:  "SOME_VAR",
+						Value: "fake value",
+					},
+				},
 			},
 			regionMap: map[string]string{
 				"arria10": "ce48969398f05f33946d560708be108a",
@@ -236,15 +242,15 @@ func TestGetPatchOpsOrchestrated(t *testing.T) {
 			afMap: map[string]string{
 				"arria10-nlb0": "d8424dc4a4a3c413f89e433683f9040b",
 			},
-			expectedOps: 3,
+			expectedOps: 5,
 		},
 		{
-			name: "More than one FPGA in Limits",
+			name: "Unequal FPGA resources in Limits and Requests 1",
 			container: corev1.Container{
 				Resources: corev1.ResourceRequirements{
 					Limits: corev1.ResourceList{
 						"fpga.intel.com/arria10-nlb0": resource.MustParse("1"),
-						"fpga.intel.com/arria10-nlb3": resource.MustParse("1"),
+						"fpga.intel.com/arria10-nlb3": resource.MustParse("2"),
 					},
 				},
 			},
@@ -258,7 +264,7 @@ func TestGetPatchOpsOrchestrated(t *testing.T) {
 			expectedErr: true,
 		},
 		{
-			name: "More than one FPGA in Requests",
+			name: "Unequal FPGA resources in Limits and Requests 2",
 			container: corev1.Container{
 				Resources: corev1.ResourceRequirements{
 					Requests: corev1.ResourceList{
@@ -299,7 +305,7 @@ func TestGetPatchOpsOrchestrated(t *testing.T) {
 			expectedErr: true,
 		},
 		{
-			name: "Unknown FPGA model in Limitss",
+			name: "Unknown FPGA model in Limits",
 			container: corev1.Container{
 				Resources: corev1.ResourceRequirements{
 					Limits: corev1.ResourceList{
@@ -343,6 +349,26 @@ func TestGetPatchOpsOrchestrated(t *testing.T) {
 			},
 			expectedErr: true,
 		},
+		{
+			name: "Wrong type of quantity",
+			container: corev1.Container{
+				Resources: corev1.ResourceRequirements{
+					Limits: corev1.ResourceList{
+						"fpga.intel.com/arria10-nlb0": resource.MustParse("1.1"),
+					},
+					Requests: corev1.ResourceList{
+						"fpga.intel.com/arria10-nlb0": resource.MustParse("1.1"),
+					},
+				},
+			},
+			regionMap: map[string]string{
+				"arria10": "ce48969398f05f33946d560708be108a",
+			},
+			afMap: map[string]string{
+				"arria10-nlb0": "d8424dc4a4a3c413f89e433683f9040b",
+			},
+			expectedErr: true,
+		},
 	}
 
 	for _, tt := range tcases {
@@ -350,6 +376,7 @@ func TestGetPatchOpsOrchestrated(t *testing.T) {
 			afMap:     tt.afMap,
 			regionMap: tt.regionMap,
 		}
+		debug.Print(tt.name)
 		ops, err := p.getPatchOpsOrchestrated(0, tt.container)
 		if tt.expectedErr && err == nil {
 			t.Errorf("Test case '%s': no error returned", tt.name)
@@ -362,88 +389,3 @@ func TestGetPatchOpsOrchestrated(t *testing.T) {
 		}
 	}
 }
-
-func TestGetEnvVars(t *testing.T) {
-	tcases := []struct {
-		name        string
-		env         []corev1.EnvVar
-		expected    string
-		expectedErr bool
-	}{
-		{
-			name: "Successful result",
-			env: []corev1.EnvVar{
-				{
-					Name:  "VARNAME1",
-					Value: "2",
-					ValueFrom: &corev1.EnvVarSource{
-						ResourceFieldRef: &corev1.ResourceFieldSelector{
-							Resource: "limits.cpu",
-							Divisor:  resource.MustParse("1"),
-						},
-					},
-				},
-				{
-					Name:  "VARNAME2",
-					Value: "4",
-					ValueFrom: &corev1.EnvVarSource{
-						ResourceFieldRef: &corev1.ResourceFieldSelector{
-							Resource: "limits.cpu",
-							Divisor:  resource.MustParse("1"),
-						},
-					},
-				},
-			},
-			expected: `, {"name":"VARNAME1","value":"2","valueFrom":{"resourceFieldRef":{"resource":"limits.cpu","divisor":"1"}}},{"name":"VARNAME2","value":"4","valueFrom":{"resourceFieldRef":{"resource":"limits.cpu","divisor":"1"}}}`,
-		},
-		{
-			name: "Disallowed env variable FPGA_REGION",
-			env: []corev1.EnvVar{
-				{
-					Name:  "FPGA_REGION",
-					Value: "fake value",
-				},
-			},
-			expectedErr: true,
-		},
-		{
-			name: "Disallowed env variable FPGA_AFU",
-			env: []corev1.EnvVar{
-				{
-					Name:  "FPGA_AFU",
-					Value: "fake value",
-				},
-			},
-			expectedErr: true,
-		},
-	}
-
-	for _, tt := range tcases {
-		container := corev1.Container{
-			Name:  "test-container",
-			Image: "test-image",
-			Resources: corev1.ResourceRequirements{
-				Limits: corev1.ResourceList{
-					"cpu": resource.MustParse("1"),
-					"fpga.intel.com/arria10": resource.MustParse("1"),
-				},
-				Requests: corev1.ResourceList{
-					"cpu": resource.MustParse("1"),
-					"fpga.intel.com/arria10": resource.MustParse("1"),
-				},
-			},
-			Env: tt.env,
-		}
-
-		output, err := getEnvVars(container)
-		if output != tt.expected {
-			t.Errorf("Test case '%s': wrong output: %s", tt.name, output)
-		}
-		if tt.expectedErr && err == nil {
-			t.Errorf("Test case '%s': no error returned", tt.name)
-		}
-		if !tt.expectedErr && err != nil {
-			t.Errorf("Test case '%s': unexpected error %+v", tt.name, err)
-		}
-	}
-}