[ovs-dev] [PATCH ovn] controller: bfd: improve debugging logs

[Mark Michelson]

On 5/10/21 5:05 PM, Mark Michelson wrote:
> Thanks Lorenzo, it looks good to me.
> 
> Acked-by: Mark Michelson <mmichels at redhat.com>
> 

I pushed this to master.

> On 4/24/21 8:46 AM, Lorenzo Bianconi wrote:
>> Add new debug logs in pinctrl_handle_bfd_msg flow path useful for
>> debugging
>>
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
>> ---
>>   controller/pinctrl.c | 28 +++++++++++++++++++++++++---
>>   1 file changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index 523a45b9a..bef9b23fa 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -6819,54 +6819,71 @@ next:
>>   static bool
>>   pinctrl_check_bfd_msg(const struct flow *ip_flow, struct dp_packet 
>> *pkt_in)
>>   {
>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 30);
>>       if (ip_flow->dl_type != htons(ETH_TYPE_IP) &&
>>           ip_flow->dl_type != htons(ETH_TYPE_IPV6)) {
>> +        VLOG_DBG_RL(&rl, "BFD action on non-IP packet (%"PRIx16")",
>> +                    ntohs(ip_flow->dl_type));
>>           return false;
>>       }
>>       if (ip_flow->nw_proto != IPPROTO_UDP) {
>> +        VLOG_DBG_RL(&rl, "BFD action on non-UDP packet %02x",
>> +                    ip_flow->nw_proto);
>>           return false;
>>       }
>>       struct udp_header *udp_hdr = dp_packet_l4(pkt_in);
>>       if (udp_hdr->udp_dst != htons(BFD_DEST_PORT)) {
>> +        VLOG_DBG_RL(&rl, "BFD action on wrong UDP dst port (%"PRIx16")",
>> +                    ntohs(udp_hdr->udp_dst));
>>           return false;
>>       }
>>       const struct bfd_msg *msg = dp_packet_get_udp_payload(pkt_in);
>>       uint8_t version = msg->vers_diag >> 5;
>>       if (version != BFD_VERSION) {
>> +        VLOG_DBG_RL(&rl, "BFD action: unsupported version %d", version);
>>           return false;
>>       }
>>       enum bfd_flags flags = msg->flags & BFD_FLAGS_MASK;
>>       if (flags & BFD_FLAG_AUTH) {
>>           /* AUTH not supported yet */
>> +        VLOG_DBG_RL(&rl, "BFD action: AUTH not supported yet");
>>           return false;
>>       }
>>       if (msg->length < BFD_PACKET_LEN) {
>> +        VLOG_DBG_RL(&rl, "BFD action: packet is too short %d", 
>> msg->length);
>>           return false;
>>       }
>>       if (!msg->mult) {
>> +        VLOG_DBG_RL(&rl, "BFD action: multiplier not set");
>>           return false;
>>       }
>>       if (flags & BFD_FLAG_MULTIPOINT) {
>> +        VLOG_DBG_RL(&rl, "BFD action: MULTIPOINT not supported yet");
>>           return false;
>>       }
>>       if (!msg->my_disc) {
>> +        VLOG_DBG_RL(&rl, "BFD action: my_disc not set");
>>           return false;
>>       }
>>       if ((flags & BFD_FLAG_FINAL) && (flags & BFD_FLAG_POLL)) {
>> +        VLOG_DBG_RL(&rl, "BFD action: wrong flags combination %08x", 
>> flags);
>>           return false;
>>       }
>>       enum bfd_state peer_state = msg->flags >> 6;
>>       if (peer_state >= BFD_STATE_INIT && !msg->your_disc) {
>> +        VLOG_DBG_RL(&rl,
>> +                    "BFD action: remote state is %s and your_disc is 
>> not set",
>> +                    bfd_get_status(peer_state));
>>           return false;
>>       }
>> @@ -6879,8 +6896,6 @@ pinctrl_handle_bfd_msg(struct rconn *swconn, 
>> const struct flow *ip_flow,
>>       OVS_REQUIRES(pinctrl_mutex)
>>   {
>>       if (!pinctrl_check_bfd_msg(ip_flow, pkt_in)) {
>> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>> -        VLOG_WARN_RL(&rl, "BFD packet discarded");
>>           return;
>>       }
>> @@ -6894,9 +6909,9 @@ pinctrl_handle_bfd_msg(struct rconn *swconn, 
>> const struct flow *ip_flow,
>>       const struct bfd_msg *msg = dp_packet_get_udp_payload(pkt_in);
>>       struct bfd_entry *entry =
>>           pinctrl_find_bfd_monitor_entry_by_disc(ip_src, msg->your_disc);
>> -    free(ip_src);
>>       if (!entry) {
>> +        free(ip_src);
>>           return;
>>       }
>> @@ -6908,6 +6923,13 @@ pinctrl_handle_bfd_msg(struct rconn *swconn, 
>> const struct flow *ip_flow,
>>                                                  entry->local_min_rx);
>>       enum bfd_state peer_state = msg->flags >> 6;
>> +
>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 30);
>> +    VLOG_DBG_RL(&rl, "rx BFD packets from %s, remote state %s, local 
>> state %s",
>> +                ip_src, bfd_get_status(peer_state),
>> +                bfd_get_status(entry->state));
>> +    free(ip_src);
>> +
>>       if (peer_state == BFD_STATE_ADMIN_DOWN &&
>>           entry->state >= BFD_STATE_INIT) {
>>           entry->state = BFD_STATE_DOWN;
>>
>