public icon indicating copy to clipboard operation
public copied to clipboard

Use of decimal64 type in BGP models

Open exa-arrcus opened this issue 5 years ago • 4 comments

There are various leafs within the BGP models that make use of the decimal64 YANG type

e.g.

  grouping bgp-common-neighbor-group-timers-config {
    description
      "Config parameters related to timers associated with the BGP
      peer";

    leaf connect-retry {
      type decimal64 {
        fraction-digits 2;
      }
      default 30;
      description
        "Time interval in seconds between attempts to establish a
        session with the peer.";
    }

    leaf hold-time {
      type decimal64 {
        fraction-digits 2;
      }
      default 90;
      description
        "Time interval in seconds that a BGP session will be
        considered active in the absence of keepalive or other
        messages from the peer.  The hold-time is typically
        set to 3x the keepalive-interval.";
      reference
        "RFC 4271 - A Border Gateway Protocol 4, Sec. 10";
    }

    leaf keepalive-interval {
      type decimal64 {
        fraction-digits 2;
      }
      default 30;
      description
        "Time interval in seconds between transmission of keepalive
        messages to the neighbor.  Typically set to 1/3 the
        hold-time.";
    }

    leaf minimum-advertisement-interval {
      type decimal64 {
        fraction-digits 2;
      }
      default 30;
      description
        "Minimum time which must elapse between subsequent UPDATE
        messages relating to a common set of NLRI being transmitted
        to a peer. This timer is referred to as
        MinRouteAdvertisementIntervalTimer by RFC 4721 and serves to
        reduce the number of UPDATE messages transmitted when a
        particular set of NLRI exhibit instability.";
      reference
        "RFC 4271 - A Border Gateway Protocol 4, Sec 9.2.1.1";
    }
  }

However this leaves an effective range of -92233720368547758.08..92233720368547758.07

  1. Why are these decimals at all? Are there any implementations supporting decimals for these types of values?
  2. By default this allows negative numbers which do not apply in any such cases

I'm curious the reasoning behind why these are not just standard unsigned integers as every implementation is likely going to have to deviate and reel this back in to acceptable ranges at a minimum?

exa-arrcus avatar May 02 '20 22:05 exa-arrcus

Hey Ebben,

  1. This was a debate point at the time we originally put together the model. There was an assertion that some software implementations would want to be able to support sub-second granularity here - and hence this was a decimal64. I think that we should re-survey whether this useful going forward.

  2. Yes, these should have a range that must be >0 for sure.

Cheers, r.

robshakir avatar May 04 '20 00:05 robshakir

Thx @robshakir - speaking on behalf of ArcOS, we currently use the native types atm but no intention for sub-second granularity thus looking to deviate this to a uint type w/ a possible range restriction.

As far as I know, most other native implementations are the same from a front-end schema perspective so it would be good to re-survey and adjust vs. deviate.

exa-arrcus avatar May 04 '20 20:05 exa-arrcus

Thanks. I've added this to the OC WG meeting agenda for tomorrow.

robshakir avatar May 04 '20 20:05 robshakir

@robshakir - resurrecting this old issue - did anything come of this?

There are a number of other places among the model sets where

  • We have decimal64 types (w/ fraction-digits)
  • We have standard integer types but a units statement dictating the granularity (e.g. "milliseconds")

And in both of the cases - if you cross reference all major implementations you will see that each needs to either deviate and change types (or units/description statements) or handle transformations in different fashions due to dropping precision. The most common is the model calling for "milliseconds" where each implementation supports "seconds" granularity.

Before issuing a PR - I'd like to gauge if this is something we want to collectively fix among the models or deal with above where this doesn't appear to match any implementation today.

earies avatar Feb 20 '22 18:02 earies

Resolved by #604

dplore avatar Jul 10 '23 19:07 dplore