The MPLS WG Archive[Date Prev][Date Next][Thread Prev][Thread Next] [Date Index][Thread Index][Author Index][Subject Index] Review of: draft-ietf-mpls-telink-mib-00.txt
In reality, I did review a pre-release of rev 01,
but that rev was pretty much the same as rev 00.
Here is my review:
- compiles/syntax-checking clean. Good!
But.....
- linelegth checker:
$ /bin/checkpage.awk <home/ietf/drafts/draft-ietf-mpls-telink-mib-01.txt
Long line at 770 with 77 chars
Long line at 771 with 76 chars
Long line at 1268 with 73 chars
-: 3 lines longer than 72 characters, max 77
- Last sentence of abstract talks about "draft". I think you want to use
"document" or "memo" so that it is still valid when that draft becomes
an RFC. Maybe this occurs at other places too, pls check
- You seem to be using "MIB" many times where "MIB Module" or "MIB document"
would be better. Remember there is only one (Single) MIB, which is composed
of many (multiple) MIB modules. One or more MIB modules may be described
and specified in a MIB document
- I have not yet checked sect 7.
- I do not see any words on the relationship betwen teLinkXxxTables
and componetXxxxTables. Would be good to write something about that
- I would suggest that maybe you should replace your 1st figure in sect 8.2
with the figire from the overview document (revision 4) section 11.2. That
seems clearer to me.
- I still have the impression that sect 5.1 (summary) and section 6 are
basically a duplicate of each other. I can live with it... but ???
- sect 8.1 under ifType you refernece [IanaFamily].
I think the reference should be to the ianaiftype.mib
- for my understanding, the figure in sect 8.2, can you explain which layers
in the stack are bundle(s), telink or component link. In fact it might be
good to add that explanantion to the text, so that people can read it too.
- In the CONTACT-INFO, pls add WG mailing list information
- In DESCRIPTION clause of MODULE-IDENTITY..
- add year (2003) to copyright statement
And I think I would change:
This MIB contains managed object definitions for
MPLS traffic engineering links as defined in:
Kompella, K., Rekhter, Y., Berger, L.,
Link Bundling in MPLS Traffic Engineering
Internet Draft <draft-ietf-mpls-bundling-04.txt>,
July 2002."
Into
This MIB module contains managed object definitions for
MPLS traffic engineering links as defined in 'Link Bundling
in MPLS Traffic Engineering'."
- In DESCRIPTION clause of teLinkENtry you still have a (TBD), while
we already know that it is 200, do we not?
Also, I do not understand the last sentence of that DESCRIPTION clause
(I dare to bet there are others who won't understand it either). SO
please clarify in the text
- May I recommend that
teLinkIpAddrType InetAddressType,
teLinkIpAddr InetAddress,
teLinkRemoteIpAddr InetAddress,
Be renamed to:
teLinkAddressType InetAddressType,
teLinkLocalAddress InetAddress,
teLinkRemoteAddress InetAddress,
First of all, there can be an unnumbered addresstype (unknown), so then
it is not an IpAddress.
Second, I think that the first address is the local address (that is on
the device where the instance of the object lives, no?
Further I wonder.. if it is an unnumbered link, it has an identifier somehow
does it not. Why would we not put that identifier in the address in that case
and describe what it looks like. In fact I wonder if instead of the
InetAddress.... TCs would it not be better to use TeHopAddres... TCs from the
MPLS-TC-MIB? It allows for TeHopAddressUnnum as an address
In any event, I would not write:
teLinkIpAddrType OBJECT-TYPE
SYNTAX InetAddressType
MAX-ACCESS read-create
STATUS current
DESCRIPTION
"For IPv4 and IPv6 numbered links, this object represents the
IP address type associated with the TE link. For
unnumbered links, a value of unknown(0) must be used."
But rather something aka:
teLinkAddressType OBJECT-TYPE
SYNTAX InetAddressType
MAX-ACCESS read-create
STATUS current
DESCRIPTION
"The type of Internet address for the TE link. Only IPv4 and
IPv6 and unknown (for unnumbered links) need to be supported."
Then forther in the Address itself I would do something aka (not that
the first sentence is REQUIRED as per RFC3291):
teLinkLocalAddress OBJECT-TYPE
SYNTAX InetAddress
MAX-ACCESS read-create
STATUS current
DESCRIPTION
"The local Internet address for the TE link. The type of this
address is determined by the value of the teLinkAddressType
object. For an unnumbered TE link, the address represents an
unnumbered interface in this format:
octets contents encoding
1-4 unnumbered interface network-byte order
"
I stole some of the text from MPLS-TC-MIB for now
- I see in a lot of places something like:
REFERENCE
"[BUNDLING]"
That is not the proper way to reference in a REFERENCE clause. When a
MIB module gets extracted from the RFC (and that happens a lot) then all
the references to the reference section are lost. So we normally use
something like:
REFERENCE "Link Bundling in MPLS Traffic Engineering, RFC aaaa."
-- RFC Editor to fill in RFC number that will be assigned to [BUNDLING]
Not that this makes for a normative reference to [BUNDLING]
- I see this coming back a few times:
teLinkMuxCapability OBJECT-TYPE
SYNTAX INTEGER {
packetSwitch1(1),
packetSwitch2(2),
packetSwitch3(3),
packetSwitch4(4),
layer2Switch(51),
tdm(100),
lambdaSwitch(150),
fiberSwitch(200)
}
So that is a good candidate for a TC. That would then also allow you to
say a bit more what packetSwitch1, packetSwitch2, etc in fact means.
Any explanation for skipping values? It is recommended (but not required)
that they are contiguous. So explaining in the description why such is not
the case is always wise. It possibly is as simple that these values
are already allocated/specified somehwere else (GMPLS-OSPF doc?) and
reused here.
- Would it make sense to elaborate a bit in the DESCRIPTION clause of
teLinkProtectionType and explain what the values exactly mean?
Or if they are explained in that GMPLS-OSPF doc that you refernce, then
at least make that statement.
- I think all the "priotity" objects have a value from 0-7 (or one that maps
to it). Does that call for a TC, in which you can then also describe a bit
more extensive what it is?
- teLinkIncomingIfId and teLinkOutgoingIfId
I am unclear if these are only for unnumbered links or also for numbered
ones. Can you make that explicit in the DESCRIPTION clauses?
- The places where you use Priority (1..8) that maps to (0..7), I expect that
you did that because they are INDEX objects and INDEX objects should not
include a zero value. However... that is the RECOMMENDED practice. If there
is a good reason to include value zero in the index range then such can
be done, and here it seems that such is justified. (I am assuming here
that the priority is defined in some other doc as being in the range of
0-7. Maybe it is even a field in some other protocol data structure.
- teLinkResourceClass
Any reference to a document that explains how that bit-valued field is
composed/conbtructed/used?
- In all the places/tables where ifIndex is used as an index, could you
specify if a specific ifType is expected (or maybe even required) for
such an interface. I have the impression that for some it is a fixed
value that is acceprtable, for other multiple values are acceptable,
and for some maybe even any value is acceptable. If you specified it,
I think that things would be clearer for me (and hopefully for others
too).
- At all those places where you use Bandwith of 1000 bits per second.
You have it in the DESCRIPTION clause. Better is to put it in a UNITS
clause, so that it is machine readable/parsable.
It is used at many places. Candidate for a TC ??
- I see this multiple times:
teLinkEncodingType OBJECT-TYPE
SYNTAX INTEGER {
packet(1),
ethernet(2),
ansiEtsiPdh(3),
sdhItuSonetAnsi(5),
digitalWrapper(7),
lambda(8),
fiber(9),
fiberChannel(11)
}
Yep... as you guessed: why is it not a TC?
- I see a few of these:
componentLinkPreferredProtection OBJECT-TYPE
SYNTAX INTEGER {
primary(1),
secondary(2)
}
Candidate for a TC I'd say
- I still see in (I believe all of) your RowStatus objects somthing like:
DESCRIPTION
"This variable is used to create, modify, and/or
delete a row in this table. All read-create objects
can only be changed when teLinkRowStatus is active."
-----------
As I said before, This really weird. Probably/Possibly You mean:
All read-create objects can be changed even when xxxRowStatus
is active."
If so, pls fix, if not, pls explain.
You may want to reread first para on page 17 of RFC2579
In any event, when a row is initially created, it cannot be 'active',
it can only become active if all columns have proper values, so one
must be able to change a row when in notReady or notInservice state.
Or when in not-existing state. That is the default. The default is also
that one cannot write (SET) any columns while a row is active, so that
is why a DESCRIPTION clause of a RowStatus object MUST specify which
columns can or cannot be written/changed when the row is active.
Seems you want to allow all columns to be written-to/changed when a row
is active, so my suggested text replacement above caters for that.
- WHen I see a table like this:
teLinkDescriptorTable
TeLinkDescriptorEntry ::= SEQUENCE {
teLinkDescriptorId Unsigned32,
teLinkEncodingType INTEGER,
teLinkDescrPriority Unsigned32,
teLinkMinReservableBandwidth Unsigned32,
teLinkMaxReservableBandwidth Unsigned32,
teLinkDescrRowStatus RowStatus,
teLinkDescrStorageType StorageType
Then I always wonder if we cannot make it easier for people to see which
objects are indeed in that table. The first object name teLinkDescriptorId
clealrly is in this table. Then we have one teLinkEncodingType that is not
so clear. Then we have one that is not as clear as the 1st one, but still
gives some clue. Then we have 2 that give not clue, then we have 2 more
that give some clue. How about naming them:
TeLinkDescriptorEntry ::= SEQUENCE {
teLinkDescriptorId Unsigned32,
teLinkDescrEncodingType INTEGER,
teLinkDescrPriority Unsigned32,
teLinkDescrMinReservableBandwidth Unsigned32,
teLinkDescrMaxReservableBandwidth Unsigned32,
teLinkDescrRowStatus RowStatus,
teLinkDescrStorageType StorageType
If you can agree with that, then pls check you other tables. The same
issue/concern comes back in a few other tables.
In fact our mib guidelines document does discuss this issue.
- Your teLinkNotifEnable would be better named: teLinkNotificationsEnabled
- I worry about the number of notifications that can get generated per minute
or per second? Can you say something about it? Do we need an abject that
lets an NMS configure how many notification an agent can send at most per
minute?
- linkBundleMismatch
I do not understand the DESCRIPTION clause at all. Specifically, if an error
is found and the notification is sent, then the 1 second later, the same
situation probably still exists. Is another notification sent?
Should such a problem not be detected when someone creates an antry in
a table and should create operation not be prevented from succeeding?
- I do not understand the difference between your ...FullCOmpliance and
...MonCompliance (by the way, in other MIB modules we have named the
latter one ...ReadOnlyCompliance). I would think that the ...FullCOmpliance
one should NOT allow for read-only at all. How can you otherwise claim that
it is for monitoring AND configuring.
- I also have trouble with:
OBJECT teLinkIpAddrType
SYNTAX INTEGER { unknown(0), ipv4(1), ipv6(2) }
MIN-ACCESS read-only
DESCRIPTION
"The dns(16) address type need not be supported.
The ipv4(1) and ipv6(2) address types need not be
supported if numbered links are not supported. The
unknown(0) address type need not be supported if
unnumbered links are not supported."
I would rather do something like:
OBJECT teLinkIpAddrType
SYNTAX INTEGER { unknown(0), ipv4(1), ipv6(2) }
MIN-ACCESS read-only
DESCRIPTION
"Only ipv4(1) and ipv6(2) address types need to be
supported for numbered links. For unnumbered links
unknown (0) needs to be supported."
In the future other types could be added to InetAddressType TC, and
so you onbly want to list here what needs to be supported, not
(in explicit form) what needs not be supported.
I wonder if you do not need ipv4z and/or ipv6z. As long as you have
evaluated this (with the WG) and decide that you do not need it, then
fine.
- I have trouble with:
OBJECT teLinkRowStatus
SYNTAX INTEGER { active(1), notInService(2),
createAndGo(4), destroy(6) }
DESCRIPTION
"The notReady(3) state need not be supported."
I think what you want to specify is that createAndWait is not needed.
Since you allow all objects to be changed in 'active' mode, the
notInService may not be needed either. (I am assuming here that I
did understand your intent... which I am not sure of). In that case
something like the following example from RFC3289 would be better:
OBJECT diffServClfrStatus
SYNTAX RowStatus { active(1) }
WRITE-SYNTAX RowStatus { createAndGo(4), destroy(6) }
DESCRIPTION
"Support for createAndWait and notInService is not required."
- I have trouble with:
OBJECT teLinkStorageType
SYNTAX INTEGER { other(1) }
DESCRIPTION
"Only other(1) needs to be supported."
I think I could live with a volatile only, but the 'other' value
is completely unspecified and seems to make no sense to me at all.
What is you intention here?
- In security considerations...
- Do All tables have the same considerations? If so you may want
to make that explicit.
- In general, our security ADs probably want some more specificity.
A statement like "all tables ....." sounds as if you are opting for
an easy out.
- On th eread-only objects... are there no issues with bandwith info?
Or priorities? On protection?
would that not reveal info that people don't want to loose?
- Sect 13.1
Not sure whre [Assigned] is referenced or if it is needed. is it?
Please note that you must make normative references to all RFCs from
whihc you IMPORT any objects or TCs or such.
- Sect 13.2
You seem to have kept a lot of old MIB boilerplate references that seem
no longer needed,.
Thanks,
Bert
|
|