Skip to content

Commit 1e28e64

Browse files
authored
Merge branch 'main' into fix-reconcile-finalizer-update
2 parents 81991d4 + dd83f47 commit 1e28e64

File tree

10 files changed

+272
-154
lines changed

10 files changed

+272
-154
lines changed

controllers/cidr_handler_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ var _ = Describe("Test CIDR Handler ", func() {
345345
scheme = runtime.NewScheme()
346346
Expect(multinicv1.AddToScheme(scheme)).To(Succeed())
347347
var err error
348-
defHandler, err = plugin.GetNetAttachDefHandler(Cfg)
348+
defHandler, err = plugin.GetNetAttachDefHandler(Cfg, scheme)
349349
Expect(err).To(BeNil())
350350
})
351351

controllers/config_controller_test.go

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
corev1 "k8s.io/api/core/v1"
1111
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1212
"k8s.io/apimachinery/pkg/types"
13-
ctrl "sigs.k8s.io/controller-runtime"
1413

1514
multinicv1 "github.com/foundation-model-stack/multi-nic-cni/api/v1"
1615
)
@@ -43,17 +42,20 @@ var _ = Describe("Test Config Controller", func() {
4342
By("new daemonset")
4443
ds := ConfigReconcilerInstance.NewCNIDaemonSet(dummyConfigName, spec.Daemon)
4544
Expect(ds).NotTo(BeNil())
46-
_, err = ConfigReconcilerInstance.Reconcile(ctx, ctrl.Request{
47-
NamespacedName: namespacedName,
48-
})
49-
Expect(err).To(BeNil())
45+
// Wait for DaemonSet to be created by the controller
46+
Eventually(func(g Gomega) {
47+
daemonset, err := ConfigReconcilerInstance.Clientset.AppsV1().DaemonSets(OPERATOR_NAMESPACE).Get(ctx, dummyConfigName, metav1.GetOptions{})
48+
g.Expect(err).To(BeNil())
49+
g.Expect(daemonset).NotTo(BeNil())
50+
}).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should(Succeed())
5051
By("deleting")
5152
err = ConfigReconcilerInstance.Client.Delete(ctx, cfg)
5253
Expect(err).To(BeNil())
53-
_, err = ConfigReconcilerInstance.Reconcile(ctx, ctrl.Request{
54-
NamespacedName: namespacedName,
55-
})
56-
Expect(err).To(BeNil())
54+
// Wait for Config to be deleted by the controller
55+
Eventually(func(g Gomega) {
56+
err := ConfigReconcilerInstance.Client.Get(ctx, namespacedName, &config)
57+
g.Expect(err).NotTo(BeNil())
58+
}).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should(Succeed())
5759
// set config ready back to true for the rest test
5860
ConfigReady = true
5961
})

controllers/multinicnetwork_test.go

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
. "github.com/onsi/gomega"
1717
corev1 "k8s.io/api/core/v1"
1818
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
19+
client "sigs.k8s.io/controller-runtime/pkg/client"
1920
//+kubebuilder:scaffold:imports
2021
)
2122

@@ -37,10 +38,24 @@ var _ = Describe("Test deploying MultiNicNetwork", func() {
3738
It("successfully create/delete network attachment definition", func() {
3839
mainPlugin, annotations, err := MultiNicnetworkReconcilerInstance.GetMainPluginConf(multinicnetwork)
3940
Expect(err).NotTo(HaveOccurred())
40-
err = MultiNicnetworkReconcilerInstance.NetAttachDefHandler.CreateOrUpdate(multinicnetwork, mainPlugin, annotations)
41+
// Create the MultiNicNetwork in the cluster
42+
err = K8sClient.Create(context.TODO(), multinicnetwork)
4143
Expect(err).NotTo(HaveOccurred())
42-
err = MultiNicnetworkReconcilerInstance.NetAttachDefHandler.DeleteNets(multinicnetwork)
44+
// Fetch the created MultiNicNetwork to get the UID
45+
fetched := &multinicv1.MultiNicNetwork{}
46+
err = K8sClient.Get(context.TODO(), client.ObjectKey{Name: multinicnetwork.Name, Namespace: multinicnetwork.Namespace}, fetched)
4347
Expect(err).NotTo(HaveOccurred())
48+
err = MultiNicnetworkReconcilerInstance.NetAttachDefHandler.CreateOrUpdate(fetched, mainPlugin, annotations)
49+
Expect(err).NotTo(HaveOccurred())
50+
err = MultiNicnetworkReconcilerInstance.NetAttachDefHandler.DeleteNets(fetched)
51+
Expect(err).NotTo(HaveOccurred())
52+
// Delete the MultiNicNetwork from the cluster
53+
err = K8sClient.Delete(context.TODO(), fetched)
54+
Expect(err).NotTo(HaveOccurred())
55+
// Wait for the MultiNicNetwork to be fully deleted
56+
Eventually(func() error {
57+
return K8sClient.Get(context.TODO(), client.ObjectKey{Name: fetched.Name, Namespace: fetched.Namespace}, &multinicv1.MultiNicNetwork{})
58+
}).ShouldNot(Succeed())
4459
})
4560
It("successfully create/delete network attachment definition on new namespace", func() {
4661
newNamespace := corev1.Namespace{
@@ -51,9 +66,16 @@ var _ = Describe("Test deploying MultiNicNetwork", func() {
5166
mainPlugin, annotations, err := MultiNicnetworkReconcilerInstance.GetMainPluginConf(multinicnetwork)
5267
Expect(err).NotTo(HaveOccurred())
5368
Expect(K8sClient.Create(context.TODO(), &newNamespace)).Should(Succeed())
54-
err = MultiNicnetworkReconcilerInstance.NetAttachDefHandler.CreateOrUpdateOnNamespace(newNamespaceName, multinicnetwork, mainPlugin, annotations)
69+
// Use a fresh MultiNicNetwork object for this test
70+
multinicnetwork2 := GetMultiNicCNINetwork("test-mn", cniVersion, cniType, cniArgs)
71+
err = K8sClient.Create(context.TODO(), multinicnetwork2)
72+
Expect(err).NotTo(HaveOccurred())
73+
fetched := &multinicv1.MultiNicNetwork{}
74+
err = K8sClient.Get(context.TODO(), client.ObjectKey{Name: multinicnetwork2.Name, Namespace: multinicnetwork2.Namespace}, fetched)
75+
Expect(err).NotTo(HaveOccurred())
76+
err = MultiNicnetworkReconcilerInstance.NetAttachDefHandler.CreateOrUpdateOnNamespace(newNamespaceName, fetched, mainPlugin, annotations)
5577
Expect(err).NotTo(HaveOccurred())
56-
err = MultiNicnetworkReconcilerInstance.NetAttachDefHandler.Delete(multinicnetwork.Name, newNamespaceName)
78+
err = MultiNicnetworkReconcilerInstance.NetAttachDefHandler.Delete(fetched.Name, newNamespaceName)
5779
Expect(err).NotTo(HaveOccurred())
5880
})
5981
})

controllers/suite_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ var _ = BeforeSuite(func() {
123123

124124
hostInterfaceHandler := NewHostInterfaceHandler(cfg, mgr.GetClient())
125125

126-
defHandler, err := plugin.GetNetAttachDefHandler(cfg)
126+
defHandler, err := plugin.GetNetAttachDefHandler(cfg, scheme.Scheme)
127127
Expect(err).ToNot(HaveOccurred())
128128

129129
clientset, err := kubernetes.NewForConfig(cfg)

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ require (
1515
k8s.io/apimachinery v0.32.0
1616
k8s.io/client-go v0.32.0
1717
sigs.k8s.io/controller-runtime v0.20.1
18+
k8s.io/utils v0.0.0-20240711033017-18e509b52bc8
1819
)
1920

2021
require (
@@ -100,7 +101,6 @@ require (
100101
k8s.io/component-base v0.32.0 // indirect
101102
k8s.io/klog/v2 v2.130.1 // indirect
102103
k8s.io/kube-openapi v0.0.0-20241105132330-32ad38e42d3f // indirect
103-
k8s.io/utils v0.0.0-20241104100929-3ea5e8cea738 // indirect
104104
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.31.0 // indirect
105105
sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3 // indirect
106106
sigs.k8s.io/structured-merge-diff/v4 v4.4.2 // indirect

internal/plugin/net_attach_def.go

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,13 @@ import (
1818
v1 "k8s.io/api/core/v1"
1919
k8serrors "k8s.io/apimachinery/pkg/api/errors"
2020
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
21+
"k8s.io/apimachinery/pkg/runtime"
2122
"k8s.io/apimachinery/pkg/runtime/schema"
2223
"k8s.io/client-go/dynamic"
2324
"k8s.io/client-go/kubernetes"
2425
"k8s.io/client-go/rest"
26+
"k8s.io/utils/ptr"
27+
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
2528
)
2629

2730
const (
@@ -90,9 +93,10 @@ type NetworkStatus struct {
9093
type NetAttachDefHandler struct {
9194
*DynamicHandler
9295
*kubernetes.Clientset
96+
Scheme *runtime.Scheme
9397
}
9498

95-
func GetNetAttachDefHandler(config *rest.Config) (*NetAttachDefHandler, error) {
99+
func GetNetAttachDefHandler(config *rest.Config, scheme *runtime.Scheme) (*NetAttachDefHandler, error) {
96100
dyn, err := dynamic.NewForConfig(config)
97101
if err != nil {
98102
return nil, err
@@ -106,6 +110,7 @@ func GetNetAttachDefHandler(config *rest.Config) (*NetAttachDefHandler, error) {
106110
return &NetAttachDefHandler{
107111
DynamicHandler: handler,
108112
Clientset: clientset,
113+
Scheme: scheme,
109114
}, nil
110115
}
111116

@@ -128,7 +133,7 @@ func (h *NetAttachDefHandler) CreateOrUpdate(net *multinicv1.MultiNicNetwork, pl
128133
}
129134
errMsg := ""
130135
for _, def := range defs {
131-
errMsg = h.createOrUpdate(def, errMsg)
136+
errMsg = h.createOrUpdate(net, def, errMsg)
132137
}
133138
if errMsg != "" {
134139
vars.NetworkLog.V(2).Info(errMsg)
@@ -141,7 +146,7 @@ func (h *NetAttachDefHandler) CreateOrUpdateOnNamespace(ns string, net *multinic
141146
if err != nil {
142147
return err
143148
}
144-
errMsg := h.createOrUpdate(def, "")
149+
errMsg := h.createOrUpdate(net, def, "")
145150
if errMsg != "" {
146151
vars.NetworkLog.V(2).Info(errMsg)
147152
return errors.New(errMsg)
@@ -150,7 +155,19 @@ func (h *NetAttachDefHandler) CreateOrUpdateOnNamespace(ns string, net *multinic
150155
}
151156

152157
// createOrUpdate creates new NetworkAttachmentDefinition resource if not exists, otherwise update
153-
func (h *NetAttachDefHandler) createOrUpdate(def *NetworkAttachmentDefinition, errMsg string) string {
158+
// Sets owner reference to the given MultiNicNetwork
159+
func (h *NetAttachDefHandler) createOrUpdate(multinicnetwork *multinicv1.MultiNicNetwork, def *NetworkAttachmentDefinition, errMsg string) string {
160+
if err := controllerutil.SetControllerReference(multinicnetwork, def, h.Scheme); err != nil {
161+
return fmt.Sprintf("failed to set controller reference: %v", err)
162+
}
163+
// Ensure Controller and BlockOwnerDeletion are set to true
164+
for i := range def.OwnerReferences {
165+
ref := &def.OwnerReferences[i]
166+
if ref.UID == multinicnetwork.UID {
167+
ref.Controller = ptr.To(true)
168+
ref.BlockOwnerDeletion = ptr.To(true)
169+
}
170+
}
154171
name := def.GetName()
155172
namespace := def.GetNamespace()
156173
result := &NetworkAttachmentDefinition{}

internal/plugin/net_attach_def_test.go

Lines changed: 79 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ import (
1616
corev1 "k8s.io/api/core/v1"
1717
"k8s.io/apimachinery/pkg/api/errors"
1818
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
19+
"k8s.io/client-go/kubernetes/scheme"
20+
"sigs.k8s.io/controller-runtime/pkg/client"
1921
//+kubebuilder:scaffold:imports
2022
)
2123

@@ -40,7 +42,7 @@ var _ = Describe("NetAttachDef test", func() {
4042

4143
BeforeAll(func() {
4244
var err error
43-
handler, err = GetNetAttachDefHandler(Cfg)
45+
handler, err = GetNetAttachDefHandler(Cfg, scheme.Scheme)
4446
Expect(err).To(BeNil())
4547

4648
ipvlanPlugin := &IPVLANPlugin{}
@@ -49,28 +51,102 @@ var _ = Describe("NetAttachDef test", func() {
4951
})
5052

5153
It("create and delete", func() {
52-
err := handler.CreateOrUpdate(multinicnetwork, mainPlugin, annotations)
54+
// Create the MultiNicNetwork in the cluster
55+
err := K8sClient.Create(ctx, multinicnetwork)
56+
Expect(err).To(BeNil())
57+
// Fetch the created MultiNicNetwork to get the UID
58+
fetched := multinicnetwork.DeepCopy()
59+
err = K8sClient.Get(ctx, client.ObjectKey{Name: multinicnetwork.Name, Namespace: multinicnetwork.Namespace}, fetched)
60+
Expect(err).To(BeNil())
61+
// Use fetched (with UID) for NAD creation
62+
err = handler.CreateOrUpdate(fetched, mainPlugin, annotations)
5363
Expect(err).To(BeNil())
5464
namespaceList := corev1.NamespaceList{}
5565
err = K8sClient.List(ctx, &namespaceList)
5666
Expect(err).To(BeNil())
5767
Expect(len(namespaceList.Items)).To(BeNumerically(">", 0))
5868
for _, namespace := range namespaceList.Items {
5969
By(fmt.Sprintf("checking creation in namespace %s", namespace.Name))
70+
Eventually(func(g Gomega) {
71+
nad, err := handler.Get(multinicnetworkName, namespace.Name)
72+
g.Expect(err).To(BeNil())
73+
// Check owner reference
74+
refs := nad.OwnerReferences
75+
g.Expect(refs).ToNot(BeEmpty())
76+
found := false
77+
for _, ref := range refs {
78+
if ref.Kind == "MultiNicNetwork" && ref.Name == fetched.Name && ref.UID == fetched.UID {
79+
found = true
80+
}
81+
}
82+
g.Expect(found).To(BeTrue(), "OwnerReference to MultiNicNetwork should be set on NAD")
83+
}).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should(Succeed())
84+
}
85+
// Delete the MultiNicNetwork
86+
err = K8sClient.Delete(ctx, fetched)
87+
Expect(err).To(BeNil())
88+
// Check if the MultiNicNetwork is actually deleted
89+
Eventually(func(g Gomega) {
90+
getErr := K8sClient.Get(ctx, client.ObjectKey{Name: fetched.Name, Namespace: fetched.Namespace}, fetched)
91+
fmt.Fprintf(GinkgoWriter, "[GinkgoWriter] MultiNicNetwork get after delete: %v\n", getErr)
92+
if getErr == nil {
93+
fmt.Fprintf(GinkgoWriter, "[GinkgoWriter] MultiNicNetwork DeletionTimestamp: %v, Finalizers: %v\n",
94+
fetched.DeletionTimestamp, fetched.Finalizers)
95+
}
96+
g.Expect(getErr).ToNot(BeNil())
97+
}).WithTimeout(10 * time.Second).WithPolling(1 * time.Second).Should(Succeed())
98+
for _, namespace := range namespaceList.Items {
99+
By(fmt.Sprintf("checking deletion in namespace %s", namespace.Name))
100+
// NOTE: Kubernetes garbage collection for CRDs is not supported in unit/envtest environments.
101+
// Only check that the OwnerReference is set correctly. GC should be tested in a real cluster.
102+
nad, err := handler.Get(multinicnetworkName, namespace.Name)
103+
if err == nil && len(nad.OwnerReferences) > 0 {
104+
ref := nad.OwnerReferences[0]
105+
// Assert OwnerReference fields are correct
106+
Expect(ref.Controller != nil && *ref.Controller).To(BeTrue())
107+
Expect(ref.BlockOwnerDeletion != nil && *ref.BlockOwnerDeletion).To(BeTrue())
108+
}
109+
}
110+
// Clean up NADs for future tests
111+
for _, namespace := range namespaceList.Items {
112+
_ = handler.Delete(multinicnetworkName, namespace.Name)
113+
}
114+
})
115+
116+
It("finalizer and owner reference work together for NAD cleanup", func() {
117+
// Add a finalizer to the MultiNicNetwork
118+
finalizer := "test.finalizer.multinicnetwork"
119+
multinicnetwork.ObjectMeta.Finalizers = append(multinicnetwork.ObjectMeta.Finalizers, finalizer)
120+
// Create the resource
121+
err := handler.CreateOrUpdate(multinicnetwork, mainPlugin, annotations)
122+
Expect(err).To(BeNil())
123+
namespaceList := corev1.NamespaceList{}
124+
err = K8sClient.List(ctx, &namespaceList)
125+
Expect(err).To(BeNil())
126+
// Confirm NADs exist
127+
for _, namespace := range namespaceList.Items {
60128
Eventually(func(g Gomega) {
61129
_, err := handler.Get(multinicnetworkName, namespace.Name)
62130
g.Expect(err).To(BeNil())
63131
}).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should(Succeed())
64132
}
133+
// Delete the MultiNicNetwork (simulate controller removing finalizer after cleanup)
134+
// In a real controller, the finalizer is removed after cleanup; here, we simulate it
135+
// by removing the finalizer and then deleting the resource
136+
multinicnetwork.ObjectMeta.Finalizers = []string{}
137+
// Simulate deletion by deleting NADs and then the MultiNicNetwork
65138
err = handler.DeleteNets(multinicnetwork)
66139
Expect(err).To(BeNil())
140+
// Now, delete the MultiNicNetwork (would be done by controller after finalizer logic)
141+
// In this test, we just check that NADs are gone and resource is not stuck
67142
for _, namespace := range namespaceList.Items {
68-
By(fmt.Sprintf("checking deletion in namespace %s", namespace.Name))
69143
Eventually(func(g Gomega) {
70144
_, err := handler.Get(multinicnetworkName, namespace.Name)
71145
g.Expect(errors.IsNotFound(err)).To(BeTrue())
72146
}).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should(Succeed())
73147
}
148+
// The MultiNicNetwork should not be stuck in Terminating (simulate by checking finalizer is gone)
149+
Expect(multinicnetwork.ObjectMeta.Finalizers).To(BeEmpty())
74150
})
75151

76152
Context("CheckDefChanged", func() {

main.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ func main() {
136136

137137
hostInterfaceHandler := controllers.NewHostInterfaceHandler(config, mgr.GetClient())
138138

139-
defHandler, err := plugin.GetNetAttachDefHandler(config)
139+
defHandler, err := plugin.GetNetAttachDefHandler(config, scheme)
140140
if err != nil {
141141
vars.SetupLog.Error(err, "unable to create NetworkAttachmentdefinition handler")
142142
os.Exit(1)

0 commit comments

Comments
 (0)