[ovs-dev] [PATCH 3/3] cfm: Expose detailed fault status in the database.
Ben Pfaff
blp at nicira.com
Wed Feb 8 20:38:40 UTC 2012
On Wed, Feb 08, 2012 at 11:46:25AM -0800, Ethan Jackson wrote:
> > 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.
OK.
> > 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.
So, my interpretation of the database has been that cfm_fault says
whether there is a fault and cfm_fault_status gives the reason for the
fault. How should a controller interpret it if there is a fault
(cfm_fault=true) but no reason, or almost equivalently if there is a
fault but the reason is just "override"? Especially if the goal of
providing the ability to override is to allow for testing (that is the
reason, right?) then I'd expect that one would want to simulate a
specific kind of fault.
Perhaps I just misunderstand.
> > (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.
Never mind that part, you're right.
> > 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.
Right, recv_fault would be a CFM_FAULT_* bitmap. On an unexpected
MAID, recv_fault |= CFM_FAULT_MAID. On an overflow, recv_fault |=
CFM_FAULT_OVERFLOW.
Not sure you saw my other comments since I didn't see any replies.
Thanks,
Ben.
More information about the dev
mailing list