The MPLS WG Archive[Date Prev][Date Next][Thread Prev][Thread Next] [Date Index][Thread Index][Author Index][Subject Index] AD review of draft-ietf-mpls-ldp-mib-10.txt
> -----Original Message----- > From: owner-mpls@UU.NET [mailto:owner-mpls@UU.NET] On Behalf > Of jcucchiara@mindspring.com > Sent: Saturday, May 10, 2003 8:18 AM > To: Wijnen, Bert (Bert); Mpls (E-mail) > Subject: Re: AD review of draft-ietf-mpls-ldp-mib-10.txt > > > > Bert, > > Thanks for this review! > > At 01:53 AM 5/9/03 +0200, Wijnen, Bert (Bert) wrote: > >First a few serious things that need fixing (or a acceptable > >answer): > > > >1. All the documents that contain a MIB module from which > > you IMPORT anything must be listed as normative references. > > I think a few are now under informative and a few may even > > be missing. > > okay. > > > > >2. I see at several place in DESCRIPTION clause of StorageType > > objects this text: > > DESCRIPTION > > "The storage type for this conceptual row. Conceptual rows > > having the value 'permanent(4)' MAY allow write-access to any > > columnar objects in the row, except for setting the > > mplsLdpEntityRowStatus to 'destroy(6)'." > > And that is very weird. It seems to allow to change the > storagetype > > object itself (which is a nono). And the cannot destroy is also > > already covered by being a permanent entry. > > The required text from RFC2579 (Storage Type TC) is something aka: > > "The storage type for this conceptual row. Conceptual rows > > having the value 'permanent(4)' need not allow write-access > > to any columnar objects in the row." > > Which you have in some other cases, and which I think expresses > > the same and is correct accoring to RFC2579 requirements. > > mplsLdpEntityAtmLRStorageType has it specified correctly! > > > > Sorry, but am confused by this, if the type is permanent(4), > then the agent does not have to support any subsequent sets > (modifications) to objects in this row, which includes > setting the RowStatus object to destroy, is this correct? > > So then once a row is permanent(4), then the agent does not > need to provide a way to remove/change this row using SNMP, > is this correct? > > We can rephrase this using the text suggested above, but just > want to make sure we understand ;) > > >3 I see: > > mplsFecAddrPrefixLength InetAddressPrefixLength, > > mplsFecAddrFamily InetAddressType, > > mplsFecAddr InetAddress, > > I believe that RFC3291 suggests to put InetAddressType BEFORE > > any objects that are to be intepreted within the context of > > the specific addressType. so the sequence should probaly be: > > mplsFecAddrFamily InetAddressType, > > mplsFecAddrPrefixLength InetAddressPrefixLength, > > mplsFecAddr InetAddress, > > > > I would also check the DESCRIPTION clauses with RFC3291. > > I am not so sure it is all in sync with prescriptions of > RFC3291 I > > am specifically worried about the reference to RFC1700 (which is > > obsolete anyway, probably you better refer to > > http://www.iana.org/assignments/address-family-numbers > > if that is what you want). But is it not just IPv4 and IPv6 > > addresses that you support (at least that is what > compliance seems to > > state? > > > The reference to ADDRESS-FAMILY is because the LDP Spec > (3036) uses in a few TLV and they do specify [RFC1700] as the > reference. Here is the Specific example from the LDP Spec (rfc3036): > > > "Prefix FEC Element value encoding: > > 0 1 2 3 > 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 > > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | Prefix (2) | Address Family | > PreLen | > > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | Prefix > | > > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > Address Family > Two octet quantity containing a value from ADDRESS FAMILY > NUMBERS in [RFC1700] that encodes the address family for the > address prefix in the Prefix field." > > > > We do not mean to cause confusion in the MIB, BUT, the LDP > Spec which is "Standards Track" document refers to the > obsoleted document (RFC1700) and the MIB is actually refering > to the LDP Spec. > > In the MIB we are trying to use descriptions based on the LDP > Spec so that folks can use this MIB to configure values for > the LDP TLVs. If we change this description too much, then > the reference back to the LDP Spec is lost. > > Originally we did use the ADDRESS-FAMILY-NUMBERS TC but > changed this for the INET-ADDRESS-MIB which is better for MIB > purposes. > > Please let us know if we can keep the DESCRIPTION as it is or > change it? > > > > In the DESCRIPTION of PrefixLength I would change > > "If the value of the 'mplsFecType' is 'hostAddress(2)' > > then this object is undefined. > > Into either: > > "If the value of the 'mplsFecType' is 'hostAddress(2)' > > then the value of object is irrelevant and ignored. Or > > (maybe better, not sure): > > "If the value of the 'mplsFecType' is 'hostAddress(2)' > > then the value should be equal to the length of the > > address in mplsFecAddr. > > Also in that same DESCRIPTION clause: > > prefix matches all addresses. In this case the > > prefix MUST also be zero (i.e. 'mplsFecAddr' will > > have the value of zero.)" > > What is a "value of zero"? I think you mean zero length. > > But in order for that to be valid for mplsFecAddr, the > > mplsFecAddrFamily value should be zero (i.e. unknown). > > > >4. How many notifications can be generated per second? > > Are you asking for a worst case scenario? > > In a network where LDP is configured on each device (and not being > reconfigured) > would not expect notifications to be generated once sessions > are established. So would not quantify X notifications per second. > > Maybe another way to ask this question is: how long does the > average LDP Session last? > > Would say most last longer than a second, but > would be good to get operator experience here. My experience and customer feedback has been that there are not often more than O(hundreds) of LDP sessions on a box, and it is unlikely that they all run over the same interface so a catestrophic node failure should not often trigger any other box's sessions to all come down simultaneously which seems like the case you are trying to handle. So I disagree that a throttling/per-second notification variable is needed in the case of LDP. --Tom > > > I worry based on the description clauses. I see a risk that it > > could be many if not implemented properly or with a proper > > throttle? Can you add some text about that? > > There are not huge amounts of LDP sessions on a device and > these (one established) are fairly stable unless an operator > reconfigures or something goes wrong with the network, but > this would fall into a worst case scenario. > > Certainly, would be good to get some operator experience here, but > in my experience, sessions (once established) are fairly > stable as long as the network's physical connections are not > being changed and > LDP is not being reconfigured. > > > > >5. I worry about a statement in a RowStatus DESCRIPTION clause > > (it is doen several times): > > NOTE: This RowStatus object should > > have the same value of the 'mplsLdpEntityRowStatus' > > related to this entry." > > I will agree that that is the ideal situation. But the rowStatus > > should always represent the real status of the row itself. > > If that is not consistent with some other row in another table, > > then that is bad, but it should not be a cause for the > > rowStatus of thos column to take an invalid value. I guess > > you did not mean to say it should... but that is how people > > might read it. > > Okay, I understand I think. We have had many issues with > folks not understanding the relationship of these tables and > essentially, any table that has a configurable row is related > to a row in the > mplsLdpEntityTable. This is outside of the scope of > RowStatus though, so will reword this to remove the "should". > > > > > > >6. MPLS-LDP-GENERIC-MIB > > W: f(mplsldpgen.mi2), (58,17) The first revision should > > match the last update for MODULE-IDENTITY mplsLdpGenericMIB > > I think you just mistyped 2002 instead of 2003. > > yes. typo. > > > > >Nits (some more serious than otehrs)... > >would be nice if they can be fixed: > > > >- I would not use capitals for FULL, READ-ONLY, NEW and such. > > There is the risk that someone wants to know if they have > > special meaning. > >- sect 3.5 under NOTE: This table > > My question: which table? > >- You have in many places a reference aka: > > REFERENCE > > "[RFC3036] LDP Specification, Section on LDP Identifiers." > > Now, once people extract the MIB module from the document (and many > > people do) then you loose the citation. So I always recommend: > > REFERENCE > > "RFC3036, LDP Specification, Section on LDP Identifiers." > >- I see: > > mplsLdpPeerTransportAddrType OBJECT-TYPE > > SYNTAX InetAddressType > > MAX-ACCESS read-only > > STATUS current > > DESCRIPTION > > "The object specifies how to interpret the address > > for the mplsLdpPeerTransportAddr object." > > .. snip > > > > mplsLdpPeerTransportAddr OBJECT-TYPE > > SYNTAX InetAddress > > MAX-ACCESS read-only > > STATUS current > > DESCRIPTION > > "The transport address advertized by the peer > > in the hello message or the Hello source address." > > > > I think that the DESCRIPTION for InetAddress object MUST contain > > the explanation how it is interpreted. The example in RFC3291 is > > so explicit and clear (or so I think). I also think that it is > > in fact an Internet address and not a transport address (the > > latter needs a port number too, does it not? see RFC3419) > > > > So something aka the next seems better (maybe even rename > > TransportAddr to IntenetAddr: > > > > mplsLdpPeerTransportAddrType OBJECT-TYPE > > SYNTAX InetAddressType > > MAX-ACCESS read-only > > STATUS current > > DESCRIPTION > > "The type of Internet address for the > > mplsLdpPeerTransportAddr object." > > .. snip > > > > mplsLdpPeerTransportAddr OBJECT-TYPE > > SYNTAX InetAddress > > MAX-ACCESS read-only > > STATUS current > > DESCRIPTION > > "The Internet address advertized by the peer in the hello > > message or the Hello source address. > > > > The type of this address is determined by the value of > > the mplsLdpPeerTransportAddressType object." > > > > If you do change this, pls check all your InetAddress objects > > okay, thank you for the text. > > > > >- mplsLdpSesKeepaliveTime > > What does value of zero mean? > > The value cannot be zero. This has to be a nonzero, 2 octet > value, which represents the amount of seconds between keep > alive messages negoatiated between the entity and the peer > for that specific session. > > The mplsLdpEntityKeepAliveHoldTimer is the object which > configures this value. This Peer also sends a value and > these are negotiated to be the smaller of the 2 values. > > This object represents the negotiated value. > > Do we put a range on this even though it is read-only and > we already have a range on the configurable counter-part? > > > The range will be the same range as for the > mplsLdpEntityKeepAliveHoldTimer object which is configurable. > > > > >- mplsLdpHelloAdjacencyHoldTimeRem > > could use a UNITS clause > > We debated this. If there are special values (in this case > for 0 and for 0xffff/65535) then if we have a UNITS "seconds" > is it clear that these other values are not seconds?? > > > > I also wonder, since 0xffff has a special meaning,does that mean > > that that is also the max value? If so, a range of > (0..65535) would > > make good sense. > > The object which is used to configure this value for the > entity is mplsLdpEntityHelloHoldTimer which does have a range. > > SO same question as above, do we put a range on this object > even if it is read-only and we already have the range for the > configurable counter-part? > > > > > > >- I see: > > mplsOutSegmentLdpLspTable OBJECT-TYPE > > SYNTAX SEQUENCE OF MplsOutSegmentLdpLspEntry > > MAX-ACCESS not-accessible > > STATUS current > > DESCRIPTION > > "A table of LDP LSP's which > > map to the InSegment Table in the > > the LSR MIB's." > > s/InSegment/OutSegment/ ?? > > s/MIB's/MIB/ ?? > > > >- I may have said this before. Could the mplsLdpSesUp and > >mplsLdpSesDown > > notification not be combined to mplsLdpSesStateChange? > > The included mplsLdpSesState object will identify if it is > an Up or a > > Down, will it not? > > > Yes, this came up. We wanted to leave this because we do > know NMS apps which do not decode the notifications too much > but resolve by saying this notification matches this other > notification. So one notification causes an alarm and the > other notification clears it. Not sure if it matters too > much in this situation so we can change it as suggested above. > > > > > >- In the DESCRIPTION clauses of your OBJECT-GROUP > definitions, you have > > langauge that talsk about "optional" and/or "required to > implement". > > That is not the goal of the OBJECT-GROUP macro. It is > needed to GROUP > > objects that logically belong together. So the DESCRIPTION should > > have text to explain why these objects belong in one group. > > The "optional" and "required" and susch is language that goes > > in DESCRIPTION clauses of the MODULE-COMPLIANCE GROUP clauses. > > Okay, thanks for this info. > > > > >- Sect 4.1 talks about mplsLdpEntityAtmLabelRangeTable while > the actual > > table name is: mplsLdpEntityAtmLRTable > > Same for FrameRelay and Generic MIB > > Sorry, yes, we changed this and missed the text. > > > > >- mplsLdpEntityAtmTable > > You talk about "extends". I think what the table does is that it > > is a "sparse augments". Your indexes are OK though. > > > Yes, this is true for the mplsLdpEntityFrameRelayTable also. > > > > >- mplsLdpEntityAtmLREntry talks about overlaps and such that may > > (should, can) not occur. But it does not say anything about what > > sort of error to return if someone tries to SNMP SET (or create) > > incorrect values. And It seems like 2 errors are possible: > > - inconcsistentName (if it is part of the (not-accessible) index) > > - inconsistentValue (if it is a value in read-create column) > > Not 100% sure... anyway, better to specify what you want to > > happen than let people guess > > > > Okay we will add to the table description. > > > >- mplsLdpEntityAtmLRRowStatus has in DESCRIPTION clause > > There must exist at least one entry in this > > table for every LDP Entity that has > > 'mplsLdpEntityOptionalParameters' object with > > a value of 'atmSessionParameters'. > > Does not seem right in RowStatus object. Maybe in the Entry > > or Table object? > > I have seen this in multiple RowStatus Objects > > We will check on this. Agree it is better in table/entry description. > > > > >- mplsLdpAtmSesTable talks about 'mplsLdpSessionTable' > > In fact many places in the doc talk about it. > > Seems you renamed the table to mplsLdpSesTable (not sure why, > > cause I find the full Session better... but that is another > > subject). > > Okay, we will check this. Sorry, had a mix and changed to to > be shorter, but sounds like you prefer the longer, i.e. session ;) > > > > >- mplsLdpEntityAtmLRRowStatus talks about > > 'mplsLdpEntityOptionalParameters' > > and so do some other places, but I cannot find the latter object > > We changed the name of the object to be mplsLdpEntityLabelType, so > will update these descriptions. > > > > >- I can live with the security considerations section. > > You might be able to shorten it somewhat if you take the last 3 > > paragraphs of each 9.x section and state that once in its own > > 9.x section. Up to you. > > Okay, we can do that. > > Thanks again for the review. > > -Joan > > > > > >Thanks, > >Bert > > > >
|
|