Re: [IPFIX] WGLC for draft-ietf-ipfix-mib-variable-export-05
Benoit Claise <[email protected]> Fri, 25 Apr 2014 15:34:44 +0200
| Newsgroups | gmane.ietf.ipfix |
|---|---|
| Message-ID | <[email protected]> |
Dear all,
Some feedback on the sections I already reviewed.
Note that I agree with Juergen's feedback.
> On Tue, Apr 15, 2014 at 12:01:11PM +1200, Nevil Brownlee wrote:
>> Hi all:
>>
>> Sorry to be a little slow with this - the dust has settled from IETF-89,
>> and I've almost managed to catch up.
>>
>> The WG Last Call for our MIB Variable Export draft starts now,
>> and will end on Wednesday, 30 April.
>>
>> Please read it, and - better still - post reviews of it to the IPFIX list!
>>
> I have reviewed the document and I believe it is not ready. The main
> points:
>
> - Terminology needs to be aligned with SMIv2
> - I spotted several errors in the examples
> - Major issues with the IE definitions in section 10
> - I also believe the introductionary text needs to improve.
>
> /js
>
> I have reviewed draft-ietf-ipfix-mib-export-05 and I think it is not
> ready and requires improvements.
>
> 1) The introduction contains details of the solution. I think it
> should instead contain the motivation and the architectural model
> currently in section 2. The new introduction should then be
> followed by a terminology section before an overview of the
> solution is provided (so that the terminology is defined). This is
> primarily text reorganization.
The following text could be cut/pasted from the Introduction to a new
section "High Level Solution Overview", somewhere after the terminology
section:
This document specifies a method for creating IPFIX Option Templates
that are used to export the extra data required to describe MIB
variables (seeSection 4.1 <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.1>).
This allows IPFIX Templates to contain any combination of fields
defined by traditional IPFIX Information Element(s) and/or MIB Object
Identifier(s). The MIB Object Identifiers can reference either non-
indexed or indexed MIB object(s). Enterprise-specific MIB Object
Identifiers are also supported.
This document also defines three standard Option Templates (see
Section 4.2 <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2>) that are used as part of the mechanism to export MIB
Object meta data:
o mibFieldOption (Section 4.2.1 <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.1>)
o mibSubFieldOption (Section 4.2.2 <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.2>)
o mibTypeOption (Section 4.2.3 <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.3>)
This document defines three classes of new IPFIX Information
Elements. These are used to export values from the MIB, export
required Object Identifier information, and optionally export type
data from a MIB Module:
o mibObjectValue Information Elements (Section 10.1 <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-10.1>)
o mibFieldOption Information Elements (Section 10.2 <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-10.2>)
o mibTypeInformation Information Elements (Section 10.3 <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-10.3>)
>
> 2) I have trouble to understand the Figure "Architectural Overview".
> I think this should be removed. Figure 4 is more useful and it is
> at the right place in the document.
Agreed.
>
> 3) I have terminology issues in several places. For example, RFC 2578
> uses the term 'columnar object' for what this document seems to
> call 'indexed object'. Why is it useful to call Flow Records Data
> Records? Using two terms for the same thing may just adds potential
> for confusion.
From RFC 7011:
+------------------+---------------------------------------------+
| | Contents |
| +--------------------+------------------------+
| Set | Template | Record |
+------------------+--------------------+------------------------+
| Data Set | / | Data Record(s) |
+------------------+--------------------+------------------------+
| Template Set | Template Record(s) | / |
+------------------+--------------------+------------------------+
| Options Template | Options Template | / |
| Set | Record(s) | |
+------------------+--------------------+------------------------+
Figure A: Terminology Summary Table
Section 3 provides:
This document prefers the more generic term "Data Record" (as opposed
to "Flow Record") in relation to the export of MIB objects.
So flow record should be changed to data records throughout the doc.
> I also think more SMIv2 terms must be imported, such
> as MIB module, MIB object, columnar object, ...
>
> 4) There is a statement that providing index information for columnar
> objects is optional. I have some difficulty to understand why this
> is useful. What does "a MIB object is used purely as type
> information" mean? How can an exporting process not have access to
> index information of a columnar object?
>
> 5) The terminology sections says a "MIB Object Identifier" is an ASCII
> character string of a certain format but it seems later a BER
> encoded representation is used for mibObjectIdentifier. This seems
> inconsistent.
>
> 6) "exporting a value from a MIB does not imply that the SNMP process
> on the device supports that MIB"
>
> Proper wording would be something like this:
>
> "exporting a value of a MIB object defined in a certain MIB module
> does not imply that the SNMP prcess on the device supports that
> MIB module"
>
> Similarly, "exporting MIBs in IPFIX" should be "exporting values of
> MIB objects".
>
> The paragraph on page 9 is also the first occurance of the term
> "MIB Field" - without a clear definition what this means.
>
> 7) This text needs rewrite:
>
> Since these values are statically defined in the MIB they are not
> expected to change frequently. However the additional information
> about the MIB may help a Collecting Process that does not have access
> to the MIB.
>
> NEW
>
> Type information is statically defined in a MIB module, it is not
> expected to change. However, the additional information about the
> MIB Object may help a Collecting Process that does not have access
> to the MIB module.
>
> 8) Is it mibTypeOption of mibSubTypeOption? Both words are used, or
> are these different things?
>
> 9) I fail to parse/understand these statements:
>
> Forf each of these the options this draft specifies exactly which
> mibObjectValue to use.
>
> [...]
>
> If the SYNTAX clause contains a Textual Convention or Subtyping the
> mibObjectSyntax Information Element SHOULD be used to export this
> detail to the Collecting Process.
>
> 10) It escapes me how mibObjectValueSequence and mibObjectValueTable
> are used / encoded. And why does the first one use an ASN.1 type
> name while the second uses an SMIv2 conceptual term? This seems
> inconsistent, either call them mibObjectValueSequence and
> mibObjectValueSequenceOf or mibObjectValueRow and
> mibObjectValueTable.
>
> 11) This text is unclear and needs a rewrite. I think I understand
> what you are trying to communicate but the wording needs
> improvements. (And it is a MIB module here, not a MIB.)
>
> This holds true even if the data carried inside the mibObjectValue or
> mibObjectIdentifier may be related to an enterprise specific MIB.
> The OID itself encodes if the Object is in an Enterprise specific MIB
> Module.
>
> 12) What does "Field Length (mib)" stand for? Is there anything
> special or is this just a plain normal "Field Length"? If so, I
> would rather remove the "(mib)" annotation.
>
> 13) I suggest to replace "more information about MIB indexing, extra
> data from the MIB" with "additional information about the MIB
> Object definition"
>
> 14) Rewrite "... a reference to a MIB that ..." - I think you mean a
> MIB Object.
>
> Rewrite "may not have access to the MIB" -> "may not have access to
> the MIB module".
>
> And then I am getting lost here:
>
> [..] It also allows the IPFIX Field Types to be extended with
> any MIB Variable already defined purely through IPFIX.
>
> It is not clear what is communicated here.
Regards, Benoit
>
> 15) Section 4.4.6 has more inaccurate usages of the term "MIB" but
> more important, I wonder what actually the OID is that is being
> shipped in mibObjectIdentifier. Is it the OID assigned to the
> object type definition or is it the OID of the instance of an
> object type? Have I overlooked where this is clearly defined?
>
> 16) Terminology: "MIB Sequence Object's INDEX clause" -> "INDEX
> clause of a conceptual row object".
>
> 17) s/then the MIB context MUST/then the context MUST/
>
> MIB context is not a defined term. There are only SNMP contexts.
> (Perhaps they should have been MIB contexts but this is a
> different story.)
>
> 18) s/the exported to export/the exporter to export/
>
> 19) Section 4.8 is rather confusing, primarily because the terminology
> is not aligned with SMIv2 terminology. This affects almost all
> sentences. In SMIv2, terms like "MIB SEQUENCE Objects" do not
> exist. SMIv2 calls this a 'conceptual row object'. This section
> needs to be rewritten to align with SMIv2 terminology (this means
> getting rid of almost all occurrences of 'sequence' (in any
> capitalization)). Terms such as "full OID" are ambiguous unless
> you say what the OID is supposed to refer to.
>
> The first item in 4.8 talks about ingoring the indexing of
> columnar objects and later says "a columnar object may be used
> purely as a data type. I have a difficulty to understand how that
> would be useful.
>
> On page 33, a paragraph refers to section 7.7. of RFC 2578 and I
> am confused what it tries to say here. So far, I assumed that the
> OIDs all refer to columnar objects or conceptual row objects and
> include no instance identifiers.
>
> I do not understand the last paragraph before section 5.
>
> 20) The first enumerated item in Section 5.1 says "flowStartSeconds
> from [RFC7012]" but then RFC 7012 does not define this anymore
> (but its predecessor RFC 5102 did). Since Section 5.2 has the same
> text, the same issue needs to be resolved there.
>
> 21) Sections 5.1. and 5.2 both contain references to themselfs "(see
> Section 5.x)" which does not seem to make much sense. Last sentence
> on page 38: s/this identical/this is identical/
>
> 22) I looked up the definition of cpmCPUTotal1minRev and it turns out
> that this is a columnar object (which makes sense since you can
> have multiple CPUs). Since you do not export information about
> which CPU the load value is coming from, how useful is the
> information and does this make a good example?
>
> 23) Section 5.3. needs a title change. It is about exporting a subset
> of a conceptual row. Perhaps something like this:
>
> 5.3. Exporting a Conceptual Row: The OSPF Neighbor Sequence
>
> The last sentence on page 40 refers to Table 7 but I assume you
> mean Table 5.
>
> 24) The BER encoding of 1.3.6.1.2.1.14.10.1 is 06082B060102010E0A01
> (10 octets) and not the 22 octets shown in Figure 28.
>
> 25) Section 5.4. also needs a better title, e.g.
>
> 5.4. Exporting an Augmented Conceptual Row: ifTable and ifXTable
>
> The text in section 5.4 confuses ifXEntry and IfXEntry. A possible
> rewrite:
>
> The ifTable defined in the IF-MIB [RFC2863] is augmented by the
> ifXTable (defined in the same MIB module). The OID of the
> ifEntry is 1.3.6.1.2.1.2.2.1 while the OID of the augmenting
> ifXEntry is 1.3.6.1.2.1.31.1.1.1. This example demonstrates how
> columnar objects from the base conceptual row and the augmenting
> row can be exported in a single mibObjectValueSequence data
> record.
>
> 26) Why is the Field Length of mibObjectValueOctetString (on page 46)
> 16 octets? The ifName's restriction is 255 octets. And should this
> not be variable length?
>
> 27) page 47: s/VLAN=11/VLAN=10/
>
> On page 48, I am surprised that ifType and ifMTU are both 16-bit
> fields. Does the template not say they are 4 octets long?
>
> 28) Section 5.5. also needs a new title and the terminology used is
> wrong as well. Perhaps it is:
>
> 5.5. Exporting a Columnar Object: ipIfStatsInForwDatagrams
>
> Please do not write 'ipIfStatsTable SEQUENCE'. The correct SMIv2
> term is conceptual table. (And also note that the ASN.1 type of a
> table is a SEQUENCE OF and not a SEQUENCE.)
>
> The template in Figure 33 says the interface index is two octets.
> This may be true for a certain exporter but may not be generally
> true - it may be worth to be explicit about the assumption made
> here that all possible interfaces are numbered such that they fit
> into 16 bits.
>
> The BER encodings and the corresponding VLEN fields are all wrong
> in Figure 35:
>
> 1.3.6.1.2.1.4.31.3.1.1 -> 060A2B06010201041F030101 (12 octets)
>
> 1.3.6.1.2.1.4.31.3.1.2 -> 060A2B06010201041F030102 (12 octets)
>
> 1.3.6.1.2.1.4.31.3.1.12 -> 060A2B06010201041F03010c (12 octets)
>
> 29) Section 5.6. needs a better title. This is about a columnar object
> where the index information is provided by IPFIX information
> elements.
>
> 5.6. Exporting a Columnar Object index by IEs: ifOutQLen
>
> s/be done be/be done by/
>
> The text below Table 8 needs to be rewritten, e.g.
>
> The ifOutQLen MIB object defined in the IF-MIB [RFC2863] provides
> the length of the output packet queue. This columnar object is
> part of the ifEntry conceptual row and indexed by the interface
> index (ifIndex).
>
> I am not sure I agree on the second paragraph on page 53. I tend
> to believe that the index information must be provided for generic
> applications to make sense out of the data.
>
> The BER encoding and the corresponding VLEN in Figure 39 is wrong:
>
> 1.3.6.1.2.1.2.2.1.21 -> 06092B0601020102020115 (11 octets)
>
> 30) I find the example in section 5.7 rather strange. Why would one
> export the ifIndex value using mibSubIdentifier and not by
> including the ifIndex proper? There does not seem to be any
> savings and this mibSubIdentifier approach of course is only
> applicable where the number of sub-identifier is constant for all
> conceptual rows. Since I do not see that this approach is needed
> nor that it adds any value in terms of more compact encodings,
> I would actually prefer this to be removed and perhaps even be
> disallowed.
>
> The BER encoding of the ifOutQLen OID is wrong, see above for the
> correct value.
>
> (The caption of Figure 44 is kind of strange because it says
> "using ifIndex" but then the example is about not using ifIndex
> but instead an opaque number.)
>
> This section 5.7. should really be removed I think.
>
> 31) In section 5.8,
>
> s/ospfNbarEntry/ospfNbrEntry/
>
> The OID encoding and the VLEN field in Figure 46 is wrong:
>
> 1.3.6.1.2.1.14.10.1 -> 06082B060102010E0A01 (10 octets)
>
> RFC 3411 uses '800002b804616263'H as an snmpEngineID in examples.
>
> 32) What is the meaning of this:
>
> If a Collecting Process receives a MIB Object Identifier that it
> cannot decode, it SHOULD log an error.
>
> What does 'cannot decode' mean? I mean, a simple collector may
> just store the records in some file / database. So what is
> expected here? And why is it an error and not a warning?
>
> I am also not sure what the last paragraph in section 7 tells me.
> What does 'purely semantic information' mean? For me, if you miss
> the semantics, the data has no value. But I understand that IPFIX
> people have a very different terminology at times.
>
> 33) It is unclear to me how the export of conceptual rows deals with
> non-existing variables (aka table holes). Is there a mechanism in
> IPFIX to indicate that a certain field of a flow record does not
> have a value?
>
> Issues with section 10:
>
> a) In the SMIv2, we distinguish between Counter32 and Counter64 and
> I think this difference important to capture since the rollover
> is different. The I-D seems to map both to mibObjectValueCounter.
>
> b) The descriptions all use the phrase "from a MIB" but I think it
> should be "of a MIB object".
>
> c) The SMIv2 type is IpAddress and not IPAddress.
>
> d) The definition of mibObjectValueCounter says "Data Type Semantics:
> totalCounter". I think this is wrong since SMIv2 counters do not
> start with zero. Furthermore, a Counter32 rolls over after 32-bit
> and not after 64-bit (but mibObjectValueCounter is an unsigned64).
>
> e) The definition of mibObjectValueGauge "Data Type Semantics:
> totalCounter". I think this is wrong, a Gauge32 is not a counter.
> And it might be useful to call this mibObjectValueGauge32.
>
> f) Why did you call mibObjectValueTime not mibObjectValueTimeTicks?
> The "Abstract Data Type: dateTimeMilliseconds" also seems to be
> wrong since TimeTicks are _not_ measured in milliseconds since
> 1970-01-01T00:00. You may want to use unsigned32 and spell out the
> semantics.
>
> g) mibObjectValueUnsigned used the "Abstract Data Type: unsigned64"
> which is wrong since there is only a 32-bit unsigned type in the
> SMIv2. I suggest to use unsigned32 and to rename this to
> mibObjectValueUnsigned32.
>
> h) The phrase "a complete MIB 'SEQUENCE OF X' or conceptual table
> value" reads strange. Similarly, "a MIB SEQUENCE or a row from a
> conceptual table" reads strange. Perhaps simply use "a complete
> conceptual table" and "a row of a conceptual table".
>
> i) The lest sentence of the Description in 10.1.11 seems to be missing
> some words.
>
> j) The description of 10.2.2 is somewhat confusing: "... that serves
> as INDEX MIB Objects of Information Elements for a mibField". In
> SMIv2, the INDEX is associated with a conceptual row.
>
> k) What does "sampled by SNMP" mean? Do you hook into the
> instrumentation or do you access things via the SNMP agent?
>
> l) p74: s/the MIB the Flow/the MIB when the Flow/
>
> (Note that you are talking about a flow here and not about a data
> record. I think this is goodness and proves that a new term is not
> needed.)
>
> m) Replace the Description in 10.3.4 with this:
>
> Description: The textual name of the MIB module that defines a MIB
> Object.
>
> n) I do not understand what mibObjectSyntax would contain for an object
> representing a conceptual table or a conceptual row. For say ifEntry,
> would it be just "SEQUENCE {
> ifIndex InterfaceIndex,
> ifDescr DisplayString,
> -- lots of stuff left out
> }"? And what would it be for the object representing the conceptual
> table? Or do you just mean what is literally in the SYNTAX clause,
> excluding the referenced ASN.1 type definitions?
>
> For other MIB Objects, do you include subtyping information such as
> range of length restrictions? What about named number enumerations
> of the BITS construct, you include the whole definition (mind you
> that RowStatus is really long).
>
> o) Remove "( also known as snmpEngineID) that" since it is potentially
> misleading.
>
> Explanation: Every SNMP engine has a unique snmpEngineID. The
> combination of a snmpEngineID value and a context names provides
> the SNMP context. Note that an SNMP engine may provide access to a
> non-local context, in which case the values of snmpEngineID and
> contextEngineID would be different.
>
>
_______________________________________________
IPFIX mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/ipfix