Update ovs-pipeline.md #2725

hongliangl · 2021-09-06T14:30:50Z

Signed-off-by: Hongliang Liu [email protected]

docs/design/ovs-pipeline.md

jianjuns · 2021-09-08T22:47:43Z

docs/design/ovs-pipeline.md

+  for more information.
+* *IN_PORT action*: an action to output the packet on the port on which it was
+  received. This is the only standard way to output the packet to the input port.
+* *session affinity*: when accessing a Service, to make sure that connections from


How about: "a load balancer feature that always selects the same backend server for connections from a particular clients. For a K8s Service, session affinity can be enabled by setting..."?

I changed a little.

* *session affinity*: a load balancer feature that always selects the same backend Endpoint for connections from a particular client. For a K8s Service, session affinity can be enabled by setting `service.spec.sessionAffinity` to "ClientIP" (default is "None"). See [K8s Service](https://kubernetes.io/docs/concepts/services-networking/service/) for more information about session affinity.

Sure. Maybe change Endpoint to endpoint (I feel no reason to capitalize endpoint).

I changed Endpoint to Pod. I thought that Pod is more accurate.

docs/design/ovs-pipeline.md

codecov-commenter · 2021-09-15T04:03:18Z

Codecov Report

Merging #2725 (5c6668a) into main (e32664e) will decrease coverage by 20.34%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main    #2725       +/-   ##
===========================================
- Coverage   61.26%   40.92%   -20.35%     
===========================================
  Files         284      158      -126     
  Lines       23562    19539     -4023     
===========================================
- Hits        14436     7996     -6440     
- Misses       7564    10796     +3232     
+ Partials     1562      747      -815

Flag	Coverage Δ
kind-e2e-tests	`?`
unit-tests	`40.92% <ø> (+0.05%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/ovs/openflow/default.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/util/runtime/runtime.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/agent/cniserver/pod_configuration_linux.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/ovs/openflow/logs.go	`9.52% <0.00%> (-90.48%)`	⬇️
pkg/apis/controlplane/register.go	`0.00% <0.00%> (-90.00%)`	⬇️
pkg/agent/nodeportlocal/k8s/annotations.go	`0.00% <0.00%> (-83.88%)`	⬇️
pkg/agent/agent_linux.go	`0.00% <0.00%> (-80.00%)`	⬇️
pkg/agent/client.go	`0.00% <0.00%> (-77.42%)`	⬇️
pkg/ovs/ovsconfig/ovs_client_linux.go	`0.00% <0.00%> (-76.93%)`	⬇️
pkg/flowaggregator/certificate.go	`0.00% <0.00%> (-76.58%)`	⬇️
... and 231 more

jianjuns

A few more comments.

jianjuns · 2021-09-16T23:56:27Z

docs/design/ovs-pipeline.md

+Flow 1 is used to match packet whose destination IP is virtual hairpin IP and
+change the destination IP of the matched packet by loading register `NXM_OF_IP_SRC`
+to `NXM_OF_IP_DST`. Bit 18 in NXM_NX_REG0 is set to 0x1, which indicates that packet
+should be output to the port on which it was received, which is done by table [L2ForwardingOutTable].


done by -> done in

Thanks，updated.

jianjuns · 2021-09-17T00:02:26Z

docs/design/ovs-pipeline.md

+
+* `load:0x2->NXM_NX_REG4[16..18]` is used to set the value of bits [16..18] in NXM_NX_REG4
+  to 0b002, which indicates that Endpoint selection has been done. Note that, the Endpoint
+  selection has not really been done yet - it will be done in the target OVS group, so 


Read it again, and feel "done in the target OVS group" is hard to understand. Should we say "done by the group action"?

done by the group action is better.

jianjuns · 2021-09-17T00:05:31Z

docs/design/ovs-pipeline.md

+The actions of the above flow:
+
+* `load:0x2->NXM_NX_REG4[16..18]` is used to set the value of bits [16..18] in NXM_NX_REG4
+  to 0b002, which indicates that Endpoint selection has been done. Note that, the Endpoint


How about: "selection "is performed""

IMO, it is a little wierde to use perform only for this sentence as other related sentences use done.

I want to use "performed" is just because it is not really "done", and for the same reason I would use "is" rather than "has been", and also quote "is performed".

Got what you meant, thanks for explaining.

jianjuns · 2021-09-17T00:06:56Z

docs/design/ovs-pipeline.md

+* `load:0x2->NXM_NX_REG4[16..18]` is used to set the value of bits [16..18] in NXM_NX_REG4
+  to 0b002, which indicates that Endpoint selection has been done. Note that, the Endpoint
+  selection has not really been done yet - it will be done in the target OVS group, so 
+  the current action should be done in the OVS group entry after Endpoint selection. However, 


I suggest to remove the "so..." sentence. It is even confusing with it.

jianjuns · 2021-09-17T00:08:53Z

docs/design/ovs-pipeline.md

+  selection has not really been done yet - it will be done in the target OVS group, so 
+  the current action should be done in the OVS group entry after Endpoint selection. However, 
+  according to PR [#2101](https://github.com/antrea-io/antrea/pull/2101), to hold more
+  Endpoints in an OVS group entry, it is a workaround way to do the current action here.


workaround way -> workaround

Still I cannot understand what you want to express here. Probably you should add more information to explain why the workaround can help hold more endpoints, or say: "However, we set the bits here, for the purpose of supporting more Endpoints in an OVS group. Check PR #2101 to learn more information."

I prefer to say "However, we set the bits here, for the purpose of supporting more Endpoints in an OVS group. Check PR #2101 to learn more information". It's a little complicated to explain the reason, and I think it is unnecessary to explain in the doc.

jianjuns · 2021-09-17T00:14:18Z

docs/design/ovs-pipeline.md

+  according to PR [#2101](https://github.com/antrea-io/antrea/pull/2101), to hold more
+  Endpoints in an OVS group entry, it is a workaround way to do the current action here.
+* `load:0x1->NXM_NX_REG0[19]` is used to set the value of bit 19 in NXM_NX_REG0 to 0x1,
+  which means that the source and destination MACs should be rewritten.


should be -> need to be

Thanks, updated.

jianjuns · 2021-09-17T00:15:28Z

docs/design/ovs-pipeline.md

+  according to PR [#2101](https://github.com/antrea-io/antrea/pull/2101), to hold more
+  Endpoints in an OVS group entry, it is a workaround way to do the current action here.
+* `load:0x1->NXM_NX_REG0[19]` is used to set the value of bit 19 in NXM_NX_REG0 to 0x1,
+  which means that the source and destination MACs should be rewritten.


should be -> need to be

Thanks, updated.

jianjuns

Comments on the SVG diagram:

Service ClusterIP/LoadBalancer traffic -> ClusterIP/LoadBalancer Service traffic. The same for NodePort.
Could we keep the version when AntreaProxy is disabled? Do not have a good idea how to do that. Could we put the fragment of "ContrackStateTable, DNATTable, AntreaPolicyEgressRuleTable, output:gw, kube-proxy", after the main diagram?

We probably need @antoninbas to check the SVG format.

hongliangl · 2021-09-18T01:30:26Z

Service ClusterIP/LoadBalancer traffic -> ClusterIP/LoadBalancer Service traffic. The same for NodePort.

Updated

Could we keep the version when AntreaProxy is disabled? Do not have a good idea how to do that. Could we put the fragment of "ContrackStateTable, DNATTable, AntreaPolicyEgressRuleTable, output:gw, kube-proxy", after the main diagram?

If keeping the version when AntreaProxy is disabled, I think we need another document. I have made some assumptions for this document as we can see the following:

This document currently makes the following assumptions:

Antrea is used in encap mode (an overlay network is created between all Nodes)
AntreaProxy is enabled
All the Nodes are Linux Nodes
IPv6 is disabled

I think we may maintain a document for each of the following cases if possible:

AntreaProxy is disabled.
AntreaProxy is enabled and ProxyAll is disabled.
AntreaProxy is enabled and ProxyAll is enabled.

But I don't have a good idea how to deal with the common parts(e.g, table 0, table 110, register introduction) of above three cases.

jianjuns · 2021-09-18T03:00:23Z

Probably not worthwhile to maintain two docs. How about just keep the pipeline SVG or a corner of it for the kube-proxy case?

hongliangl · 2021-09-18T03:37:42Z

Probably not worthwhile to maintain two docs. How about just keep the pipeline SVG or a corner of it for the kube-proxy case?

If we keep the pipeline SVG with AntreaProxy disabled, I think we need also to explain table DnatTable and add more explanation for table ServiceHairpinTable, HairpinSNATTable(They should not exist when AntreaProxy is disabled).
For table SpoofGuardTable, when AntreaProxy is disabled, the IP packet goes to ConntrackTable, otherwise the IP packet should go to ServiceHairpinTable. I think we should explain this, but this may confuse readers. Since AntreaProxy is enabled by default, I think we may not keep the pipeline SVG with AntreaProxy disabled.

a corner of it for the kube-proxy case?

Do you mean that we add a extra chapter to explain the case with AntreaProxy disabled?

jianjuns · 2021-09-18T06:26:07Z

Do you mean that we add a extra chapter to explain the case with AntreaProxy disabled?
Yeah, maybe we can add a section to talk about AntreaProxy disabled. It just needs to talk about the differences, like DNATTable.

jianjuns · 2021-09-18T06:27:59Z

It is ok to me too, if you like to merge the current PR. I can create another PR to add the contents with kube-proxy back.

hongliangl · 2021-09-22T00:40:41Z

It is ok to me too, if you like to merge the current PR. I can create another PR to add the contents with kube-proxy back.

Updated, I have add contents with kube-proxy back.

jianjuns · 2021-09-22T01:02:36Z

docs/design/ovs-pipeline.md

+![OVS pipeline](../assets/ovs-pipeline-antrea-proxy.svg)
+
+If feature AntreaProxy is disabled, the pipeline is as the following:
+
 ![OVS pipeline](../assets/ovs-pipeline.svg)


How about adding a separate section under Tables that talks about the case when kube-proxy is used, and put the diagram and DNATTable there?
@antoninbas : thoughts?

yes, that sounds good to me. We should just emphasize the default case, which is AntreaProxy enabled.

antoninbas

took a quick look at the SVG file and the formatting looks good to me; I didn't review the rest of this PR

antoninbas · 2021-09-22T21:41:56Z

docs/design/ovs-pipeline.md

+![OVS pipeline](../assets/ovs-pipeline-antrea-proxy.svg)
+
+If feature AntreaProxy is disabled, the pipeline is as the following:
+
 ![OVS pipeline](../assets/ovs-pipeline.svg)


yes, that sounds good to me. We should just emphasize the default case, which is AntreaProxy enabled.

antoninbas · 2021-09-22T21:42:08Z

docs/design/ovs-pipeline.md

+If feature AntreaProxy is enabled, the pipeline is as the following:
+
+![OVS pipeline](../assets/ovs-pipeline-antrea-proxy.svg)
+
+If feature AntreaProxy is disabled, the pipeline is as the following:


s/as the following/as follows

hongliangl · 2021-09-23T01:39:37Z

took a quick look at the SVG file and the formatting looks good to me; I didn't review the rest of this PR

Thanks.

jianjuns · 2021-09-23T16:50:59Z

docs/design/ovs-pipeline.md

+
+### DNATTable (40)
+
+This table is create when feature AntreaProxy is disabled. Its only job is to


is created only when AntreaProxy is disabled

jianjuns · 2021-09-23T16:53:49Z

docs/design/ovs-pipeline.md

+### DNATTable (40)
+
+This table is create when feature AntreaProxy is disabled. Its only job is to
+send traffic destined to Services through the local gateway, without any


gateway -> gateway interface, to be clearer.

Thanks, updated.

jianjuns · 2021-09-23T16:54:59Z

docs/design/ovs-pipeline.md

+Policies.
+
+The table-miss flow entry (flow 2) for this table forwards all non-Service
+traffic to the next table, [EgressRuleTable].


EgressRuleTable -> AntreaPolicyEgressRuleTable

Thanks, updated.

jianjuns · 2021-09-23T18:14:46Z

docs/design/ovs-pipeline.md

@@ -929,12 +1135,53 @@ The first flow outputs all unicast packets to the correct port (the port was
 resolved by the "dmac" table, [L2ForwardingCalcTable]). IP packets for which
 [L2ForwardingCalcTable] did not set bit 16 of NXM_NX_REG0 will be dropped.

+## Tables (AntreaProxy is disabled)
+
+![OVS pipeline](../assets/ovs-pipeline.svg)


Could you rename DnatTable to DNATTable in the SVG?

Sure, updated, @antoninbas could you please review the updated SVG file? Thanks!

Signed-off-by: Hongliang Liu <[email protected]>

hongliangl · 2021-09-25T09:13:24Z

/skip-all

hongliangl · 2021-09-25T09:14:41Z

/skip-test-ipv6-conformance
/skip-test-ipv6-e2e
/skip-test-ipv6-networkpolicy
/skip-test-ipv6-only-conformance
/skip-test-ipv6-only-networkpolicy
/skip-test-windows-conformance
/skip-test-windows-e2e
/skip-test-windows-networkpolicy

jianjuns · 2021-10-18T22:51:13Z

@hongliangl : I am going to merge this PR. Do you have any further revision to make?

jianjuns · 2021-10-25T18:22:51Z

/skip-all

hongliangl · 2021-10-26T00:22:29Z

@hongliangl : I am going to merge this PR. Do you have any further revision to make?

@jianjuns , thanks for merging this. No more further revision.

hongliangl requested review from antoninbas, jianjuns and tnqn September 6, 2021 14:31

hongliangl force-pushed the ovs-pipeline-doc branch 2 times, most recently from 083d6ae to 3c38a98 Compare September 8, 2021 01:54

jianjuns reviewed Sep 8, 2021

View reviewed changes

hongliangl requested a review from jianjuns September 12, 2021 16:27

jianjuns reviewed Sep 17, 2021

View reviewed changes

hongliangl force-pushed the ovs-pipeline-doc branch from d86efd8 to e59e93b Compare September 17, 2021 02:20

hongliangl requested a review from jianjuns September 17, 2021 02:21

hongliangl force-pushed the ovs-pipeline-doc branch from e59e93b to abc563b Compare September 17, 2021 04:58

jianjuns previously approved these changes Sep 17, 2021

View reviewed changes

jianjuns reviewed Sep 17, 2021

View reviewed changes

hongliangl dismissed jianjuns’s stale review via 4ed10db September 18, 2021 01:18

hongliangl force-pushed the ovs-pipeline-doc branch from abc563b to 4ed10db Compare September 18, 2021 01:18

hongliangl requested a review from jianjuns September 18, 2021 01:30

hongliangl force-pushed the ovs-pipeline-doc branch from 4ed10db to db5611e Compare September 18, 2021 01:47

hongliangl force-pushed the ovs-pipeline-doc branch from db5611e to c66f85e Compare September 18, 2021 03:38

hongliangl force-pushed the ovs-pipeline-doc branch from 4583dee to 120202c Compare September 22, 2021 00:44

jianjuns reviewed Sep 22, 2021

View reviewed changes

antoninbas reviewed Sep 22, 2021

View reviewed changes

hongliangl force-pushed the ovs-pipeline-doc branch from 120202c to 30655ab Compare September 23, 2021 01:39

hongliangl requested review from antoninbas and jianjuns September 23, 2021 01:39

hongliangl force-pushed the ovs-pipeline-doc branch 2 times, most recently from 7366e62 to b613906 Compare September 23, 2021 01:49

jianjuns reviewed Sep 23, 2021

View reviewed changes

Update ovs-pipeline.md

5c6668a

Signed-off-by: Hongliang Liu <[email protected]>

hongliangl force-pushed the ovs-pipeline-doc branch from b613906 to 5c6668a Compare September 24, 2021 06:36

hongliangl requested a review from jianjuns September 24, 2021 06:37

jianjuns approved these changes Sep 24, 2021

View reviewed changes

jianjuns merged commit f45c104 into antrea-io:main Oct 25, 2021

hongliangl deleted the ovs-pipeline-doc branch April 28, 2022 12:00


		### DNATTable (40)

		This table is create when feature AntreaProxy is disabled. Its only job is to

Update ovs-pipeline.md #2725

Update ovs-pipeline.md #2725

Conversation

hongliangl commented Sep 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 15, 2021 • edited Loading

Codecov Report

jianjuns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl Sep 17, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

hongliangl commented Sep 18, 2021

jianjuns commented Sep 18, 2021

hongliangl commented Sep 18, 2021

jianjuns commented Sep 18, 2021

jianjuns commented Sep 18, 2021

hongliangl commented Sep 22, 2021

jianjuns Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl commented Sep 23, 2021

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

hongliangl commented Sep 25, 2021

hongliangl commented Sep 25, 2021

jianjuns commented Oct 18, 2021

jianjuns commented Oct 25, 2021

hongliangl commented Oct 26, 2021

jianjuns Sep 16, 2021 •

edited

Loading

codecov-commenter commented Sep 15, 2021 •

edited

Loading

jianjuns Sep 17, 2021 •

edited

Loading

hongliangl Sep 17, 2021 •

edited

Loading

jianjuns Sep 22, 2021 •

edited

Loading