feat: initial implementation of evpn gw cni #14

mardim91 · 2024-02-22T12:32:12Z

Initial implementation of EVPN GW CNI. Currently only Intel Mt.Evans IPUs are supported by this CNI. Target is to support more IPUs/DPUs in the future. It has an integration with the EVPN GW API .

Dockerfile

Signed-off-by: Dimitrios Markou <[email protected]>

codecov · 2024-02-22T13:37:39Z

Codecov Report

Attention: Patch coverage is 0% with 1103 lines in your changes are missing coverage. Please review.

Project coverage is 0.00%. Comparing base (7020632) to head (12625ed).

❗ Current head 12625ed differs from pull request most recent head 5c47616. Consider uploading reports for the commit 5c47616 to get more accurate results

Files	Patch %	Lines
pkg/utils/utils.go	0.00%	277 Missing ⚠️
cmd/main.go	0.00%	204 Missing ⚠️
pkg/sriov/sriov.go	0.00%	153 Missing ⚠️
pkg/utils/packet.go	0.00%	153 Missing ⚠️
pkg/config/config.go	0.00%	130 Missing ⚠️
pkg/xpu/xpu.go	0.00%	113 Missing ⚠️
pkg/utils/pci_allocator.go	0.00%	45 Missing ⚠️
pkg/utils/netlink_manager.go	0.00%	22 Missing ⚠️
pkg/types/types.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           main     #14     +/-   ##
======================================
  Coverage      0   0.00%             
======================================
  Files         0       9      +9     
  Lines         0    1104   +1104     
======================================
- Misses        0    1104   +1104

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

glimchb · 2024-02-22T14:10:18Z

Dockerfile


-FROM docker.io/library/golang:1.21.6-alpine as builder


please do not remove docker.io

Added back in the line below.

glimchb · 2024-02-22T14:10:38Z

pkg/utils/netlink_manager.go

+// Copyright (C) 2023 Nordix Foundation.
+
+package utils
+


duplication with https://github.com/opiproject/opi-evpn-bridge/blob/main/pkg/utils/netlink.go

@glimchb in your netlink wrapper library that you want me to use each function there takes as parameter a context.

In this case here should I use an empty context right ? Like context.Background() ? Is this enough ?

yes, context.Background() is perfectly valid
or context.WithTimeout(context.Background(), 3) if you care about timeouts of the operation...

@glimchb I think is not so trivial to just import that library and use it. The reason is that this library expects that there is some otel server that runs somewhere. If not then I get errors back related to the tracer.start(ctx) function
https://github.com/opiproject/opi-evpn-bridge/blob/main/pkg/utils/netlink.go#L50

We need to make the tracer.start code section optional in order to skip it if there is no otel service

WDYT ?

errors should not be fatal, service just trying to send traces to otel collector should not fail...

I also suggest to spin a jaeger like I do in all other OPI repos, example: https://github.com/opiproject/opi-evpn-bridge/blob/5440fb9f85ed80d74658412a0cf3ff352b8d29dd/docker-compose.yml#L221-L235

@glimchb Even if that doesn't cause a fatal fail due to the fact that it waits until it timeouts (because there is no otel collector) make the calls extremely long and delayed.

Also a CNI is just a binary that you throw in a K8s system and just works. Is highly unlikely to have CNI depend on an OTEL collector like jaeger. In most of the systems there is no Jaegger.

I think the implementation that we have today in the netlink wrapper is not good enough to just import it from a CNI. I believe we should make some changes to at least make the trace part optional.

WDYT ?

Hello @glimchb can you please respond to my comment above ?

you can add if n.tracer != nil in https://github.com/opiproject/opi-evpn-bridge/blob/main/pkg/utils/netlink.go#L50

done

opiproject/opi-evpn-bridge#380

glimchb · 2024-02-22T14:11:51Z

pkg/xpu/xpu.go

+// SPDX-License-Identifier: Apache-2.0
+// Copyright (C) 2023 Nordix Foundation.
+// Copyright (c) 2024 Ericsson AB.
+


this code should just import and use https://github.com/opiproject/godpu library

library imported in line 15 below

glimchb · 2024-02-22T14:12:22Z

README.md


-[![Linters](https://github.com/opiproject/opi-gateway-evpn-cni/actions/workflows/linters.yml/badge.svg)](https://github.com/opiproject/opi-gateway-evpn-cni/actions/workflows/linters.yml)


why badges removed ?

Badges included again in latest.

glimchb · 2024-02-22T14:13:13Z

README.md


-## I Want To Contribute


this section must stay

Now in line 205 below

glimchb

code dup
ci failed
no tests
no godpu usage
readme removed badges
readme : i want to contribute section must stay

mardim91 · 2024-02-22T14:14:46Z

@glimchb Thank you for the review.

I will take a look

sandersms · 2024-03-28T14:57:07Z

@mardim91 Any update on the resolution of the comments?

mardim91 · 2024-03-28T15:00:04Z

@mardim91 Any update on the resolution of the comments?

Hello @sandersms,

No I haven't found the time to work on that yet. I will do it beginning of Q2. I was working on this PR: opiproject/opi-evpn-bridge#377
please take a look when you find some time.

Signed-off-by: Dimitrios Markou <[email protected]>

Dockerfile


-FROM docker.io/library/golang:1.21.6-alpine as builder
+FROM docker.io/library/golang:1.20-alpine3.19 as builder


mardim91 · 2024-05-03T12:11:34Z

Hello @sandersms I have resolved all the above comments.

Signed-off-by: Dimitrios Markou <[email protected]>

mardim91 · 2024-05-13T17:15:09Z

I have converted this to draft as the dependent PR in opi-evpn-bridge is still unmerged

opiproject/opi-evpn-bridge#380

Signed-off-by: Dimitrios Markou <[email protected]>

mardim91 · 2024-05-14T13:49:58Z

I have converted this to draft as the dependent PR in opi-evpn-bridge is still unmerged

opiproject/opi-evpn-bridge#380

The dependent PRs 0, 1 has been merged so this PR is ready

sandersms

Changes and updates from comments look to be resolved.

mardim91 · 2024-05-17T11:45:32Z

Hello @glimchb,

I have resolved all the comments that you have left. I will propose that maybe we can merge this one as the first PR to the project and then if we need to fix or add anything more we can create new PRs on top.

This PR is a working one and is quite some time open so I am thinking that we can merge it as the comments are resolved and all the dependent PRs in the OPI-EVPN-BR project are merged.

rechecking changes

sandersms · 2024-05-21T16:42:35Z

code dup

ci failed

no tests

no godpu usage

readme removed badges

readme : i want to contribute section must stay

code dup fixed
ci checks completed
godpu inclusion resolved
readme updated to include badges and contribution section

Tests for the feature are missing and should be included for validation. Issue #16 opened to cover the missing aspect that needs to be added.

sandersms

Re-review of changes look good.

Tests still missing. Added an issue for the functionality to be addressed in subsequent PR

Reported items are fixed and one issue opened for the test routines. No response on approval and needs to move forward.

mardim91 requested a review from a team as a code owner February 22, 2024 12:32

github-advanced-security bot found potential problems Feb 22, 2024

View reviewed changes

Dockerfile Fixed Show fixed Hide fixed

feat: initial implementation of evpn gw cni

12625ed

Signed-off-by: Dimitrios Markou <[email protected]>

mardim91 force-pushed the evpn-gw-cni branch from d09a7fc to 12625ed Compare February 22, 2024 13:36

glimchb reviewed Feb 22, 2024

View reviewed changes

glimchb previously requested changes Feb 22, 2024

View reviewed changes

fix: use godpu library for grpc calls

bff971a

Signed-off-by: Dimitrios Markou <[email protected]>

mardim91 mentioned this pull request Apr 29, 2024

fix(evpn-bridge): make tracer optional opiproject/opi-evpn-bridge#380

Merged

fix: save all the interface names

5c47616

Signed-off-by: Dimitrios Markou <[email protected]>

github-advanced-security bot found potential problems May 3, 2024

View reviewed changes

Dockerfile

FROM docker.io/library/golang:1.21.6-alpine as builder

FROM docker.io/library/golang:1.20-alpine3.19 as builder

Check warning

Code scanning / Scorecard

Pinned-Dependencies Medium

score is 0: containerImage not pinned by hash
Click Remediation section below to solve this issue

fix: initialize netlink wrapper in packet lib

b00f85e

Signed-off-by: Dimitrios Markou <[email protected]>

mardim91 marked this pull request as draft May 13, 2024 17:13

mardim91 mentioned this pull request May 14, 2024

add extra netlink funcs opiproject/opi-evpn-bridge#383

Merged

fix: bump opi-evpn-bridge version

8331878

Signed-off-by: Dimitrios Markou <[email protected]>

mardim91 marked this pull request as ready for review May 14, 2024 13:47

sandersms previously approved these changes May 14, 2024

View reviewed changes

sandersms requested review from glimchb and sandersms May 21, 2024 16:15

sandersms mentioned this pull request May 21, 2024

Tests should be included for the gateway-evpn-cni code #15

Closed

sandersms approved these changes May 21, 2024

View reviewed changes

sandersms mentioned this pull request May 21, 2024

No Tests included for gateway-evpn-cni code #16

Open

sandersms requested review from glimchb and removed request for glimchb May 28, 2024 16:05

sandersms merged commit a2207f8 into opiproject:main Jun 6, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial implementation of evpn gw cni #14

feat: initial implementation of evpn gw cni #14

mardim91 commented Feb 22, 2024

codecov bot commented Feb 22, 2024 •

edited

Loading

glimchb Feb 22, 2024

mardim91 May 3, 2024

sandersms May 21, 2024

glimchb Feb 22, 2024

mardim91 Feb 27, 2024

glimchb Feb 27, 2024 •

edited

Loading

mardim91 Feb 27, 2024 •

edited

Loading

glimchb Feb 27, 2024 •

edited

Loading

mardim91 Feb 28, 2024

mardim91 Mar 5, 2024

glimchb Mar 7, 2024 •

edited

Loading

mardim91 May 3, 2024

glimchb Feb 22, 2024

mardim91 May 3, 2024

sandersms May 21, 2024

glimchb Feb 22, 2024

mardim91 May 3, 2024

sandersms May 21, 2024

glimchb Feb 22, 2024

mardim91 May 3, 2024

sandersms May 21, 2024

glimchb left a comment

mardim91 commented Feb 22, 2024

sandersms commented Mar 28, 2024

mardim91 commented Mar 28, 2024 •

edited

Loading

mardim91 commented May 3, 2024

mardim91 commented May 13, 2024

mardim91 commented May 14, 2024

sandersms left a comment

mardim91 commented May 17, 2024

sandersms commented May 21, 2024 •

edited

Loading

sandersms left a comment


		[![Linters](https://github.com/opiproject/opi-gateway-evpn-cni/actions/workflows/linters.yml/badge.svg)](https://github.com/opiproject/opi-gateway-evpn-cni/actions/workflows/linters.yml)


		FROM docker.io/library/golang:1.21.6-alpine as builder
		FROM docker.io/library/golang:1.20-alpine3.19 as builder

feat: initial implementation of evpn gw cni #14

feat: initial implementation of evpn gw cni #14

Conversation

mardim91 commented Feb 22, 2024

codecov bot commented Feb 22, 2024 • edited Loading

Codecov Report

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

glimchb Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

mardim91 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

glimchb Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glimchb Mar 7, 2024 • 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

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

glimchb left a comment

Choose a reason for hiding this comment

mardim91 commented Feb 22, 2024

sandersms commented Mar 28, 2024

mardim91 commented Mar 28, 2024 • edited Loading

mardim91 commented May 3, 2024

mardim91 commented May 13, 2024

mardim91 commented May 14, 2024

sandersms left a comment

Choose a reason for hiding this comment

mardim91 commented May 17, 2024

sandersms commented May 21, 2024 • edited Loading

sandersms left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 22, 2024 •

edited

Loading

glimchb Feb 27, 2024 •

edited

Loading

mardim91 Feb 27, 2024 •

edited

Loading

glimchb Feb 27, 2024 •

edited

Loading

glimchb Mar 7, 2024 •

edited

Loading

mardim91 commented Mar 28, 2024 •

edited

Loading

sandersms commented May 21, 2024 •

edited

Loading