feat: [NPM] call policy reconcile in dataplane #1188

huntergregory · 2022-01-08T00:12:39Z

Adds a go-routine for policy manager reconciling, which:

adds the jump from AZURE-NPM to FORWARD chain
deletes stale chains

Lock stale chains when:

modifying the stale chains cache
deleting chains that were in staleChains (in reconcile())
adding policy chains/rules via iptables-restore in case a chain is in staleChains

As an optimization, policy manager can "force lock" staleChains, which pauses stale chain deletion until next reconcile period.

vakalapa · 2022-01-12T19:45:41Z

npm/pkg/dataplane/policies/policymanager_linux.go


 	// 3. Delete policy chains in the background.
+	// lock here since stale chains are only affected if we successfully remove policies
+	pMgr.staleChains.forceLock()


calculate creator:

acquire reconcileLock (stalechain lock )

Delete jump rules

restore

nitishm · 2022-01-12T19:51:16Z

npm/pkg/dataplane/dataplane.go

 	*ipsets.IPSetManagerCfg
 	*policies.PolicyManagerCfg
+	// helpful for UTs (defaults to false for external packages)
+	disableGoRoutines bool


can we rename this to something like concurrencyEnabled or concurrencyDisabled

Looking at what it does below - maybe something like disableReconcile?

nitishm · 2022-01-12T19:54:15Z

npm/pkg/dataplane/dataplane.go

+	if !dp.disableGoRoutines {
+		dp.policyMgr.Reconcile(dp.stopChannel)
+	}


so if this is disabled we're not running reconcile at all? 😕 or am I misunderstanding this

yes, this is just for unit testing because we test exec calls, and don't want to intermingle exec calls from reconciling with other exec calls

npm/pkg/dataplane/policies/chain-management_linux.go

nitishm · 2022-01-18T18:18:57Z

npm/pkg/dataplane/policies/chain-management_linux_test.go

+		pMgr.reconcileManager.Lock()
+		defer pMgr.reconcileManager.Unlock()
+		start <- struct{}{}
+		require.NoError(t, pMgr.cleanupChains(testChains))


for cleanupChains function can we move

azure-container-networking/npm/pkg/dataplane/policies/chain-management_linux.go

Lines 227 to 237 in fea8283

errCode, err := pMgr.runIPTablesCommand(util.IptablesDestroyFlag, chain)

if err != nil && errCode != doesNotExistErrorCode {

// add to staleChains if it's not one of the iptablesAzureChains

pMgr.staleChains.add(chain)

currentErrString := fmt.Sprintf("failed to clean up chain %s with err [%v]", chain, err)

if aggregateError == nil {

aggregateError = npmerrors.SimpleError(currentErrString)

} else {

aggregateError = npmerrors.SimpleErrorWrapper(fmt.Sprintf("%s and had previous error", currentErrString), aggregateError)

}

}

to the default case in the select loop above it?

nitishm · 2022-01-18T18:45:46Z

test/integration/npm/main.go


 func main() {
-	dp, err := dataplane.NewDataPlane(nodeName, common.NewIOShim(), dpCfg)
+	dp, err := dataplane.NewDataPlane(nodeName, common.NewIOShim(), dpCfg, nil)


Any reason to keep the stop channel nil? Can we pass a legit channel?

no reason. I can do that

test/integration/npm/main.go

nitishm

functionality wise I think it LGTM. But I have some nits and comments/questions

vakalapa · 2022-01-18T20:50:10Z

npm/pkg/dataplane/dataplane.go

 		return nil, err
 	}
+	// necessary for UTs because of ioshim
+	if !dp.disableReconcileForUTs {


Just call it disableReconcile, and if needed we can make this as a toggle tomorrow to stop if required.

Or should we remove this flag, and move reconcile to a function called RunDPTasks, and call RunDPTasks in relevant place after newDP is created ? We can skip running this for UTs, and in main.go we can call dp.RunDPTasks.

Removed this flag and created a dp.RunPeriodicTasks()

This reverts commit de3d6e2.

vakalapa

Great work ! 💯

nitishm

LGTM

call policy reconcile in dataplane

2158914

huntergregory requested review from matmerr, nitishm and vakalapa January 8, 2022 00:12

huntergregory added 2 commits January 11, 2022 15:08

lock to staleChains

a5dcc10

allow interruption of deleting stale chains while reconciling

d9f2e1a

huntergregory added the npm Related to NPM. label Jan 12, 2022

huntergregory added 2 commits January 11, 2022 17:08

fix lint

7a83224

switch reconcile period back from seconds to minutes

66be271

vakalapa reviewed Jan 12, 2022

View reviewed changes

nitishm reviewed Jan 12, 2022

View reviewed changes

address comments

fea8283

nitishm reviewed Jan 18, 2022

View reviewed changes

npm/pkg/dataplane/policies/chain-management_linux.go Show resolved Hide resolved

nitishm reviewed Jan 18, 2022

View reviewed changes

npm/pkg/dataplane/policies/chain-management_linux.go Show resolved Hide resolved

nitishm reviewed Jan 18, 2022

View reviewed changes

test/integration/npm/main.go Show resolved Hide resolved

nitishm suggested changes Jan 18, 2022

View reviewed changes

vakalapa reviewed Jan 18, 2022

View reviewed changes

huntergregory added 6 commits January 18, 2022 14:31

address comments

e433728

remove RunPeriodicTasks from GenericDP interface

a0d1377

fix build error

de3d6e2

merge with master

261acda

Revert "fix build error"

e1e4a3e

This reverts commit de3d6e2.

make RunPeriodicTasks an interface method again

729f7f8

vakalapa approved these changes Jan 18, 2022

View reviewed changes

nitishm approved these changes Jan 18, 2022

View reviewed changes

huntergregory merged commit 3d1e739 into master Jan 19, 2022

huntergregory deleted the call-reconcile-in-dp branch January 19, 2022 04:00

	errCode, err := pMgr.runIPTablesCommand(util.IptablesDestroyFlag, chain)
	if err != nil && errCode != doesNotExistErrorCode {
	// add to staleChains if it's not one of the iptablesAzureChains
	pMgr.staleChains.add(chain)
	currentErrString := fmt.Sprintf("failed to clean up chain %s with err [%v]", chain, err)
	if aggregateError == nil {
	aggregateError = npmerrors.SimpleError(currentErrString)
	} else {
	aggregateError = npmerrors.SimpleErrorWrapper(fmt.Sprintf("%s and had previous error", currentErrString), aggregateError)
	}
	}

feat: [NPM] call policy reconcile in dataplane #1188

feat: [NPM] call policy reconcile in dataplane #1188

Uh oh!

Conversation

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

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!

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!

Uh oh!

nitishm 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!

Choose a reason for hiding this comment

Uh oh!

vakalapa left a comment

Choose a reason for hiding this comment

Uh oh!

nitishm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

huntergregory commented Jan 8, 2022 •

edited

Loading