Subject: [Ips] FW: Initial review of http://www.ietf.org/internet-drafts/draft-i etf-ips-fcmgmt-mib-04.txt

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Wed, 4 Feb 2004 11:33:19 +0100

Thanks Mike for the first MIB Doctor review.

authors/wg members,
Pls copy Mike on any responses, cause I suspect he is not
subscribed to IPS mailinglist (Mike tell us if you are,
in which case people do not specifically have to copy you).

Thanks,
Bert 

-----Original Message-----
From: Michael MacFaden [mailto:[email-masked]]
Sent: woensdag 4 februari 2004 7:02
To: Wijnen, Bert (Bert)
Cc: Anurag Maunder
Subject: Initial review of
http://www.ietf.org/internet-drafts/draft-ietf-ips-fcmgmt-mib-04.txt

Hi Bert,

My review of the fiber channel management MIB module:

http://www.macfaden.com/ietf/fc-mgmt-mib-4-reviewed.txt

Regards,
Mike

_______________________________________________
Ips mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/ips

---

From: Keith McCloghrie <[email-masked]>
Date: Fri, 6 Feb 2004 14:54:31 -0800 (PST)

Bert/Mike,

Thanks for the review.

Below, please find :

a) my respones to the comments, and
b) the changes that I plan to make as a result of the review.

Thanks,
Keith.

> http://www.macfaden.com/ietf/fc-mgmt-mib-4-reviewed.txt

------------------------------------------------------------------------------

> Feb 3, 2004 MIB DOCTOR Review: http://www.ietf.org/internet-drafts/draft-ietf-ips-fcmgmt-mib-04.txt
> 
> Performed by Michael R. MacFaden, Kazeon Systems, Inc
> according to draft-ietf-ops-mib-review-guidelines-02.txt
> 
> A. Documentation Guidelines 
>    3.1 o MIB boilerplate section       -> present
>            current version?            -> YES
>    o Narrative sections                -> present
>            Scope described?            -> YES
>            Spelling?                   -> No obvious mistakes 
>            Module structure described? -> YES
>            Dependent on other modules? -> YES
>              If so, which ones?        -> IF-MIB
>            Relates to other modules?   -> YES 
>              If so, which ones?        -> HOST-RESOURCES-MIB, ENTITY-MIB, 
>            References ([XXX])?         -> Yes, they are correct and resolve properly
> 
>            Reviewer Comments:
>            1. "high performance capability" isn't further defined so first 
>               sentence is so much fluff.

This sentence which appears in the first sentence of the "Short Intro to
Fibre Channel" section, was copied from the Introduction section of
Fibre Channel standards documents.  The intent is to give a high-level
indication of what Fibre Channel is aimed at, and to phrase it so as to
remain valid in the future as and when the specific bandwidth supported
by Fibre Channel is higher than it is today.  It is too early in the
into to be definitive.  Thus, it isn't "fluff".

>            2. Text describing the MIB module's structure (Sect 4.1-11) is better than the GROUP DESCRIPTION
>               clauses in the compliance section.
>            3. IF-MIB description was superbly done.
>    o Definitions section present        -> present
>        Written to SMIv2 standards?   -> YES
>        Issues with compilation?      -> YES
>    o Intellectual Property section                   -> present
>           Matches current boilerplate?               -> YES
>           Any proprietary rights claimed?            -> NO
>    o References section                              -> present
>       Matches current boilerplate (2578-80,3410)?    -> YES 
>       Normative/informative sections separate?       -> YES
>       MIB modules listed in IMPORTS included?        -> 
>    o Security section                                -> present
>        Matches current boilerplate?                  -> YES
>        Specific Read/write objects listed?           -> YES
>        Security risk spelled out?                    -> YES
>        Specific Read only objects listed?       
> 
>       Reviewer Comments:
>          There was only one specific security risk described. More could be "spelled out."
>          For instance, setting fcmSwitchDomainId to an invalid value could lead to 
>          denial of service in some configurations.
             
No matter how much one writes in this section, it is always true that 
"more could be spelled out".

>    o IANA Considerations  section                   -> not present
>        Was a namespace defined?                     -> NO
>           
>    o Copyright Notices section                      -> present
>        Correct year specified?                      -> NO
>        Copyright found in MODULE-IDENTITY?          -> NO
      
Noted.
 
>    o Definitions section detail 
> 
>    Here are the diagnostics from certain mib syntax checkers:
> 
> Diagnostics  from Version 0.4.2 smilint -l9 
> C:\local\smi\mibs\ietf>smilint -m -s -l 9 ./FC-MGMT-MIB
> ./FC-MGMT-MIB:2213: [5] {group-unref} warning: current group `fcmLinkBasicGroup' is not referenced in this module

OK, I will add a GROUP clause to "make it clear that the optional status
is intended".

> ./FC-MGMT-MIB:62: [5] {type-without-format} warning: type `DomainIdOrZero' has no format specification
> ./FC-MGMT-MIB:94: [5] {type-without-format} warning: type `FcBbCredit' has no format specification
> ./FC-MGMT-MIB:106: [5] {type-without-format} warning: type `FcDataFieldSize' has no format specification
> ./FC-MGMT-MIB:171: [5] {type-without-format} warning: type `PhysicalIndexOrZero' has no format specification
> ./FC-MGMT-MIB:181: [5] {type-without-format} warning: type `HrSWInstalledIndexOrZero' has no format specification
> ./FC-MGMT-MIB:192: [5] {type-without-format} warning: type `MilliSeconds' has no format specification
> ./FC-MGMT-MIB:198: [5] {type-without-format} warning: type `MicroSeconds' has no format specification

It seems to me like the compiler has a bug here, since all of these are, or
have a TC as their syntax and for all but two of them, the TC is defined 
with a range, e.g., DomainIdOrZero is defined as:

  DomainIdOrZero ::= TEXTUAL-CONVENTION
      STATUS current
      DESCRIPTION
            "The Domain Id (of a FC switch), or zero if the no Domain Id
            has been assigned."
      SYNTAX  Integer32 (0..239)

The two exceptions are MilliSeconds and MicroSeconds which are both
Unsigned32 and the inherent (0..4294967295) is correct for both.
 
> Diagnostics from MIB Smithy 2.4.2
> 
> [FC-MGMT-MIB:fcmgmtCompliance:FC-MGMT-MIB:fcmInstanceSoftwareIndex] : warning : SYNTAX in variation and OBJECT-TYPE differ in type names 'Integer32' and 'HrSWInstalledIndexOrZero'.
> [FC-MGMT-MIB:fcmgmtCompliance:FC-MGMT-MIB:fcmInstancePhysicalIndex] : warning : SYNTAX in variation and OBJECT-TYPE differ in type names 'Integer32' and 'PhysicalIndexOrZero'.

Yes, but both HrSWInstalledIndexOrZero and PhysicalIndexOrZero are TCs
which have Integer32 as their underlying syntax.

> [FC-MGMT-MIB:fcmLinkEnd2UnitType] : warning : BITS named-number list values should start at zero.
> [FC-MGMT-MIB:fcmFLoginClassesAgreed] : warning : BITS named-number list values should start at zero.

Another bug in the compiler.  Both objects have a TC as their syntax where
the TC has a named-number list which does start at zero.

> [FC-MGMT-MIB:fcmISPortEntry] : warning : All INDEX objects in this table are auxiliary (i.e., from other tables).
> [FC-MGMT-MIB:fcmFxPortEntry] : warning : All INDEX objects in this table are auxiliary (i.e., from other tables).

Eh?  The compiler has another bug, since:

a) auxiliary does NOT mean "i.e., from other tables", rather RFC 2758
section 7.7 defines "auxiliary" as follows:

   Objects which are both specified in the INDEX clause of a conceptual
   row and also columnar objects of the same conceptual row are termed
   auxiliary objects. 

Thus, the error message as stated is actually wrong.

b) It is common practice for media-specific MIBs which extend the
IF-MIB to define a table which is INDEX-ed only by ifIndex.

> [FC-MGMT-MIB:fcmPortFcOperClass] : warning : BITS named-number list values should start at zero.
> [FC-MGMT-MIB:fcmPortFcCapClass] : warning : BITS named-number list values should start at zero.
> [FC-MGMT-MIB:fcmPortEntry] : warning : All INDEX objects in this table are auxiliary (i.e., from other tables).
> [FC-MGMT-MIB:fcmInstanceFunctions] : warning : BITS named-number list values should start at zero.
> Validation complete - 0 error(s), 10 warning(s)
 
See above.

>   Reviewer Comments:  
>    1. The TC's could possibly go into a separate TC document for fiber channel: FcNameIdOrZero, DomainIdOrZero, FcAddressId
>       as it is mentioned there might be multiple Fiber channel mib modules in the future.

While your "could" and "possibly" are correct, but we have already
written several of these follow-on FC MIB modules, and they are
IMPORT-ing TCs from this MIB without any problems.  So, this would be a
disruptive change for no benefit.

>    2. FcAddressId doesn't have an *OrZero version, which might be needed at a later date.

It's syntax is "OCTET STRING (SIZE(0 | 3))" and the zero-length value
is already used (see fcmLinkEnd2FcAddressId).

>    3. FcPortType enumeration "dynamic(3),  -- determined dynamically"  seems redundant, prefer comment point to underlying spec

No, it's not redundant because "dynamic" could also mean that the value
is constantly changing, and saying "determined dynamically" removes that
possibility.

>    4. PhysicalIndexOrZero, HrSWInstalledIndexOrZero are very useful and generally applicable to many mib modules.
>       They look strangly wrong being defined here and should be defined in another TC, like Entity-MIB, or some common TC-MIB.

But they aren't, and thus, the best that this MIB module can do is to
define the TC's itself.

>    5. MicroSeconds, MilliSeconds should also be in some generic tc-mib...

But they aren't, and thus, the best that this MIB module can do is to
define the TC's itself.

>    6. fcmInstanceIndex DESCRIPTION is problematic for me. Either it stays static or it doesn't. 
>       Having something "desireable" is worthless to those writing mangement applications, they have to assume worst case always.

Not worthless, because if agent implementors heed the advice, then mgmt
apps can coded to check whether the index values have been retained
over the last restart, and only if not, do they need to re-synchronize
their database.  That is, it is an optimization for the common case of
index values being retained, but the fallback still works in the case 
(hopefully less common) when all knowledge of index values is lost.

>       There are problems with the description. Sentences start without capitalization.

"Sentences" ??   I see only one such sentence-break, for which the fix
is to delete the the period before "but" (because it's bad grammar to
start a sentence with "But".)

>       fcmInstanceTable has very critical data. Would like to have a LastChange entry per row to know when values were changed or
>         a "configuration change" notification if lastChange object is too much overhead.

But the data is not, in general, both critial and dynamic.  Specifically,
the only things in this table that change are: 
- the FabricId if it's based on the WWN of the Principal switch;
    -- a notification of a Principal switch change is being proposed as
       an extension to this MIB.
- the read-write objects (but only when an app changes them);
    -- changes in these values are not critical to the operation of the
       network, and the synchronization of mgmt apps should not have to
       rely on notifications from agents for such non-critical data.
- the Status object,
    -- to monitor this object for changes, set an RMON threshold on it.
       Note that the typical situation is to have just one instance;
       even if there are multiple instances, there will not be many.
       Having an RMON threshold will be more effective than having a
       LastChange object.

>    7. fcmInstanceWwn, but does it matter which WWN is selected? Will managment stations expect one vendor implemetnation over another?

The ability to have multiple instances is defined as a flexibility to
allow for many/any different choices by vendors.  Vendors are free to
choose to have one, or more, instances using whatever methodology they wish.
Thus, we cannot prescribe how the WWN is selected.

This flexibility is necessary because otherwise there is a genuine risk
that a vendor will implement a set of Fibre Channel interfaces in a manner
which cannot be represented by this MIB.

>       Wouldn't it be the same as fcmSwitchDomainId if fcmSwitchPrincipal is True?

It could be but it needn't be, and the circumstance you ask about is a
corner case which for example never applies to HBAs.

>    8. fcmInstanceStatus could use an associated object to get timestamp of the last state change

While implementation feedback might tell us that, I think it's too
soon to be sure.  So, let's save this comment until we update the MIB from
Proposed to Draft Standard.

>    9. fcmInstanceStatus doesn't seem to be implementable in a consistent way across vendors. Needs better definition
>     ideally this state is defined in the underlying technology specifications.

On the contrary, a) there is no one underlying technology, and b) the
underlying technology specifications are the subject of future MIBs,
not this one.  This object serves as a generic summary of the underlying
specifics (e.g., somewhat like ifOperStatus does for an interface.)

>    10. fcmInstanceTextName/fcmInstanceDescr, are there any DEFAULT values for these? Suspect vendor implementations will
>       provide something. Better to define the intended purpose of these objects better.
>             "A textual name for this management instance and the Fibre
>             Channel entity/entities that it is managing."
>       might be 
>             "A textual name for this management instance and the Fibre
>             Channel entity/entities that it is managing set by the operator or to a default string "XXXX" by the device."

"XXXX" can not be specified in this MIB without restricting the necessary
flexibility, anything that could be specified in this MIB would not be
specific enough.  The whole idea of this object is to be the name specified
by the operator/NM-app, to have a default value changes the semantics.

>    11. fcmPortAdminType - Assuming a valid value change can't be done for some device resource specific case, how can
>                           the error be reported? A generic snmp error to the set is necessary but not sufficient I think.

Why would it not be sufficient ??

>    12. Not sure why these object's syntax wasn't a TC? They will probably be added to in time as well.
>             fcmPortCapProtocols
>             fcmPortTransmitterType 
>             fcmPortConnectorType -- this should probably follow the MAU-MIB convention or we need to standardize 'JackType'
>                                     in a new TC mib as there is now overlap...

Actually, not for the latter two because they are not generic definitions.
Rather they are the specific enumerations listed for the function in
T11's GS-3 speciofication.  Sicne a future revision of the GS-3 spec
could revise them individually, they cannot be defined as common TCs.
The first one could, but I don't see any rule that it violates which
would justify a change at this time.

>    13. fcmPortStatsTable doesn't have an explicitly specified discontinuity object -- is ifXTable entry the one that applies?

Yes.

>                Also, the counters described here should have REFERNCE clauses to counters defined in the underlying technology 
>                specs. While the descriptions are generally good, they do not cover all possible cases when 
>                one should be incremented or not -- that leads to incompatible implementations. 

Many of them are not specifically defined in any Fibre Channel
specification.  If any are ambiguous, please suggest alternative
wording.  Otherwise, I suggest we wait for implementation feedback.
                
>    14. fcmPortLcStatsTable doesn't have split counters?  Is a SNMPv1 agent then really useful when running high speed interfaces?

No, but at the time (several years ago now) when this was discussed
in the WG, many Storage vendors only had SNMPv1 agents, and as a
result, this table was defined at the explicit direction of the WG.

>             DESCRIPTION
>             "A list of Counter32-based statistics which are a shadow of
>             the Counter64 statistics in the fcmPortStatsTable, for
>             systems which do not support Counter64."
>       I suggest the text mention it should "shadow" the lower 32 bits?
 
If that isn't what "shadow" means, then I don't know what it does mean,
and we shouldn't use the term at all.

>    15. If errors are not so common an occurance across instances, the fcmPortErrorsTable might benefit from a *LastChange object
>        so not all objects have to be polled to determine any error state change. Alternately provide a summary counter that
>        is a simple sum of the number of errors. That should allow for better scaling of this table for mgmt polling using SNMP/UDP.

The summary counter for many of these is already available: ifInErrors.
       
>    16. Error table should have REFERENCE clauses to point out where in the underlying spec these counters are defined. 
>        This appears to be done, but is inconsistent. fcmPortLinkFailures has a REFERENCE but fcmPortLinkResets wasn't?
 
As I recall, it is consistent; specifically, all those for which
there is an appropriate section to reference have a REFERENCE clause.

>    17. fcmFxPortTable needs a discontinutity object. if fcmPortOperType is changed from Fx to some other then back within
>        a given polling interval, the data collected from this table will be incorrect.

No, because there are no Counter-based objects in this table.

>    18. Objects in fcmFxPortTable, fcmFLoginTable could use REFERENCE clauses.

Sigh !!  I will look again to see what I can find.

>    19. fcmFLoginNxPortIndex doesn't specify what behaviors should be across SNMP agent reset or system reboots like
>        fcmInstanceIndex is required to do. So I'm a bit concerned what the indexing looks like for all the tables on
>        system reset or agent reset.

I will add a sentence saying that after a value of fcmFLoginNxPortIndex
is assigned to a particular Nx_Port, it can be re-used when and only
when it is assigned to the same Nx_Port, or, after a reset of the value
of the relevant instance of ifCounterDiscontinuityTime.

>    20. fcmLinkEnd2AgentAddress OBJECT-TYPE is  SYNTAX      SnmpAdminString, not InetAddressType? Why?

Because the value is received from the otrher end of the link, and if the
other end supplies a bogus value,m then that bpgus value needs to be stored
in the MIB.

>    21. 
> End of Report

------------------------------------------------------------------------------

- fix the Copyright dates, and insert a Copyright statement in
  the MODULE-IDENTITY.

- add
       GROUP fcmLinkBasicGroup
       DESCRIPTION
            "This group is optional."

- delete the period in  "... across restarts.  (but note, it is ..."

- add an explicit statement that discontinuities in the counters in
  the fcmPortStatsTable can only occur when the value of the relevant
  isntance of ifCounterDiscontinuityTime is zero.

- look again to see what references I can find to put in REFERENCE clauses
  of objects in fcmFxPortTable, fcmFLoginTable.

- add a sentence saying that after a value of fcmFLoginNxPortIndex
  is assigned to a particular Nx_Port, it can be re-used when and only
  when it is assigned to the same Nx_Port, or, after a reset of the value
  of the relevant instance of ifCounterDiscontinuityTime.