Subject: [Gen-art] Gen-art review of draft-ietf-forces-model-14.txt

From: Elwyn Davies <[email-masked]>
Date: Fri, 05 Sep 2008 20:45:40 +0100

I have been selected as the General Area Review Team (Gen-ART)
reviewer for this draft (for background on Gen-ART, please see
_http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html_).

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-forces-model-14.txt
Reviewer: Elwyn Davies
Review Date: 5 Setember 2008
IETF LC End Date: 8 September 2008
IESG Telechat date: (if known) -

Summary: Nearly ready for IESG. Generally this is a very well 
constructed and written document dealing with a very complex problem. 
There are quite a number of minor points that ought to be addressed 
(although not vast fpr a 132 page document) plus a (largish) number of 
editorial nits (of which many are related to highly inconsistent choice 
of what to use for the singular form of metadata - metadata or metadatum 
- in many cases both are used in the same sentence!) The other 
consideration that needs to be addressed is the use of normative 
language in s4 - this is discussed below.
Caveat: I haven't done detailed checks of the XML schema and the long 
bits of XML.

Comments:

Minor Issues:
=============

s2.3: States that the protocol can be used to discover the Inter-FE 
topology: is what is meant here? or the intra-FE tolopology? Maybe be a 
forward reference to s5.3.4 might clarify this if Inter-FE is right?

s3.1.1.3, para 2: 'For example, when
certain features of an LFB class are optional, the CE MUST be able to
determine whether those optional features are supported by a given
LFB instance.'
I have a vague feeling that this conflicts with the ‘suck it and see’ 
approach of coarse capabilities from the previous section. Also putting 
a MUST into an example is unsatisfactory. It isn’t necessary here as 
this is just the concepts section.

s3.2, para 8 (p16): 'In addition, the CE MUST understand where and what 
type of header
modifications (e.g., tunnel header append or strip) are performed by
the FEs. Further, the CE MUST verify that the various LFBs along a
datapath within an FE are compatible to link together.'

I don't believe these are normative MUSTs. There is nothing the model 
can do to enforce them.

s3.2.1, para 8 (p19): '...based on a FRAMETYPE metadata (N=2), or to fork
into color specific paths after metering using the COLOR metadata
(red, yellow, green; N=3), etc.'

The FRAMETYPE and COLOR hargon may be confusing to a novice reader and 
are not strictly necessary.

s3.2.5: Event target terminology: It may be a bit late to change but the 
term 'target' seems to convey the wrong idea. The 'target' actually is 
the source of the event. Maybe 'anchor' might be better (taking our cue 
from xml2rfc).

s4: General: The use of 'MUST' in many places but almost always 'may' is 
IMO confusing. I think the problem is that the normative language is 
(AFAICS) used to constrain the semantics of the XML schema - it isn't 
about protocol behaviour. Now this is a reasonable use for this sort of 
language but I think that at least some of the 'may's should also be 
MAY. One way I thought about this could be to think of the section as 
specifying the bahaviour of a tool that checks the semantics of the XML. 
Stating this explicitly could then make it much clearer whether the 
right language is being used. Please consider how to improve this.

s4.3: 'The load element MUST contain the label of the library document to be
included and may contain a URL to specify where the library can be
retrieved.'

Does anything need to be said about where it would come from if the 
location os missing?

s4.5: The format of 'float16' needs to be defined. There probably needs 
to be a reference to the right IEEE document that defines float32 and 
float64.

s4.5: 'The boolean data type is defined here because it is
so common, even though it can be built by sub-ranging the uchar data
type.'

Two issues here: 1) sub-ranging hasn’t been talked about yet; 2) this 
puts doubt into peoples’ minds about the size of the represenation.. is 
a boolean 8 bits or 1 bit? Does this matter?

ss4.5, s4.5.6, para 1, s4.7.6.4, s8.3, 8.4 (and maybe elsewhere): These 
sections refer to message names from the ForCES protocol. This is not a 
good idea since it creates a bidirectional dependency between the docs. 
generic terminology could be used rather than specific message names. I 
am not entirely sure how to fix s8, but the current situation is not 
totally desirable.

s4.5, last para: 'The predefined type alias is somewhere between the 
atomic and
compound data types. It behaves like a structure, one component of
which has special behavior.' BUT *what is* an alias?? We should be told!

s4.5.3: <arry> Elelemt definition: Because of the talk about key fields 
and use of structure components, it might be cleaner to place this after 
structs and unions.

s4.5.3, para 2: 'The latter should be handled by capability components
of LFB classes, and should never be included in a data type array
which is regarded as of unlimited size' In the last part of this 
sentence, it should be clarified if you are talking about 'maxlength' 
attribute.

s4.5.3, para 4: 'The result of this construct MUST always be a compound 
type, even if
the array has a fixed size of 1.' This is an inherent characteristic of 
the type rather than something that is an implementation decision. Maybe 
‘is necessarily’. Presumably the point is that it can’t be used fror 
deriving <atomuc> elements. Same applies to struct (s4.5.4, para 5) and 
union (s4.5.5, para 2).

s4.5.3, para 8: 'o In any instance of the array, each declared key MUST 
be unique
within that instance. No two components of an array may have the
same values on all the fields which make up a key.' The second part of 
this statement would be better rewritten in normative language.

s4.5.3, next to last para: 'When the current context is an array, (e.g., 
when declaring a key for
an array whose content is an array) then the only valid value for the
field identifier is an explicit number.' Should you say anythng about an 
index into a variable aize array where the index might be out of bounds 
for some instances? (nasty corner case!)

s4.5.4, para 3: 'The <component> element MUST include a componentID 
attribute. This
provides the numeric ID for this component, for use by the protocol.'
Presumably it is NOT required that numeric IDs are in sequence and start 
from 1.. the id’s and the definition order does not say anything about 
the order of compnents in the model… or do they have any knock-on impact 
into the protocol?? There is (or maybe) a distinction between C, where 
order is HIGHLY significamt, and the model. This also has an effect on 
extensions in augmentations.

s4.5.4, para 4: 'This is indicated by including the optional derivedFrom 
attribute in
the struct declaration before the definition of the augmenting or
replacing components': A forward ref to 4.5.7 would avoid the ‘how?’ 
question.

s4.5.6, para 2: 'The target of a component declared by an <alias> element is
determined by the component’s properties.': It would be good to clarify 
that these are properties of the component in the alias (If I have 
understood this right.. I was unsure I understood correctly).

s4.5.7: Augmentations allow additions to structures: Does this 
explicitly exclude unions? If so it woud be a good idea to make it 
clear. If not they need to be talked about.

s4.5.7, para 1: 'The type of an
existing component can be replaced in the definition of an augmenting
structure, but only with an augmentation derived from the current
type of the existing component.' This would better redrafted in proper 
normative language.

s4.5.7, last para: 'Component names and component IDs for new components
within the augmentation must not be the same as those in the
structure type being augmented.':
Make it clear if there are (or are not) any constraints on the numeric 
IDs. I think the intention is that they are arbitrary and not 
necessarily in sequence either withn themselves or with the ones in the 
base class but I may be wrong!

s4.7.1, last para: 'It is assumed that a derived class is backwards 
compatible with its
base class.'
What does this say about the ports and capabilities? Backwards 
compatibility was previously discussed wholly in terms of the components 
of LFB classes.

s4.7.6, para 2: 'The <events> element contains 0 or more <event> 
elements': Why 0 or more? <events> is optional and all the other similar 
cases require 1 or more if present.

s4.7.6.1: See earlier discussion of the use of 'target' for events.

s4.8, last para and s4.8.6: 'PL protocol': Is this the ForCES protocol 
in some earlier teminology?

s4.6.5, dataType spec: <component componentID="3">
<name>threshold</name>
<synopsis> comparison value for level crossing events
</synopsis>
<optional/>
<typeRef>uint32</typeRef>
</component>
<component componentID="4">
<name>eventHysteresis</name>
<synopsis> region to suppress event recurrence notices
</synopsis>
<optional/>
<typeRef>uint32</typeRef>
</component>
The implication of this appears to be that thresholds must be unsigned 
integer values and the hysteresis is then naturally an integer. Is this 
intended? Is there any reason why a threshold couldn’t be defined on a 
floating point number (apart form the high computational requirements?

s9.2: maybe should reiterate that Class Identifiers are 32 bit quantities.

Editorial:
==========

(I have sent a marked up Microsoft Word file with the minor editorial 
issues to the editors and they are not repeated here.)

s1,LFB Model definition: May require a definition of the term 'frame'.

s1, FE Topology definition: It is necessary to expand the abbrevations 
imported from RFC 3654 - in practice I think there are not many - the 
one here is NE.

Figure 2: Note meanings of P and M in text.

s3.2.4.2, last para: 'vice versa' is superfluous - either the metadata 
are the same or not.. which one was originally defined first is only on 
the mind of the designer.

[S4.7.1: Enlighten me please! Is a 'vifid' like a Triffid with video?]

s4.8.5, para 1: 'The hysteresis field is used to suppress generation of
notifications for oscillations around a condition value, and **is
described in the text for events**': I don’t think it is.. there is 
brief reference in s3. Alternative: reference s4.8.5.2 below.

s4.8.5.3, last para: 'it is
reset to 0. In this conceptual implementation the reset behavior
when a notification is generated can be thought of as setting the
counter to 1.': The dual use of 'reset' in this is slightly confusing.

s8.4: Uses the term 'variance property' whan talking about the 
capability for 'hysteresis' - I don't think this term is used elsewhere.

_______________________________________________
Gen-art mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/gen-art

---

From: "Joel M. Halpern" <[email-masked]>
Date: Fri, 05 Sep 2008 22:41:42 -0400

Thanks Elwyn.  I will go through these carefully in the near future. 
Given the amount you found, it is clearly worth while submitting a new 
version before IESG review.

Yours,
Joel

---

From: "Joel M. Halpern" <[email-masked]>
Date: Fri, 05 Sep 2008 22:43:33 -0400

Yours,
Joel

---

From: "Joel M. Halpern" <[email-masked]>
Date: Sun, 07 Sep 2008 20:04:03 -0400

Thanks.  Resolution point by point below.
(I have built, but not yet submitted, -15 which contains these changes.)
I hope you will have time to check the effect, once I have submitted the 
revision. (Presumably in a week, when the last call ends.  Unless the 
chair asks me to submit it sooner.)
The intention is to use the metadatum for a single piece of information, 
and metadata for multiple pieces of information.   We already made one 
pass fixing that, and obviously did not catch them all.

Joel

PS: I have also applied to editorial fixes you provided.

Elwyn Davies wrote:
...
> Comments:
> 
> Minor Issues:
> =============
> 
> s2.3: States that the protocol can be used to discover the Inter-FE 3

This should be intra-FE.  While there has been discussion of discovering 
inter-FE topology, that would be a specific additional protocol and LFB 
  Class, not the model or general protocol.

> this is just the concepts section.
Will change to "needs to be able".  This is indeed a case where we 
concluded something more than trial and error was needed.  (And yes, 
this was clearly an erroneous use of "MUST".)

> I don't believe these are normative MUSTs. There is nothing the model 
You are correct, these are not protocol or model MUSTs, and so this 
needs rewording.  I have changed most of the must/MUST to "needs to" and 
the verification I changed to "works to" since it is more a description 
of what the CE is going to do, not what it is required by RFC to do.

> are not strictly necessary.
I cleaned up the metadata / metadatum, and added a sentence describing 
FRAMETYPE and COLOR, so as to have a more useful example.

> the source of the event. Maybe 'anchor' might be better (taking our cue 
If this is a big issue, I can try to fix it.  However, it would require 
a lot of changes, and I am concerned that I would miss some, and leave 
an even worse mess.  Source is probably a better word than target, but 
even taht isn't quite right, since the "source" is the condition itself, 
i.e. the base value going over a threshold, or changing too often, or...
For now, I am leaving this one alone.  Feel free to push if after the 
rest you think this is more important than I am giving credit for.

> right language is being used. Please consider how to improve this.
I have gone through this section, looking at the "may" usages.
Most of them are indeed MAY.  Thanks for catching this.  I have 
corrected any ones that are defining document document in a "what is 
correct, what is permitted" sense to upper case.  There are a few 
sentences (particularly early in the section) that are more of "this may 
happen to be the case" and I left those lower case.
One interesting case is the paragraph describing compound data types. 
It says that they "may" be defined in one of four ways, and then gives 
four sentences each saying it "may" be done in one way.  This could be 
worded entirely without "may" (a sentence "there are four possibilities" 
followed by a four item list) so this seems to be more of an English 
description of what is the case, rather than a restriction / permission. 
  So I have left it lower case.  (While I do know that some folks think 
it is silly to even care, I actually am trying to make this more 
consistent with capitalized usage for quasi-conformance language.)

> Does anything need to be said about where it would come from if the 
I don't think the working group talked about this much.  The design team 
wanted to leave this open, because it seemed likely to be specific to 
any implementation.  The library name in the load and the provides tag 
in library definitions may be sufficient in some environments.
Even where the name is not sufficient, it is not clear we want to 
require URLs.  For example, in an RFC that is loading a library defined 
by another RFC, there is no place to reference for the load URL.

> to be a reference to the right IEEE document that defines float32 and 
I think we can avoid being specific here.  The format used to transfer 
these values is actually the domain of the protocol spec, I think.
If you have a reference you want me to put in, I am happy to do so.

> a boolean 8 bits or 1 bit? Does this matter?> 
The size for reporting is defined by the protocol.
(Yes, having an explicit type does allow the protocol more latitude.  If 
we only subranged uchar, that document would have no choice.  As it 
happens, the protocol defines it as taking 8 bits to report I think.)
I don't think the subrange thing is a big deal.  It is more an 
acknowledgment that when you get there, don't worry about the 
possibilities.  I have added an explicit forward reference.

> am not entirely sure how to fix s8, but the current situation is not 
We were trying, for a time, to avoid mutual dependence.  We gave up and 
explicitly decided to allow such dependence.  In part, this allows us to 
avoid having two terms for the same thing, one generic term and one 
specific term.  For example, the way that alias targets are encoded is 
closely related to the way that the protocol uses the identifiers 
defined in the model.  If some other protocol tried a different approach 
to using the identifiers, the alias definition would make no sense.

> which has special behavior.' BUT *what is* an alias?? We should be told!
Fair enough.  I have added text explaining it.

> and use of structure components, it might be cleaner to place this after 
Unless this is important, I am going to leave this as it is.  For 
several different aspects of this document, there is no "good" ordering. 
  (And at this point reordering produces internal oddities I would 
rather not deal with.)  If you conclude that this is more important, let 
me know.

> sentence, it should be clarified if you are talking about 'maxlength' 
Reworded to clarify the meaning (which you inferred correctly in spite 
of my sloppiness.)

> deriving <atomuc> elements. Same applies to struct (s4.5.4, para 5) and 
Changed to "is always a" instead of "MUST always be"

> this statement would be better rewritten in normative language.
I added "That is," to the front of the second sentence.  It is not a 
different constraint, but rather an explanation of the effect of the 
first constraint.  Trying to turn the second sentence into normative 
language results in the first sentence.

> for some instances? (nasty corner case!)
I added an extra paragraph indicating that referencing non-existent 
optional elements, whether array or structure components, will result in 
errors in creation.

> extensions in augmentations.
I added text (for both structs and LFBclass Definitions, indicating that 
the values do not need to be contiguous or in order, but that contiguous 
sets are helpful for human readability.

> replacing components': A forward ref to 4.5.7 would avoid the ‘how?’ 
Done.

> understood this right.. I was unsure I understood correctly).
I fixed the first sentence to end "element is determined by teh 
information in the component's properties."  I had managed to leave 
mixed sentence fragments, which resulted in a lack of English.
I am not sure this is sufficient, but I am sure it is better.

> clear. If not they need to be talked about.
Good catch.  We never talked about this.  I think that augmenting a 
union would be inherently incompatible.  So I have added text saying 
that augmentations MUST not be applied to unions.

> type of the existing component.' This would better redrafted in proper 
Added "MAY" rather than "can" language.  (I left "existing component 
cannot be deleted" because that is a statement of fact, not compliance. 
  The model has (deliberately) no way to delete as part of augmentation.)

> base class but I may be wrong!
You are correct, and I added text.  (I did not simply copy the struct 
and LFB text, since this text needs to emphasis that the values must be 
different from the existing ones.

> compatibility was previously discussed wholly in terms of the components 
I have added text making it clear that a derived class is adding fields, 
and the addition restriction applies to components, input and output 
ports, events, and capabilities.  Again, there is no syntax for 
deletion, so it can not be done.

> cases require 1 or more if present.
Because...  Specifically, because I thought the LFB designer, if he 
anticipated needing events later, might want to reserve the event base 
ID.  Do I need to add text saying so?

> in some earlier teminology?
I fixed both of these for ForCES protocol.  The PL is more specific, but 
there is no need for or benefit in that specificity here.

> floating point number (apart form the high computational requirements?
Yes, Threshold is defined as an unsigned integer.
This was a case of datatyping putting me in a box.  What I really want 
is for threshold to be the same type is the underlying component to 
which this event is being applied (unsigned, signed, floating).  But 
there was no way to define things that would work.  And similarly, the 
hysteresis should be allowed to be floating, for floating base.
But I just could not define things that way.

Now, having said that, the uses of float in these models is rare.
Is that really an excuse?  No.

This was raised once before, on the working group list.  I agree it is 
unfortunate.  But no one could come up with a practical solution.  And 
it does not seem fatal.  (I tried making them both unions, and it was 
just a mess.  Because to reference the base type in order to have a 
discriminator to tell you what you have / what is allowed is just awful.)

> s9.2: maybe should reiterate that Class Identifiers are 32 bit quantities.
added
> Editorial:
I added some parenthetical (packet) s to try to help.

> imported from RFC 3654 - in practice I think there are not many - the 
I fixed NE.  I don't know if there are others.

> Figure 2: Note meanings of P and M in text.
Yes, added.
> the mind of the designer.
Okay.

> [S4.7.1: Enlighten me please! Is a 'vifid' like a Triffid with video?]
I am not sure what one of my predecessors actually had specifically in 
mind for a "vifid" metadatum, but it is a valid name for a matadatum.
I changed it to "vpnid" anyway.

> brief reference in s3. Alternative: reference s4.8.5.2 below.
Thanks.  Changed.

> counter to 1.': The dual use of 'reset' in this is slightly confusing.
I removed the last sentence.  I also changed the "reset to 0" to "set to 
0".  So there are now 0 resets in the paragraph.

> capability for 'hysteresis' - I don't think this term is used elsewhere.
No, it is not used.  It should have been (and is now) hystereseis.)

---

From: Jamal Hadi Salim <[email-masked]>
Date: Mon, 08 Sep 2008 09:35:30 -0400

> chair asks me to submit it sooner.)

IMO, if Elwyn thinks it will make it easier to recheck, then i would
suggest issuing -15. We certainly need a revision after the response to
your rebutal(and your rebutal seemed legible enough to me). 
Summary: its Elwyn's call.

cheers,
jamal

---

From: Elwyn Davies <[email-masked]>
Date: Mon, 08 Sep 2008 15:01:30 +0100

I had a look through Joel's responses which seemed to be pretty much 
sensible (although I think changing 'target' for 'anchor' wouldn't be a 
big deal - but I won't make a big deal about this).  If Joel want to 
send me a pre-release I would be happy to do a quick check later today, 
but I can't gurantee to do much before next weekend after that (and I am 
not sure how good my connectivity will be for the rest of the week).  I 
think you should probably wait to the end of LC to make a new formal 
release.

Regards,
Elwyn