Flexible pipeline - #1 Architecture #3035

hongliangl · 2021-11-17T07:32:17Z

Architecture code of flexible pipeline.

Signed-off-by: Hongliang Liu lhongliang@vmware.com

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

luolanzone · 2021-12-01T14:05:33Z

pkg/agent/openflow/feature_templates.go

+		template = &pipelineTemplate{
+			stageTables: map[binding.StageID][]tableRequest{
+				binding.ClassifierStage: {
+					tableRequest{ClassifierTable, 0x7f},


anywhere we can learn why here is setting as 0x7f? or will there be any update for existing ovs-pipeline design doc? https://github.com/antrea-io/antrea/blob/main/docs/design/ovs-pipeline.md

I agree. How the table priorities are decided? Should we define constants?

The order of table within a stage is decided by this priority. We don't need to define constants because priorities should be the same within a stage, otherwise the order of tables cannot be decided by priority. @jianjuns @luolanzone

I do not understand your explanation. How 0x7f, 0x6f, 0x7d in this file are decided? If I add a new table later, how I decide the priority value?

luolanzone · 2021-12-01T14:07:54Z

pkg/agent/openflow/feature_templates.go

+				break
+			}
+		}
+	} else if protocol == ofProtocolARP {


I think here you can use switch to make the whole structure more clear.

Thanks, updated in #3058

luolanzone · 2021-12-01T14:18:06Z

pkg/agent/openflow/fields.go

+	RewriteMACRegMark = binding.NewOneBitRegMark(0, 11, "RewriteMAC")
+	// reg0[12]: Mark to indicate the packet is denied(Drop/Reject).
+	CnpDenyRegMark = binding.NewOneBitRegMark(0, 12, "CNPDeny")
+	// reg0[13..14]: Field to indicate disposition of Antrea Policy. It could have more bits to support more disposition
 	// that Antrea policy support in the future.


s/Antrea policy/Antrea Policy/

luolanzone · 2021-12-01T14:20:50Z

pkg/agent/openflow/fields.go

 	// reg4[16..19]: Field to store the union value of Endpoint state and the mark of whether Service type is NodePort.
 	NodePortUnionField = binding.NewRegField(4, 16, 19, "NodePortUnion")
 	// reg4[21]: Mark to indicate the packet is from local AntreaFlexibleIPAM Pod.
 	// NotAntreaFlexibleIPAMRegMark will be used with RewriteMACRegMark, thus the reg id must not be same due to the limitation of ofnet library.
 	AntreaFlexibleIPAMRegMark    = binding.NewOneBitRegMark(4, 21, "AntreaFlexibleIPAM")
 	NotAntreaFlexibleIPAMRegMark = binding.NewOneBitZeroRegMark(4, 21, "AntreaFlexibleIPAM")
+	// reg4[22..23]: Field to store the state of a connection of Service NodePort/LoadBalancer from gateway which
+	// requires SNAT or not.
+	//	- 0b01: connection requires SNAT and is not marked with a ct mark.


not sure which one is correct but I suppose all ct mark string should be the same, either CT mark or ct mark in comments.

Maybe CT mark.

luolanzone · 2021-12-01T14:22:59Z

pkg/agent/openflow/network_policy.go

+	"antrea.io/antrea/pkg/agent/config"
+	"antrea.io/antrea/pkg/agent/openflow/cookie"


move this to below antrea.io import section

luolanzone · 2021-12-01T14:24:44Z

pkg/agent/openflow/network_policy.go

+	globalConjMatchFlowCache map[string]*conjMatchFlowContext
+	conjMatchFlowLock        sync.Mutex // Lock for access globalConjMatchFlowCache
+	// policyCache is a storage that supports listing policyRuleConjunction with different indexers.
+	// It's guaranteed that one policyRuleConjunction is processed by at most one goroutine at any given time.


s/It's guaranteed/It guarantees/

luolanzone · 2021-12-01T14:29:29Z

pkg/agent/openflow/traceflow.go

+	enableProxy,
+	enableAntreaPolicy,
+	supportEncap bool) feature {
+	gatewayIPs := make(map[binding.Protocol]net.IP)


I didn't see this variable gatewayIPs being used in following codes.

Thanks, it's updated in #3058

luolanzone · 2021-12-01T14:32:01Z

pkg/agent/openflow/vm_connectivity.go

+
+package openflow
+
+type featureVMConnectivity struct {


what's this for? a placeholder?

A placeholder, removed in #3058

luolanzone · 2021-12-01T14:33:13Z

pkg/ovs/openflow/ofctrl_pipeline.go

@@ -0,0 +1,113 @@
+// Copyright 2019 Antrea Authors


s/2019/2021/

jianjuns · 2021-12-03T00:48:31Z

pkg/agent/openflow/interfaces.go

+
+type pipelineTemplate struct {
+	// Declare the tables and the corresponding priorities in the expected stage.
+	// If it is expected to enforce a packet to enter other tables in the same stage after leaving the current table,


If it is expected to enforce -> To force

Actually I do not understand this sentence. Do you mean "a packet will enter all tables in the same stage in order of table priority"?

Do you mean "a packet will enter all tables in the same stage in order of table priority"?
Order of tables within a stage is decided by their priorities. The packet will pass through all tables within the stage by the order which is decided by the priorities.

What you think about replacing the sentence with my version: "A packet will enter all tables in the same stage in order of table priority"?

jianjuns · 2021-12-03T00:50:15Z

pkg/agent/openflow/interfaces.go

+	binding "antrea.io/antrea/pkg/ovs/openflow"
+)
+
+type featureID int


Can we add comments to explain the terms in this file? Or another way explain them in a separate doc. It is impossible to understand them just looking at code.

Updated in #3058

jianjuns · 2021-12-03T00:51:28Z

pkg/agent/openflow/interfaces.go

+
+// A table with a higher priority is assigned with a lower tableID, which means a packet should enter the table
+// before others with lower priorities in the same stage.
+type tableRequest struct {


Taking this one as an example, we should explain what is a tableRequest, before talking about fields in the struct.

Updated in #3058

jianjuns · 2021-12-03T01:01:21Z

pkg/agent/openflow/interfaces.go

+	c.ofTable = binding.NewOFTable(id, c.name, 0, 0)
+}
+
+// A table with a higher priority is assigned with a lower tableID, which means a packet should enter the table


We need to rephrase this. There is no relation between "a higher priority is assigned with a lower tableID" and "a packet should enter the table before others with lower priorities in the same stage".

Maybe: A packet enters tables with higher priorities before tables with lower priorities. A table with a higher priority is assigned with a smaller tableID.

But what is tableID? Do you mean tableRequest.priority? Then we should just say: A smaller value indicates a higher priority.

Updated in #3058

The part of code has been simplified. Please review #3058. @jianjuns Thanks!

jianjuns · 2021-12-03T01:07:06Z

pkg/agent/openflow/feature_templates.go

+		template = &pipelineTemplate{
+			stageTables: map[binding.StageID][]tableRequest{
+				binding.ClassifierStage: {
+					tableRequest{ClassifierTable, 0x7f},


I agree. How the table priorities are decided? Should we define constants?

hongliangl changed the title ~~Flexible pipeline - Architecture~~ Flexible pipeline - #1 Architecture Nov 17, 2021

hongliangl force-pushed the flexible-pipeline-#1 branch 3 times, most recently from fc49ff5 to 05eee0b Compare November 29, 2021 03:24

Flexible pipeline #1: Architecture

19fb48e

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl force-pushed the flexible-pipeline-#1 branch from 05eee0b to 19fb48e Compare November 29, 2021 08:23

luolanzone reviewed Dec 1, 2021

View reviewed changes

jianjuns reviewed Dec 3, 2021

View reviewed changes

hongliangl closed this Dec 6, 2021

hongliangl mentioned this pull request Dec 7, 2021

Flexible pipeline #3058

Merged

hongliangl deleted the flexible-pipeline-#1 branch February 8, 2022 00:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flexible pipeline - #1 Architecture #3035

Flexible pipeline - #1 Architecture #3035

hongliangl commented Nov 17, 2021

luolanzone Dec 1, 2021

jianjuns Dec 3, 2021

hongliangl Dec 7, 2021

jianjuns Dec 7, 2021

luolanzone Dec 1, 2021

hongliangl Dec 7, 2021

luolanzone Dec 1, 2021

luolanzone Dec 1, 2021

hongliangl Dec 7, 2021

luolanzone Dec 1, 2021

luolanzone Dec 1, 2021

luolanzone Dec 1, 2021

hongliangl Dec 7, 2021

luolanzone Dec 1, 2021

hongliangl Dec 7, 2021

luolanzone Dec 1, 2021

hongliangl Dec 7, 2021

jianjuns Dec 3, 2021

jianjuns Dec 3, 2021

hongliangl Dec 7, 2021

jianjuns Dec 7, 2021

jianjuns Dec 3, 2021

hongliangl Dec 7, 2021

jianjuns Dec 3, 2021

hongliangl Dec 7, 2021

jianjuns Dec 3, 2021

hongliangl Dec 7, 2021

hongliangl Dec 9, 2021

jianjuns Dec 3, 2021

		"antrea.io/antrea/pkg/agent/config"
		"antrea.io/antrea/pkg/agent/openflow/cookie"

Flexible pipeline - #1 Architecture #3035

Flexible pipeline - #1 Architecture #3035

Conversation

hongliangl commented Nov 17, 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

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

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