github/intel-device-plugins-for-kubernetes
7
0
Fork 0
mirror of https://github.com/intel/intel-device-plugins-for-kubernetes.git synced 2025-06-03 03:59:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

fpga_webhook: never reject already mutated CRs

Browse Source 
For some reason the API server may want to pass an already mutated CR
through the webhook once again. The webhook must accept such CR with
no additional transformations.

This patch adds support for such idempotence by maintaining
a set of identity mappings which effectively resolve to themselves. No
patching is applied to them.
This commit is contained in:
Dmitry Rozhkov 2022-03-14 18:13:32 +02:00
parent 42689adc21
commit 2cb914225c
3 changed files with 197 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

60
pkg/fpgacontroller/patcher/patcher.go
View File

@ -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 {

178
pkg/fpgacontroller/patcher/patcher_test.go
View File

@ -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")
regionAlias := &fpgav2.FpgaRegion{
ObjectMeta: metav1.ObjectMeta{
Name: "arria10-alias",
},
Spec: fpgav2.FpgaRegionSpec{
InterfaceID: "ce48969398f05f33946d560708be108a",
},
}
if len(p.resourceModeMap) != 1 || len(p.afMap) != 1 || len(p.resourceMap) != 1 {
t.Error("Failed to add AF 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},
},
}
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")
}
p.AddRegion(region)
if len(p.resourceModeMap) != 1 || len(p.resourceMap) != 1 {
t.Error("Failed to add fpga region to patcher")
}
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)
}

4
pkg/internal/containers/containers.go
View File

@ -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
}