> A map from string to bool is almost like a set of strings that
> contains only the keys whose values would be "true". Did you consider
> that representation? The only difference is that the map form makes
> it clear that the "false" keys are supported.
Sure it's equivalent. I personally prefer this way because you get
some type checking which I think helps prevent bugs. I don't see a
reason to use a less accurate model of the data when the database
currently supports exactly what we want.
> Did you think about writing a function to convert a single CFM_FAULT_*
> bit to a string? Then you would avoid duplicating the string names in
> more than one place and you could write ds_put_cfm_fault() and the CFM
> fault status code in iface_refresh_cfm_stats() as loops.
Probably a good idea.
> If I'm reading the code correctly, turning on the override will now
> cause a fault to be written into the database but without anything in
> cfm_fault_status. If I'm right about that, then that's pretty
> confusing. I'd suggest allowing/requiring whoever sets the override
> to specify the fault reasons to report, and possibly to report in the
> database that the fault status was overridden.
I was a bit conflicted on this as I wrote the patch, but I think the
cleanest way to do it is just propagate the override status to the
database.
> (Should the admin be able to override a CFM to *not* faulted? That
> seems reasonably useful.)
They currently can using the set-fault command. I'm not sure
precisely what you're suggesting.
> It *might* be slightly cleaner to replace maid_recv and overflow_recv
> by a single "recv_fault" or similar that is also a bitmap of
> CFM_FAULT_*. Then you could, instead of:
This was the original behavior, we had an unexpected receive flag
which included both of these bits of information. I think a
controller may want the ability to distinguish between an unexpected
MAID and an overflow as the response is probably quite different. One
is due to a misconfiguration of some sort, while the other is due to a
limitation of the switch.
>> + cfm->fault = 0;
>> +
>> + if (cfm->maid_recv) {
>> + cfm->maid_recv = false;
>> + cfm->fault |= CFM_FAULT_MAID;
>> + }
>> +
>> + if (cfm->overflow_recv) {
>> + cfm->overflow_recv = false;
>> + cfm->fault |= CFM_FAULT_OVERFLOW;
>> + }
>
> Just do:
>
> cfm->fault = cfm->recv_fault;
> cfm->recv_fault = 0;
>
> Some more inline comments below:
>
>> @@ -1554,10 +1555,24 @@ iface_refresh_cfm_stats(struct iface *iface)
>> fault = ofproto_port_get_cfm_fault(iface->port->bridge->ofproto,
>> iface->ofp_port);
>> if (fault >= 0) {
>> + char *keys[]= {"recv", "maid", "rdi", "loopback", "overflow"};
>> bool fault_bool = fault;
>> + size_t i = 0;
>> +
>> + bool vals[ARRAY_SIZE(keys)];
>> +
>> + vals[i++] = fault & CFM_FAULT_RECV;
>> + vals[i++] = fault & CFM_FAULT_MAID;
>> + vals[i++] = fault & CFM_FAULT_RDI;
>> + vals[i++] = fault & CFM_FAULT_LOOPBACK;
>> + vals[i++] = fault & CFM_FAULT_OVERFLOW;
>
> If you keep the above as-is, then I'd consider adding:
> assert(i == ARRAY_SIZE(keys));
> here.
>
>> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
>> index 6f2c458..993dd5d 100644
>> --- a/vswitchd/vswitch.ovsschema
>> +++ b/vswitchd/vswitch.ovsschema
>> @@ -1,6 +1,6 @@
>> {"name": "Open_vSwitch",
>> - "version": "6.5.0",
>> - "cksum": "2847700438 16419",
>> + "version": "7.0.0",
>> + "cksum": "3395223101 16535",
>
> Why did you increment the major version? As far as I can tell, the
> new schema is compatible with the old one, because it adds new fields
> but does not remove any old ones.
>
>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>> index 7be7891..812caa4 100644
>> --- a/vswitchd/vswitch.xml
>> +++ b/vswitchd/vswitch.xml
>> @@ -1699,6 +1699,35 @@
>> </p>
>> </column>
>>
>> + <column name="cfm_fault_status" key="recv">
>> + When true, indicates a CFM fault was triggered due to a lack fo CCMs
>
> s/fo/of/
>
>> + received on the <ref table="Interface"/>.
>> + </column>
>> +
>> + <column name="cfm_fault_status" key="rdi">
>> + When true, indicates a CFM fault was triggered due to the reception
>> of
>> + a CCM with the RDI bit flagged. Endpoints set the RDI bit in their
>> + CCMs when they are not receiving CCMs themselves. This typically
>> + indicates a unidirectinoal connectivity failure.
>> + </column>
>> +
>> + <column name="cfm_fault_status" key="maid">
>> + When true, indicates a CFM fault was triggered due to the reception
>> of
>> + a CCM with a MAID other than the one Open vSwitch uses.
>
> Would you mind adding a few words here describing the implications of
> "a MAID other than the one Open vSwitch uses"?
>
>> + </column>
>> +
>> + <column name="cfm_fault_status" key="loopback">
>> + When true, indicates a CFM fault was triggered due to the reception
>> of
>> + a CCM advertising the same MPID configured in the
>> + <ref column="cfm_mpid"/> of this <ref table="Interface"/> this may
>
> "this" starts a new sentence so use a period and a capital letter
> here?
>
>> + indicate a loop in the network.
>> + </column>
>> +
>> + <column name="cfm_fault_status" key="overflow">
>> + When true, indicates a CFM fault was triggered because the CFM
>> module
>> + received CCMs from more remote endpoints than it can keep track of.
>> + </column>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
