Update eBPF config cache and local l3af-config-store file for a deleted interface #374

jaysheth2 · 2024-04-21T22:43:07Z

When an interface is deleted on the fly, and a delete/remove call is received for the that interface, all the programs associated with that interface should be deleted. Hence, in the PR, on receiving a delete call for an unloaded-interface, the local copy of the programs stored in go-interface and the l3af-config-store file is deleted for cleanup

… the l3af-config-store Signed-off-by: Jay Sheth <[email protected]>

sanfern · 2024-04-22T12:43:43Z

kf/nfconfig.go

@@ -1312,6 +1323,9 @@ func (c *NFConfigs) DeleteEbpfPrograms(bpfProgs []models.L3afBPFProgramNames) er
 			if err := c.SaveConfigsToConfigStore(); err != nil {
 				return fmt.Errorf("SaveConfigsToConfigStore failed to save configs %w", err)
 			}
+			//if err := c.SaveConfigsToConfigStore(); err != nil {
+			//	return fmt.Errorf("SaveConfigsToConfigStore failed to save configs %w", err)
+			//}


Please remove commented code.

charleskbliu0

Looks good

pmoroney · 2024-04-23T14:55:14Z

kf/nfconfig.go

-		errOut := fmt.Errorf("%s interface name not found in the host", ifaceName)
+		errOut := fmt.Errorf("%s interface name not found in the host, and deleting all EBPF programs associated to ", ifaceName)
+		if c.EgressTCBpfs[ifaceName] != nil {
+			c.EgressTCBpfs[ifaceName] = nil


This sets the lists to nil, are there any additional cleanups that need to happen or is this enough to remove the reference counts and allow the garbage collector to lazily clean up?

Once the interface is deleted, all the programs associated with will automatically be deleted by the kernel.
I have added a separate function for cleanup all the L3AFd related parameters and information related to the deleted interface.

…face Signed-off-by: Jay Sheth <[email protected]>

Signed-off-by: Jay Sheth <[email protected]>

pmoroney

LGTM, thanks!

Signed-off-by: Jay Sheth <[email protected]>

sanfern · 2024-04-25T13:28:14Z

kf/bpf.go

-		if err := b.UnloadProgram(ifaceName, direction); err != nil {
-			return fmt.Errorf("BPFProgram %s unload failed on interface %s with error: %w", b.Program.Name, ifaceName, err)
+	allInterfaces, _ := getHostInterfaces()
+	for iface := range allInterfaces {


We should fetch interface from map and avoid for loop.

Signed-off-by: Jay Sheth <[email protected]>

sanfern · 2024-04-26T16:57:55Z

kf/nfconfig.go

+				for e := bpfList.Front(); e != nil; e = e.Next() {
+					data := e.Value.(*BPF)
+					if err := data.Stop(ifaceName, models.XDPIngressType, c.HostConfig.BpfChainingEnabled); err != nil {
+						log.Error().Err(err).Msgf("failed to remove map file for program %s => %s", ifaceName, data.Program.Name)
+					}
+				}


Suggested change

for e := bpfList.Front(); e != nil; e = e.Next() {

data := e.Value.(*BPF)

if err := data.Stop(ifaceName, models.XDPIngressType, c.HostConfig.BpfChainingEnabled); err != nil {

log.Error().Err(err).Msgf("failed to remove map file for program %s => %s", ifaceName, data.Program.Name)

}

}

c.StopNRemoveAllBPFPrograms(ifaceName, models.XDPIngressType)

sanfern · 2024-04-26T17:03:17Z

kf/nfconfig.go

+				for e := bpfList.Front(); e != nil; e = e.Next() {
+					data := e.Value.(*BPF)
+					if err := data.Stop(ifaceName, models.IngressType, c.HostConfig.BpfChainingEnabled); err != nil {
+						log.Error().Err(err).Msgf("failed to remove map file for program %s => %s", ifaceName, data.Program.Name)
+					}
+				}


Suggested change

for e := bpfList.Front(); e != nil; e = e.Next() {

data := e.Value.(*BPF)

if err := data.Stop(ifaceName, models.IngressType, c.HostConfig.BpfChainingEnabled); err != nil {

log.Error().Err(err).Msgf("failed to remove map file for program %s => %s", ifaceName, data.Program.Name)

}

}

c.StopNRemoveAllBPFPrograms(ifaceName, models.IngressType)

sanfern · 2024-04-26T17:04:22Z

kf/nfconfig.go

+				for e := bpfList.Front(); e != nil; e = e.Next() {
+					data := e.Value.(*BPF)
+					if err := data.Stop(ifaceName, models.EgressType, c.HostConfig.BpfChainingEnabled); err != nil {
+						log.Error().Err(err).Msgf("failed to remove map file for program %s => %s", ifaceName, data.Program.Name)
+					}
+				}


Suggested change

for e := bpfList.Front(); e != nil; e = e.Next() {

data := e.Value.(*BPF)

if err := data.Stop(ifaceName, models.EgressType, c.HostConfig.BpfChainingEnabled); err != nil {

log.Error().Err(err).Msgf("failed to remove map file for program %s => %s", ifaceName, data.Program.Name)

}

}

c.StopNRemoveAllBPFPrograms(ifaceName, models.EgressType)

sanfern · 2024-04-26T17:05:35Z

kf/nfconfig.go

+		if c.IngressXDPBpfs[ifaceName] != nil {
+			if err := c.StopNRemoveAllBPFPrograms(ifaceName, models.XDPIngressType); err != nil {
+				log.Warn().Err(err).Msg("failed to Close Ingress XDP BPF Program")
+			}
+		}
+		if c.IngressTCBpfs[ifaceName] != nil {
+			if err := c.StopNRemoveAllBPFPrograms(ifaceName, models.IngressType); err != nil {
+				log.Warn().Err(err).Msg("failed to Close Ingress XDP BPF Program")
+			}
+		}
+		if c.EgressTCBpfs[ifaceName] != nil {
+			if err := c.StopNRemoveAllBPFPrograms(ifaceName, models.EgressType); err != nil {
+				log.Warn().Err(err).Msg("failed to Close Ingress XDP BPF Program")
+			}
+		}
+		errOut := fmt.Errorf("%s interface name not found in the host, Stop called ", ifaceName)


Can you move this code block into member function and call both the places.

Signed-off-by: Jay Sheth <[email protected]>

sanfern

LGTM

charleskbliu0

Looks good

fix for clearing up NF configuration stored withing the interface and…

a0603d7

… the l3af-config-store Signed-off-by: Jay Sheth <[email protected]>

jaysheth2 marked this pull request as ready for review April 21, 2024 22:43

jaysheth2 requested review from sanfern, charleskbliu0, jniesz, dthaler, ajayR006, dalalkaran and pmoroney as code owners April 21, 2024 22:43

sanfern requested changes Apr 22, 2024

View reviewed changes

charleskbliu0 previously approved these changes Apr 22, 2024

View reviewed changes

pmoroney reviewed Apr 23, 2024

View reviewed changes

Addressing reviewers' comments + Adding cleanup for the deleted inter…

81d271c

…face Signed-off-by: Jay Sheth <[email protected]>

jaysheth2 dismissed charleskbliu0’s stale review via 81d271c April 24, 2024 00:41

fixing test cases

289b3dc

Signed-off-by: Jay Sheth <[email protected]>

jaysheth2 linked an issue Apr 24, 2024 that may be closed by this pull request

Delete L3AFD - related information once a network interface is deleted on-the-fly #375

Closed

pmoroney previously approved these changes Apr 24, 2024

View reviewed changes

reducing duplicate code

cda6cd0

Signed-off-by: Jay Sheth <[email protected]>

jaysheth2 dismissed pmoroney’s stale review via cda6cd0 April 25, 2024 01:22

jaysheth2 added 2 commits April 25, 2024 01:24

minor fixes

4833546

Signed-off-by: Jay Sheth <[email protected]>

minor fixes

10ead3f

Signed-off-by: Jay Sheth <[email protected]>

sanfern requested changes Apr 25, 2024

View reviewed changes

jaysheth2 added 2 commits April 25, 2024 17:27

optimizing kf/bpf.go for loop

f400660

Signed-off-by: Jay Sheth <[email protected]>

optimizing the code

15ab523

Signed-off-by: Jay Sheth <[email protected]>

sanfern requested changes Apr 26, 2024

View reviewed changes

jaysheth2 added 2 commits April 26, 2024 17:21

Addressing reviewer's comments

99bf5aa

Signed-off-by: Jay Sheth <[email protected]>

Addressing reviewer's comments

2f5ab2f

Signed-off-by: Jay Sheth <[email protected]>

sanfern approved these changes Apr 26, 2024

View reviewed changes

charleskbliu0 approved these changes Apr 26, 2024

View reviewed changes

sanfern merged commit 57b9b25 into main Apr 27, 2024
8 checks passed

sanfern added the bug Something isn't working label Apr 27, 2024

jaysheth2 deleted the jay-hv-del-fix branch June 12, 2024 06:16

sanfern changed the title ~~Fix for clearing up NF config stored withing the interface and the l3af-config-store file~~ Update eBPF config cache and local l3af-config-store file for a deleted interface Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update eBPF config cache and local l3af-config-store file for a deleted interface #374

Update eBPF config cache and local l3af-config-store file for a deleted interface #374

jaysheth2 commented Apr 21, 2024

sanfern Apr 22, 2024

jaysheth2 Apr 25, 2024

charleskbliu0 left a comment

pmoroney Apr 23, 2024

jaysheth2 Apr 24, 2024

pmoroney left a comment

sanfern Apr 25, 2024

jaysheth2 Apr 25, 2024

sanfern Apr 26, 2024

sanfern Apr 26, 2024

sanfern Apr 26, 2024

sanfern Apr 26, 2024

sanfern left a comment

charleskbliu0 left a comment

Update eBPF config cache and local l3af-config-store file for a deleted interface #374

Update eBPF config cache and local l3af-config-store file for a deleted interface #374

Conversation

jaysheth2 commented Apr 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleskbliu0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmoroney left a 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

sanfern left a comment

Choose a reason for hiding this comment

charleskbliu0 left a comment

Choose a reason for hiding this comment