From 08ec2d8b8aebb4eed5542dcabee0711c59d295a9 Mon Sep 17 00:00:00 2001 From: Yerlan Baiturinov Date: Fri, 17 Jan 2025 13:15:05 +0000 Subject: [PATCH 1/9] fix: ensure jq is installed and add coverage generation in GitHub Actions Signed-off-by: Yerlan Baiturinov --- .github/workflows/test.yaml | 7 +++++++ test/image/Dockerfile | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 4aa930117b..3d29fde1fb 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -7,6 +7,7 @@ on: pull_request: branches: [main] workflow_dispatch: + permissions: actions: read contents: read @@ -15,6 +16,7 @@ permissions: pull-requests: write security-events: write issues: write + jobs: test-image: runs-on: ubuntu-latest @@ -32,6 +34,11 @@ jobs: PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number }} run: | make test-image IMAGE_NAMESPACE=${{ github.repository }} PLATFORM=linux/amd64 + + - name: Generate coverage + run: | + make coverage + - name: Upload Artifacts uses: actions/upload-artifact@v4 with: diff --git a/test/image/Dockerfile b/test/image/Dockerfile index 8c681a3fee..9d5dd44627 100644 --- a/test/image/Dockerfile +++ b/test/image/Dockerfile @@ -4,7 +4,7 @@ FROM mcr.microsoft.com/oss/go/microsoft/golang@sha256:88225e171f29fe5f1f6ffca8eb ENV CGO_ENABLED=0 COPY . /go/src/github.com/microsoft/retina WORKDIR /go/src/github.com/microsoft/retina -RUN tdnf install -y clang16 lld16 bpftool libbpf-devel make git +RUN tdnf install -y clang16 lld16 bpftool libbpf-devel make git jq RUN go generate /go/src/github.com/microsoft/retina/pkg/plugin/... # RUN go mod edit -module retina # RUN make all generate From 0fb0297a212c5464f9e9091d0d9691fb8adce1d7 Mon Sep 17 00:00:00 2001 From: Yerlan Baiturinov Date: Fri, 17 Jan 2025 16:20:44 +0000 Subject: [PATCH 2/9] fix: debug the GitHub actions Signed-off-by: Yerlan Baiturinov --- .github/workflows/test.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 3d29fde1fb..b2337bc6c8 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -37,6 +37,7 @@ jobs: - name: Generate coverage run: | + pwd && ls -la make coverage - name: Upload Artifacts From 280a6cba4e217316ffb5a7d393d6a39521460a3e Mon Sep 17 00:00:00 2001 From: Yerlan Baiturinov Date: Fri, 17 Jan 2025 16:48:33 +0000 Subject: [PATCH 3/9] fix: Turn on --output flag on Docker build for test actions Signed-off-by: Yerlan Baiturinov --- .github/workflows/test.yaml | 7 +------ Makefile | 2 ++ 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index b2337bc6c8..e751018cfc 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -35,13 +35,8 @@ jobs: run: | make test-image IMAGE_NAMESPACE=${{ github.repository }} PLATFORM=linux/amd64 - - name: Generate coverage - run: | - pwd && ls -la - make coverage - - name: Upload Artifacts uses: actions/upload-artifact@v4 with: name: coverage-files - path: ./coverage* + path: ./output/coverage* diff --git a/Makefile b/Makefile index 14db13ab96..190c04f098 100644 --- a/Makefile +++ b/Makefile @@ -241,6 +241,7 @@ container-docker: buildx # util target to build container images using docker bu image_metadata_filename="image-metadata-$$image_name-$(TAG).json"; \ touch $$image_metadata_filename; \ echo "Building $$image_name for $$os/$$arch "; \ + mkdir -p $(OUTPUT_DIR); \ docker buildx build \ --platform $(PLATFORM) \ --metadata-file=$$image_metadata_filename \ @@ -253,6 +254,7 @@ container-docker: buildx # util target to build container images using docker bu --build-arg VERSION=$(VERSION) $(EXTRA_BUILD_ARGS) \ --target=$(TARGET) \ -t $(IMAGE_REGISTRY)/$(IMAGE):$(TAG) \ + --output type=local,dest=$(OUTPUT_DIR) \ $(BUILDX_ACTION) \ $(CONTEXT_DIR) From 50c2a15df3fc292361c5c29df52ea579d1420655 Mon Sep 17 00:00:00 2001 From: Yerlan Baiturinov Date: Tue, 21 Jan 2025 10:46:10 +0000 Subject: [PATCH 4/9] fix: Clean up interfaceLockMap entries on endpoint deletion Signed-off-by: Yerlan Baiturinov --- pkg/plugin/packetparser/packetparser_linux.go | 21 +++++++++++++++++-- .../packetparser/packetparser_linux_test.go | 18 +++++++++++++--- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/pkg/plugin/packetparser/packetparser_linux.go b/pkg/plugin/packetparser/packetparser_linux.go index fb54c8bb5e..94bd8d3fa8 100644 --- a/pkg/plugin/packetparser/packetparser_linux.go +++ b/pkg/plugin/packetparser/packetparser_linux.go @@ -389,17 +389,34 @@ func (p *packetParser) endpointWatcherCallbackFn(obj interface{}) { switch event.Type { case endpoint.EndpointCreated: + // Create mutex only when needed + lockMapVal, _ := p.interfaceLockMap.LoadOrStore(ifaceKey, &sync.Mutex{}) + mu := lockMapVal.(*sync.Mutex) + mu.Lock() + defer mu.Unlock() + p.l.Debug("Endpoint created", zap.String("name", iface.Name)) p.createQdiscAndAttach(iface, Veth) case endpoint.EndpointDeleted: + // Get the mutex only if it exists + lockMapVal, exists := p.interfaceLockMap.Load(ifaceKey) + if !exists { + return + } + mu := lockMapVal.(*sync.Mutex) + mu.Lock() + defer mu.Unlock() + p.l.Debug("Endpoint deleted", zap.String("name", iface.Name)) - // Clean. + // Clean tcMap. if value, ok := p.tcMap.Load(ifaceKey); ok { v := value.(*tcValue) p.clean(v.tc, v.qdisc) - // Delete from map. p.tcMap.Delete(ifaceKey) } + + // Clean interfaceLockMap. + p.interfaceLockMap.Delete(ifaceKey) default: // Unknown. p.l.Debug("Unknown event", zap.String("type", event.Type.String())) diff --git a/pkg/plugin/packetparser/packetparser_linux_test.go b/pkg/plugin/packetparser/packetparser_linux_test.go index aa27cfa356..fd76a6df47 100644 --- a/pkg/plugin/packetparser/packetparser_linux_test.go +++ b/pkg/plugin/packetparser/packetparser_linux_test.go @@ -162,18 +162,24 @@ func TestEndpointWatcherCallbackFn_EndpointDeleted(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() + // Initialize packetParser with both maps. p := &packetParser{ cfg: cfgPodLevelEnabled, l: log.Logger().Named("test"), interfaceLockMap: &sync.Map{}, + tcMap: &sync.Map{}, } - p.tcMap = &sync.Map{} + + // Create test interface attributes. linkAttr := netlink.LinkAttrs{ Name: "test", HardwareAddr: []byte("test"), NetNsID: 1, } key := ifaceToKey(linkAttr) + + // Pre-populate both maps to simulate existing interface + p.interfaceLockMap.Store(key, &sync.Mutex{}) p.tcMap.Store(key, &tcValue{nil, &tc.Object{}}) // Create EndpointDeleted event. @@ -182,10 +188,16 @@ func TestEndpointWatcherCallbackFn_EndpointDeleted(t *testing.T) { Obj: linkAttr, } + // Execute the callback. p.endpointWatcherCallbackFn(e) - _, ok := p.tcMap.Load(key) - assert.False(t, ok) + // Verify both maps are cleaned up. + _, tcMapExists := p.tcMap.Load(key) + _, lockMapExists := p.interfaceLockMap.Load(key) + + // Assert both maps are cleaned up + assert.False(t, tcMapExists, "tcMap entry should be deleted") + assert.False(t, lockMapExists, "interfaceLockMap entry should be deleted") } func TestCreateQdiscAndAttach(t *testing.T) { From 6b11dd50b113b04c710535a8d3aa65a242fe4903 Mon Sep 17 00:00:00 2001 From: Yerlan Baiturinov Date: Tue, 21 Jan 2025 10:58:31 +0000 Subject: [PATCH 5/9] fix: Remove obsolete Mutex creation in EndpointWatcher in PacketParser pacage Signed-off-by: Yerlan Baiturinov --- pkg/plugin/packetparser/packetparser_linux.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pkg/plugin/packetparser/packetparser_linux.go b/pkg/plugin/packetparser/packetparser_linux.go index 94bd8d3fa8..0c473c120d 100644 --- a/pkg/plugin/packetparser/packetparser_linux.go +++ b/pkg/plugin/packetparser/packetparser_linux.go @@ -380,12 +380,7 @@ func (p *packetParser) endpointWatcherCallbackFn(obj interface{}) { } iface := event.Obj.(netlink.LinkAttrs) - ifaceKey := ifaceToKey(iface) - lockMapVal, _ := p.interfaceLockMap.LoadOrStore(ifaceKey, &sync.Mutex{}) - mu := lockMapVal.(*sync.Mutex) - mu.Lock() - defer mu.Unlock() switch event.Type { case endpoint.EndpointCreated: From 1a1187a43fed3429fc99b043af23b0e37ee0827d Mon Sep 17 00:00:00 2001 From: Yerlan Baiturinov Date: Tue, 21 Jan 2025 11:08:22 +0000 Subject: [PATCH 6/9] fix: PacketParser Test linter processed Signed-off-by: Yerlan Baiturinov --- pkg/plugin/packetparser/packetparser_linux_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/plugin/packetparser/packetparser_linux_test.go b/pkg/plugin/packetparser/packetparser_linux_test.go index fd76a6df47..50903852a1 100644 --- a/pkg/plugin/packetparser/packetparser_linux_test.go +++ b/pkg/plugin/packetparser/packetparser_linux_test.go @@ -167,7 +167,7 @@ func TestEndpointWatcherCallbackFn_EndpointDeleted(t *testing.T) { cfg: cfgPodLevelEnabled, l: log.Logger().Named("test"), interfaceLockMap: &sync.Map{}, - tcMap: &sync.Map{}, + tcMap: &sync.Map{}, } // Create test interface attributes. From 3e78e6bd4900457714fe51e0dd095c95f7831460 Mon Sep 17 00:00:00 2001 From: Yerlan Baiturinov Date: Tue, 21 Jan 2025 21:23:30 +0000 Subject: [PATCH 7/9] fix: Get rid of interfaceLockMap mutex from Packet Parser Signed-off-by: Yerlan Baiturinov --- pkg/plugin/packetparser/packetparser_linux.go | 19 ------------------- .../packetparser/packetparser_linux_test.go | 13 +++---------- pkg/plugin/packetparser/types_linux.go | 2 -- 3 files changed, 3 insertions(+), 31 deletions(-) diff --git a/pkg/plugin/packetparser/packetparser_linux.go b/pkg/plugin/packetparser/packetparser_linux.go index 0c473c120d..2f9f3012d1 100644 --- a/pkg/plugin/packetparser/packetparser_linux.go +++ b/pkg/plugin/packetparser/packetparser_linux.go @@ -225,7 +225,6 @@ func (p *packetParser) Init() error { } p.tcMap = &sync.Map{} - p.interfaceLockMap = &sync.Map{} return nil } @@ -384,24 +383,9 @@ func (p *packetParser) endpointWatcherCallbackFn(obj interface{}) { switch event.Type { case endpoint.EndpointCreated: - // Create mutex only when needed - lockMapVal, _ := p.interfaceLockMap.LoadOrStore(ifaceKey, &sync.Mutex{}) - mu := lockMapVal.(*sync.Mutex) - mu.Lock() - defer mu.Unlock() - p.l.Debug("Endpoint created", zap.String("name", iface.Name)) p.createQdiscAndAttach(iface, Veth) case endpoint.EndpointDeleted: - // Get the mutex only if it exists - lockMapVal, exists := p.interfaceLockMap.Load(ifaceKey) - if !exists { - return - } - mu := lockMapVal.(*sync.Mutex) - mu.Lock() - defer mu.Unlock() - p.l.Debug("Endpoint deleted", zap.String("name", iface.Name)) // Clean tcMap. if value, ok := p.tcMap.Load(ifaceKey); ok { @@ -409,9 +393,6 @@ func (p *packetParser) endpointWatcherCallbackFn(obj interface{}) { p.clean(v.tc, v.qdisc) p.tcMap.Delete(ifaceKey) } - - // Clean interfaceLockMap. - p.interfaceLockMap.Delete(ifaceKey) default: // Unknown. p.l.Debug("Unknown event", zap.String("type", event.Type.String())) diff --git a/pkg/plugin/packetparser/packetparser_linux_test.go b/pkg/plugin/packetparser/packetparser_linux_test.go index 50903852a1..836a0cb8b6 100644 --- a/pkg/plugin/packetparser/packetparser_linux_test.go +++ b/pkg/plugin/packetparser/packetparser_linux_test.go @@ -166,7 +166,6 @@ func TestEndpointWatcherCallbackFn_EndpointDeleted(t *testing.T) { p := &packetParser{ cfg: cfgPodLevelEnabled, l: log.Logger().Named("test"), - interfaceLockMap: &sync.Map{}, tcMap: &sync.Map{}, } @@ -178,8 +177,7 @@ func TestEndpointWatcherCallbackFn_EndpointDeleted(t *testing.T) { } key := ifaceToKey(linkAttr) - // Pre-populate both maps to simulate existing interface - p.interfaceLockMap.Store(key, &sync.Mutex{}) + // Pre-populate the map to simulate existing interface p.tcMap.Store(key, &tcValue{nil, &tc.Object{}}) // Create EndpointDeleted event. @@ -192,12 +190,10 @@ func TestEndpointWatcherCallbackFn_EndpointDeleted(t *testing.T) { p.endpointWatcherCallbackFn(e) // Verify both maps are cleaned up. - _, tcMapExists := p.tcMap.Load(key) - _, lockMapExists := p.interfaceLockMap.Load(key) + _, ok := p.tcMap.Load(key) // Assert both maps are cleaned up - assert.False(t, tcMapExists, "tcMap entry should be deleted") - assert.False(t, lockMapExists, "interfaceLockMap entry should be deleted") + assert.False(t, ok, "tcMap entry should be deleted") } func TestCreateQdiscAndAttach(t *testing.T) { @@ -239,7 +235,6 @@ func TestCreateQdiscAndAttach(t *testing.T) { cfg: cfgPodLevelEnabled, l: log.Logger().Named("test"), objs: pObj, - interfaceLockMap: &sync.Map{}, endpointIngressInfo: &ebpf.ProgramInfo{ Name: "ingress", }, @@ -429,7 +424,6 @@ func TestStartWithDataAggregationLevelLow(t *testing.T) { objs: pObj, reader: mockReader, recordsChannel: make(chan perf.Record, buffer), - interfaceLockMap: &sync.Map{}, endpointIngressInfo: &ebpf.ProgramInfo{ Name: "ingress", }, @@ -508,7 +502,6 @@ func TestStartWithDataAggregationLevelHigh(t *testing.T) { objs: pObj, reader: mockReader, recordsChannel: make(chan perf.Record, buffer), - interfaceLockMap: &sync.Map{}, endpointIngressInfo: &ebpf.ProgramInfo{ Name: "ingress", }, diff --git a/pkg/plugin/packetparser/types_linux.go b/pkg/plugin/packetparser/types_linux.go index 4fc06f35c3..082a79d5f7 100644 --- a/pkg/plugin/packetparser/types_linux.go +++ b/pkg/plugin/packetparser/types_linux.go @@ -114,8 +114,6 @@ type packetParser struct { tcMap *sync.Map reader perfReader enricher enricher.EnricherInterface - // interfaceLockMap is a map of key to *sync.Mutex. - interfaceLockMap *sync.Map endpointIngressInfo *ebpf.ProgramInfo endpointEgressInfo *ebpf.ProgramInfo hostIngressInfo *ebpf.ProgramInfo From 31aa74c5007a4e105b9c4e6c9e391a4d8d988b1a Mon Sep 17 00:00:00 2001 From: Yerlan Baiturinov Date: Wed, 22 Jan 2025 14:16:10 +0000 Subject: [PATCH 8/9] fix: Revert getting rid of interfaceLockMap Signed-off-by: Yerlan Baiturinov --- pkg/plugin/packetparser/packetparser_linux.go | 31 ++++++++++++++----- .../packetparser/packetparser_linux_test.go | 13 ++++++-- pkg/plugin/packetparser/types_linux.go | 2 ++ 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/pkg/plugin/packetparser/packetparser_linux.go b/pkg/plugin/packetparser/packetparser_linux.go index 2f9f3012d1..28351938a9 100644 --- a/pkg/plugin/packetparser/packetparser_linux.go +++ b/pkg/plugin/packetparser/packetparser_linux.go @@ -225,6 +225,7 @@ func (p *packetParser) Init() error { } p.tcMap = &sync.Map{} + p.interfaceLockMap = &sync.Map{} return nil } @@ -372,7 +373,6 @@ func (p *packetParser) clean(rtnl nltc, qdisc *tc.Object) { } func (p *packetParser) endpointWatcherCallbackFn(obj interface{}) { - // Contract is that we will receive an endpoint event pointer. event := obj.(*endpoint.EndpointEvent) if event == nil { return @@ -383,19 +383,34 @@ func (p *packetParser) endpointWatcherCallbackFn(obj interface{}) { switch event.Type { case endpoint.EndpointCreated: - p.l.Debug("Endpoint created", zap.String("name", iface.Name)) - p.createQdiscAndAttach(iface, Veth) + // Get or create mutex atomically + lockMapVal, loaded := p.interfaceLockMap.LoadOrStore(ifaceKey, &sync.Mutex{}) + mu := lockMapVal.(*sync.Mutex) + mu.Lock() + defer mu.Unlock() + + // Only proceed with creation if this is a new interface + if !loaded { + p.l.Debug("Endpoint created", zap.String("name", iface.Name)) + p.createQdiscAndAttach(iface, Veth) + } + case endpoint.EndpointDeleted: - p.l.Debug("Endpoint deleted", zap.String("name", iface.Name)) - // Clean tcMap. + lockMapVal, ok := p.interfaceLockMap.Load(ifaceKey) + if !ok { + return + } + mu := lockMapVal.(*sync.Mutex) + mu.Lock() + defer mu.Unlock() + + // Clean up operations if value, ok := p.tcMap.Load(ifaceKey); ok { v := value.(*tcValue) p.clean(v.tc, v.qdisc) p.tcMap.Delete(ifaceKey) } - default: - // Unknown. - p.l.Debug("Unknown event", zap.String("type", event.Type.String())) + p.interfaceLockMap.Delete(ifaceKey) } } diff --git a/pkg/plugin/packetparser/packetparser_linux_test.go b/pkg/plugin/packetparser/packetparser_linux_test.go index 836a0cb8b6..50903852a1 100644 --- a/pkg/plugin/packetparser/packetparser_linux_test.go +++ b/pkg/plugin/packetparser/packetparser_linux_test.go @@ -166,6 +166,7 @@ func TestEndpointWatcherCallbackFn_EndpointDeleted(t *testing.T) { p := &packetParser{ cfg: cfgPodLevelEnabled, l: log.Logger().Named("test"), + interfaceLockMap: &sync.Map{}, tcMap: &sync.Map{}, } @@ -177,7 +178,8 @@ func TestEndpointWatcherCallbackFn_EndpointDeleted(t *testing.T) { } key := ifaceToKey(linkAttr) - // Pre-populate the map to simulate existing interface + // Pre-populate both maps to simulate existing interface + p.interfaceLockMap.Store(key, &sync.Mutex{}) p.tcMap.Store(key, &tcValue{nil, &tc.Object{}}) // Create EndpointDeleted event. @@ -190,10 +192,12 @@ func TestEndpointWatcherCallbackFn_EndpointDeleted(t *testing.T) { p.endpointWatcherCallbackFn(e) // Verify both maps are cleaned up. - _, ok := p.tcMap.Load(key) + _, tcMapExists := p.tcMap.Load(key) + _, lockMapExists := p.interfaceLockMap.Load(key) // Assert both maps are cleaned up - assert.False(t, ok, "tcMap entry should be deleted") + assert.False(t, tcMapExists, "tcMap entry should be deleted") + assert.False(t, lockMapExists, "interfaceLockMap entry should be deleted") } func TestCreateQdiscAndAttach(t *testing.T) { @@ -235,6 +239,7 @@ func TestCreateQdiscAndAttach(t *testing.T) { cfg: cfgPodLevelEnabled, l: log.Logger().Named("test"), objs: pObj, + interfaceLockMap: &sync.Map{}, endpointIngressInfo: &ebpf.ProgramInfo{ Name: "ingress", }, @@ -424,6 +429,7 @@ func TestStartWithDataAggregationLevelLow(t *testing.T) { objs: pObj, reader: mockReader, recordsChannel: make(chan perf.Record, buffer), + interfaceLockMap: &sync.Map{}, endpointIngressInfo: &ebpf.ProgramInfo{ Name: "ingress", }, @@ -502,6 +508,7 @@ func TestStartWithDataAggregationLevelHigh(t *testing.T) { objs: pObj, reader: mockReader, recordsChannel: make(chan perf.Record, buffer), + interfaceLockMap: &sync.Map{}, endpointIngressInfo: &ebpf.ProgramInfo{ Name: "ingress", }, diff --git a/pkg/plugin/packetparser/types_linux.go b/pkg/plugin/packetparser/types_linux.go index 082a79d5f7..4fc06f35c3 100644 --- a/pkg/plugin/packetparser/types_linux.go +++ b/pkg/plugin/packetparser/types_linux.go @@ -114,6 +114,8 @@ type packetParser struct { tcMap *sync.Map reader perfReader enricher enricher.EnricherInterface + // interfaceLockMap is a map of key to *sync.Mutex. + interfaceLockMap *sync.Map endpointIngressInfo *ebpf.ProgramInfo endpointEgressInfo *ebpf.ProgramInfo hostIngressInfo *ebpf.ProgramInfo From fdab1aa06aa807aee03030a9b079c80978f1d4b3 Mon Sep 17 00:00:00 2001 From: Yerlan Baiturinov Date: Wed, 22 Jan 2025 19:18:19 +0000 Subject: [PATCH 9/9] fix: Add default case into endpointWatcherCallbackFn Signed-off-by: Yerlan Baiturinov --- pkg/plugin/packetparser/packetparser_linux.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/plugin/packetparser/packetparser_linux.go b/pkg/plugin/packetparser/packetparser_linux.go index 28351938a9..d26e620fea 100644 --- a/pkg/plugin/packetparser/packetparser_linux.go +++ b/pkg/plugin/packetparser/packetparser_linux.go @@ -411,6 +411,9 @@ func (p *packetParser) endpointWatcherCallbackFn(obj interface{}) { p.tcMap.Delete(ifaceKey) } p.interfaceLockMap.Delete(ifaceKey) + default: + // Unknown. + p.l.Debug("Unknown event", zap.String("type", event.Type.String())) } }