Merge pull request #920 from rojkov/patcher-mappings

mythi · web-flow · commit 0999eb23ad95 · 2022-03-29T12:09:28.000+03:00
fpga_webhook: never reject already mutated CRs
diff --git a/pkg/fpgacontroller/patcher/patcher.go b/pkg/fpgacontroller/patcher/patcher.go
@@ -80,6 +80,11 @@ type Patcher struct {
 	afMap           map[string]*fpgav2.AcceleratorFunction
 	resourceMap     map[string]string
 	resourceModeMap map[string]string
+
+	// This set is needed to maintain the webhook's idempotence: it must be possible to
+	// resolve actual resources (AFs and regions) to themselves. For this we build
+	// a set of identities which must be accepted by the webhook without any transformation.
+	identitySet map[string]int
 }
 
 func newPatcher(log logr.Logger) *Patcher {
@@ -88,6 +93,21 @@ func newPatcher(log logr.Logger) *Patcher {
 		afMap:           make(map[string]*fpgav2.AcceleratorFunction),
 		resourceMap:     make(map[string]string),
 		resourceModeMap: make(map[string]string),
+		identitySet:     make(map[string]int),
+	}
+}
+
+func (p *Patcher) incIdentity(id string) {
+	// Initialize to 1 or increment by 1.
+	p.identitySet[id] = p.identitySet[id] + 1
+}
+
+func (p *Patcher) decIdentity(id string) {
+	counter := p.identitySet[id]
+	if counter > 1 {
+		p.identitySet[id] = counter - 1
+	} else {
+		delete(p.identitySet, id)
 	}
 }
 
@@ -103,9 +123,13 @@ func (p *Patcher) AddAf(accfunc *fpgav2.AcceleratorFunction) error {
 			return err
 		}
 
-		p.resourceMap[namespace+"/"+accfunc.Name] = rfc6901Escaper.Replace(namespace + "/" + devtype)
+		mapping := rfc6901Escaper.Replace(namespace + "/" + devtype)
+		p.resourceMap[namespace+"/"+accfunc.Name] = mapping
+		p.incIdentity(mapping)
 	} else {
-		p.resourceMap[namespace+"/"+accfunc.Name] = rfc6901Escaper.Replace(namespace + "/region-" + accfunc.Spec.InterfaceID)
+		mapping := rfc6901Escaper.Replace(namespace + "/region-" + accfunc.Spec.InterfaceID)
+		p.resourceMap[namespace+"/"+accfunc.Name] = mapping
+		p.incIdentity(mapping)
 	}
 
 	p.resourceModeMap[namespace+"/"+accfunc.Name] = accfunc.Spec.Mode
@@ -118,24 +142,32 @@ func (p *Patcher) AddRegion(region *fpgav2.FpgaRegion) {
 	p.Lock()
 
 	p.resourceModeMap[namespace+"/"+region.Name] = regiondevel
-	p.resourceMap[namespace+"/"+region.Name] = rfc6901Escaper.Replace(namespace + "/region-" + region.Spec.InterfaceID)
+	mapping := rfc6901Escaper.Replace(namespace + "/region-" + region.Spec.InterfaceID)
+	p.resourceMap[namespace+"/"+region.Name] = mapping
+	p.incIdentity(mapping)
 }
 
 func (p *Patcher) RemoveAf(name string) {
 	defer p.Unlock()
 	p.Lock()
 
-	delete(p.afMap, namespace+"/"+name)
-	delete(p.resourceMap, namespace+"/"+name)
-	delete(p.resourceModeMap, namespace+"/"+name)
+	nname := namespace + "/" + name
+
+	p.decIdentity(p.resourceMap[nname])
+	delete(p.afMap, nname)
+	delete(p.resourceMap, nname)
+	delete(p.resourceModeMap, nname)
 }
 
 func (p *Patcher) RemoveRegion(name string) {
 	defer p.Unlock()
 	p.Lock()
 
-	delete(p.resourceMap, namespace+"/"+name)
-	delete(p.resourceModeMap, namespace+"/"+name)
+	nname := namespace + "/" + name
+
+	p.decIdentity(p.resourceMap[nname])
+	delete(p.resourceMap, nname)
+	delete(p.resourceModeMap, nname)
 }
 
 // sanitizeContainer filters out env variables reserved for CRI hook.
@@ -160,6 +192,16 @@ func sanitizeContainer(container corev1.Container) corev1.Container {
 	return container
 }
 
+func (p *Patcher) getNoopsOrError(name string) ([]string, error) {
+	if _, isVirtual := p.identitySet[rfc6901Escaper.Replace(name)]; isVirtual {
+		// `name` is not a real mapping, but a virtual one for an actual resource which
+		// needs to be resolved to itself with no transformations.
+		return []string{}, nil
+	}
+
+	return nil, errors.Errorf("no such resource: %q", name)
+}
+
 func (p *Patcher) getPatchOps(containerIdx int, container corev1.Container) ([]string, error) {
 	container = sanitizeContainer(container)
 
@@ -180,7 +222,7 @@ func (p *Patcher) getPatchOps(containerIdx int, container corev1.Container) ([]s
 	for rname, quantity := range requestedResources {
 		mode, found := p.resourceModeMap[rname]
 		if !found {
-			return nil, errors.Errorf("no such resource: %q", rname)
+			return p.getNoopsOrError(rname)
 		}
 
 		switch mode {
diff --git a/pkg/fpgacontroller/patcher/patcher_test.go b/pkg/fpgacontroller/patcher/patcher_test.go
@@ -31,13 +31,27 @@ func init() {
 	_ = flag.Set("v", "4")
 }
 
+func checkExpectedError(t *testing.T, expectedErr bool, err error, testName string) {
+	t.Helper()
+
+	if expectedErr && err == nil {
+		t.Errorf("Test case '%s': no error returned", testName)
+	}
+
+	if !expectedErr && err != nil {
+		t.Errorf("Test case '%s': unexpected error: %+v", testName, err)
+	}
+}
+
 func TestPatcherStorageFunctions(t *testing.T) {
 	goodAf := &fpgav2.AcceleratorFunction{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "arria10-nlb0",
 		},
 		Spec: fpgav2.AcceleratorFunctionSpec{
-			AfuID: "d8424dc4a4a3c413f89e433683f9040b",
+			AfuID:       "d8424dc4a4a3c413f89e433683f9040b",
+			InterfaceID: "ce48969398f05f33946d560708be108a",
+			Mode:        af,
 		},
 	}
 	brokenAf := &fpgav2.AcceleratorFunction{
@@ -58,37 +72,103 @@ func TestPatcherStorageFunctions(t *testing.T) {
 			InterfaceID: "ce48969398f05f33946d560708be108a",
 		},
 	}
-
-	p := newPatcher(ctrl.Log.WithName("test"))
-
-	if err := p.AddAf(goodAf); err != nil {
-		t.Error("unexpected error")
-	}
-
-	if len(p.resourceModeMap) != 1 || len(p.afMap) != 1 || len(p.resourceMap) != 1 {
-		t.Error("Failed to add AF to patcher")
-	}
-
-	if err := p.AddAf(brokenAf); err == nil {
-		t.Error("AddAf() must fail")
-	}
-
-	p.RemoveAf(goodAf.Name)
-
-	if len(p.resourceModeMap) != 0 || len(p.afMap) != 0 || len(p.resourceMap) != 0 {
-		t.Error("Failed to remove AF from patcher")
+	regionAlias := &fpgav2.FpgaRegion{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "arria10-alias",
+		},
+		Spec: fpgav2.FpgaRegionSpec{
+			InterfaceID: "ce48969398f05f33946d560708be108a",
+		},
 	}
 
-	p.AddRegion(region)
-
-	if len(p.resourceModeMap) != 1 || len(p.resourceMap) != 1 {
-		t.Error("Failed to add fpga region to patcher")
+	tcases := []struct {
+		name                        string
+		afsToAdd                    []*fpgav2.AcceleratorFunction
+		afsToRemove                 []*fpgav2.AcceleratorFunction
+		regionsToAdd                []*fpgav2.FpgaRegion
+		regionsToRemove             []*fpgav2.FpgaRegion
+		expectedResourceModeMapSize int
+		expectedResourceMapSize     int
+		expectedIdentitySetSize     int
+		expectedErr                 bool
+	}{
+		{
+			name:                        "Add one good af",
+			afsToAdd:                    []*fpgav2.AcceleratorFunction{goodAf},
+			expectedResourceModeMapSize: 1,
+			expectedResourceMapSize:     1,
+			expectedIdentitySetSize:     1,
+		},
+		{
+			name:        "Add one broken af",
+			afsToAdd:    []*fpgav2.AcceleratorFunction{brokenAf},
+			expectedErr: true,
+		},
+		{
+			name:        "Add one and remove one good af",
+			afsToAdd:    []*fpgav2.AcceleratorFunction{goodAf},
+			afsToRemove: []*fpgav2.AcceleratorFunction{goodAf},
+		},
+		{
+			name:                        "Add one good region",
+			regionsToAdd:                []*fpgav2.FpgaRegion{region},
+			expectedResourceModeMapSize: 1,
+			expectedResourceMapSize:     1,
+			expectedIdentitySetSize:     1,
+		},
+		{
+			name:            "Add one and remove one good region",
+			regionsToAdd:    []*fpgav2.FpgaRegion{region},
+			regionsToRemove: []*fpgav2.FpgaRegion{region},
+		},
+		{
+			name:                        "Add one region and one alias then remove one alias",
+			regionsToAdd:                []*fpgav2.FpgaRegion{region, regionAlias},
+			regionsToRemove:             []*fpgav2.FpgaRegion{region},
+			expectedResourceModeMapSize: 1,
+			expectedResourceMapSize:     1,
+			expectedIdentitySetSize:     1,
+		},
+		{
+			name:                        "Add one region and one alias",
+			regionsToAdd:                []*fpgav2.FpgaRegion{region, regionAlias},
+			expectedResourceModeMapSize: 2,
+			expectedResourceMapSize:     2,
+			expectedIdentitySetSize:     1,
+		},
+		{
+			name:            "Add one region and one alias then remove all",
+			regionsToAdd:    []*fpgav2.FpgaRegion{region, regionAlias},
+			regionsToRemove: []*fpgav2.FpgaRegion{region, regionAlias},
+		},
 	}
 
-	p.RemoveRegion(region.Name)
-
-	if len(p.resourceModeMap) != 0 || len(p.resourceMap) != 0 {
-		t.Error("Failed to remove fpga region from patcher")
+	for _, tt := range tcases {
+		t.Run(tt.name, func(t *testing.T) {
+			p := newPatcher(ctrl.Log.WithName("test"))
+			for _, af := range tt.afsToAdd {
+				err := p.AddAf(af)
+				checkExpectedError(t, tt.expectedErr, err, tt.name)
+			}
+			for _, reg := range tt.regionsToAdd {
+				p.AddRegion(reg)
+			}
+			for _, af := range tt.afsToRemove {
+				p.RemoveAf(af.Name)
+			}
+			for _, reg := range tt.regionsToRemove {
+				p.RemoveRegion(reg.Name)
+			}
+			if tt.expectedResourceModeMapSize != len(p.resourceModeMap) {
+				t.Errorf("wrong size of resourceModeMap. Expected %d, but got %d", tt.expectedResourceModeMapSize, len(p.resourceModeMap))
+			}
+			if tt.expectedResourceMapSize != len(p.resourceMap) {
+				t.Errorf("wrong size of resourceMap. Expected %d, but got %d", tt.expectedResourceMapSize, len(p.resourceMap))
+			}
+			if tt.expectedIdentitySetSize != len(p.identitySet) {
+				t.Errorf("wrong size of identitySet. Expected %d, but got %d", tt.expectedIdentitySetSize, len(p.identitySet))
+			}
+		})
 	}
 }
 
@@ -245,6 +325,41 @@ func TestGetPatchOps(t *testing.T) {
 			},
 			expectedOps: 4,
 		},
+		{
+			name: "Skip handling for an identity mapping",
+			container: corev1.Container{
+				Resources: corev1.ResourceRequirements{
+					Limits: corev1.ResourceList{
+						"fpga.intel.com/af-ce4.d84.zkiWk5jwXzOUbVYHCL4QithCTcSko8QT-J5DNoP5BAs": resource.MustParse("1"),
+					},
+					Requests: corev1.ResourceList{
+						"fpga.intel.com/af-ce4.d84.zkiWk5jwXzOUbVYHCL4QithCTcSko8QT-J5DNoP5BAs": resource.MustParse("1"),
+					},
+				},
+			},
+			afs: []*fpgav2.AcceleratorFunction{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "arria10-nlb0",
+					},
+					Spec: fpgav2.AcceleratorFunctionSpec{
+						AfuID:       "d8424dc4a4a3c413f89e433683f9040b",
+						InterfaceID: "ce48969398f05f33946d560708be108a",
+						Mode:        af,
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "arria10-nlb0-alias",
+					},
+					Spec: fpgav2.AcceleratorFunctionSpec{
+						AfuID:       "d8424dc4a4a3c413f89e433683f9040b",
+						InterfaceID: "ce48969398f05f33946d560708be108a",
+						Mode:        af,
+					},
+				},
+			},
+		},
 		{
 			name: "Successful handling for regiondevel mode",
 			container: corev1.Container{
@@ -399,12 +514,7 @@ func TestGetPatchOps(t *testing.T) {
 				p.AddRegion(region)
 			}
 			ops, err := p.getPatchOps(0, tt.container)
-			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)
-			}
+			checkExpectedError(t, tt.expectedErr, err, tt.name)
 			if len(ops) != tt.expectedOps {
 				t.Errorf("test case '%s': expected %d ops, but got %d\n%v", tt.name, tt.expectedOps, len(ops), ops)
 			}
diff --git a/pkg/internal/containers/containers.go b/pkg/internal/containers/containers.go
@@ -27,7 +27,7 @@ func GetRequestedResources(container corev1.Container, ns string) (map[string]in
 	// Container may happen to have Requests, but not Limits. Check Requests first,
 	// then in the next loop iterate over Limits.
 	for resourceName, resourceQuantity := range container.Resources.Requests {
-		rname := strings.ToLower(string(resourceName))
+		rname := string(resourceName)
 		if !strings.HasPrefix(rname, ns) {
 			continue
 		}
@@ -42,7 +42,7 @@ func GetRequestedResources(container corev1.Container, ns string) (map[string]in
 	resources := make(map[string]int64)
 
 	for resourceName, resourceQuantity := range container.Resources.Limits {
-		rname := strings.ToLower(string(resourceName))
+		rname := string(resourceName)
 		if !strings.HasPrefix(rname, ns) {
 			continue
 		}
