[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: MIB review of <draft-glenn-mo-aggr-mib-05.txt>



Hi,
     Apologies for multiple copies of this mail. It seems that some
recipients had problems receiving the earlier mail. I am resending
it. This time there is no attachment. The entire text is in the mail
body itself. That may help.

     Glenn M Keeni

Bert/Glenn Waters

Wijnen, Bert (Bert) wrote:


>>>> Thanks for the review Glenn Waters.
>>>>
>>>> Author (teh other Glenn) and RFC-Editor, we hope
>>>> this helps to improve the quality of the document.
>>>>


Thanks for the review. It sure has improved the quality of the document.
I have submitted a revised I-D
	Title		: The Managed Object Aggregation MIB
	Author(s)	: G. Keeni
	Filename	: draft-glenn-mo-aggr-mib-06.txt
	Pages		: 37
	Date		: 2005-9-19

I have addressed all the points raised. There is agreement on most
of the points. On some points there was lack of clarity, I hope that
I am clear enough this time. The list of the changes is in the appendix
of the document. The response to the review comments are attached.

Thanks and Cheers

     Glenn








The following gives the gist of the changes to the document. The comments
by the reviewers are numbered "#n", the relevant part of the document is
excerpted followed by the comment prefixed by "gww>" or "bert>".
The response follows after a blank line and is prefixed by "gmk>".

#1   ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
     Polling for an instance (TAgMOIx) of a time-based aggregate MO
     (TAgMOx):
     TAgMOx = aggr{'n' polled samples of an instance (MOI) of MO
                    at intervals       = 'i' microseconds}

               +--------+---------+-------+
        Query: |Get req | TAgMOIx | NULL  |
               +--------+---------+-------+


               +--------+---------+--------------------------------------+
     Response: |Get resp| TAgMOIx | t,Val(t),Val(t+i),.,Val(t + (n-1)*i) |
               +--------+---------+--------------------------------------+

   gww> where do you distinguish between MOI1 through MOIn

   gmk> Done. In the case of Time-based aggregation there is only one MO
   gmk> instance that is sampled at regular intervals. The figure was not
   gmk> clear enough. The figure has been updated.

#2 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

         CompressedAggrMOValue ::= TEXTUAL-CONVENTION
   bert> For naming consistency this should be named some like AggrCompressedMOValue
   bert> or AggrMOCompressedValue.

   gmk>  Done.
#3 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
          AggrCtlEntry ::= SEQUENCE {
             aggrEntryID
                           SnmpAdminString,
             aggrAgMOIndex
                           Unsigned32,
             aggrAgMODescr
                           SnmpAdminString,
             aggrCompressionAlgorithm
                           INTEGER,
             aggrEntryOwner
                           OwnerString,
             aggrEntryStorageType
                           StorageType,
             aggrEntryStatus
                           RowStatus
          }
   bert> For naming consistency all objects for this table should
   bert> start with aggrCtl

   gmk> Done. Changed prefix to aggrCtl
#4 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         aggrAgMOIndex OBJECT-TYPE
              SYNTAX Unsigned32 (1..2147483647)
              MAX-ACCESS read-create
              STATUS current
              DESCRIPTION
                " A pointer to a group of MOs identified by aggrMOEntryID
                  in the aggrMOTable. This is the group of MOs that will
                  be aggregated."
              ::= { aggrCtlEntry 2 }
   gww> Hmmmm. The indexing in the aggrMOTable is { aggrMOEntryID, aggrMOEntryMOID }
   gww> which are both defined as 0..65535. Interesting way to "index" another table.
   gww> I think I understand what is going on... the first two bytes of aggrAgMOIndex
   gww> refer to aggrMOEntryID and the second two bytes refer to aggrMOEntryMOID. If
   gww> this is indeed the case there should be some text describing this. I would
   gww> say this is unconventional and would strongly recommend that two objects
   gww> be defined in this table to break out the indices.

   gmk> The explanation was not clear:
   gmk>
   gmk> The MO instance in the aggrMOTable is
   gmk>          aggrMOInstance. <aggrMOEntryID>.<aggrMOEntryMOID>
   gmk>                                X               X
   gmk>                                |               |
   gmk>                                V               |
   gmk>        +-----------------------+               |
   gmk>        | Defines the group of  |               |
   gmk>        | MOs. Will have the    |               |
   gmk>        | same value as         |               |
   gmk>        | aggrCtlAgMOIndex in   |               |
   gmk>        | the corresponding row |               |
   gmk>        | in aggrCtlTable       |               |
   gmk>        +-----------------------+               V
   gmk>                     +--------------------------+
   gmk>                     | Identifies an MO in the  |
   gmk>                     | the group of MOs defined |
   gmk>                     | by <aggrMOEntryID>       |
   gmk>                     +--------------------------+
   gmk>
   gmk> Does that help?
#5 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

         aggrEntryStorageType OBJECT-TYPE
              SYNTAX StorageType
              MAX-ACCESS read-create
              STATUS current
              DESCRIPTION
                 "This object defines whether the parameters defined in
                  this row are kept in volatile storage and lost upon
                  reboot or are backed up by non-volatile (permanent)
                  storage.
   gww> aggrCtlEntry has a comment that says entries "are required to survive
   gww> reboot". This description implies otherwise. This needs to be fixed.

   gmk> The description of aggrCtlEntry  says
   gmk>          "....
   gmk>            Entries in this table are required to survive a reboot
   gmk>            of the managed entity depending on the value of the
   gmk>            corresponding aggrCtlEntryStorageType instance
   gmk>           ...
   gmk>          "
   gmk> Does that clear the conflict ? Or, have I missed something ?
#6 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

      -- The Table of primary(simple) MOs
      -- Each row in this table represents a MO which will be aggregated.
      -- The aggrMOEntryID index is used to identify the group of MOs
      -- that will be aggregated. The aggrMOIndex instance in the
      -- corresponding row of the aggrCtlTable will have value equal to
      -- the value of aggrMOEntryID.
      -- The aggrMOEntryMOID index is used to identify an MO in the group.
      --
   bert> Move these comments into description clause for the table (and edit
   bert> appropriately.

   gmk> Done.
#7 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         aggrMOEntryID OBJECT-TYPE
              SYNTAX Unsigned32 (0..65535)
   bert> Index values normally start at 1. Only in rare cases should they start at 0.

   gmk> Corrected.
#8 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         aggrMOEntryMOID OBJECT-TYPE
              SYNTAX Unsigned32 (0..65535)
   bert> Index values normally start at 1. Only in rare cases should they start at 0.

   gmk> Corrected.
#9 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         AggrDataEntry ::= SEQUENCE {
            aggrDataRec
   gww> Recommend aggrDataRecord

   gmk> Done
#10++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
            aggrDataRecC
   bert> Recommend aggrDataRecordCompressed

   gmk> Done
#11++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
                       CompressedAggrMOValue,
            aggrErrorRec
   bert> Recommend aggrDataErrorRecord

   gmk> Done
#11++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         aggrDataRec OBJECT-TYPE
              SYNTAX AggrMOValue
              MAX-ACCESS read-only
              STATUS current
              DESCRIPTION
                "The snapshot value of the aggregated MO.
                 Note that the access privileges to this object will be
                 governed by the access privileges of the component
                 objects. Thus, an entity attempting to access an instance
                 of this MO MUST have access rights to all the component
                 instance objects and this MO instance.
                "
   gww> Hmmm. Does this mean that whenever this object is retrieved then the
   gww> access to each MO will be checked? So, this is "legal" design, however,
   gww> I cannot imagine many implementations actually performing this check.
   gww> If indeed they do not check the access rights then we have broken the
   gww> security model. I would recommend that this is clearly discussed in the
   gww> security section below.

   gmk> Done. Added a paragraph to the Security Considerations section
#12++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

         aggrDataRecC OBJECT-TYPE
              SYNTAX CompressedAggrMOValue
              MAX-ACCESS read-only
              STATUS current
              DESCRIPTION
                "The compressed value of the aggregated MO.
                 The compression algorithm will depend on the
                 aggrCompressionAlgorithm given in the corresponding
                 aggrCtlEntry. In case the value of the corresponding
                 aggrCompressionAlgorithm is (1) 'none' then this MO
                 will be inaccessible.
   gww> inaccessible is not good. I would recommend that this object is
   gww> accessible but empty. Since no compression algorithms are defined I
   gww> would strongly recommend that any references to compression are removed
   gww> and issue a new version of this MIB module when compression algorithm(s)
   gww> are required.

   gmk> Done. I have added a compression algorithm. Compression is important.
#13++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

          -- Compliance statements
         aggrDataCompliance MODULE-COMPLIANCE
   bert> Prefer a name such as aggrMibCompliance

   gmk> Done.
#14++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         aggrDataGroup    OBJECT-GROUP
   bert> Prefer a name such as aggrMibBasicGroup

   gmk> Done.
#15++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         TAggrMOErrorStatus ::= TEXTUAL-CONVENTION
           STATUS       current
           DESCRIPTION
             "This data type is used to model the error status of the
              sampled MO instance. The error status for a sampled MO
              instance is given in terms of two elements -
                o the moIndex which indicates the sample number of the MO
                  instance (starting at 1) in the value of the time
                  aggregated MO instance.
                o the moError which indicates the error that was
                  encountered in sampling that MO instance.
              The syntax in ASN.1 Notation will be
              ErrorStatus :: = SEQUENCE {
                 moIndex  Integer32,
                 moError  SnmpPduErrorStatus
              }
              TAggrMOErrorStatus ::= SEQUENCE OF {
                 ErrorStatus
              }
              Note1: the command responder will supply values for all
                     the samples of the MO instance. If an error is
                     encountered for a sample then the corresponding
                     value will have an ASN.1 value NULL and, an error
                     will be flagged in the corresponding
                     TAggrMOErrorStatus object.
                     Only MOs for which errors have been encountered will
                     have the corresponding moIndex and moError values set.
              Note2: the error code for the component MO instances will be
                     in accordance with the SnmpPduErrorStatus TC defined
                     in the DISMAN-SCHEDULE-MIB[RFC3231].
             "
           SYNTAX      Opaque (SIZE (0..1024))
   gww> Any reason to define this again. This looks just like the AggrMOErrorStatus,
   gww> recommend that AggrMOErrorStatus is reused.

   gmk> Yes these actually define different conventions. These are very similar but
   gmk> with some basic differences. To try to use the same TC for both would make
   gmk> the description clumsy.
#16++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

         TimeAggrMOValue ::= TEXTUAL-CONVENTION
           STATUS       current
           DESCRIPTION
             "This data type is used to model the time aggregated MOs. It
              will be a sequence of values. The syntax in ASN.1 Notation
              will be
              MOValue :: = SEQUENCE {
                   value ObjectSyntax
              }
              TimeAggrMOValue ::= SEQUENCE OF {
                   MOSampleValue
              }
              Where, the first MOSampleValue will always be the timestamp
              of the first sample in the aggregated object. The subsequent
              values are the values of the MO instance sampled at the
              specified intervals for the specified number of times.
              Note: the command generator will need to know the
                    constituent MO instance and the sampling interval to
                    correctly interpret TimeAggrMOValue.
             "
           SYNTAX      Opaque (SIZE (0..1024))

   gww> I know this is just in a description clause but I don't see MOSampleValue
   gww> defined anywhere in this document. Should be fixed.

   gmk> Done.   s/MOValue/MOSampleValue/g
#17++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
   --
   -- The time-based aggregation control table
   -- There will be a row for each TAgMO
   --
   gww> Move these comments into description clause for the table (and edit
   gww> appropriately.

   gmk> Done.
#18++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
          TAggrCtlEntry ::= SEQUENCE {
             tAggrEntryID
                           SnmpAdminString,
             tAggrMOInstance
                           OBJECT IDENTIFIER,
             tAggrAgMODescr
                           SnmpAdminString,
             tAggrInterval
                           Gauge32,
             tAggrSamples
                           Gauge32,
             tAggrCompressionAlgorithm
                           INTEGER,
             tAggrEntryOwner
                           OwnerString,
             tAggrEntryStorageType
                           StorageType,
             tAggrEntryStatus
                           RowStatus
          }

   gww> All entries in this table should start with tAggrCtl

   gmk> Done. Changed the prefix from tAggr to tAggrCtl
#19++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         tAggrInterval OBJECT-TYPE
              SYNTAX Gauge32
   gww> This should be an Integer32

   gmk> Done.
#20++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         tAggrSamples OBJECT-TYPE
              SYNTAX Gauge32
   gww> This should be an Integer32

   gmk> Done.
#21++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         --
         -- tAggrDataTable: The Table of Data. Each row represents a Data
         --                 set. tAggrEntryID is the key to the table. It
         --                 is used to identify instances of the
         --                 TAgMO that are present in the table.
         --
   gww> Move these comments into description clause for the table (and edit
   gww> appropriately.

   gmk> Done.
#22++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         TAggrDataEntry ::= SEQUENCE {
            tAggrDataRec
                       TimeAggrMOValue,
            tAggrDataRecC
                       CompressedTimeAggrMOValue,
            tAggrErrorRec
                       TAggrMOErrorStatus
            }
   gww> See comments about aggrDataEntry and rename these columns similarly.

   gmk> Done.
#23++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

          -- Compliance statements
         tAggrDataCompliance MODULE-COMPLIANCE
   gww> Rename to tAggrMibCompliance

   gmk> Done.
#24++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
              MODULE  -- this module
                  MANDATORY-GROUPS { tAggrDataGroup }
              ::= { tAggrCompliances 1 }

          -- Units of conformance
         tAggrDataGroup    OBJECT-GROUP
   gww> Rename to tAggrMibBasicGroup

   gmk> Done.
#25++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

      It is RECOMMENDED that implementers consider the security features as
      provided by the SNMPv3 framework (see [RFC3410], section 8),
   gww> editorial: should be section 9

   gmk> The template on http://www.ops.ietf.org/mib-security.html states
   gmk>    It is RECOMMENDED that implementers consider the security features as
   gmk>    provided by the SNMPv3 framework (see [RFC3410], section 8),
   gmk>    .....
   gmk> I looked at rfc3410. Section 8 is relevant particularly the last few
   gmk> paragraphs of section 8.2
   gmk> Am I missing something ?
#26++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++


   8.  IANA Considerations

      IANA should assign a base arc each in the 'experimental' OID tree for
      the 'aggrMIB' MODULE-IDENTITY and the 'tAggrMIB' MODULE-IDENTITY
      defined in the AGGREGATE-MIB and the TIME-AGGREGATE-MIB respectively.
   gww> Should have an editor's note here since the above text can be removed
   gww> prior to publication. See mib review document section 3.5.3

   gmk> Done.
#27++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

   9.  References

   9.1 Normative References

   [RFC3231]   Levi. D. and Schoenwaelder, J., "Definitions of Managed
               Objects for Scheduling Management Operations", RFC3231,
               January 2002
   gww> Missing reference to RMON-MIB
   gww> Missing reference to RFC3411

   gmk> Done. References added.