| Message ID | 1456857559-99437-3-git-send-email-dball@vmware.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Darrell. The commit message should begin with a brief summary, no more than about 75 columns wide, and that should be the entire first paragraph of the commit message. When it's longer than that, the subject of the email message becomes too wide. I think that the commit message needs to be more extensive, too. It should explain the higher-level goal here. The vocabulary here is somewhat confusing. It refers to "physical endpoints" a lot. Are there logical endpoints as well? If not, is the word "physical" helpful? Could you > + <column name="chassis_port"> > + The port on the transport node the physical endpoint resides on. > + Context is the associated transport node. > + </column> I don't know what is supposed to go in chassis_port. I see that it's optional--in what case is it not populated? > + <column name="type"> > + The encapsulation type of the physical endpoint. Types include > + single vlan for now, but later support for mpls label stack, > + IP tunnel outer header and dual vlan. Note that encapsulations > + are often directionally different, meaning the encapsulation > + expected on ingress is different from the encapsulation > + for egressing packets; mpls labels and IP tunnels being > + two examples. > + </column> > + I don't think we should include "mpls_label_stack" as an option if we're not going to fully define what it means. I don't understand what a type of "later" means. > + <column name="ingress_encap"> > + The ingress encapsulation type of the physical endpoint. > + </column> > + > + <column name="egress_encap"> > + The egress encapsulation type of the physical endpoint. > + </column> The above documentation, and the schema itself, do not define the possible ingress and egress encapsulation types, so I wouldn't have any idea what's supposed to go here. > @@ -1278,6 +1328,7 @@ tcp.flags = RST; > </column> > > <column name="tag"> > + This column will be deprecated. > If set, indicates that the port represents a connection to a specific > VLAN on a locally accessible network. The VLAN ID is used to match > incoming traffic and is also added to outgoing traffic. It's confusing to say that a column "will be" deprecated. Is it deprecated or not? What's the replacement? > @@ -1286,15 +1337,18 @@ tcp.flags = RST; > > <group title="VTEP Options"> > <p> > + Will be deprecated. > These options apply to logical ports with <ref column="type"/> of > <code>vtep</code>. > </p> > > <column name="options" key="vtep-physical-switch"> > + This column "may" be deprecated. > Required. The name of the VTEP gateway. > </column> > > <column name="options" key="vtep-logical-switch"> > + This column "may" be deprecated. > Required. A logical switch name connected by the VTEP gateway. Must > be set when <ref column="type"/> is <code>vtep</code>. > </column> I'm even more confused when it says that a column "may" be deprecated.
Thanks Ben On 3/18/16, 11:16 AM, "dev on behalf of Ben Pfaff" <dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote: >Hi Darrell. > >The commit message should begin with a brief summary, no more than about >75 columns wide, and that should be the entire first paragraph of the >commit message. When it's longer than that, the subject of the email >message becomes too wide. I’ll adjust Each patch should have had “RFC” in the commit message as well > >I think that the commit message needs to be more extensive, too. It >should explain the higher-level goal here. I’ll include more high level text in the commit message instead of just the cover letter > >The vocabulary here is somewhat confusing. It refers to "physical >endpoints" a lot. Are there logical endpoints as well? If not, is the >word "physical" helpful? Could you Agreed; its not not clear In the subsequent non-RFC patch series covering physical-logical separation, I used the following text "Physical endpoints are defined here as endpoints at the border of a physical network. This presently applies to localnet and gateways.” I’m consolidating 2 non-RFC patch series and will send out today/tomorrow > > >> + <column name="chassis_port"> >> + The port on the transport node the physical endpoint resides on. >> + Context is the associated transport node. >> + </column> > >I don't know what is supposed to go in chassis_port. I see that it's >optional--in what case is it not populated? Will specify as "real interface, such as eth0, used to connect to a physical provider network” ? > >> + <column name="type"> >> + The encapsulation type of the physical endpoint. Types include >> + single vlan for now, but later support for mpls label stack, >> + IP tunnel outer header and dual vlan. Note that encapsulations >> + are often directionally different, meaning the encapsulation >> + expected on ingress is different from the encapsulation >> + for egressing packets; mpls labels and IP tunnels being >> + two examples. >> + </column> >> + > >I don't think we should include "mpls_label_stack" as an option if we're >not going to fully define what it means. I had put it there as a hint how it could be used in future I’ll give an example for mpls using a single label that differentiates ingress and egress cases and see it helps; I'll just mention that it can be extended > >I don't understand what a type of "later" means. Thanks That should not be there > >> + <column name="ingress_encap"> >> + The ingress encapsulation type of the physical endpoint. >> + </column> >> + >> + <column name="egress_encap"> >> + The egress encapsulation type of the physical endpoint. >> + </column> > >The above documentation, and the schema itself, do not define the >possible ingress and egress encapsulation types, so I wouldn't have any >idea what's supposed to go here. I’ll be more clear the xml file and only include in the schema what is presently used in the code > >> @@ -1278,6 +1328,7 @@ tcp.flags = RST; >> </column> >> >> <column name="tag"> >> + This column will be deprecated. >> If set, indicates that the port represents a connection to a specific >> VLAN on a locally accessible network. The VLAN ID is used to match >> incoming traffic and is also added to outgoing traffic. > >It's confusing to say that a column "will be" deprecated. Is it >deprecated or not? What's the replacement? I won’t be adding any such comments in the xml file going forward I’ll remove the field from the xml file when it time to deprecate it > >> @@ -1286,15 +1337,18 @@ tcp.flags = RST; >> >> <group title="VTEP Options"> >> <p> >> + Will be deprecated. >> These options apply to logical ports with <ref column="type"/> of >> <code>vtep</code>. >> </p> >> >> <column name="options" key="vtep-physical-switch"> >> + This column "may" be deprecated. >> Required. The name of the VTEP gateway. >> </column> >> >> <column name="options" key="vtep-logical-switch"> >> + This column "may" be deprecated. >> Required. A logical switch name connected by the VTEP gateway. Must >> be set when <ref column="type"/> is <code>vtep</code>. >> </column> > >I'm even more confused when it says that a column "may" be deprecated. agreed >_______________________________________________ >dev mailing list >dev@openvswitch.org >http://openvswitch.org/mailman/listinfo/dev
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml index 1ea35d5..689d1c8 100644 --- a/ovn/ovn-sb.xml +++ b/ovn/ovn-sb.xml @@ -1114,6 +1114,49 @@ tcp.flags = RST; </group> </table> + <table name="Physical_Endpoint" title="Endpoints on physical access nets "> + <p> + Each row in this table identifies an endpoint on a physical access + network. The <ref table="Port_Binding"/> table references physical + endpoints for localnet and gateways. A physical endpoint comprises + both a location on a chassis port and encapsulation for access. + A given chassis port location can have several encapsulations + and there physical endpoints. + </p> + + <column name="name"> + Unique name to reference the physical endpoint. + </column> + + <column name="chassis"> + The transport node the physical endpoint resides on. + </column> + + <column name="chassis_port"> + The port on the transport node the physical endpoint resides on. + Context is the associated transport node. + </column> + + <column name="type"> + The encapsulation type of the physical endpoint. Types include + single vlan for now, but later support for mpls label stack, + IP tunnel outer header and dual vlan. Note that encapsulations + are often directionally different, meaning the encapsulation + expected on ingress is different from the encapsulation + for egressing packets; mpls labels and IP tunnels being + two examples. + </column> + + <column name="ingress_encap"> + The ingress encapsulation type of the physical endpoint. + </column> + + <column name="egress_encap"> + The egress encapsulation type of the physical endpoint. + </column> + + </table> + <table name="Port_Binding" title="Physical-Logical Port Bindings"> <p> Most rows in this table identify the physical location of a logical port. @@ -1168,6 +1211,13 @@ tcp.flags = RST; <code>ovn-controller</code>/<code>ovn-controller-vtep</code>. </column> + <column name="phys_endpt"> + The physical endpoint that the logical port resides on. + To successfully identify a phys_endpt, this column must be a + <ref table="Physical_Endpoint"/> record. This is populated by a + <code>CMS</code> physical management component. + </column> + <column name="tunnel_key"> <p> A number that represents the logical port in the key (e.g. STT key or @@ -1278,6 +1328,7 @@ tcp.flags = RST; </column> <column name="tag"> + This column will be deprecated. If set, indicates that the port represents a connection to a specific VLAN on a locally accessible network. The VLAN ID is used to match incoming traffic and is also added to outgoing traffic. @@ -1286,15 +1337,18 @@ tcp.flags = RST; <group title="VTEP Options"> <p> + Will be deprecated. These options apply to logical ports with <ref column="type"/> of <code>vtep</code>. </p> <column name="options" key="vtep-physical-switch"> + This column "may" be deprecated. Required. The name of the VTEP gateway. </column> <column name="options" key="vtep-logical-switch"> + This column "may" be deprecated. Required. A logical switch name connected by the VTEP gateway. Must be set when <ref column="type"/> is <code>vtep</code>. </column>
Signed-off-by: Darrell Ball <dball@vmware.com> --- ovn/ovn-sb.xml | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)