From 870cc0cc329e33217d390a38cac5450d3ae21ac2 Mon Sep 17 00:00:00 2001 From: Nik Mitra Date: Wed, 24 Sep 2025 11:30:41 -0700 Subject: [PATCH 1/3] Modify daemon and config to only manage listed resource names --- deployment/ib-kubernetes-configmap.yaml | 1 + pkg/config/config.go | 24 +++++++++++++++++++++ pkg/daemon/daemon.go | 28 +++++++++++++++++-------- 3 files changed, 44 insertions(+), 9 deletions(-) diff --git a/deployment/ib-kubernetes-configmap.yaml b/deployment/ib-kubernetes-configmap.yaml index de6b6bc..264ad42 100644 --- a/deployment/ib-kubernetes-configmap.yaml +++ b/deployment/ib-kubernetes-configmap.yaml @@ -11,3 +11,4 @@ data: # DEFAULT_LIMITED_PARTITION: "0x0001" # optional ENABLE_IP_OVER_IB: "false" # default false ENABLE_INDEX0_FOR_PRIMARY_PKEY: "true" # default true + MANAGED_RESOURCE_NAMES: "" # required to be non-empty diff --git a/pkg/config/config.go b/pkg/config/config.go index ac9ce52..86165a9 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -2,6 +2,7 @@ package config import ( "fmt" + "strings" "github.com/caarlos0/env/v11" "github.com/rs/zerolog/log" @@ -21,6 +22,9 @@ type DaemonConfig struct { EnableIPOverIB bool `env:"ENABLE_IP_OVER_IB" envDefault:"false"` // Enable index0 for primary pkey GUID additions EnableIndex0ForPrimaryPkey bool `env:"ENABLE_INDEX0_FOR_PRIMARY_PKEY" envDefault:"true"` + // Managed resource names + ManagedResourcesString string `env:"MANAGED_RESOURCE_NAMES"` + ManagedResources map[string]bool } type GUIDPoolConfig struct { @@ -55,6 +59,17 @@ func (dc *DaemonConfig) ReadConfig() error { log.Info().Msg("Default limited partition is not set.") } + // If managed resource names is set - log at startup + log.Info().Msgf("ib-kubernetes will manage the following resources: %s.", dc.ManagedResourcesString) + // Parse the managed resource names string into a set + dc.ManagedResources = make(map[string]bool) + for _, resource := range strings.Split(dc.ManagedResourcesString, ",") { + if resource == "" { + continue + } + dc.ManagedResources[resource] = true + } + return err } @@ -67,5 +82,14 @@ func (dc *DaemonConfig) ValidateConfig() error { if dc.Plugin == "" { return fmt.Errorf("no plugin selected") } + + if len(dc.ManagedResources) == 0 { + return fmt.Errorf("no managed resources names were provided") + } return nil } + +func (dc *DaemonConfig) IsManagedResource(resourceName string) bool { + _, ok := dc.ManagedResources[resourceName] + return ok +} diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 7ef6d90..92b1366 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -112,9 +112,9 @@ func NewDaemon() (Daemon, error) { // Pass configuration from daemon to the plugin pluginConfig := map[string]interface{}{ - "ENABLE_IP_OVER_IB": daemonConfig.EnableIPOverIB, - "DEFAULT_LIMITED_PARTITION": daemonConfig.DefaultLimitedPartition, - "ENABLE_INDEX0_FOR_PRIMARY_PKEY": daemonConfig.EnableIndex0ForPrimaryPkey, + "ENABLE_IP_OVER_IB": daemonConfig.EnableIPOverIB, + "DEFAULT_LIMITED_PARTITION": daemonConfig.DefaultLimitedPartition, + "ENABLE_INDEX0_FOR_PRIMARY_PKEY": daemonConfig.EnableIndex0ForPrimaryPkey, } if err := smClient.SetConfig(pluginConfig); err != nil { log.Warn().Msgf("Failed to set configuration on subnet manager plugin: %v", err) @@ -206,6 +206,14 @@ func (d *daemon) getIbSriovNetwork(networkID string) (string, *utils.IbSriovCniS } log.Debug().Msgf("networkName attachment %v", netAttInfo) + // Check if this network's resource is managed by this daemon + resourceName := netAttInfo.Annotations["k8s.v1.cni.cncf.io/resourceName"] + if resourceName == "" || !d.config.IsManagedResource(resourceName) { + // TODO(Nik) dev qol, check if someone else manages this resource or if it is orphan + // checkResourceOwner(networkNamespace, networkName) + return "", nil, fmt.Errorf("network %s uses resource %s which is not managed by this daemon", networkName, resourceName) + } + networkSpec := make(map[string]interface{}) err = json.Unmarshal([]byte(netAttInfo.Spec.Config), &networkSpec) if err != nil { @@ -245,9 +253,10 @@ func getPodNetworkInfo(netName string, pod *kapi.Pod, netMap networksMap) (*podN } // addPodFinalizer adds the GUID cleanup finalizer to a pod -func (d *daemon) addPodFinalizer(pod *kapi.Pod) error { +func (d *daemon) addPodFinalizer(pod *kapi.Pod, networkName string) error { return wait.ExponentialBackoff(backoffValues, func() (bool, error) { - if err := d.kubeClient.AddFinalizerToPod(pod, PodGUIDFinalizer); err != nil { + podFinalizer := fmt.Sprintf("%s-%s", PodGUIDFinalizer, networkName) + if err := d.kubeClient.AddFinalizerToPod(pod, podFinalizer); err != nil { log.Warn().Msgf("failed to add finalizer to pod %s/%s: %v", pod.Namespace, pod.Name, err) return false, nil @@ -257,9 +266,10 @@ func (d *daemon) addPodFinalizer(pod *kapi.Pod) error { } // removePodFinalizer removes the GUID cleanup finalizer from a pod -func (d *daemon) removePodFinalizer(pod *kapi.Pod) error { +func (d *daemon) removePodFinalizer(pod *kapi.Pod, networkName string) error { return wait.ExponentialBackoff(backoffValues, func() (bool, error) { - if err := d.kubeClient.RemoveFinalizerFromPod(pod, PodGUIDFinalizer); err != nil { + podFinalizer := fmt.Sprintf("%s-%s", PodGUIDFinalizer, networkName) + if err := d.kubeClient.RemoveFinalizerFromPod(pod, podFinalizer); err != nil { log.Warn().Msgf("failed to remove finalizer from pod %s/%s: %v", pod.Namespace, pod.Name, err) return false, nil @@ -557,7 +567,7 @@ func (d *daemon) AddPeriodicUpdate() { } // Add finalizer to pod since it now has a GUID that needs cleanup - if err = d.addPodFinalizer(pi.pod); err != nil { + if err = d.addPodFinalizer(pi.pod, networkName); err != nil { log.Error().Msgf("failed to add finalizer to pod %s/%s: %v", pi.pod.Namespace, pi.pod.Name, err) continue } else { @@ -728,7 +738,7 @@ func (d *daemon) DeletePeriodicUpdate() { // Remove finalizer from pod after successfully cleaning up GUID if pod, exists := podGUIDMap[guidAddr.String()]; exists { - if err = d.removePodFinalizer(pod); err != nil { + if err = d.removePodFinalizer(pod, networkName); err != nil { log.Error().Msgf("failed to remove finalizer from pod %s/%s: %v", pod.Namespace, pod.Name, err) } else { log.Info().Msgf("removed finalizer %s from pod %s/%s", From 21e2e8ca26ddfac9a18f10df600043bdf210d365 Mon Sep 17 00:00:00 2001 From: Nik Mitra Date: Wed, 24 Sep 2025 11:31:25 -0700 Subject: [PATCH 2/3] Add testing for multi ib-kubernetes environments --- Makefile | 5 +- go.mod | 5 + go.sum | 24 + hack/crds/network-attachment-definition.yaml | 44 ++ pkg/config/config_test.go | 23 +- pkg/daemon/daemon_e2e_test.go | 541 +++++++++++++++++++ pkg/daemon/daemon_suite_test.go | 98 ++++ pkg/daemon/daemon_test.go | 490 +++++++++++++++++ 8 files changed, 1223 insertions(+), 7 deletions(-) create mode 100644 hack/crds/network-attachment-definition.yaml create mode 100644 pkg/daemon/daemon_e2e_test.go create mode 100644 pkg/daemon/daemon_suite_test.go create mode 100644 pkg/daemon/daemon_test.go diff --git a/Makefile b/Makefile index 97a1996..8305f21 100644 --- a/Makefile +++ b/Makefile @@ -140,8 +140,9 @@ test-race: GOFLAGS=-race ## Run tests with race detector $(TEST_TARGETS): NAME=$(MAKECMDGOALS:test-%=%) $(TEST_TARGETS): test -test: | plugins; $(info running $(NAME:%=% )tests...) @ ## Run tests - $Q $(GO) test $(GOFLAGS) -timeout $(TIMEOUT)s $(ARGS) ./... +LOCALBIN ?= $(shell pwd)/bin +test: | envtest plugins; $(info running $(NAME:%=% )tests...) @ ## Run tests + $Q KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" $(GO) test $(GOFLAGS) -timeout $(TIMEOUT)s $(ARGS) ./... .PHONY: test-coverage test-coverage: | plugins-coverage envtest gocovmerge gcov2lcov ## Run coverage tests diff --git a/go.mod b/go.mod index dc8472a..cc705ae 100644 --- a/go.mod +++ b/go.mod @@ -23,9 +23,11 @@ require ( github.com/containernetworking/cni v1.2.0-rc1 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/emicklei/go-restful/v3 v3.12.1 // indirect + github.com/evanphx/json-patch/v5 v5.9.0 // indirect github.com/fsnotify/fsnotify v1.7.0 // indirect github.com/fxamacker/cbor/v2 v2.7.0 // indirect github.com/go-logr/logr v1.4.2 // indirect + github.com/go-logr/zapr v1.3.0 // indirect github.com/go-openapi/jsonpointer v0.21.0 // indirect github.com/go-openapi/jsonreference v0.21.0 // indirect github.com/go-openapi/swag v0.23.0 // indirect @@ -52,6 +54,8 @@ require ( github.com/spf13/pflag v1.0.5 // indirect github.com/stretchr/objx v0.5.2 // indirect github.com/x448/float16 v0.8.4 // indirect + go.uber.org/multierr v1.11.0 // indirect + go.uber.org/zap v1.26.0 // indirect golang.org/x/net v0.29.0 // indirect golang.org/x/oauth2 v0.23.0 // indirect golang.org/x/sys v0.25.0 // indirect @@ -65,6 +69,7 @@ require ( gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect + k8s.io/apiextensions-apiserver v0.31.0 // indirect k8s.io/klog/v2 v2.130.1 // indirect k8s.io/kube-openapi v0.0.0-20240903163716-9e1beecbcb38 // indirect k8s.io/utils v0.0.0-20240902221715-702e33fdd3c3 // indirect diff --git a/go.sum b/go.sum index fba2974..06bc3c4 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,11 @@ github.com/Masterminds/semver/v3 v3.2.1 h1:RN9w6+7QoMeJVGyfmbcgs28Br8cvmnucEXnY0rYXWg0= github.com/Masterminds/semver/v3 v3.2.1/go.mod h1:qvl/7zhW3nngYb5+80sSMF+FG2BjYrf8m9wsX0PNOMQ= +github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= +github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= github.com/caarlos0/env/v11 v11.2.2 h1:95fApNrUyueipoZN/EhA8mMxiNxrBwDa+oAZrMWl3Kg= github.com/caarlos0/env/v11 v11.2.2/go.mod h1:JBfcdeQiBoI3Zh1QRAWfe+tpiNTmDtcCj/hHHHMx0vc= +github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= +github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/containernetworking/cni v1.2.0-rc1 h1:AKI3+pXtgY4PDLN9+50o9IaywWVuey0Jkw3Lvzp0HCY= github.com/containernetworking/cni v1.2.0-rc1/go.mod h1:Lt0TQcZQVDju64fYxUhDziTgXCDe3Olzi9I4zZJLWHg= github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSVTIJ3seZv2GcEnc= @@ -11,6 +15,8 @@ github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/emicklei/go-restful/v3 v3.12.1 h1:PJMDIM/ak7btuL8Ex0iYET9hxM3CI2sjZtzpL63nKAU= github.com/emicklei/go-restful/v3 v3.12.1/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc= +github.com/evanphx/json-patch/v5 v5.9.0 h1:kcBlZQbplgElYIlo/n1hJbls2z/1awpXxpRi0/FOJfg= +github.com/evanphx/json-patch/v5 v5.9.0/go.mod h1:VNkHZ/282BpEyt/tObQO8s5CMPmYYq14uClGH4abBuQ= github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo= github.com/fsnotify/fsnotify v1.4.9/go.mod h1:znqG4EE+3YCdAaPaxE2ZRY/06pZUdp0tY4IgpuI1SZQ= github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA= @@ -33,6 +39,8 @@ github.com/go-task/slim-sprig/v3 v3.0.0/go.mod h1:W848ghGpv3Qj3dhTPRyJypKRiqCdHZ github.com/godbus/dbus/v5 v5.0.4/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5xrFpKfA= github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= +github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da h1:oI5xCqsCo564l8iNU+DwB5epxmsaqB+rhGL0m5jtYqE= +github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.4.0-rc.1/go.mod h1:ceaxUfeHdC40wWswd/P6IGgMaK3YpKi5j83Wpe3EHw8= github.com/golang/protobuf v1.4.0-rc.1.0.20200221234624-67d41d38c208/go.mod h1:xKAWHe0F5eneWXFV3EuXVDTCmh+JuBKY0li0aMyXATA= @@ -105,6 +113,14 @@ github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINE github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/prometheus/client_golang v1.19.1 h1:wZWJDwK+NameRJuPGDhlnFgx8e8HN3XHQeLaYJFJBOE= +github.com/prometheus/client_golang v1.19.1/go.mod h1:mP78NwGzrVks5S2H6ab8+ZZGJLZUq1hoULYBAYBw1Ho= +github.com/prometheus/client_model v0.6.1 h1:ZKSh/rekM+n3CeS952MLRAdFwIKqeY8b62p8ais2e9E= +github.com/prometheus/client_model v0.6.1/go.mod h1:OrxVMOVHjw3lKMa8+x6HeMGkHMQyHDk9E3jmP2AmGiY= +github.com/prometheus/common v0.55.0 h1:KEi6DK7lXW/m7Ig5i47x0vRzuBsHuvJdi5ee6Y3G1dc= +github.com/prometheus/common v0.55.0/go.mod h1:2SECS4xJG1kd8XF9IcM1gMX6510RAEL65zxzNImwdc8= +github.com/prometheus/procfs v0.15.1 h1:YagwOFzUgYfKKHX6Dr+sHT7km/hxC76UB0learggepc= +github.com/prometheus/procfs v0.15.1/go.mod h1:fB45yRUv8NstnjriLhBQLuOUt+WW4BsoGhij/e3PBqk= github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4= github.com/rs/xid v1.5.0/go.mod h1:trrq9SKmegXys3aeAKXMUTdJsYXVwGY3RLcfgqegfbg= @@ -125,6 +141,8 @@ github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= +go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= +go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0= go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y= go.uber.org/zap v1.26.0 h1:sI7k6L95XOKS281NhVKOFCUNIvv9e0w4BF8N3u+tCRo= @@ -132,6 +150,8 @@ go.uber.org/zap v1.26.0/go.mod h1:dtElttAiwGvoJ/vj4IwHBS/gXsEu/pZ50mUIRWuG0so= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= +golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 h1:2dVuKD2vS7b0QIHQbpyTISPd0LeHDbnYEryqj5Q1ug8= +golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56/go.mod h1:M4RDyNAINzryxdtnbRXRL/OHtkFuWGRjvuhBJpk2IlY= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -181,6 +201,8 @@ golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8T golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +gomodules.xyz/jsonpatch/v2 v2.4.0 h1:Ci3iUJyx9UeRx7CeFN8ARgGbkESwJK+KB9lLcWxY/Zw= +gomodules.xyz/jsonpatch/v2 v2.4.0/go.mod h1:AH3dM2RI6uoBZxn3LVrfvJ3E0/9dG4cSrbuBJT4moAY= google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8= google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0= google.golang.org/protobuf v0.0.0-20200228230310-ab0ca4ff8a60/go.mod h1:cfTl7dwQJ+fmap5saPgwCLgHXTUD7jkjRqWcaiX5VyM= @@ -209,6 +231,8 @@ gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= k8s.io/api v0.31.0 h1:b9LiSjR2ym/SzTOlfMHm1tr7/21aD7fSkqgD/CVJBCo= k8s.io/api v0.31.0/go.mod h1:0YiFF+JfFxMM6+1hQei8FY8M7s1Mth+z/q7eF1aJkTE= +k8s.io/apiextensions-apiserver v0.31.0 h1:fZgCVhGwsclj3qCw1buVXCV6khjRzKC5eCFt24kyLSk= +k8s.io/apiextensions-apiserver v0.31.0/go.mod h1:b9aMDEYaEe5sdK+1T0KU78ApR/5ZVp4i56VacZYEHxk= k8s.io/apimachinery v0.31.0 h1:m9jOiSr3FoSSL5WO9bjm1n6B9KROYYgNZOb4tyZ1lBc= k8s.io/apimachinery v0.31.0/go.mod h1:rsPdaZJfTfLsNJSQzNHQvYoTmxhoOEofxtOsF3rtsMo= k8s.io/client-go v0.31.0 h1:QqEJzNjbN2Yv1H79SsS+SWnXkBgVu4Pj3CJQgbx0gI8= diff --git a/hack/crds/network-attachment-definition.yaml b/hack/crds/network-attachment-definition.yaml new file mode 100644 index 0000000..515d59f --- /dev/null +++ b/hack/crds/network-attachment-definition.yaml @@ -0,0 +1,44 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: network-attachment-definitions.k8s.cni.cncf.io +spec: + group: k8s.cni.cncf.io + scope: Namespaced + names: + plural: network-attachment-definitions + singular: network-attachment-definition + kind: NetworkAttachmentDefinition + shortNames: + - net-attach-def + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + description: 'NetworkAttachmentDefinition is a CRD schema specified by the Network Plumbing + Working Group to express the intent for attaching pods to one or more logical or physical + networks. More information available at: https://github.com/k8snetworkplumbingwg/multi-net-spec' + type: object + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this represen + tation of an object. Servers should convert recognized schemas to the + latest internal value, and may reject unrecognized values. More info: + https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + type: string + metadata: + type: object + spec: + description: 'NetworkAttachmentDefinition spec defines the desired state of a network attachment' + type: object + properties: + config: + description: 'NetworkAttachmentDefinition config is a JSON-formatted CNI configuration' + type: string \ No newline at end of file diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 6f330d4..7d7554d 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -22,6 +22,7 @@ var _ = Describe("Configuration", func() { Expect(os.Setenv("DAEMON_SM_PLUGIN_PATH", "/custom/plugins/location")).ToNot(HaveOccurred()) Expect(os.Setenv("DEFAULT_LIMITED_PARTITION", "0x2")).ToNot(HaveOccurred()) Expect(os.Setenv("ENABLE_IP_OVER_IB", "true")).ToNot(HaveOccurred()) + Expect(os.Setenv("MANAGED_RESOURCE_NAMES", "intel.com/ib_sriov,nvidia.com/ib_sriov")).ToNot(HaveOccurred()) err := dc.ReadConfig() Expect(err).ToNot(HaveOccurred()) @@ -32,10 +33,12 @@ var _ = Describe("Configuration", func() { Expect(dc.PluginPath).To(Equal("/custom/plugins/location")) Expect(dc.DefaultLimitedPartition).To(Equal("0x2")) Expect(dc.EnableIPOverIB).To(BeTrue()) + Expect(dc.ManagedResources).To(Equal(map[string]bool{"intel.com/ib_sriov": true, "nvidia.com/ib_sriov": true})) }) It("Read configuration with default values", func() { dc := &DaemonConfig{} Expect(os.Setenv("DAEMON_SM_PLUGIN", "ufm")).ToNot(HaveOccurred()) + Expect(os.Setenv("MANAGED_RESOURCE_NAMES", "intel.com/ib_sriov,nvidia.com/ib_sriov")).ToNot(HaveOccurred()) err := dc.ReadConfig() Expect(err).ToNot(HaveOccurred()) @@ -46,6 +49,7 @@ var _ = Describe("Configuration", func() { Expect(dc.PluginPath).To(Equal("/plugins")) Expect(dc.DefaultLimitedPartition).To(Equal("")) // Default should be empty Expect(dc.EnableIPOverIB).To(BeFalse()) // Default should be false + Expect(dc.ManagedResources).To(Equal(map[string]bool{"intel.com/ib_sriov": true, "nvidia.com/ib_sriov": true})) }) It("Read configuration with new environment variables", func() { dc := &DaemonConfig{} @@ -53,11 +57,13 @@ var _ = Describe("Configuration", func() { Expect(os.Setenv("DAEMON_SM_PLUGIN", "ufm")).ToNot(HaveOccurred()) Expect(os.Setenv("DEFAULT_LIMITED_PARTITION", "0x1")).ToNot(HaveOccurred()) Expect(os.Setenv("ENABLE_IP_OVER_IB", "true")).ToNot(HaveOccurred()) + Expect(os.Setenv("MANAGED_RESOURCE_NAMES", "intel.com/ib_sriov,nvidia.com/ib_sriov")).ToNot(HaveOccurred()) err := dc.ReadConfig() Expect(err).ToNot(HaveOccurred()) Expect(dc.DefaultLimitedPartition).To(Equal("0x1")) Expect(dc.EnableIPOverIB).To(BeTrue()) + Expect(dc.ManagedResources).To(Equal(map[string]bool{"intel.com/ib_sriov": true, "nvidia.com/ib_sriov": true})) }) }) Context("ValidateConfig", func() { @@ -67,7 +73,8 @@ var _ = Describe("Configuration", func() { GUIDPool: GUIDPoolConfig{ RangeStart: "02:00:00:00:00:00:00:10", RangeEnd: "02:00:00:00:00:00:00:FF"}, - Plugin: "noop"} + ManagedResources: map[string]bool{"intel.com/ib_sriov": true, "nvidia.com/ib_sriov": true}, + Plugin: "noop"} err := dc.ValidateConfig() Expect(err).ToNot(HaveOccurred()) @@ -83,17 +90,23 @@ var _ = Describe("Configuration", func() { Expect(err).To(HaveOccurred()) }) It("Validate configuration with guid pool start not set", func() { - dc := &DaemonConfig{PeriodicUpdate: 10, Plugin: "ufm"} + dc := &DaemonConfig{PeriodicUpdate: 10, Plugin: "ufm", ManagedResources: map[string]bool{"intel.com/ib_sriov": true, "nvidia.com/ib_sriov": true}} err := dc.ValidateConfig() Expect(err).ToNot(HaveOccurred()) }) It("Validate configuration with guid pool end not set", func() { dc := &DaemonConfig{ - PeriodicUpdate: 10, - GUIDPool: GUIDPoolConfig{RangeStart: "02:00:00:00:00:00:00:00"}, - Plugin: "ufm"} + PeriodicUpdate: 10, + GUIDPool: GUIDPoolConfig{RangeStart: "02:00:00:00:00:00:00:00"}, + ManagedResources: map[string]bool{"intel.com/ib_sriov": true, "nvidia.com/ib_sriov": true}, + Plugin: "ufm"} err := dc.ValidateConfig() Expect(err).ToNot(HaveOccurred()) }) + It("Validate configuration with no managed resources", func() { + dc := &DaemonConfig{PeriodicUpdate: 10, Plugin: "ufm"} + err := dc.ValidateConfig() + Expect(err).To(HaveOccurred()) + }) }) }) diff --git a/pkg/daemon/daemon_e2e_test.go b/pkg/daemon/daemon_e2e_test.go new file mode 100644 index 0000000..96b7a9c --- /dev/null +++ b/pkg/daemon/daemon_e2e_test.go @@ -0,0 +1,541 @@ +package daemon + +import ( + "fmt" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/stretchr/testify/mock" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + + netapi "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" + netclientset "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/client/clientset/versioned" + + "github.com/Mellanox/ib-kubernetes/pkg/config" + "github.com/Mellanox/ib-kubernetes/pkg/guid" + "github.com/Mellanox/ib-kubernetes/pkg/watcher" + resEventHandler "github.com/Mellanox/ib-kubernetes/pkg/watcher/handler" +) + +var _ = Describe("Daemon E2E Tests", func() { + var ( + testDaemon *daemon + ) + + BeforeEach(func() { + // Create test daemon - uses global cfg, kubernetesClient, netClient from suite + testDaemon = createTestDaemonForE2E() + }) + + Context("Daemon Control Loop E2E", func() { + It("Should process pod creation and add finalizers through control loop", func() { + By("Creating a NetworkAttachmentDefinition with managed resource") + nad := &netapi.NetworkAttachmentDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ib-network", + Namespace: "default", + Annotations: map[string]string{ + "k8s.v1.cni.cncf.io/resourceName": "intel.com/ib_sriov", + }, + }, + Spec: netapi.NetworkAttachmentDefinitionSpec{ + Config: `{ + "type": "ib-sriov", + "pkey": "0x1", + "capabilities": { + "infinibandGUID": true + } + }`, + }, + } + Expect(kubernetesClient.Create(ctx, nad)).To(Succeed()) + + By("Creating a Pod with InfiniBand network annotation") + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "k8s.v1.cni.cncf.io/networks": `[{ + "name": "test-ib-network", + "namespace": "default" + }]`, + }, + }, + Spec: corev1.PodSpec{ + NodeName: "test-node", + Containers: []corev1.Container{ + { + Name: "test-container", + Image: "busybox", + }, + }, + }, + } + Expect(kubernetesClient.Create(ctx, pod)).To(Succeed()) + + By("Starting the daemon's watcher to detect the pod") + podEventHandler := resEventHandler.NewPodEventHandler() + ibClient := &testK8sClient{ + client: kubernetesClient, + netClient: netClient, + } + + podWatcher := watcher.NewWatcher(podEventHandler, ibClient) + stopWatcher := podWatcher.RunBackground() + defer stopWatcher() + + // Give the watcher time to detect the pod + time.Sleep(100 * time.Millisecond) + + By("Running one cycle of AddPeriodicUpdate to process the pod") + testDaemon.watcher = podWatcher + testDaemon.kubeClient = ibClient + testDaemon.AddPeriodicUpdate() + + By("Verifying that the pod has the InfiniBand finalizer added") + // Need to check that the pod annotation was updated and finalizer was added + Eventually(func() bool { + updatedPod := &corev1.Pod{} + err := kubernetesClient.Get(ctx, client.ObjectKey{Name: "test-pod", Namespace: "default"}, updatedPod) + if err != nil { + return false + } + + // Check for finalizer + expectedFinalizer := fmt.Sprintf("%s-%s", PodGUIDFinalizer, "test-ib-network") + for _, finalizer := range updatedPod.Finalizers { + if finalizer == expectedFinalizer { + return true + } + } + return false + }, time.Second*5, time.Millisecond*100).Should(BeTrue()) + + By("Verifying that the NAD has the InfiniBand finalizer added") + Eventually(func() bool { + updatedNAD := &netapi.NetworkAttachmentDefinition{} + err := kubernetesClient.Get(ctx, client.ObjectKey{Name: "test-ib-network", Namespace: "default"}, updatedNAD) + if err != nil { + return false + } + + // Check for finalizer + for _, finalizer := range updatedNAD.Finalizers { + if finalizer == GUIDInUFMFinalizer { + return true + } + } + return false + }, time.Second*5, time.Millisecond*100).Should(BeTrue()) + + By("Verifying that the pod annotation was updated with GUID") + Eventually(func() bool { + updatedPod := &corev1.Pod{} + err := kubernetesClient.Get(ctx, client.ObjectKey{Name: "test-pod", Namespace: "default"}, updatedPod) + if err != nil { + return false + } + + // Check that network annotation includes GUID + networkAnnot := updatedPod.Annotations["k8s.v1.cni.cncf.io/networks"] + return len(networkAnnot) > 0 && networkAnnot != `[{"name": "test-ib-network", "namespace": "default"}]` + }, time.Second*5, time.Millisecond*100).Should(BeTrue()) + + By("Deleting the pod to trigger cleanup") + Expect(kubernetesClient.Delete(ctx, pod)).To(Succeed()) + + // Give the watcher time to detect the pod deletion + time.Sleep(100 * time.Millisecond) + + By("Running one cycle of DeletePeriodicUpdate to process the pod deletion") + testDaemon.DeletePeriodicUpdate() + + By("Verifying that the NAD finalizer is removed when no pods are using it") + Eventually(func() bool { + updatedNAD := &netapi.NetworkAttachmentDefinition{} + err := kubernetesClient.Get(ctx, client.ObjectKey{Name: "test-ib-network", Namespace: "default"}, updatedNAD) + if err != nil { + return false + } + + // Check that finalizer is removed + for _, finalizer := range updatedNAD.Finalizers { + if finalizer == GUIDInUFMFinalizer { + return false // Still has finalizer + } + } + return true // Finalizer removed + }, time.Second*5, time.Millisecond*100).Should(BeTrue()) + }) + + It("Should handle multiple pods with the same network", func() { + By("Creating a NetworkAttachmentDefinition") + nad := &netapi.NetworkAttachmentDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "shared-ib-network", + Namespace: "default", + Annotations: map[string]string{ + "k8s.v1.cni.cncf.io/resourceName": "intel.com/ib_sriov", + }, + }, + Spec: netapi.NetworkAttachmentDefinitionSpec{ + Config: `{ + "type": "ib-sriov", + "pkey": "0x2", + "capabilities": { + "infinibandGUID": true + } + }`, + }, + } + Expect(kubernetesClient.Create(ctx, nad)).To(Succeed()) + + By("Creating two pods using the same network") + pod1 := createTestPodForE2E("pod1", "default", "shared-ib-network") + pod2 := createTestPodForE2E("pod2", "default", "shared-ib-network") + + Expect(kubernetesClient.Create(ctx, pod1)).To(Succeed()) + Expect(kubernetesClient.Create(ctx, pod2)).To(Succeed()) + + By("Starting the daemon's watcher and processing pods") + podEventHandler := resEventHandler.NewPodEventHandler() + ibClient := &testK8sClient{ + client: kubernetesClient, + netClient: netClient, + } + + podWatcher := watcher.NewWatcher(podEventHandler, ibClient) + stopWatcher := podWatcher.RunBackground() + defer stopWatcher() + + testDaemon.watcher = podWatcher + testDaemon.kubeClient = ibClient + + // Give the watcher time to detect the pods + time.Sleep(100 * time.Millisecond) + + By("Running AddPeriodicUpdate to process both pods") + testDaemon.AddPeriodicUpdate() + + By("Verifying both pods have finalizers") + expectedFinalizer := fmt.Sprintf("%s-%s", PodGUIDFinalizer, "shared-ib-network") + + for _, podName := range []string{"pod1", "pod2"} { + Eventually(func() bool { + pod := &corev1.Pod{} + err := kubernetesClient.Get(ctx, client.ObjectKey{Name: podName, Namespace: "default"}, pod) + if err != nil { + return false + } + + for _, finalizer := range pod.Finalizers { + if finalizer == expectedFinalizer { + return true + } + } + return false + }, time.Second*5, time.Millisecond*100).Should(BeTrue()) + } + + By("Deleting one pod") + Expect(kubernetesClient.Delete(ctx, pod1)).To(Succeed()) + time.Sleep(100 * time.Millisecond) + + By("Running DeletePeriodicUpdate") + testDaemon.DeletePeriodicUpdate() + + By("Verifying NAD finalizer is NOT removed because pod2 still exists") + Consistently(func() bool { + updatedNAD := &netapi.NetworkAttachmentDefinition{} + err := kubernetesClient.Get(ctx, client.ObjectKey{Name: "shared-ib-network", Namespace: "default"}, updatedNAD) + if err != nil { + return false + } + + // Check that finalizer still exists + for _, finalizer := range updatedNAD.Finalizers { + if finalizer == GUIDInUFMFinalizer { + return true // Still has finalizer (expected) + } + } + return false + }, time.Second*2, time.Millisecond*100).Should(BeTrue()) + + By("Deleting the second pod") + Expect(kubernetesClient.Delete(ctx, pod2)).To(Succeed()) + time.Sleep(100 * time.Millisecond) + + By("Running DeletePeriodicUpdate again") + testDaemon.DeletePeriodicUpdate() + + By("Verifying NAD finalizer is removed now that no pods are using it") + Eventually(func() bool { + updatedNAD := &netapi.NetworkAttachmentDefinition{} + err := kubernetesClient.Get(ctx, client.ObjectKey{Name: "shared-ib-network", Namespace: "default"}, updatedNAD) + if err != nil { + return false + } + + // Check that finalizer is removed + for _, finalizer := range updatedNAD.Finalizers { + if finalizer == GUIDInUFMFinalizer { + return false // Still has finalizer + } + } + return true // Finalizer removed + }, time.Second*5, time.Millisecond*100).Should(BeTrue()) + }) + + It("Should ignore networks with unmanaged resources", func() { + By("Creating a NetworkAttachmentDefinition with unmanaged resource") + nad := &netapi.NetworkAttachmentDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unmanaged-network", + Namespace: "default", + Annotations: map[string]string{ + "k8s.v1.cni.cncf.io/resourceName": "unmanaged.com/resource", + }, + }, + Spec: netapi.NetworkAttachmentDefinitionSpec{ + Config: `{ + "type": "ib-sriov", + "pkey": "0x1", + "capabilities": { + "infinibandGUID": true + } + }`, + }, + } + Expect(kubernetesClient.Create(ctx, nad)).To(Succeed()) + + By("Creating a Pod using the unmanaged network") + pod := createTestPodForE2E("unmanaged-pod", "default", "unmanaged-network") + Expect(kubernetesClient.Create(ctx, pod)).To(Succeed()) + + By("Starting the daemon's watcher and processing the pod") + podEventHandler := resEventHandler.NewPodEventHandler() + ibClient := &testK8sClient{ + client: kubernetesClient, + netClient: netClient, + } + + podWatcher := watcher.NewWatcher(podEventHandler, ibClient) + stopWatcher := podWatcher.RunBackground() + defer stopWatcher() + + testDaemon.watcher = podWatcher + testDaemon.kubeClient = ibClient + + // Give the watcher time to detect the pod + time.Sleep(100 * time.Millisecond) + + By("Running AddPeriodicUpdate") + testDaemon.AddPeriodicUpdate() + + By("Verifying that no finalizers are added to the pod") + Consistently(func() bool { + updatedPod := &corev1.Pod{} + err := kubernetesClient.Get(ctx, client.ObjectKey{Name: "unmanaged-pod", Namespace: "default"}, updatedPod) + if err != nil { + return false + } + + // Check that no IB finalizers were added + for _, finalizer := range updatedPod.Finalizers { + if finalizer == fmt.Sprintf("%s-%s", PodGUIDFinalizer, "unmanaged-network") { + return false // Found finalizer (should not happen) + } + } + return true // No finalizer found (expected) + }, time.Second*2, time.Millisecond*100).Should(BeTrue()) + + By("Verifying that no finalizers are added to the NAD") + Consistently(func() bool { + updatedNAD := &netapi.NetworkAttachmentDefinition{} + err := kubernetesClient.Get(ctx, client.ObjectKey{Name: "unmanaged-network", Namespace: "default"}, updatedNAD) + if err != nil { + return false + } + + // Check that no IB finalizers were added + for _, finalizer := range updatedNAD.Finalizers { + if finalizer == GUIDInUFMFinalizer { + return false // Found finalizer (should not happen) + } + } + return true // No finalizer found (expected) + }, time.Second*2, time.Millisecond*100).Should(BeTrue()) + }) + }) +}) + +// Helper functions for e2e tests + +func createTestDaemonForE2E() *daemon { + // Create test configuration for e2e testing + testConfig := config.DaemonConfig{ + PeriodicUpdate: 1, + GUIDPool: config.GUIDPoolConfig{ + RangeStart: "02:00:00:00:00:00:00:00", + RangeEnd: "02:00:00:00:00:00:00:FF", + }, + Plugin: "noop", // Use noop plugin for testing + PluginPath: "/test", + ManagedResourcesString: "intel.com/ib_sriov,nvidia.com/ib_sriov", + ManagedResources: map[string]bool{"intel.com/ib_sriov": true, "nvidia.com/ib_sriov": true}, + EnableIPOverIB: false, + EnableIndex0ForPrimaryPkey: true, + DefaultLimitedPartition: "", + } + + // Create GUID pool + guidPool, _ := guid.NewPool(&testConfig.GUIDPool) + + // Create mock subnet manager client (noop implementation) + smClient := &mockSubnetManagerClient{} + smClient.On("Name").Return("noop-test") + smClient.On("Spec").Return("1.0") + smClient.On("Validate").Return(nil) + smClient.On("SetConfig", mock.Anything).Return(nil) + smClient.On("ListGuidsInUse").Return([]string{}, nil) + smClient.On("AddGuidsToPKey", mock.Anything, mock.Anything).Return(nil) + smClient.On("AddGuidsToLimitedPKey", mock.Anything, mock.Anything).Return(nil) + smClient.On("RemoveGuidsFromPKey", mock.Anything, mock.Anything).Return(nil) + + return &daemon{ + config: testConfig, + guidPool: guidPool, + smClient: smClient, + guidPodNetworkMap: make(map[string]string), + } +} + +func createTestPodForE2E(name, namespace, networkName string) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Annotations: map[string]string{ + "k8s.v1.cni.cncf.io/networks": fmt.Sprintf(`[{ + "name": "%s", + "namespace": "%s" + }]`, networkName, namespace), + }, + }, + Spec: corev1.PodSpec{ + NodeName: "test-node", + Containers: []corev1.Container{ + { + Name: "test-container", + Image: "busybox", + }, + }, + }, + } +} + +// testK8sClient wraps the real Kubernetes clients for e2e testing +type testK8sClient struct { + client client.Client + netClient netclientset.Interface +} + +func (c *testK8sClient) GetNetworkAttachmentDefinition(namespace, name string) (*netapi.NetworkAttachmentDefinition, error) { + return c.netClient.K8sCniCncfIoV1().NetworkAttachmentDefinitions(namespace).Get(ctx, name, metav1.GetOptions{}) +} + +func (c *testK8sClient) GetPods(namespace string) (*corev1.PodList, error) { + podList := &corev1.PodList{} + err := c.client.List(ctx, podList, client.InNamespace(namespace)) + return podList, err +} + +func (c *testK8sClient) SetAnnotationsOnPod(pod *corev1.Pod, annotations map[string]string) error { + pod.Annotations = annotations + return c.client.Update(ctx, pod) +} + +func (c *testK8sClient) AddFinalizerToPod(pod *corev1.Pod, finalizer string) error { + // Check if finalizer already exists + for _, existingFinalizer := range pod.Finalizers { + if existingFinalizer == finalizer { + return nil + } + } + + pod.Finalizers = append(pod.Finalizers, finalizer) + return c.client.Update(ctx, pod) +} + +func (c *testK8sClient) RemoveFinalizerFromPod(pod *corev1.Pod, finalizer string) error { + newFinalizers := make([]string, 0, len(pod.Finalizers)) + for _, existingFinalizer := range pod.Finalizers { + if existingFinalizer != finalizer { + newFinalizers = append(newFinalizers, existingFinalizer) + } + } + + pod.Finalizers = newFinalizers + return c.client.Update(ctx, pod) +} + +func (c *testK8sClient) AddFinalizerToNetworkAttachmentDefinition(namespace, name, finalizer string) error { + netAttDef, err := c.GetNetworkAttachmentDefinition(namespace, name) + if err != nil { + return err + } + + // Check if finalizer already exists + for _, existingFinalizer := range netAttDef.Finalizers { + if existingFinalizer == finalizer { + return nil + } + } + + netAttDef.Finalizers = append(netAttDef.Finalizers, finalizer) + _, err = c.netClient.K8sCniCncfIoV1().NetworkAttachmentDefinitions(namespace).Update( + ctx, netAttDef, metav1.UpdateOptions{}) + return err +} + +func (c *testK8sClient) RemoveFinalizerFromNetworkAttachmentDefinition(namespace, name, finalizer string) error { + netAttDef, err := c.GetNetworkAttachmentDefinition(namespace, name) + if err != nil { + return err + } + + newFinalizers := make([]string, 0, len(netAttDef.Finalizers)) + for _, existingFinalizer := range netAttDef.Finalizers { + if existingFinalizer != finalizer { + newFinalizers = append(newFinalizers, existingFinalizer) + } + } + + netAttDef.Finalizers = newFinalizers + _, err = c.netClient.K8sCniCncfIoV1().NetworkAttachmentDefinitions(namespace).Update( + ctx, netAttDef, metav1.UpdateOptions{}) + return err +} + +func (c *testK8sClient) GetRestClient() rest.Interface { + // Create a kubernetes clientset and return the core REST client + // This is required for the watcher's cache.NewListWatchFromClient() + clientset, err := kubernetes.NewForConfig(cfg) + if err != nil { + panic(fmt.Sprintf("Failed to create kubernetes clientset: %v", err)) + } + return clientset.CoreV1().RESTClient() +} + +func (c *testK8sClient) PatchPod(pod *corev1.Pod, patchType types.PatchType, patchData []byte) error { + // Use controller-runtime client patch + patch := client.RawPatch(patchType, patchData) + return c.client.Patch(ctx, pod, patch) +} diff --git a/pkg/daemon/daemon_suite_test.go b/pkg/daemon/daemon_suite_test.go new file mode 100644 index 0000000..25180fb --- /dev/null +++ b/pkg/daemon/daemon_suite_test.go @@ -0,0 +1,98 @@ +package daemon + +import ( + "context" + "fmt" + "path/filepath" + goruntime "runtime" + "testing" + "time" + + netapi "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" + netclientset "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/client/clientset/versioned" + "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/client/clientset/versioned/scheme" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +// These tests use Ginkgo (BDD-style Go testing framework). Refer to +// http://onsi.github.io/ginkgo/ to learn more about Ginkgo. + +var cfg *rest.Config +var kubernetesClient client.Client +var netClient netclientset.Interface +var testEnv *envtest.Environment +var ctx context.Context +var cancel context.CancelFunc + +func TestDaemon(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Daemon Suite") +} + +var _ = BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + ctx, cancel = context.WithCancel(context.Background()) + + By("bootstrapping test environment") + + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "hack", "crds")}, + ErrorIfCRDPathMissing: true, + BinaryAssetsDirectory: filepath.Join("..", "..", "bin", "k8s", + fmt.Sprintf("1.30.0-%s-%s", goruntime.GOOS, goruntime.GOARCH)), + } + + // cfg is defined in this file globally. + var err error + cfg, err = testEnv.Start() + Expect(err).NotTo(HaveOccurred()) + Expect(cfg).NotTo(BeNil()) + + // Create scheme and add NetworkAttachmentDefinition types + utilruntime.Must(clientgoscheme.AddToScheme(scheme.Scheme)) + utilruntime.Must(netapi.AddToScheme(scheme.Scheme)) + + kubernetesClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) + Expect(err).NotTo(HaveOccurred()) + Expect(kubernetesClient).NotTo(BeNil()) + + // Create NetworkAttachmentDefinition client + netClient, err = netclientset.NewForConfig(cfg) + Expect(err).NotTo(HaveOccurred()) + + // Create necessary namespaces + testNs := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}} + Expect(kubernetesClient.Create(ctx, testNs)).To(Succeed()) +}) + +var _ = AfterSuite(func() { + By("tearing down the test environment") + cancel() + + if testEnv != nil { + err := (func() (err error) { + // Need to sleep if the first stop fails due to a bug: + // https://github.com/kubernetes-sigs/controller-runtime/issues/1571 + sleepTime := 1 * time.Millisecond + for i := 0; i < 12; i++ { // Exponentially sleep up to ~4s + if err = testEnv.Stop(); err == nil { + return + } + sleepTime *= 2 + time.Sleep(sleepTime) + } + return + })() + Expect(err).NotTo(HaveOccurred()) + } +}) diff --git a/pkg/daemon/daemon_test.go b/pkg/daemon/daemon_test.go new file mode 100644 index 0000000..74ff592 --- /dev/null +++ b/pkg/daemon/daemon_test.go @@ -0,0 +1,490 @@ +package daemon + +import ( + "encoding/json" + "fmt" + "net" + + v1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/stretchr/testify/mock" + kapi "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/Mellanox/ib-kubernetes/pkg/config" + "github.com/Mellanox/ib-kubernetes/pkg/guid" + k8sClientMock "github.com/Mellanox/ib-kubernetes/pkg/k8s-client/mocks" +) + +// Mock subnet manager client for testing +type mockSubnetManagerClient struct { + mock.Mock +} + +func (m *mockSubnetManagerClient) Name() string { + args := m.Called() + return args.String(0) +} + +func (m *mockSubnetManagerClient) Spec() string { + args := m.Called() + return args.String(0) +} + +func (m *mockSubnetManagerClient) Validate() error { + args := m.Called() + return args.Error(0) +} + +func (m *mockSubnetManagerClient) AddGuidsToPKey(pkey int, guids []net.HardwareAddr) error { + args := m.Called(pkey, guids) + return args.Error(0) +} + +func (m *mockSubnetManagerClient) AddGuidsToLimitedPKey(pkey int, guids []net.HardwareAddr) error { + args := m.Called(pkey, guids) + return args.Error(0) +} + +func (m *mockSubnetManagerClient) RemoveGuidsFromPKey(pkey int, guids []net.HardwareAddr) error { + args := m.Called(pkey, guids) + return args.Error(0) +} + +func (m *mockSubnetManagerClient) ListGuidsInUse() ([]string, error) { + args := m.Called() + return args.Get(0).([]string), args.Error(1) +} + +func (m *mockSubnetManagerClient) SetConfig(config map[string]interface{}) error { + args := m.Called(config) + return args.Error(0) +} + +// Create a mock daemon for testing +func createTestDaemon() (*daemon, *k8sClientMock.Client, *mockSubnetManagerClient) { + kubeClientMock := &k8sClientMock.Client{} + smClientMock := &mockSubnetManagerClient{} + + // Set up basic mock responses + smClientMock.On("Name").Return("test-sm") + smClientMock.On("Spec").Return("1.0") + smClientMock.On("Validate").Return(nil) + smClientMock.On("SetConfig", mock.Anything).Return(nil) + smClientMock.On("ListGuidsInUse").Return([]string{}, nil) + + // Create test configuration + testConfig := config.DaemonConfig{ + PeriodicUpdate: 1, + GUIDPool: config.GUIDPoolConfig{ + RangeStart: "02:00:00:00:00:00:00:00", + RangeEnd: "02:00:00:00:00:00:00:FF", + }, + Plugin: "test", + PluginPath: "/test", + ManagedResourcesString: "intel.com/ib_sriov,nvidia.com/ib_sriov", + ManagedResources: map[string]bool{"intel.com/ib_sriov": true, "nvidia.com/ib_sriov": true}, + EnableIPOverIB: false, + EnableIndex0ForPrimaryPkey: true, + DefaultLimitedPartition: "", + } + + // Create GUID pool + guidPool, _ := guid.NewPool(&testConfig.GUIDPool) + + return &daemon{ + config: testConfig, + kubeClient: kubeClientMock, + guidPool: guidPool, + smClient: smClientMock, + guidPodNetworkMap: make(map[string]string), + }, kubeClientMock, smClientMock +} + +// Helper function to create test pods with InfiniBand network annotations +func createTestPod(name, namespace string, networks []testNetworkSpec) *kapi.Pod { + networkAnnotations := make([]map[string]interface{}, 0) + + for _, network := range networks { + netAnnot := map[string]interface{}{ + "name": network.Name, + "namespace": network.Namespace, + } + if network.GUID != "" { + netAnnot["cni-args"] = map[string]interface{}{ + "guid": network.GUID, + "mellanox.infiniband.app": "configured", + } + } else if network.EnableIB { + netAnnot["cni-args"] = map[string]interface{}{ + "mellanox.infiniband.app": "configured", + } + } + networkAnnotations = append(networkAnnotations, netAnnot) + } + + netAnnotJson, _ := json.Marshal(networkAnnotations) + + return &kapi.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + UID: "test-pod-uid", + Annotations: map[string]string{ + v1.NetworkAttachmentAnnot: string(netAnnotJson), + }, + }, + Spec: kapi.PodSpec{ + NodeName: "test-node", + }, + } +} + +type testNetworkSpec struct { + Name string + Namespace string + GUID string + EnableIB bool +} + +// Helper function to create test NetworkAttachmentDefinition +func createTestNAD(name, namespace, resourceName string) *v1.NetworkAttachmentDefinition { + spec := map[string]interface{}{ + "type": "ib-sriov", + "pkey": "0x1", + "capabilities": map[string]bool{"infinibandGUID": true}, + } + specJson, _ := json.Marshal(spec) + + return &v1.NetworkAttachmentDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Annotations: map[string]string{ + "k8s.v1.cni.cncf.io/resourceName": resourceName, + }, + }, + Spec: v1.NetworkAttachmentDefinitionSpec{ + Config: string(specJson), + }, + } +} + +var _ = Describe("Daemon Finalizer Tests", func() { + var ( + testDaemon *daemon + kubeClient *k8sClientMock.Client + smClient *mockSubnetManagerClient + ) + + BeforeEach(func() { + testDaemon, kubeClient, smClient = createTestDaemon() + }) + + Describe("Double finalizer create and delete", func() { + Context("When creating pods with both nic types", func() { + It("Should add finalizers to both pod and NAD, then remove them on delete", func() { + // Create test pod with two different InfiniBand networks + networks := []testNetworkSpec{ + {Name: "ib-network1", Namespace: "default", EnableIB: true}, + {Name: "ib-network2", Namespace: "default", EnableIB: true}, + } + testPod := createTestPod("test-pod", "default", networks) + + // Create corresponding NADs + nad1 := createTestNAD("ib-network1", "default", "intel.com/ib_sriov") + nad2 := createTestNAD("ib-network2", "default", "nvidia.com/ib_sriov") + + // Mock k8s client responses for NADs + kubeClient.On("GetNetworkAttachmentDefinition", "default", "ib-network1").Return(nad1, nil) + kubeClient.On("GetNetworkAttachmentDefinition", "default", "ib-network2").Return(nad2, nil) + + // Mock finalizer addition on pods + kubeClient.On("AddFinalizerToPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network1")).Return(nil) + kubeClient.On("AddFinalizerToPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network2")).Return(nil) + + // Mock finalizer addition on NADs + kubeClient.On("AddFinalizerToNetworkAttachmentDefinition", "default", "ib-network1", GUIDInUFMFinalizer).Return(nil) + kubeClient.On("AddFinalizerToNetworkAttachmentDefinition", "default", "ib-network2", GUIDInUFMFinalizer).Return(nil) + + // Mock pod annotation updates + kubeClient.On("SetAnnotationsOnPod", testPod, mock.AnythingOfType("map[string]string")).Return(nil) + + // Mock SM client calls + smClient.On("AddGuidsToPKey", 1, mock.AnythingOfType("[]net.HardwareAddr")).Return(nil) + + // Test adding finalizers for network1 + err := testDaemon.addPodFinalizer(testPod, "ib-network1") + Expect(err).ToNot(HaveOccurred()) + + err = testDaemon.addNADFinalizer("default", "ib-network1") + Expect(err).ToNot(HaveOccurred()) + + // Test adding finalizers for network2 + err = testDaemon.addPodFinalizer(testPod, "ib-network2") + Expect(err).ToNot(HaveOccurred()) + + err = testDaemon.addNADFinalizer("default", "ib-network2") + Expect(err).ToNot(HaveOccurred()) + + // Verify finalizer calls were made + kubeClient.AssertCalled(GinkgoT(), "AddFinalizerToPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network1")) + kubeClient.AssertCalled(GinkgoT(), "AddFinalizerToPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network2")) + kubeClient.AssertCalled(GinkgoT(), "AddFinalizerToNetworkAttachmentDefinition", "default", "ib-network1", GUIDInUFMFinalizer) + kubeClient.AssertCalled(GinkgoT(), "AddFinalizerToNetworkAttachmentDefinition", "default", "ib-network2", GUIDInUFMFinalizer) + + // Now test finalizer removal + // Mock Pod list for checking if any pods using networks + emptyPodList := &kapi.PodList{Items: []kapi.Pod{}} + kubeClient.On("GetPods", kapi.NamespaceAll).Return(emptyPodList, nil) + + // Mock finalizer removal from pods + kubeClient.On("RemoveFinalizerFromPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network1")).Return(nil) + kubeClient.On("RemoveFinalizerFromPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network2")).Return(nil) + + // Mock finalizer removal from NADs + kubeClient.On("RemoveFinalizerFromNetworkAttachmentDefinition", "default", "ib-network1", GUIDInUFMFinalizer).Return(nil) + kubeClient.On("RemoveFinalizerFromNetworkAttachmentDefinition", "default", "ib-network2", GUIDInUFMFinalizer).Return(nil) + + // Mock SM client GUID removal + smClient.On("RemoveGuidsFromPKey", 1, mock.AnythingOfType("[]net.HardwareAddr")).Return(nil) + + // Test removing finalizers + err = testDaemon.removePodFinalizer(testPod, "ib-network1") + Expect(err).ToNot(HaveOccurred()) + + err = testDaemon.removePodFinalizer(testPod, "ib-network2") + Expect(err).ToNot(HaveOccurred()) + + err = testDaemon.removeNADFinalizerIfSafe("default", "ib-network1") + Expect(err).ToNot(HaveOccurred()) + + err = testDaemon.removeNADFinalizerIfSafe("default", "ib-network2") + Expect(err).ToNot(HaveOccurred()) + + // Verify finalizer removal calls were made + kubeClient.AssertCalled(GinkgoT(), "RemoveFinalizerFromPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network1")) + kubeClient.AssertCalled(GinkgoT(), "RemoveFinalizerFromPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network2")) + kubeClient.AssertCalled(GinkgoT(), "RemoveFinalizerFromNetworkAttachmentDefinition", "default", "ib-network1", GUIDInUFMFinalizer) + kubeClient.AssertCalled(GinkgoT(), "RemoveFinalizerFromNetworkAttachmentDefinition", "default", "ib-network2", GUIDInUFMFinalizer) + }) + }) + }) + + Describe("Finalizer exclusivity", func() { + Context("When external finalizers are present", func() { + It("Should not remove NAD finalizer if other pods are still using the network", func() { + // Create test pod using network with GUID (so it's detected as using the network) + networks := []testNetworkSpec{ + {Name: "ib-network1", Namespace: "default", GUID: "02:00:00:00:00:00:00:01", EnableIB: true}, + } + testPod2 := createTestPod("test-pod2", "default", networks) + + // Create NAD + nad := createTestNAD("ib-network1", "default", "intel.com/ib_sriov") + + // Mock k8s client responses + kubeClient.On("GetNetworkAttachmentDefinition", "default", "ib-network1").Return(nad, nil) + + // Mock Pod list showing other pods still using the network + podsUsingNetwork := &kapi.PodList{ + Items: []kapi.Pod{*testPod2}, // testPod2 is still using the network with GUID + } + kubeClient.On("GetPods", kapi.NamespaceAll).Return(podsUsingNetwork, nil) + + // Attempt to remove NAD finalizer - should not remove it because other pods exist + err := testDaemon.removeNADFinalizerIfSafe("default", "ib-network1") + Expect(err).ToNot(HaveOccurred()) + + // Verify that RemoveFinalizerFromNetworkAttachmentDefinition was NOT called + kubeClient.AssertNotCalled(GinkgoT(), "RemoveFinalizerFromNetworkAttachmentDefinition", "default", "ib-network1", GUIDInUFMFinalizer) + }) + + It("Should handle co-existing with other ib-kubernetes finalizers", func() { + // This test simulates the scenario where an external ib-kubernetes adds a finalizer + // and this ib-kubernetes should not remove the finalizer. + + networks := []testNetworkSpec{ + {Name: "ib-network1", Namespace: "default", EnableIB: true}, + } + testPod := createTestPod("test-pod", "default", networks) + + // Add an external finalizer to the pod metadata + externalFinalizer := fmt.Sprintf("%s-%s", PodGUIDFinalizer, "external-ib-network1") + testPod.Finalizers = []string{ + fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network1"), + externalFinalizer, // Placed by a separate ib-kubernetes + } + + nad := createTestNAD("ib-network1", "default", "intel.com/ib_sriov") + + // Mock k8s client responses + kubeClient.On("GetNetworkAttachmentDefinition", "default", "ib-network1").Return(nad, nil) + kubeClient.On("RemoveFinalizerFromPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network1")).Return(nil) + + // Mock empty pods list for NAD finalizer removal + emptyPodList := &kapi.PodList{Items: []kapi.Pod{}} + kubeClient.On("GetPods", kapi.NamespaceAll).Return(emptyPodList, nil) + kubeClient.On("RemoveFinalizerFromNetworkAttachmentDefinition", "default", "ib-network1", GUIDInUFMFinalizer).Return(nil) + + // ib-kubernetes can successfully remove its own finalizer + err := testDaemon.removePodFinalizer(testPod, "ib-network1") + Expect(err).ToNot(HaveOccurred()) + + err = testDaemon.removeNADFinalizerIfSafe("default", "ib-network1") + Expect(err).ToNot(HaveOccurred()) + + // Verify ib-kubernetes finalizer was removed + kubeClient.AssertCalled(GinkgoT(), "RemoveFinalizerFromPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network1")) + kubeClient.AssertCalled(GinkgoT(), "RemoveFinalizerFromNetworkAttachmentDefinition", "default", "ib-network1", GUIDInUFMFinalizer) + + // Ensure the external finalizer is still present + Expect(testPod.Finalizers).To(ContainElement(externalFinalizer)) + }) + }) + }) + + Describe("Create exclusivity", func() { + Context("When pod uses network not managed by this daemon", func() { + It("Should not add finalizers to unmanaged networks", func() { + // Create NAD with unmanaged resource + nad := createTestNAD("non-ib-network", "default", "unmanaged.com/resource") + + // Mock k8s client to return the NAD + kubeClient.On("GetNetworkAttachmentDefinition", "default", "non-ib-network").Return(nad, nil) + + // Test getIbSriovNetwork - should return nil for unmanaged resource + networkName, ibCniSpec, err := testDaemon.getIbSriovNetwork("default_non-ib-network") + Expect(err).To(MatchError(fmt.Errorf("network %s uses resource %s which is not managed by this daemon", "non-ib-network", "unmanaged.com/resource"))) + Expect(networkName).To(BeEmpty()) + Expect(ibCniSpec).To(BeNil()) + }) + + It("Should not process networks without resourceName annotation", func() { + // Create NAD without resourceName annotation + nad := &v1.NetworkAttachmentDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-resource-network", + Namespace: "default", + Annotations: map[string]string{}, // No resourceName annotation + }, + Spec: v1.NetworkAttachmentDefinitionSpec{ + Config: `{"type": "ib-sriov", "pkey": "0x1"}`, + }, + } + + kubeClient.On("GetNetworkAttachmentDefinition", "default", "no-resource-network").Return(nad, nil) + + // Test getIbSriovNetwork - should return nil for network without resourceName + networkName, ibCniSpec, err := testDaemon.getIbSriovNetwork("default_no-resource-network") + Expect(err).To(MatchError(fmt.Errorf("network %s uses resource %s which is not managed by this daemon", "no-resource-network", ""))) + Expect(networkName).To(BeEmpty()) + Expect(ibCniSpec).To(BeNil()) + }) + }) + + Context("When pod uses managed network", func() { + It("Should add finalizers for managed resources", func() { + // Create test pod with managed network + networks := []testNetworkSpec{ + {Name: "managed-network", Namespace: "default", EnableIB: true}, + } + testPod := createTestPod("test-pod", "default", networks) + + // Create NAD with managed resource + nad := createTestNAD("managed-network", "default", "intel.com/ib_sriov") + + kubeClient.On("GetNetworkAttachmentDefinition", "default", "managed-network").Return(nad, nil) + kubeClient.On("AddFinalizerToPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "managed-network")).Return(nil) + kubeClient.On("AddFinalizerToNetworkAttachmentDefinition", "default", "managed-network", GUIDInUFMFinalizer).Return(nil) + + // Test getIbSriovNetwork - should return valid spec for managed resource + networkName, ibCniSpec, err := testDaemon.getIbSriovNetwork("default_managed-network") + Expect(err).ToNot(HaveOccurred()) + Expect(networkName).To(Equal("managed-network")) + Expect(ibCniSpec).ToNot(BeNil()) + Expect(ibCniSpec.Type).To(Equal("ib-sriov")) + Expect(ibCniSpec.PKey).To(Equal("0x1")) + + // Test finalizer addition + err = testDaemon.addPodFinalizer(testPod, "managed-network") + Expect(err).ToNot(HaveOccurred()) + + err = testDaemon.addNADFinalizer("default", "managed-network") + Expect(err).ToNot(HaveOccurred()) + + // Verify finalizer methods were called for managed network + kubeClient.AssertCalled(GinkgoT(), "AddFinalizerToPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "managed-network")) + kubeClient.AssertCalled(GinkgoT(), "AddFinalizerToNetworkAttachmentDefinition", "default", "managed-network", GUIDInUFMFinalizer) + }) + }) + }) + + Describe("GUID Pool Management with Finalizers", func() { + Context("When allocating and releasing GUIDs", func() { + It("Should properly manage GUID allocation lifecycle with finalizers", func() { + // Create test pod + networks := []testNetworkSpec{ + {Name: "ib-network", Namespace: "default", EnableIB: true}, + } + testPod := createTestPod("test-pod", "default", networks) + + // Create NAD + nad := createTestNAD("ib-network", "default", "intel.com/ib_sriov") + + kubeClient.On("GetNetworkAttachmentDefinition", "default", "ib-network").Return(nad, nil) + kubeClient.On("AddFinalizerToPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network")).Return(nil) + kubeClient.On("AddFinalizerToNetworkAttachmentDefinition", "default", "ib-network", GUIDInUFMFinalizer).Return(nil) + kubeClient.On("RemoveFinalizerFromPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network")).Return(nil) + kubeClient.On("RemoveFinalizerFromNetworkAttachmentDefinition", "default", "ib-network", GUIDInUFMFinalizer).Return(nil) + + // Mock empty pod list for safe NAD finalizer removal + emptyPodList := &kapi.PodList{Items: []kapi.Pod{}} + kubeClient.On("GetPods", kapi.NamespaceAll).Return(emptyPodList, nil) + + // Generate a GUID + guidAddr, err := testDaemon.guidPool.GenerateGUID() + Expect(err).ToNot(HaveOccurred()) + + podNetworkID := fmt.Sprintf("%s_ib-network", testPod.UID) + allocatedGUID := guidAddr.String() + + // Test GUID allocation + err = testDaemon.allocatePodNetworkGUID(allocatedGUID, podNetworkID, testPod.UID) + Expect(err).ToNot(HaveOccurred()) + + // Verify GUID was allocated + Expect(testDaemon.guidPodNetworkMap[allocatedGUID]).To(Equal(podNetworkID)) + + // Add finalizers + err = testDaemon.addPodFinalizer(testPod, "ib-network") + Expect(err).ToNot(HaveOccurred()) + + err = testDaemon.addNADFinalizer("default", "ib-network") + Expect(err).ToNot(HaveOccurred()) + + // Release GUID and remove finalizers + err = testDaemon.guidPool.ReleaseGUID(allocatedGUID) + Expect(err).ToNot(HaveOccurred()) + + delete(testDaemon.guidPodNetworkMap, allocatedGUID) + + err = testDaemon.removePodFinalizer(testPod, "ib-network") + Expect(err).ToNot(HaveOccurred()) + + err = testDaemon.removeNADFinalizerIfSafe("default", "ib-network") + Expect(err).ToNot(HaveOccurred()) + + // Verify GUID was released and mapping cleared + Expect(testDaemon.guidPodNetworkMap[allocatedGUID]).To(BeEmpty()) + + // Verify finalizer calls + kubeClient.AssertCalled(GinkgoT(), "AddFinalizerToPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network")) + kubeClient.AssertCalled(GinkgoT(), "AddFinalizerToNetworkAttachmentDefinition", "default", "ib-network", GUIDInUFMFinalizer) + kubeClient.AssertCalled(GinkgoT(), "RemoveFinalizerFromPod", testPod, fmt.Sprintf("%s-%s", PodGUIDFinalizer, "ib-network")) + kubeClient.AssertCalled(GinkgoT(), "RemoveFinalizerFromNetworkAttachmentDefinition", "default", "ib-network", GUIDInUFMFinalizer) + }) + }) + }) +}) From 6d3ae1cc7c47dfc5fd39c417c875df800b99cba1 Mon Sep 17 00:00:00 2001 From: Nik Mitra Date: Fri, 26 Sep 2025 10:19:53 -0700 Subject: [PATCH 3/3] Modify pod annotations update to safely allow concurrent writes --- pkg/daemon/daemon.go | 98 ++++++++++++++++++++++++++-------- pkg/daemon/daemon_e2e_test.go | 16 +++--- pkg/daemon/daemon_test.go | 3 +- pkg/k8s-client/client.go | 48 +++++++++-------- pkg/k8s-client/mocks/Client.go | 38 +++++++------ 5 files changed, 136 insertions(+), 67 deletions(-) diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 92b1366..9456099 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -209,8 +209,7 @@ func (d *daemon) getIbSriovNetwork(networkID string) (string, *utils.IbSriovCniS // Check if this network's resource is managed by this daemon resourceName := netAttInfo.Annotations["k8s.v1.cni.cncf.io/resourceName"] if resourceName == "" || !d.config.IsManagedResource(resourceName) { - // TODO(Nik) dev qol, check if someone else manages this resource or if it is orphan - // checkResourceOwner(networkNamespace, networkName) + // TODO(Nik) qol, check if someone else manages this resource or if it is orphan return "", nil, fmt.Errorf("network %s uses resource %s which is not managed by this daemon", networkName, resourceName) } @@ -254,8 +253,8 @@ func getPodNetworkInfo(netName string, pod *kapi.Pod, netMap networksMap) (*podN // addPodFinalizer adds the GUID cleanup finalizer to a pod func (d *daemon) addPodFinalizer(pod *kapi.Pod, networkName string) error { + podFinalizer := fmt.Sprintf("%s-%s", PodGUIDFinalizer, networkName) return wait.ExponentialBackoff(backoffValues, func() (bool, error) { - podFinalizer := fmt.Sprintf("%s-%s", PodGUIDFinalizer, networkName) if err := d.kubeClient.AddFinalizerToPod(pod, podFinalizer); err != nil { log.Warn().Msgf("failed to add finalizer to pod %s/%s: %v", pod.Namespace, pod.Name, err) @@ -267,8 +266,8 @@ func (d *daemon) addPodFinalizer(pod *kapi.Pod, networkName string) error { // removePodFinalizer removes the GUID cleanup finalizer from a pod func (d *daemon) removePodFinalizer(pod *kapi.Pod, networkName string) error { + podFinalizer := fmt.Sprintf("%s-%s", PodGUIDFinalizer, networkName) return wait.ExponentialBackoff(backoffValues, func() (bool, error) { - podFinalizer := fmt.Sprintf("%s-%s", PodGUIDFinalizer, networkName) if err := d.kubeClient.RemoveFinalizerFromPod(pod, podFinalizer); err != nil { log.Warn().Msgf("failed to remove finalizer from pod %s/%s: %v", pod.Namespace, pod.Name, err) @@ -480,44 +479,102 @@ func syncGUIDPool(smClient plugins.SubnetManagerClient, guidPool guid.Pool) erro // Update and set Pod's network annotation. // If failed to update annotation, pod's GUID added into the list to be removed from Pkey. -func (d *daemon) updatePodNetworkAnnotation(pi *podNetworkInfo, removedList *[]net.HardwareAddr) error { +func (d *daemon) updatePodNetworkAnnotation(pi *podNetworkInfo, removedList *[]net.HardwareAddr) { if pi.ibNetwork.CNIArgs == nil { pi.ibNetwork.CNIArgs = &map[string]interface{}{} } (*pi.ibNetwork.CNIArgs)[utils.InfiniBandAnnotation] = utils.ConfiguredInfiniBandPod - netAnnotations, err := json.Marshal(pi.networks) - if err != nil { - return fmt.Errorf("failed to dump networks %+v of pod into json with error: %v", pi.networks, err) - } - - pi.pod.Annotations[v1.NetworkAttachmentAnnot] = string(netAnnotations) // Try to set pod's annotations in backoff loop - if err = wait.ExponentialBackoff(backoffValues, func() (bool, error) { - log.Info().Msgf("updatePodNetworkAnnotation(): Updating pod annotation for pod: %s with anootation: %s", pi.pod.Name, pi.pod.Annotations) + if err := wait.ExponentialBackoff(backoffValues, func() (bool, error) { + + // Get latest annotations state to avoid conflicts + latestPodAnnotations, networks, err := d.getLatestPodAnnotations(pi.pod) + if err != nil { + log.Warn().Msgf("failed to get latest pod annotations for %s/%s: %v", pi.pod.Namespace, pi.pod.Name, err) + return false, nil + } + + targetNetwork, err := utils.GetPodNetwork(networks, pi.ibNetwork.Name) + if err != nil { + return false, fmt.Errorf("failed to locate network %s in pod %s/%s annotations: %v", pi.ibNetwork.Name, pi.pod.Namespace, pi.pod.Name, err) + } + + err = updateInfiniBandNetwork(targetNetwork, pi.ibNetwork) + if err != nil { + return false, fmt.Errorf("failed to update infiniband network for pod %s/%s: %v", pi.pod.Namespace, pi.pod.Name, err) + } + + netAnnotations, err := json.Marshal(networks) + if err != nil { + return false, fmt.Errorf("failed to marshal updated networks for pod %s/%s: %v", pi.pod.Namespace, pi.pod.Name, err) + } + + if latestPodAnnotations == nil { + return false, fmt.Errorf("latestPodAnnotations is nil for pod %s/%s", pi.pod.Namespace, pi.pod.Name) + } + + latestPodAnnotations[v1.NetworkAttachmentAnnot] = string(netAnnotations) + pi.pod.Annotations = latestPodAnnotations + + log.Info().Msgf("updatePodNetworkAnnotation(): Updating pod annotation for pod: %s/%s", pi.pod.Namespace, pi.pod.Name) if err = d.kubeClient.SetAnnotationsOnPod(pi.pod, pi.pod.Annotations); err != nil { if kerrors.IsNotFound(err) { return false, err } - log.Warn().Msgf("failed to update pod annotations with err: %v", err) + if kerrors.IsConflict(err) { + log.Warn().Msgf("conflict while updating pod annotations for %s/%s, will retry", pi.pod.Namespace, pi.pod.Name) + return false, nil + } + log.Warn().Msgf("failed to update pod annotations for %s/%s with err: %v", pi.pod.Namespace, pi.pod.Name, err) return false, nil } - log.Info().Msgf("updatePodNetworkAnnotation(): Success on updating pod annotation for pod: %s with anootation: %s", pi.pod.Name, pi.pod.Annotations) + + log.Info().Msgf("updatePodNetworkAnnotation(): Success on updating pod annotation for pod: %s/%s with annotations: %s", pi.pod.Namespace, pi.pod.Name, pi.pod.Annotations) return true, nil }); err != nil { - log.Error().Msgf("failed to update pod annotations") + log.Error().Msgf("failed to update pod annotations for %s/%s with error: %v", pi.pod.Namespace, pi.pod.Name, err) if err = d.guidPool.ReleaseGUID(pi.addr.String()); err != nil { - log.Warn().Msgf("failed to release guid \"%s\" from removed pod \"%s\" in namespace "+ - "\"%s\" with error: %v", pi.addr.String(), pi.pod.Name, pi.pod.Namespace, err) + log.Warn().Msgf("failed to release guid \"%s\" from removed pod \"%s\" in namespace \"%s\" with error: %v", pi.addr.String(), pi.pod.Name, pi.pod.Namespace, err) } else { delete(d.guidPodNetworkMap, pi.addr.String()) } *removedList = append(*removedList, pi.addr) } +} + +// Retrieves the latest annotations for a pod and returns the annotations and the pod networks. +func (d *daemon) getLatestPodAnnotations(pod *kapi.Pod) (map[string]string, []*v1.NetworkSelectionElement, error) { + latestPod, err := d.kubeClient.GetPod(pod.Namespace, pod.Name) + if err != nil { + return nil, nil, err + } + + networks, err := netAttUtils.ParsePodNetworkAnnotation(latestPod) + if err != nil { + return nil, nil, err + } + return latestPod.Annotations, networks, nil +} + +// Replaces target network with source network, erroring if source is already configured. +func updateInfiniBandNetwork(target *v1.NetworkSelectionElement, source *v1.NetworkSelectionElement) error { + if target == nil || source == nil { + return fmt.Errorf("target or source network is nil") + } + + if target.CNIArgs != nil { + if (*target.CNIArgs)[utils.InfiniBandAnnotation] == utils.ConfiguredInfiniBandPod { + return fmt.Errorf("target network is already configured") + } + } + + target.InfinibandGUIDRequest = source.InfinibandGUIDRequest + target.CNIArgs = source.CNIArgs return nil } @@ -609,10 +666,7 @@ func (d *daemon) AddPeriodicUpdate() { var removedGUIDList []net.HardwareAddr for _, pi := range passedPods { log.Info().Msgf("Updating annotations for the pod %s, network %s", pi.pod.Name, pi.ibNetwork.Name) - err = d.updatePodNetworkAnnotation(pi, &removedGUIDList) - if err != nil { - log.Error().Msgf("%v", err) - } + d.updatePodNetworkAnnotation(pi, &removedGUIDList) } if ibCniSpec.PKey != "" && len(removedGUIDList) != 0 { diff --git a/pkg/daemon/daemon_e2e_test.go b/pkg/daemon/daemon_e2e_test.go index 96b7a9c..76aa22a 100644 --- a/pkg/daemon/daemon_e2e_test.go +++ b/pkg/daemon/daemon_e2e_test.go @@ -9,7 +9,6 @@ import ( "github.com/stretchr/testify/mock" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" @@ -457,6 +456,15 @@ func (c *testK8sClient) GetPods(namespace string) (*corev1.PodList, error) { return podList, err } +func (c *testK8sClient) GetPod(namespace, name string) (*corev1.Pod, error) { + pod := &corev1.Pod{} + key := client.ObjectKey{Namespace: namespace, Name: name} + if err := c.client.Get(ctx, key, pod); err != nil { + return nil, err + } + return pod, nil +} + func (c *testK8sClient) SetAnnotationsOnPod(pod *corev1.Pod, annotations map[string]string) error { pod.Annotations = annotations return c.client.Update(ctx, pod) @@ -533,9 +541,3 @@ func (c *testK8sClient) GetRestClient() rest.Interface { } return clientset.CoreV1().RESTClient() } - -func (c *testK8sClient) PatchPod(pod *corev1.Pod, patchType types.PatchType, patchData []byte) error { - // Use controller-runtime client patch - patch := client.RawPatch(patchType, patchData) - return c.client.Patch(ctx, pod, patch) -} diff --git a/pkg/daemon/daemon_test.go b/pkg/daemon/daemon_test.go index 74ff592..bd1f8b2 100644 --- a/pkg/daemon/daemon_test.go +++ b/pkg/daemon/daemon_test.go @@ -209,7 +209,8 @@ var _ = Describe("Daemon Finalizer Tests", func() { kubeClient.On("AddFinalizerToNetworkAttachmentDefinition", "default", "ib-network2", GUIDInUFMFinalizer).Return(nil) // Mock pod annotation updates - kubeClient.On("SetAnnotationsOnPod", testPod, mock.AnythingOfType("map[string]string")).Return(nil) + kubeClient.On("GetPod", testPod.Namespace, testPod.Name).Return(testPod, nil) + kubeClient.On("SetAnnotationsOnPod", mock.AnythingOfType("*v1.Pod"), mock.AnythingOfType("map[string]string")).Return(nil) // Mock SM client calls smClient.On("AddGuidsToPKey", 1, mock.AnythingOfType("[]net.HardwareAddr")).Return(nil) diff --git a/pkg/k8s-client/client.go b/pkg/k8s-client/client.go index 6b3452e..08d1c20 100644 --- a/pkg/k8s-client/client.go +++ b/pkg/k8s-client/client.go @@ -2,8 +2,8 @@ package k8sclient import ( "context" - "encoding/json" "fmt" + "reflect" "time" netapi "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" @@ -11,7 +11,6 @@ import ( "github.com/rs/zerolog/log" kapi "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" @@ -20,8 +19,8 @@ import ( type Client interface { GetPods(namespace string) (*kapi.PodList, error) + GetPod(namespace, name string) (*kapi.Pod, error) SetAnnotationsOnPod(pod *kapi.Pod, annotations map[string]string) error - PatchPod(pod *kapi.Pod, patchType types.PatchType, patchData []byte) error GetNetworkAttachmentDefinition(namespace, name string) (*netapi.NetworkAttachmentDefinition, error) GetRestClient() rest.Interface AddFinalizerToNetworkAttachmentDefinition(namespace, name, finalizer string) error @@ -65,33 +64,38 @@ func (c *client) GetPods(namespace string) (*kapi.PodList, error) { return c.clientset.CoreV1().Pods(namespace).List(context.TODO(), metav1.ListOptions{}) } +// GetPod obtains a single Pod resource from the kubernetes api server +func (c *client) GetPod(namespace, name string) (*kapi.Pod, error) { + log.Debug().Msgf("getting pod namespace: %s, name: %s", namespace, name) + return c.clientset.CoreV1().Pods(namespace).Get(context.TODO(), name, metav1.GetOptions{}) +} + // SetAnnotationsOnPod takes the pod object and map of key/value string pairs to set as annotations func (c *client) SetAnnotationsOnPod(pod *kapi.Pod, annotations map[string]string) error { log.Info().Msgf("Setting annotation on pod, namespace: %s, podName: %s, annotations: %v", pod.Namespace, pod.Name, annotations) - var err error - var patchData []byte - patch := struct { - Metadata map[string]interface{} `json:"metadata"` - }{ - Metadata: map[string]interface{}{ - "annotations": annotations, - }, - } - podDesc := pod.Namespace + "/" + pod.Name - patchData, err = json.Marshal(&patch) + // Get the latest version of the pod + currentPod, err := c.clientset.CoreV1().Pods(pod.Namespace).Get(context.TODO(), pod.Name, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("failed to set annotations on pod %s: %v", podDesc, err) + return err } - return c.PatchPod(pod, types.MergePatchType, patchData) -} -// PatchPod applies the patch changes -func (c *client) PatchPod(pod *kapi.Pod, patchType types.PatchType, patchData []byte) error { - log.Debug().Msgf("patch pod, namespace: %s, podName: %s", pod.Namespace, pod.Name) - _, err := c.clientset.CoreV1().Pods(pod.Namespace).Patch( - context.TODO(), pod.Name, patchType, patchData, metav1.PatchOptions{}) + // Check if there are any conflicts with the current view of the pod's annotations and the latest version. + if currentPod.Annotations != nil { + if !reflect.DeepEqual(currentPod.Annotations, pod.Annotations) { + return fmt.Errorf("conflict with the current view of the pod's annotations and the latest version") + } + } + + currentPod.Annotations = annotations + + // Update the pod with retry and backoff + err = wait.ExponentialBackoff(backoffValues, func() (bool, error) { + _, err = c.clientset.CoreV1().Pods(pod.Namespace).Update( + context.Background(), currentPod, metav1.UpdateOptions{}) + return err == nil, nil + }) return err } diff --git a/pkg/k8s-client/mocks/Client.go b/pkg/k8s-client/mocks/Client.go index e78ef67..9e553ae 100644 --- a/pkg/k8s-client/mocks/Client.go +++ b/pkg/k8s-client/mocks/Client.go @@ -6,7 +6,6 @@ import corev1 "k8s.io/api/core/v1" import mock "github.com/stretchr/testify/mock" import rest "k8s.io/client-go/rest" -import types "k8s.io/apimachinery/pkg/types" import v1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" // Client is an autogenerated mock type for the Client type @@ -60,6 +59,29 @@ func (_m *Client) GetPods(namespace string) (*corev1.PodList, error) { return r0, r1 } +// GetPod provides a mock function with given fields: namespace, name +func (_m *Client) GetPod(namespace string, name string) (*corev1.Pod, error) { + ret := _m.Called(namespace, name) + + var r0 *corev1.Pod + if rf, ok := ret.Get(0).(func(string, string) *corev1.Pod); ok { + r0 = rf(namespace, name) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*corev1.Pod) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, string) error); ok { + r1 = rf(namespace, name) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // GetRestClient provides a mock function with given fields: func (_m *Client) GetRestClient() rest.Interface { ret := _m.Called() @@ -76,20 +98,6 @@ func (_m *Client) GetRestClient() rest.Interface { return r0 } -// PatchPod provides a mock function with given fields: pod, patchType, patchData -func (_m *Client) PatchPod(pod *corev1.Pod, patchType types.PatchType, patchData []byte) error { - ret := _m.Called(pod, patchType, patchData) - - var r0 error - if rf, ok := ret.Get(0).(func(*corev1.Pod, types.PatchType, []byte) error); ok { - r0 = rf(pod, patchType, patchData) - } else { - r0 = ret.Error(0) - } - - return r0 -} - // AddFinalizerToNetworkAttachmentDefinition provides a mock function with given fields: namespace, name, finalizer func (_m *Client) AddFinalizerToNetworkAttachmentDefinition(namespace string, name string, finalizer string) error { ret := _m.Called(namespace, name, finalizer)