[Windows] Replace the private patch in the dependent hcsshim library #2931

wenyingd · 2021-10-25T11:21:01Z

Use OVS Port externalIDs to cache the externalIDS in containerd
runtime instead of HNSEndpoint
Create the OVS Interface with type "" if the host interface is
not created (in containerd case), and update the type as "internal"
after the host interface is created
Connect the OVS port to the host interface in function "reconcile" if
the OVS port's ofport number is -1, by updating the OVS Interface type
as "internal".

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

codecov-commenter · 2021-10-25T13:58:47Z

Codecov Report

Merging #2931 (5f2929b) into main (1917bb5) will decrease coverage by 1.35%.
The diff coverage is 23.52%.

@@            Coverage Diff             @@
##             main    #2931      +/-   ##
==========================================
- Coverage   59.62%   58.26%   -1.36%     
==========================================
  Files         292      292              
  Lines       24724    24742      +18     
==========================================
- Hits        14741    14417     -324     
- Misses       8364     8752     +388     
+ Partials     1619     1573      -46

Flag	Coverage Δ
kind-e2e-tests	`44.73% <20.58%> (-1.93%)`	⬇️
unit-tests	`40.13% <2.94%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/agent_linux.go	`3.67% <0.00%> (ø)`
...g/agent/cniserver/interface_configuration_linux.go	`16.15% <ø> (ø)`
pkg/agent/cniserver/pod_configuration_linux.go	`66.66% <0.00%> (-33.34%)`	⬇️
pkg/ovs/ovsconfig/ovs_client.go	`47.35% <20.00%> (-1.60%)`	⬇️
pkg/agent/cniserver/pod_configuration.go	`53.33% <33.33%> (-1.79%)`	⬇️
...gent/controller/noderoute/node_route_controller.go	`54.91% <100.00%> (ø)`
...g/agent/apiserver/handlers/featuregates/handler.go	`0.00% <0.00%> (-82.36%)`	⬇️
...kg/apiserver/registry/system/supportbundle/rest.go	`19.54% <0.00%> (-55.18%)`	⬇️
pkg/support/dump_others.go	`0.00% <0.00%> (-51.73%)`	⬇️
...ver/registry/controlplane/nodestatssummary/rest.go	`50.00% <0.00%> (-50.00%)`	⬇️
... and 23 more

wenyingd · 2021-10-29T06:30:50Z

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

wenyingd · 2021-10-29T13:25:35Z

/test-integration
/test-all
/test-windows-all
/test-ipv6-all
/test-ipv6-only-all
/test-windows-proxyall-e2e

jianjuns

@tnqn : do you still have concerns on the proposed approach.

jianjuns · 2021-11-15T18:57:03Z

pkg/agent/cniserver/pod_configuration.go

 	for _, containerConfig := range knownInterfaces {
+		// Find the OVS ports which are not connected to host interfaces, this is useful on Windows if the runtime is


, this -> . This

Could we add some comments somewhere (pod_configuration.go, or pod_configuration_windows.go) to briefly describe the process of interface creation on Windows with Docker and containerd? It can be very short, but at least for readers to understand the CNI call sequence and what happen at each call.

Sure, I would update.

pkg/agent/cniserver/pod_configuration.go

pkg/agent/cniserver/pod_configuration_windows.go

pkg/ovs/ovsconfig/ovs_client.go

tnqn · 2021-11-16T17:26:25Z

pkg/ovs/ovsconfig/ovs_client.go

+		return 0, NewTransactionError(err, temporary)
+	}
+
+	// Wait until OVS allocated the ofport to the Interface.


It seems the following code is duplicate with GetOFPort. Why don't we just call GetOFPort on caller side?

They are different. The wait condition in function GetOFPort is field "ofport" is not empty, but here we wait until the "ofport" field is not "-1".

That's still one line difference, could we share the code? For example, make GetOFPort take one argument waitUntilValid, set it true for the new scenario on client side.

tnqn · 2021-11-16T17:31:45Z

@tnqn : do you still have concerns on the proposed approach.

I'm fine with the approach. I have comments for the code itself.

wenyingd · 2021-11-22T09:30:44Z

/test-all

wenyingd · 2021-11-26T04:16:01Z

@tnqn Could you help review it again?

pkg/agent/cniserver/pod_configuration.go

tnqn · 2021-11-26T08:57:42Z

pkg/ovs/ovsconfig/ovs_client.go

+		return 0, NewTransactionError(err, temporary)
+	}
+
+	// Wait until OVS allocated the ofport to the Interface.


That's still one line difference, could we share the code? For example, make GetOFPort take one argument waitUntilValid, set it true for the new scenario on client side.

pkg/agent/cniserver/interface_configuration_windows.go

wenyingd · 2021-11-26T13:14:12Z

/test-all
/test-windows-all
/test-ipv6-all
/test-ipv6-only-all
/test-windows-proxyall-e2e

tnqn

LGTM overall, one comment left

pkg/ovs/ovsconfig/ovs_client.go

1. Use OVS Port externalIDs to cache the externalIDS in containerd runtime instead of HNSEndpoint 2. Create the OVS Interface with type "" if the host interface is not created (in containerd case), and update the type as "internal" after the host interface is created 3. Connect the OVS port to the host interface in function "reconcile" if the OVS port's ofport number is -1, by updating the OVS Interface type as "internal". Signed-off-by: wenyingd <wenyingd@vmware.com>

wenyingd · 2021-11-29T03:19:04Z

/test-integration
/test-all
/test-windows-all
/test-ipv6-all
/test-ipv6-only-all
/test-windows-proxyall-e2e

tnqn

LGTM

wenyingd · 2021-11-29T08:11:03Z

/test-e2e
/test-flexible-ipam-e2e
/test-ipv6-conformance
/test-ipv6-e2e
/test-windows-all

wenyingd · 2021-11-30T00:35:21Z

/test-windows-networkpolicy
/test-ipv6-conformance
/test-e2e
/test-windows-proxyall-e2e

wenyingd force-pushed the issue_2913 branch 3 times, most recently from e1e8e29 to 808dd94 Compare October 25, 2021 13:14

wenyingd force-pushed the issue_2913 branch 2 times, most recently from c4b73b9 to d27ffdb Compare October 29, 2021 06:12

wenyingd changed the title ~~[WIP] [Windows] Remove AddtionalParams in hcsshim.HNSEndpoint~~ [Windows] Remove AddtionalParams in hcsshim.HNSEndpoint Oct 29, 2021

wenyingd mentioned this pull request Oct 29, 2021

Stop replacing github.com/Microsoft/hcsshim with private fork #2913

Closed

wenyingd force-pushed the issue_2913 branch from d27ffdb to 3274476 Compare October 29, 2021 12:22

wenyingd changed the title ~~[Windows] Remove AddtionalParams in hcsshim.HNSEndpoint~~ [Windows] Replace the private patch in the dependent hcsshim library Oct 29, 2021

wenyingd force-pushed the issue_2913 branch from 3274476 to ba6b1f2 Compare November 1, 2021 05:06

wenyingd requested review from tnqn and jianjuns November 12, 2021 00:20

jianjuns reviewed Nov 15, 2021

View reviewed changes

tnqn reviewed Nov 16, 2021

View reviewed changes

wenyingd force-pushed the issue_2913 branch from ba6b1f2 to 401f259 Compare November 17, 2021 07:00

wenyingd requested review from tnqn and jianjuns November 25, 2021 08:31

tnqn reviewed Nov 26, 2021

View reviewed changes

wenyingd force-pushed the issue_2913 branch 2 times, most recently from b01ebbf to d79b11e Compare November 26, 2021 10:31

tnqn reviewed Nov 26, 2021

View reviewed changes

pkg/ovs/ovsconfig/ovs_client.go Outdated Show resolved Hide resolved

wenyingd force-pushed the issue_2913 branch from d79b11e to 5f2929b Compare November 29, 2021 02:38

tnqn approved these changes Nov 29, 2021

View reviewed changes

tnqn merged commit 9a52de9 into antrea-io:main Nov 30, 2021

wenyingd deleted the issue_2913 branch August 15, 2022 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Replace the private patch in the dependent hcsshim library #2931

[Windows] Replace the private patch in the dependent hcsshim library #2931

wenyingd commented Oct 25, 2021 •

edited

Loading

codecov-commenter commented Oct 25, 2021 •

edited

Loading

wenyingd commented Oct 29, 2021

wenyingd commented Oct 29, 2021

jianjuns left a comment

jianjuns Nov 15, 2021

jianjuns Nov 15, 2021

wenyingd Nov 17, 2021

tnqn Nov 16, 2021

wenyingd Nov 17, 2021

tnqn Nov 26, 2021

tnqn commented Nov 16, 2021

wenyingd commented Nov 22, 2021

wenyingd commented Nov 26, 2021

tnqn Nov 26, 2021

wenyingd commented Nov 26, 2021

tnqn left a comment

wenyingd commented Nov 29, 2021

tnqn left a comment

wenyingd commented Nov 29, 2021

wenyingd commented Nov 30, 2021

		for _, containerConfig := range knownInterfaces {
		// Find the OVS ports which are not connected to host interfaces, this is useful on Windows if the runtime is

[Windows] Replace the private patch in the dependent hcsshim library #2931

[Windows] Replace the private patch in the dependent hcsshim library #2931

Conversation

wenyingd commented Oct 25, 2021 • edited Loading

codecov-commenter commented Oct 25, 2021 • edited Loading

Codecov Report

wenyingd commented Oct 29, 2021

wenyingd commented Oct 29, 2021

jianjuns left a comment

Choose a reason for hiding this comment

jianjuns Nov 15, 2021

Choose a reason for hiding this comment

jianjuns Nov 15, 2021

Choose a reason for hiding this comment

wenyingd Nov 17, 2021

Choose a reason for hiding this comment

tnqn Nov 16, 2021

Choose a reason for hiding this comment

wenyingd Nov 17, 2021

Choose a reason for hiding this comment

tnqn Nov 26, 2021

Choose a reason for hiding this comment

tnqn commented Nov 16, 2021

wenyingd commented Nov 22, 2021

wenyingd commented Nov 26, 2021

tnqn Nov 26, 2021

Choose a reason for hiding this comment

wenyingd commented Nov 26, 2021

tnqn left a comment

Choose a reason for hiding this comment

wenyingd commented Nov 29, 2021

tnqn left a comment

Choose a reason for hiding this comment

wenyingd commented Nov 29, 2021

wenyingd commented Nov 30, 2021

wenyingd commented Oct 25, 2021 •

edited

Loading

codecov-commenter commented Oct 25, 2021 •

edited

Loading