| Message ID | b72f189fd4a42ae16c88bd27bebf6e461270f61e.1707057608.git.tredaelli@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | [ovs-dev] bfd: Improve state change log message. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 4 Feb 2024, at 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". > > Reported-by: Alex Stupnikov <astupnik@redhat.com> > Reported-at: https://bugzilla.redhat.com/2258496 > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com> Thanks for making this change Timothy! It looks fine to me. Acked-by: Eelco Chaudron <echaudro@redhat.com>
On Mon, Feb 05, 2024 at 08:20:54AM +0100, Eelco Chaudron wrote: > > > On 4 Feb 2024, at 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". > > > > Reported-by: Alex Stupnikov <astupnik@redhat.com> > > Reported-at: https://bugzilla.redhat.com/2258496 > > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com> > > Thanks for making this change Timothy! It looks fine to me. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> Acked-by: Simon Horman <horms@ovn.org>
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. Thoughts? Best regards, Ilya Maximets.
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. Maybe a follow-up patch to change this in all BFD-related messages to make them more uniform? > Thoughts? > > Best regards, Ilya Maximets.
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. > > Maybe a follow-up patch to change this in all BFD-related messages to make them > more uniform? > > >> Thoughts? >> >> Best regards, Ilya Maximets. >
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. >>
diff --git a/lib/bfd.c b/lib/bfd.c index 9698576d0..8033c5117 100644 --- a/lib/bfd.c +++ b/lib/bfd.c @@ -1131,8 +1131,9 @@ bfd_set_state(struct bfd *bfd, enum state state, enum diag diag) if (!VLOG_DROP_INFO(&rl)) { struct ds ds = DS_EMPTY_INITIALIZER; - ds_put_format(&ds, "%s: BFD state change: %s->%s" - " \"%s\"->\"%s\".\n", + ds_put_format(&ds, "%s: BFD state change: %s->%s," + " previous failure \"%s\", current failure" + " \"%s\".\n", bfd->name, bfd_state_str(bfd->state), bfd_state_str(state), bfd_diag_str(bfd->diag), bfd_diag_str(diag));
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". Reported-by: Alex Stupnikov <astupnik@redhat.com> Reported-at: https://bugzilla.redhat.com/2258496 Signed-off-by: Timothy Redaelli <tredaelli@redhat.com> --- lib/bfd.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)