Bump up cni and plugins libraries to v1.1.1 #4425

wenyingd · 2022-11-30T10:33:02Z

Bump up cni and plugins version
Removed the copied code from third_party

Signed-off-by: wenyingd wenyingd@vmware.com

lgtm-com · 2022-11-30T11:02:14Z

This pull request fixes 1 alert when merging 6cb662c into 5547e73 - view on LGTM.com

fixed alerts:

1 for Size computation for allocation may overflow

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

wenyingd · 2022-11-30T14:47:48Z

/test-all

codecov · 2022-11-30T14:56:23Z

Codecov Report

Merging #4425 (1cd598f) into main (d5dd02e) will decrease coverage by 0.17%.
The diff coverage is 100.00%.

❗ Current head 1cd598f differs from pull request most recent head 0ecc30f. Consider uploading reports for the commit 0ecc30f to get more accurate results

@@            Coverage Diff             @@
##             main    #4425      +/-   ##
==========================================
- Coverage   69.86%   69.70%   -0.17%     
==========================================
  Files         403      403              
  Lines       59778    58608    -1170     
==========================================
- Hits        41765    40851     -914     
+ Misses      15189    14964     -225     
+ Partials     2824     2793      -31

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`38.66% <50.00%> (+0.32%)`	⬆️
integration-tests	`34.39% <66.66%> (-0.05%)`	⬇️	Carriedforward from 0ecc30f
kind-e2e-tests	`47.32% <66.66%> (+0.39%)`	⬆️	Carriedforward from 0ecc30f
unit-tests	`59.87% <100.00%> (-0.04%)`	⬇️	Carriedforward from 0ecc30f

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/cniserver/ipam/ipam_delegator.go	`68.18% <ø> (ø)`
pkg/agent/cniserver/ipam/ipam_service.go	`92.47% <ø> (+8.60%)`	⬆️
pkg/agent/cniserver/pod_configuration_linux.go	`100.00% <ø> (ø)`
pkg/agent/cniserver/pod_configuration_windows.go	`75.00% <ø> (ø)`
pkg/agent/cniserver/server_linux.go	`100.00% <ø> (ø)`
pkg/agent/cniserver/server_windows.go	`85.18% <ø> (ø)`
pkg/agent/cniserver/sriov.go	`31.37% <ø> (ø)`
pkg/agent/secondarynetwork/ipam/ipam_delegator.go	`3.70% <ø> (ø)`
pkg/agent/secondarynetwork/podwatch/controller.go	`75.76% <ø> (ø)`
...g/agent/cniserver/interface_configuration_linux.go	`80.60% <100.00%> (+8.19%)`	⬆️
... and 47 more

luolanzone · 2022-12-02T08:46:08Z

@wenyingd , I suppose this can be closed?

wenyingd · 2022-12-02T08:53:06Z

@wenyingd , I suppose this can be closed?

No, @tnqn and I had a discussion on how to resolve the issue, and we would like to create two patches, both are working. The first one ( #4428 ) would copy some code from the latest dependent library containernetworking to support setting MAC when creating veth pair. And it is expected to be merged in main branch and back port to the previous releases. Then we do not need to bump up the dependent library version. And this one is the 2nd, which is existing only on main branch. This one is expected to be based on the first one, the content is to remove the copied code, and bump up the dependent library version.

This patch is with a lower priority, and can not be included in this release.

luolanzone · 2022-12-02T08:56:31Z

Got it, thanks for explanation.

tnqn · 2022-12-06T03:46:52Z

@wenyingd maybe you could update the PR title to "Bump up cni and plugins libraries to v1.1.1" to avoid confusion since the MAC change issue is already resolved by another PR and this is to reduce code redundancy

wenyingd · 2022-12-06T03:48:27Z

@wenyingd maybe you could update the PR title to "Bump up cni and plugins libraries to v1.1.1" to avoid confusion since the MAC change issue is already resolved by another PR and this is to reduce code redundancy

updated accordingly.

wenyingd · 2023-02-06T09:21:47Z

/test-all
/test-windows-all

luolanzone

LGTM overall, a few nits and questions.

luolanzone · 2023-02-07T02:10:49Z

pkg/agent/cniserver/interface_configuration_linux_test.go

@@ -300,52 +300,46 @@ func TestAdvertiseContainerAddr(t *testing.T) {
 			name:    "interface-not-found",
 			runInNS: true,
 			result: &current.Result{IPs: []*current.IPConfig{
-				{Version: "4", Interface: &interfaceID, Address: ipv4CIDR, Gateway: ipv4Gateway},
-				{Version: "6", Interface: &interfaceID, Address: ipv6CIDR, Gateway: ipv6Gateway},
+				{Interface: &interfaceID, Address: ipv4CIDR, Gateway: ipv4Gateway},


So the Version is no longer required in cni v1.0? I am wondering will this work properly with old plugins? I mean the cni plugins which is installed by build/image/scripts/install_cni

Yes, Version property is not existing in the Result type since 1.1. I have added function to convert the Result in CNI Response to the previous version format if antrea CNI configuration not using the latest version.

pkg/agent/cniserver/ipam/antrea_ipam_test.go

luolanzone · 2023-02-07T02:22:00Z

pkg/agent/cniserver/server.go

@@ -519,6 +520,8 @@ func (s *CNIServer) CmdAdd(ctx context.Context, request *cnipb.CniCmdRequest) (*
 		klog.Errorf("Failed to configure interfaces for container %s: %v", cniConfig.ContainerId, err)
 		return s.configInterfaceFailureResponse(err), nil
 	}
+	cniVersion := cniConfig.CNIVersion
+	cniResult, _ := result.Result.GetAsVersion(cniVersion)


Should we capture the error here?

I think it is not needed as we should always use a supported CNI version.

pkg/agent/secondarynetwork/ipam/testing/mock_ipam.go

tnqn

LGTM

tnqn · 2023-02-24T12:44:40Z

@wenyingd The PR needs a rebase as go.mod and go.sum has been updated and git can't handle it even it doesn't show conflict. The build would fail if the PR is merged as is.

1. Bump up github.com/containernetworking/cni version to v1.1.2 2. Bump up github.com/containernetworking/plugins v1.1.1 Signed-off-by: wenyingd <wenyingd@vmware.com>

wenyingd · 2023-02-24T14:23:10Z

@wenyingd The PR needs a rebase as go.mod and go.sum has been updated and git can't handle it even it doesn't show conflict. The build would fail if the PR is merged as is.

Updated.

wenyingd · 2023-02-24T14:41:10Z

/test-all
/test-windows-all
/test-ipv6-all
/test-ipv6-only-all

tnqn · 2023-02-27T02:41:11Z

/skip-e2e which failed on known flaky test

wenyingd force-pushed the gw_mac_modification branch from 6cb662c to 268cecc Compare November 30, 2022 14:37

vicky-liu added this to the Antrea v1.10 release milestone Dec 2, 2022

tnqn removed this from the Antrea v1.10 release milestone Dec 6, 2022

wenyingd changed the title ~~[WIP] Fix issue that antrea-gw0 MAC is changed~~ [WIP] Bump up cni and plugins libraries to v1.1.1 Dec 6, 2022

wenyingd force-pushed the gw_mac_modification branch 5 times, most recently from 218dec5 to 48d0800 Compare December 15, 2022 11:20

wenyingd changed the title ~~[WIP] Bump up cni and plugins libraries to v1.1.1~~ Bump up cni and plugins libraries to v1.1.1 Jan 9, 2023

wenyingd force-pushed the gw_mac_modification branch from 48d0800 to 1259688 Compare January 19, 2023 06:29

wenyingd force-pushed the gw_mac_modification branch from 1259688 to e6353fb Compare February 6, 2023 07:30

wenyingd requested review from luolanzone and tnqn February 6, 2023 10:28

luolanzone reviewed Feb 7, 2023

View reviewed changes

tnqn previously approved these changes Feb 24, 2023

View reviewed changes

Bump up containernetworking libraries version

0ecc30f

1. Bump up github.com/containernetworking/cni version to v1.1.2 2. Bump up github.com/containernetworking/plugins v1.1.1 Signed-off-by: wenyingd <wenyingd@vmware.com>

wenyingd dismissed tnqn’s stale review via 0ecc30f February 24, 2023 14:21

wenyingd force-pushed the gw_mac_modification branch from e6353fb to 0ecc30f Compare February 24, 2023 14:21

tnqn approved these changes Feb 27, 2023

View reviewed changes

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Feb 27, 2023

tnqn merged commit be83891 into antrea-io:main Feb 27, 2023

tnqn mentioned this pull request Mar 2, 2023

Bump up cni version to v1.0.1 #2912

Closed

wenyingd deleted the gw_mac_modification branch May 30, 2023 06:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump up cni and plugins libraries to v1.1.1 #4425

Bump up cni and plugins libraries to v1.1.1 #4425

wenyingd commented Nov 30, 2022 •

edited

Loading

lgtm-com bot commented Nov 30, 2022

wenyingd commented Nov 30, 2022

codecov bot commented Nov 30, 2022 •

edited

Loading

luolanzone commented Dec 2, 2022

wenyingd commented Dec 2, 2022 •

edited

Loading

luolanzone commented Dec 2, 2022

tnqn commented Dec 6, 2022

wenyingd commented Dec 6, 2022

wenyingd commented Feb 6, 2023

luolanzone left a comment

luolanzone Feb 7, 2023

wenyingd Feb 8, 2023

luolanzone Feb 7, 2023

wenyingd Feb 23, 2023

tnqn left a comment

tnqn commented Feb 24, 2023

wenyingd commented Feb 24, 2023

wenyingd commented Feb 24, 2023

tnqn commented Feb 27, 2023

Bump up cni and plugins libraries to v1.1.1 #4425

Bump up cni and plugins libraries to v1.1.1 #4425

Conversation

wenyingd commented Nov 30, 2022 • edited Loading

lgtm-com bot commented Nov 30, 2022

wenyingd commented Nov 30, 2022

codecov bot commented Nov 30, 2022 • edited Loading

Codecov Report

luolanzone commented Dec 2, 2022

wenyingd commented Dec 2, 2022 • edited Loading

luolanzone commented Dec 2, 2022

tnqn commented Dec 6, 2022

wenyingd commented Dec 6, 2022

wenyingd commented Feb 6, 2023

luolanzone left a comment

Choose a reason for hiding this comment

luolanzone Feb 7, 2023

Choose a reason for hiding this comment

wenyingd Feb 8, 2023

Choose a reason for hiding this comment

luolanzone Feb 7, 2023

Choose a reason for hiding this comment

wenyingd Feb 23, 2023

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Feb 24, 2023

wenyingd commented Feb 24, 2023

wenyingd commented Feb 24, 2023

tnqn commented Feb 27, 2023

wenyingd commented Nov 30, 2022 •

edited

Loading

codecov bot commented Nov 30, 2022 •

edited

Loading

wenyingd commented Dec 2, 2022 •

edited

Loading