feat: [NPM] Adding DPShim layer in controller pods #1206

vakalapa · 2022-01-24T21:25:50Z

Reason for Change:

Adding DPShim component,

DPShim works as a translator, cache and transporter of Resource events into sets/policies and relays them to gRPC server. The gRPC server will distribute these goal states to all the registered daemon clients.

DPShim:

Cache locally all the sets and policies
On Apply Dataplane, convert dirty cache to relevant GoalState events and send to gRPC server
periodically delete unused sets
Provide a function for clients on first time registration to get the state of whole cache

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

npm/pkg/controlplane/types.go

npm/pkg/dataplane/dpshim/dpshim.go

nitishm · 2022-01-27T18:43:26Z

npm/pkg/dataplane/dpshim/dpshim.go

 }

+func (dp *DPShim) getIPSet(setName string) *controlplane.ControllerIPSets {
+	return dp.setCache[setName]


Should this be getCache?

getIPSet is in line with our interface, so prefer to keep it this. Unless you have strong reservations ?

npm/pkg/dataplane/dpshim/dpshim.go

nitishm · 2022-01-27T18:45:44Z

npm/pkg/dataplane/dpshim/dpshim.go

+	dp.Lock()
+	defer dp.Unlock()


might make more sense to move the lock/unlock to the helper functions deleteIPSet

I am following the practice of Exposed function deal with locks and internal functions to not worry about locks. Internal functions by default we can assume that they are to be handled with care. Also, createIPSet is called from some external functions and we cannot deal with locking and unlocking always. Wdyt ?

nitishm · 2022-01-27T18:49:12Z

npm/pkg/dataplane/dpshim/dpshim.go

+	dp.Lock()
+	defer dp.Unlock()


If we have this in createIPSet we wouldn't have to lock on a large code block

except for the exists check everything needs to be locked anyway, so i believe trying to lock for each loop might add more overhead. wdyt ?

nitishm · 2022-01-27T18:55:22Z

npm/pkg/dataplane/dpshim/dpshim.go

+		}
+
+		policy := dp.policyCache[policyKey]
+		toApplyPolicies[idx] = policy


dont need to use ID - we can just do this,

toApplyPolicies = append(toApplyPolicies, policy)

if you use append, you should set the initial capacity of the slice to avoid resizing: make([]thing, 0, len(cache))

nitishm · 2022-01-27T18:55:36Z

npm/pkg/dataplane/dpshim/dpshim.go

+			return nil, npmerrors.Errorf(npmerrors.AddPolicy, false, fmt.Sprintf("policy %s not found", policyKey))
+		}
+
+		policy := dp.policyCache[policyKey]


What if the policy doesnt exist?

Thats the check we are doing one line above ?

nitishm · 2022-01-27T18:56:12Z

npm/pkg/dataplane/dpshim/dpshim.go

+	idx := 0
+
+	for policyKey := range dp.dirtyCache.toDeletePolicies {
+		toDeletePolicies[idx] = policyKey


Same - make use of append

Avoiding resizing since we know the size of data.

nitishm · 2022-01-27T18:57:38Z

npm/pkg/dataplane/dpshim/dpshim.go

+				dp.checkSetReferences()
+				err := dp.ApplyDataPlane()
+				if err != nil {
+					klog.Errorf("deleteUnusedSets: failed to apply dataplane %v", err)


I've been curious about error propagation in go routines. Should we create an error channel to feedback errors and handle them in the caller code?

This is across our codebase I mean

nitishm

Functionality looks good - just some nit picking

huntergregory

dpshim looks pretty good to me. Made several suggestions. This PR seems dependent on #1202 for validation logic

huntergregory · 2022-01-27T18:28:27Z

npm/pkg/controlplane/gobutils.go

+func DecodeStrings(payload *bytes.Buffer) ([]string, error) {
 	if payload == nil {
-		return "", npmerrors.SimpleError("failed to decode, payload is nil")
+		return nil, npmerrors.SimpleError("failed to decode, payload is nil")


should we check if len(payload) == 0 instead?

similarly for other decode funcs

npm/pkg/controlplane/types.go

huntergregory · 2022-01-27T18:36:09Z

npm/pkg/controlplane/types.go

+		IPSetMetadata:   metadata,
+		IPPodMetadata:   make(map[string]*dp.PodMetadata),
+		MemberIPSets:    make(map[string]*ipsets.IPSetMetadata),
+		ipsetReference:  make(map[string]struct{}),


do we need this to be a map? What about having referCount like in ipsets.IPSet?

npm/pkg/dataplane/dataplane.go

npm/pkg/dataplane/ipsets/ipsetmanager_test.go

npm/pkg/dataplane/dpshim/dpshim.go

huntergregory · 2022-01-27T19:16:14Z

npm/pkg/dataplane/dpshim/dpshim.go

+		}
+
+		set := dp.setCache[prefixedSetName]
+		if set.IPSetMetadata.GetSetKind() != ipsets.HashSet {


we should generalize addToSet, deleteFromSet, addToList, ... validations into a dp func for both dpshim and Dataplane

Agreed lets plan on how we can achieve this properly.

huntergregory · 2022-01-27T19:18:12Z

npm/pkg/dataplane/dpshim/dpshim.go

+	}
+
+	if len(goalStates) == 0 {
+		klog.Info("ApplyDataPlane: No changes to apply")


could this ever happen?

huntergregory · 2022-01-27T19:21:50Z

npm/pkg/dataplane/dpshim/dpshim.go

+		}
+
+		policy := dp.policyCache[policyKey]
+		toApplyPolicies[idx] = policy


if you use append, you should set the initial capacity of the slice to avoid resizing: make([]thing, 0, len(cache))

npm/pkg/dataplane/dpshim/dpshim.go

huntergregory · 2022-02-11T20:49:06Z

npm/http/server/server.go

 	// prometheus handlers
 	if config.Toggles.EnablePrometheusMetrics {
 		rs.router.Handle(api.NodeMetricsPath, metrics.GetHandler(metrics.NodeMetrics))
 		rs.router.Handle(api.ClusterMetricsPath, metrics.GetHandler(metrics.ClusterMetrics))


is this nil check for the encoder important for v1?

This is not relevant in V1 case, this is specifically for daemon where we will need the PProf endpoint but epmencoder is not yet supported. That is a todo item for later.

npm/pkg/dataplane/ipsets/ipsetmanager_test.go

huntergregory · 2022-02-11T20:57:20Z

npm/pkg/dataplane/dpshim/dpshim.go

 	}()
-	dp.Lock()
-	defer dp.Unlock()
+	dp.lock()


Unlock in the defer func? with current code, dp will be unlocked before dp.ApplyDataplane() because defers are processed LIFO

I see that ApplyDataplane locks too. Maybe we make a little applyDataplane that doesn't lock?

This is intentional and keeping the same design from our existing DP, we do not anticipate another process coming in between and making changes.

Even if it does i do not think there is a major issue, except for one, i.e. if a Set is added/created and immediately deleted then kernel might complain about it does not exists. But i think you had suggested a fix for it to use --exists, is that in master ?

will it add system overhead to lock/unlock twice for each Policy call though? We haven't solved the created then deleted edge case yet (in the non-fan-out model, we thought that situation shouldn't happen)

npm/pkg/dataplane/dpshim/dpshim.go

npm/pkg/controlplane/goalstateprocessor/goalstateprocessor.go

npm/pkg/controlplane/controllers/v1/networkPolicyController.go

npm/pkg/controlplane/goalstateprocessor/goalstateprocessor.go

nitishm

Logic seems wrong in b7ffd35#r806133602

nitishm

LGTM

huntergregory

🚀

vakalapa added 5 commits January 18, 2022 16:12

intial touches to create sets

21fb28e

adding initial touches to dpshim

7fc94ba

fixing merge issues

c2c0d4f

deprecating initialize DP func

8f3a732

feat: [NPM] Adding DPShim layer in controller pods

4bbd107

vakalapa added enhancement npm Related to NPM. labels Jan 24, 2022

vakalapa added 3 commits January 24, 2022 13:39

correcting an import error

0ceb821

Adding some UTs

9dd312e

adding a UT

3854b84

nitishm reviewed Jan 27, 2022

View reviewed changes

npm/pkg/controlplane/types.go Outdated Show resolved Hide resolved

nitishm reviewed Jan 27, 2022

View reviewed changes

npm/pkg/dataplane/dpshim/dpshim.go Show resolved Hide resolved

nitishm reviewed Jan 27, 2022

View reviewed changes

npm/pkg/dataplane/dpshim/dpshim.go Show resolved Hide resolved

nitishm reviewed Jan 27, 2022

View reviewed changes

nitishm suggested changes Jan 27, 2022

View reviewed changes

huntergregory suggested changes Jan 27, 2022

View reviewed changes

vakalapa commented Jan 27, 2022

View reviewed changes

npm/pkg/dataplane/dpshim/dpshim.go Outdated Show resolved Hide resolved

vakalapa added 7 commits February 1, 2022 10:23

Addressing some comments

75dcb9f

merging master

145a63c

Moving an UT to linux specifc file

1db2ad9

Merge branch 'master' into vakr/dpshim

662c152

Fixing some issues with controller pod

5080662

Adding some dns poilices and logs for debugging

c871bcf

Moving aroudn outchannel to help with hydration of new clients

5c3fb23

vakalapa added 8 commits February 7, 2022 20:50

removeing pass by ref

252f2ab

Adding http server in daemon for pprof

db94ab7

Adding a new grpc option to wait

dc9f86a

Fixing 100% cpu in daemon

a8c6045

Fixing some logic in list management

5f13ed5

Applying some golints

a29cb82

Merging with master

02781aa

adding mutex

6930b04

huntergregory reviewed Feb 11, 2022

View reviewed changes

Addressing comments and solving a bug. Cyclonus seems to be good now

b7ffd35

huntergregory reviewed Feb 14, 2022

View reviewed changes

npm/pkg/controlplane/goalstateprocessor/goalstateprocessor.go Outdated Show resolved Hide resolved

nitishm reviewed Feb 14, 2022

View reviewed changes

npm/pkg/controlplane/controllers/v1/networkPolicyController.go Show resolved Hide resolved

nitishm reviewed Feb 14, 2022

View reviewed changes

npm/pkg/controlplane/goalstateprocessor/goalstateprocessor.go Outdated Show resolved Hide resolved

nitishm suggested changes Feb 14, 2022

View reviewed changes

vakalapa added 3 commits February 14, 2022 11:18

Fixing a bug

76f4f5b

Addressing a comment

59483ac

fixing an issue and addressing comments

faf2b59

nitishm approved these changes Feb 14, 2022

View reviewed changes

huntergregory approved these changes Feb 14, 2022

View reviewed changes

vakalapa merged commit ad516c0 into master Feb 15, 2022

vakalapa deleted the vakr/dpshim branch February 15, 2022 00:10

feat: [NPM] Adding DPShim layer in controller pods #1206

feat: [NPM] Adding DPShim layer in controller pods #1206

Uh oh!

Conversation

vakalapa commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitishm left a comment

Choose a reason for hiding this comment

Uh oh!

huntergregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vakalapa commented Jan 24, 2022 •

edited

Loading