Add inventory support #1026

TaoZou1 · 2025-02-20T06:47:17Z

No description provided.

cmd/main.go

zhengxiexie · 2025-02-24T08:32:10Z

cmd/main.go

@@ -281,6 +288,8 @@ func startServiceController(mgr manager.Manager, nsxClient *nsx.Client) {
 		networkpolicycontroller.StartNetworkPolicyController(mgr, commonService, vpcService)
 		service.StartServiceLbController(mgr, commonService)
 		subnetbindingcontroller.StartSubnetBindingController(mgr, subnetService, subnetBindingService)
+		inventoryControll := inventory.NewInventoryController(mgr.GetClient(), inventoryService, cf)
+		inventoryControll.SetupWithManager(mgr)


Better comply with StartXXController?

zhengxiexie · 2025-02-24T08:34:26Z

pkg/controllers/inventory/inventory_controller.go

+		log.Error(err, "Create pod Informer error")
+	}
+
+	podInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{


Unhandled error

pkg/controllers/inventory/inventory_controller.go

zhengxiexie · 2025-02-24T08:41:42Z

pkg/controllers/inventory/inventory_controller.go

+package inventory
+
+import (
+	"context"


Imports are not sorted

zhengxiexie · 2025-02-24T08:43:26Z

pkg/controllers/inventory/inventory_controller.go

+	return nil
+}
+
+func (c *InventoryController) handlePod(obj interface{}) {


Some receivers are c, and others are r, could you unify them?

zhengxiexie · 2025-02-24T08:47:51Z

pkg/controllers/inventory/inventory_controller.go

+	case cache.DeletedFinalStateUnknown:
+		pod, ok = obj1.Obj.(*v1.Pod)
+		if !ok {
+			log.Error(errors.New("Unknown obj"), "DeletedFinalStateUnknown Obj is not *v1.Pod")


Use fmt.Errorof as others modules.

zhengxiexie · 2025-02-24T09:01:32Z

pkg/nsx/services/inventory/inventory.go

+}
+
+const (
+	operation_create = "CREATE"


Use camel case instead of snake case

zhengxiexie · 2025-02-24T09:01:51Z

pkg/nsx/services/inventory/inventory.go

+	operation_delete = "DELETE"
+	operation_none   = "NONE"
+
+	INVENTORY_STATUS_UP      = "UP"


zhengxiexie · 2025-02-24T09:50:13Z

This implementation is quite similar to the existing controllers. We can continue using Kubebuilder's Reconcile pattern if we convert the Kubernetes objects into inventory request queue items, which should not lead to any significant issues.

Inventory controller can't use the Reconcile provider by the kube builder. If each inventory request is put in the inventory queue handled by kube builder, each inventory CR will be created. In a large scale system, there should be lots of inventory CR.

zhengxiexie · 2025-02-24T09:56:19Z

Consider replacing the inventoryObjectQueue with a channel? I think this scenario is highly suited for the producer-consumer pattern.

zhengxiexie · 2025-02-24T10:42:33Z

pkg/nsx/services/inventory/inventory.go

+		log.Info("Send update to NSX clusterId ", "ContainerInventoryData", s.requestBuffer)
+		// TODO, check the context.TODO() be replaced by InventoryClient related todo
+		resp, err := s.NSXClient.InventoryClient.ContainerInventoryApi.AddContainerInventoryUpdateUpdates(context.TODO(), s.NSXConfig.Cluster, containerinventory.ContainerInventoryData{ContainerInventoryObjects: s.requestBuffer})
+


I wonder AddContainerInventoryUpdateUpdates if the batch operation request is asynchronous，

How do you know if the process is successful or not?

If failed, after updateInventoryStore, how do you guarantee the consistency of the local store and NSX side?

zhengxiexie · 2025-02-24T11:12:10Z

pkg/nsx/services/inventory/inventory.go

+	return
+}
+
+func (s *InventoryService) compareAndMergeUpdate(pre interface{}, cur interface{}) (string, map[string]interface{}) {


This function is too large, I think we can also comply with builder and compare pattern.
Splitting it into small files is good for scaling.

zhengxiexie · 2025-02-24T11:19:51Z

BTW, I think these functions, especially the func compareAndMergeupdate are a bit too free and unrestrained, not very standardized or unified, and not very easy to expand. You can see that in other modules, we all follow the standards -- buildXX, wrapXX, GetXX, ListXX, CreateOrUpdateXX, Cleanup pattern

zhengxiexie · 2025-02-25T03:44:02Z

pkg/controllers/inventory/inventory_controller.go

+	return nil
+}
+
+func (r *InventoryController) handlePodEvent(event watch.Event) {


Plan where to use it?

zhengxiexie · 2025-02-25T03:48:14Z

pkg/controllers/inventory/inventory_controller.go

+	switch obj1 := obj.(type) {
+	case *v1.Pod:
+		pod = obj1
+	case cache.DeletedFinalStateUnknown:


What does DeletedFinalStateUnknown mean, why other controllers don't have it?

zhengxiexie · 2025-02-25T03:49:13Z

pkg/controllers/inventory/inventory_controller.go

+		}
+	}
+	log.V(1).Info("Inventory processing Pod", "namespace", pod.Namespace, "name", pod.Name)
+	key, _ := keyFunc(pod)


Here keyFunc is cache.DeletionHandlingMetaNamespaceKeyFunc, why?

zhengxiexie · 2025-02-25T03:51:05Z

pkg/controllers/inventory/inventory_controller.go

+}
+
+func (c *InventoryController) processNextInventoryWorkItem() bool {
+	key, quit := c.inventoryObjectQueue.Get()


If use chan here, we don't need lock any more.

zhengxiexie · 2025-02-25T04:02:17Z

pkg/nsx/services/inventory/inventory.go

+
+		case ContainerApplicationInstance:
+			pod := &corev1.Pod{}
+			err := s.Client.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: namespace}, pod)


So you keep a inventoryObjectQueue which contains only keys, any by client.get to check if we delete or add.
Why not construct a key and operation(delete/update/create) object into queue at handlePod, then we don't need to Get here again.

zhengxiexie · 2025-02-25T04:30:07Z

pkg/controllers/inventory/inventory_controller.go

+	Client               client.Client
+	service              *inventory.InventoryService
+	inventoryObjectQueue workqueue.TypedRateLimitingInterface[any]
+	keyBuffer            inventory.KeySet


I think keyBuffer is redundant, inventoryObjectQueue is enough, if you use channel, you don't need to get obj from key again at all, The pseudocode is as follows

func (b *BatchProcessor) Run(stopCh <-chan struct{}) {
batch := make([]NSXOperation, 0, batchSize)
ticker := time.NewTicker(flushInterval)

for { select { case op := <-b.operations: batch = append(batch, op) if len(batch) >= batchSize { b.processBatch(batch) batch = batch[:0] } case <-ticker.C: if len(batch) > 0 { b.processBatch(batch) batch = batch[:0] } case <-stopCh: return } }

}

zhengxiexie · 2025-02-25T05:02:17Z

To summarize, I think we can maintain two channels, one is of inventoryObjectQueue, another is of ContainerInventoryObject, We can try to postpone process batching as much as possible.

A goroutine watches all k8s objects and puts all obj including adding or deleting events to inventoryObjectQueue, no need to consider batch or periodical at this stage.
A goroutine gets this obj, like other controllers, builds this obj, compares it with local in-store, transforms it to ContainerInventoryObject, and then puts it to another channel.
The last goroutine is responsible for aggregating the objects by resource type from the channel, then triggering every API client of InventoryClient to send batch requests or periodical requests.

This method has several benefits:

It only uses two channels, following the principle of Occam's razor, reducing unnecessary queues or key indexes.
Each goroutine is only responsible for handling its own tasks and decoupling them.
It follows our existing controller, where each object has its own build, compare, and store methods, which can reuse existing functions and reduce maintenance costs.
It considers batch processing or scheduled processing at the end, making the logic clearer.

zhengxiexie · 2025-02-27T06:37:49Z

/e2e

zhengxiexie · 2025-02-27T06:47:19Z

/e2e

zhengxiexie · 2025-02-27T07:19:29Z

/e2e

zhengxiexie · 2025-02-27T07:27:22Z

/e2e

TaoZou1 · 2025-02-27T08:10:37Z

/e2e

zhengxiexie · 2025-02-28T03:18:23Z

/e2e

TaoZou1 · 2025-02-28T05:41:36Z

/e2e

TaoZou1 · 2025-02-28T06:51:56Z

/e2e

Add inventory controller, service. Add ApplicationInstanceStore, ClusteStore Test Done 1. inventory cluster could be created 2. pod inventory could be reported

TaoZou1 requested a review from zhengxiexie February 20, 2025 06:47

TaoZou1 force-pushed the inventory branch 6 times, most recently from 6de446c to c2a85a1 Compare February 24, 2025 03:12

zhengxiexie reviewed Feb 24, 2025

View reviewed changes

zhengxiexie reviewed Feb 25, 2025

View reviewed changes

TaoZou1 force-pushed the inventory branch 3 times, most recently from 1a8213c to 4b6586e Compare February 27, 2025 06:25

TaoZou1 force-pushed the inventory branch from 4b6586e to c20d38c Compare February 28, 2025 02:13

TaoZou1 force-pushed the inventory branch from c20d38c to 2d998ec Compare February 28, 2025 06:00

TaoZou1 force-pushed the inventory branch from 2d998ec to 7102581 Compare February 28, 2025 06:53

Add inventory support

329567e

Add inventory controller, service. Add ApplicationInstanceStore, ClusteStore Test Done 1. inventory cluster could be created 2. pod inventory could be reported

TaoZou1 force-pushed the inventory branch from 7102581 to 329567e Compare February 28, 2025 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add inventory support #1026

Add inventory support #1026

TaoZou1 commented Feb 20, 2025

zhengxiexie Feb 24, 2025

TaoZou1 Feb 25, 2025

zhengxiexie Feb 24, 2025

zhengxiexie Feb 24, 2025

zhengxiexie Feb 24, 2025

zhengxiexie Feb 24, 2025

zhengxiexie Feb 24, 2025

TaoZou1 Feb 25, 2025

zhengxiexie Feb 24, 2025

TaoZou1 Feb 25, 2025

zhengxiexie commented Feb 24, 2025

zhengxiexie commented Feb 24, 2025

zhengxiexie Feb 24, 2025

zhengxiexie Feb 24, 2025

zhengxiexie commented Feb 24, 2025 •

edited

Loading

zhengxiexie Feb 25, 2025

zhengxiexie Feb 25, 2025

zhengxiexie Feb 25, 2025

zhengxiexie Feb 25, 2025

zhengxiexie Feb 25, 2025

zhengxiexie Feb 25, 2025

zhengxiexie commented Feb 25, 2025

zhengxiexie commented Feb 27, 2025

zhengxiexie commented Feb 27, 2025

zhengxiexie commented Feb 27, 2025

zhengxiexie commented Feb 27, 2025

TaoZou1 commented Feb 27, 2025

zhengxiexie commented Feb 28, 2025

TaoZou1 commented Feb 28, 2025

TaoZou1 commented Feb 28, 2025

Add inventory support #1026

Are you sure you want to change the base?

Add inventory support #1026

Conversation

TaoZou1 commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengxiexie commented Feb 24, 2025

zhengxiexie commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengxiexie commented Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengxiexie commented Feb 25, 2025

zhengxiexie commented Feb 27, 2025

zhengxiexie commented Feb 27, 2025

zhengxiexie commented Feb 27, 2025

zhengxiexie commented Feb 27, 2025

TaoZou1 commented Feb 27, 2025

zhengxiexie commented Feb 28, 2025

TaoZou1 commented Feb 28, 2025

TaoZou1 commented Feb 28, 2025

zhengxiexie commented Feb 24, 2025 •

edited

Loading