diff --git a/cmd/fpga_admissionwebhook/fpga_admissionwebhook_test.go b/cmd/fpga_admissionwebhook/fpga_admissionwebhook_test.go index b2ec9c59..32dce0e4 100644 --- a/cmd/fpga_admissionwebhook/fpga_admissionwebhook_test.go +++ b/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)) } } } diff --git a/cmd/fpga_admissionwebhook/patcher.go b/cmd/fpga_admissionwebhook/patcher.go index e350e578..0696d53d 100644 --- a/cmd/fpga_admissionwebhook/patcher.go +++ b/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()) - ops = append(ops, op) - - if afuID != "" { - oldVars, err := getEnvVars(container) - if err != nil { - return nil, err - } - op = fmt.Sprintf(envAddOp, containerIdx, interfaceID, afuID, oldVars) - ops = append(ops, op) + quantity, ok := resourceQuantity.AsInt64() + if !ok { + return nil, errors.New("Resource quantity isn't of integral type") } - mutated = true + 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)))) } - mutated = false - for resourceName, resourceQuantity := range container.Resources.Requests { - interfaceID, _, err := p.parseResourceName(string(resourceName)) - if err != nil { - return nil, err - } - - if interfaceID == "" { - continue - } - - 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()) + for interfaceID, quantity := range regions { + op := fmt.Sprintf(resourceAddOp, containerIdx, "limits", rfc6901Escaper.Replace(namespace+"/region-"+interfaceID), quantity) ops = append(ops, op) - mutated = true + op = fmt.Sprintf(resourceAddOp, containerIdx, "requests", rfc6901Escaper.Replace(namespace+"/region-"+interfaceID), quantity) + ops = append(ops, op) + } + + if len(envVars) > 0 { + for _, envvar := range container.Env { + envVars[envvar.Name] = envvar.Value + } + data := struct { + ContainerIdx int + EnvVars map[string]string + }{ + ContainerIdx: containerIdx, + EnvVars: envVars, + } + 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 -} diff --git a/cmd/fpga_admissionwebhook/patcher_test.go b/cmd/fpga_admissionwebhook/patcher_test.go index bb2522d5..05b0247b 100644 --- a/cmd/fpga_admissionwebhook/patcher_test.go +++ b/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) - } - } -}