On 6 Feb 2024, at 13:58, Ilya Maximets wrote:
> On 2/6/24 13:50, Eelco Chaudron wrote: >> >> >> On 6 Feb 2024, at 13:46, Ilya Maximets wrote: >> >>> On 2/4/24 15:40, Timothy Redaelli wrote: >>>> A log message like this one: >>>> >>>> 2024-01-09T06:45:17.201Z|00071|bfd(handler2)|INFO|ovn-0af536-0: BFD state >>>> change: down->up "Neighbor Signaled Session Down"->"Neighbor Signaled >>>> Session >>>> Down". >>>> >>>> can be hard to read since '->' usually represents a status change, but >>>> in this case the diagnostic code stays constant. Update the log message to >>>> avoid such ambiguity. The log message for the above event become: >>>> >>>> 2024-01-09T06:45:16.211Z|00026|bfd(handler3)|INFO|ovn-0af536-0: BFD state >>>> change: up->down, previous failure: "Neighbor Signaled Session Down", >>>> current failure: "Neighbor Signaled Session Down". >>> >>> Hi, Timothy. Thanks for the patch! >>> >>> Though I'm not sure that the new version is fully correct either. >>> Since this message is BFD-specific, we should use BFD-specific language. >>> Diagnostic is not a failure in a general case, so logging them as a failure >>> is not correct, IMO. >>> >>> We may also want to specify that it is a local diagnostic and not a remote >>> one to avoid any further ambiguity. >>> >>> However, if the confusion is caused by the fact that it doesn't change, >>> while >>> the arrow suggests that it did, maybe we can just re-structure the message >>> in a following way: >>> >>> BFD state change: >>> (bfd.SessionState: up, bfd.LocalDiag: Neighbor Signaled Session Down) -> >>> (bfd.SessionState: down, bfd.LocalDiag: Neighbor Signaled Session Down) >>> >>> (no need to split lines, I did that only for ease of reading) >>> >>> Since at least one part will change, the arrow should not be confusing >>> anymore. >>> >>> Spelling out bfd.Xx names as in the RFC is probably not necessary since >>> we're >>> printing out simple words (state, diagnostic) in other places, so can do >>> that >>> here as well. >> >> Good idea, I do like the RFC naming of the events. Maybe to save space, we >> could remove the “bfd.” prefix, but keep the SessionState, etc. > > I think, if we use the names from RFC, we should use them with a prefix. > The prefix is there to avoid confusion between state machine variables > and BFD packet fields that are very similarly named. I’m fine with keeping the prefix. If we do not want this for some other reason, we could use _ for packet field names with spaces. >> Maybe a follow-up patch to change this in all BFD-related messages to make >> them >> more uniform? >> >> >>> Thoughts? >>> >>> Best regards, Ilya Maximets. >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
