Skip to main content

Telechat Review of draft-ietf-opsawg-vmm-mib-03
review-ietf-opsawg-vmm-mib-03-genart-telechat-kyzivat-2015-06-23-00

Request Review of draft-ietf-opsawg-vmm-mib
Requested revision No specific revision (document currently at 04)
Type Telechat Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2015-06-23
Requested 2015-05-28
Authors Hirochika Asai , Michael MacFaden , Jürgen Schönwälder , Keiichi Shima , Tina Tsou (Ting ZOU)
I-D last updated 2015-06-23
Completed reviews Genart Last Call review of -02 by Paul Kyzivat (diff)
Genart Telechat review of -03 by Paul Kyzivat (diff)
Secdir Last Call review of -02 by Christian Huitema (diff)
Opsdir Last Call review of -02 by Dan Romascanu (diff)
Opsdir Telechat review of -03 by Dan Romascanu (diff)
Assignment Reviewer Paul Kyzivat
State Completed
Request Telechat review on draft-ietf-opsawg-vmm-mib by General Area Review Team (Gen-ART) Assigned
Reviewed revision 03 (document currently at 04)
Result Ready w/nits
Completed 2015-06-23
review-ietf-opsawg-vmm-mib-03-genart-telechat-kyzivat-2015-06-23-00
I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at
< 

http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please wait for direction from your document shepherd
or AD before posting a new version of the draft.

Document: draft-ietf-opsawg-vmm-mib-03
Reviewer: Paul Kyzivat
Review Date: 2014-06-05
IETF LC End Date: 2015-05-18
IESG Telechat date: 2015-06-11



Summary: This draft is ready for publication as a proposed standard. All 


of my previous comments have been addressed in correspondence.




Major issues:

None

Minor issues:

none

Nits/editorial comments:

In the new changes for -03:



In Section 4 I see one change to use normative language in this section, 


as I suggested, that seems problematic:




   An implementation MUST support both, either of, or none
   of per-VM notifications and bulk notifications.



This *looks* normative, but with all the "or"s it ends up requiring 


*nothing*. I suggest replacing "MUST" with "can".




In Section 6.1:

There is one occurrence of "SHOULD not" that ought to be "SHOULD NOT".

There are two occurrences of "in byte" that should be "in bytes".

In Section 6.2, in the definition of IANAStorageMediaType:

I wonder why there isn't a category for Flash Drives.


On 5/28/15 11:48 AM, Juergen Schoenwaelder wrote:



And the proper way to state conformance requirements in MIB modules
is to use the conformance definitions.

/js

On Thu, May 28, 2015 at 11:05:12AM -0400, Paul Kyzivat wrote:



Hirochika,

Thanks for addressing my comments. I have one comment on -03:

I see a change to use normative language in this section. But this
particular one seems problematic:

    An implementation MUST support both, either of, or none
    of per-VM notifications and bulk notifications.

This *looks* normative, but with all the "or"s it ends up requiring
*nothing*. I suggest replacing "MUST" with "can".

	Thanks,
	Paul

On 5/28/15 6:42 AM, Hirochika Asai wrote:



Hi,

We have submitted -03 after the revision addressing Dan’s comments.

Best,
Hirochika Asai





On May 22, 2015, at 10:36 PM, joel jaeggli <joelja at bogus.com> wrote:

Hi, please do submitt 03

Also take a look at Dan Romascanu's

opsdir reivew.

On 5/14/15 1:08 AM, Hirochika Asai wrote:



Dear Paul Kyzivat,


Thank you for your review.

Since the Last call is in process, we do not submit the (current)
revised version but reply with inline comments and the revised version
attached in this mail.





* Figure 2: A few things are fuzzy about this figure:

-- The meaning/purpose of the part above the "====", and its
relationship to the rest of the diagram, isn't clear to me. Is it a
legend, explaining the notation for transient vs. finite states?



My bad.  Thank you for your indication.  In that figure, we expect to
explain the notation of two types of boxes and a symbol of “!”.  So
we have modified the figure to explicitly denote they are the legend.

NEW:

Notation:

    +-------------+
    | vmOperState | : Finite state; the first line presents the
    |             |   `vmOperState', and the second line presents a
    +-------------+   notification generated if applicable.

    + - - - - - - +
    | vmOperState | : Transient state; first line presents the
    |             |   `vmOperState', and the second line presents a
    + - - - - - - +   notification generated if applicable.

    !               : Notification; a text followed by the symbol "!"
                      denotes a notification generated.

=====================================================================

+--------------+   + - - - - - - - +     +-------------+
|  suspended   |<--|  suspending   |     |   paused    |
| !vmSuspended |   | !vmSuspending |     |  !vmPaused  |
+--------------+   + - - - - - - - +     +-------------+
      |                ^                    ^
      |                |                    |
      v                |                    |
+ - - - - - - +   +-------------+<----------+    + - - - - - - -+
|  resuming   |-->|   running   |<-------------->|  migrating   |
| !vmResuming |   |  !vmRunning |                | !vmMigrating |
+ - - - - - - +   +-------------+                + - - - - - - -+
                       |      ^                        ^
                       |      |                        |
                       |      +-------------------+    |
                       |                          |    |
                       v                          v    v
                + - - - - - - - - +          +-------------+
                |  shuttingdown   |--------->|  shutdown   |
                | !vmShuttingdown |          | !vmShutdown |
                + - - - - - - - - +          +-------------+
                                                 ^      |
                                                 |      v !vmDeleted
+ - - - - - -+   +------------+     + - - - - - - +    (Deleted from
|  blocked   |   |  crashed   |     |  preparing  |     vmTable)
| !vmBlocked |   | !vmCrashed |     |             |
+ - - - - - -+   +------------+     + - - - - - - +






-- what is the point of the 'preparing' state? There is no way in, and
the only way out is to shutdown. (I could understand it as a starting
state if there was a path to running.) While it is described later, in
this figure it seems to have no purpose.
-- the 'blocked' and 'crashed' states have no way either in or out.
Surely there must be some path into these states, and some path out (at
least to shutdown or deleted.)








I see from the later definitions that arbitrary state transitions can
be represented. Is Figure 2 intended to normatively constrain the state
transitions? Or does it only provide an overview of "expected"
transitions?

I don't feel I understand the intent sufficiently to suggest changes to
remedy my confusion.




We modified the caption of Figure 2 to denote it is the overview, and
added the explanation that the detailed state transition is summarized
in Appendix A.  Although there is no way from/to blocked and crashed as
well as one way from preparing as you pointed out, we consider adding it
makes the figure complicated.  Thus, thanks to your suggestion, we
changed it to the overview and promotes readers to refer to Appendix A.

The caption of Figure 2:

OLD:
The state transition of a virtual machine

NEW:
The overview of the state transition of a virtual machine

The paragraph referring to Figure 2:

OLD:
The transition of `vmOperState' by the write access to `vmAdminState'
and the notifications generated by the operational state changes are
summarized in Figure 2.

NEW:
The overview of the transition of `vmOperState' by the write access to
`vmAdminState' and the notifications generated by the operational state
changes are illustrated in Figure 2.  The detailed state transition is
summarized in Appendix A.






* Section 5

This says "Hypervisors *shall* implement HOST-RESOURCES-MIB." This
sounds normative. If so, 'shall' should be replaced with MUST.



The MIB module imports HOST-RESOURCES-MIB, so we replace it with MUST.

OLD:
HOST-RESOURCES-MIB defines the MIB objects for managing host systems.
Hypervisors shall implement HOST-RESOURCES-MIB.  On systems implementing
HOST-RESOURCES-MIB, the objects of HOST-RESOURCES-MIB indicate resources
of a hypervisor.   Some objects of HOST-RESOURCES-MIB shall also be used
to indicate physical resources through indexes.

NEW:
HOST-RESOURCES-MIB defines the MIB objects for managing host systems.
Hypervisors MUST implement HOST-RESOURCES-MIB.  On systems implementing
HOST-RESOURCES-MIB, the objects of HOST-RESOURCES-MIB indicate resources
of a hypervisor.  Some objects of HOST-RESOURCES-MIB are used to
indicate physical resources through indexes.





The same issue with 'shall' is present in the 2nd paragraph refering to
virtual machines.








Also in the 2nd paragraph I can't parse or fully understand the last
sentence. ("This document defines the objects of these information.")
Changing 'these' to 'this' would make it grammatical, but still not
very clear. I guess you mean something like: "This document defines the
relationship between the objects visible to virtual machine operators
and those visible to hypervisor operators.”



We figured that this paragraph was not appropriate to be included here,
so removed it.  Note that it explains an operational difference between
this document and HOST-RESOURCES-MIB that may be individually
implemented in systems on "virtual machines”, i.e., guest OS.  No need
to mention it in this document.





* Section 8 - Security Considerations:

I see no 2119 language in this section, but I see language that sounds
normative to me. E.g., "When SNMPv3 strong security is not used, these
objects ***should*** have access of read-only, not read-write." If
these statements are intended to be normative then please use 2119
language.



We change *should* to *SHOULD* as 2119 language.  Also, we capitalize
2119 language in this document.
As for RFC 2119 language, we modified several words to more appropriate
ones in the new version.





The rest leaves me concerned about security. But I will leave it to a
security review to dig into.

Nits/editorial comments:

* The introduction says that this has been derived from "enterprise
specific" MIB modules. But the examples sound more "product-specific"
than enterprise-specific. I guess you mean modules created by the
enterprise producing the product, so maybe it is ok, but it struck me
as odd. (Please feel free to leave this as-is if the usage is
appropriate in context.)



I agree that the examples except for VMware are product-specific than
enterprise specific.

OLD:
The design of this MIB module has been derived from enterprise specific
MIB modules, namely a MIB module for managing guests of the Xen
hypervisor, a MIB module for managing virtual machines controlled by the
VMware hypervisor, and a MIB module using the libvirt programming
interface to access different hypervisors.

NEW:
The design of this MIB module has been derived from product-specific MIB
modules, namely a MIB module for managing guests of the Xen hypervisor,
a MIB module for managing virtual machines controlled by the VMware
hypervisor, and a MIB module using the libvirt programming interface to
access different hypervisors.





* Page 22, DESCRIPTION of vmHvSoftware:

This says "This value should not include its version, and it should be
included in `vmHvVersion'." IIUC 'and' is the wrong word to use here -
'as' would convey the intended meaning.




Thank you.  ‘as’ is what we intended.


Best regards,
Hirochika Asai





On May 11, 2015, at 3:15 AM, Paul Kyzivat <pkyzivat at alum.mit.edu> wrote:

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<

http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

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

Document: draft-ietf-opsawg-vmm-mib-02
Reviewer: Paul Kyzivat
Review Date:
IETF LC End Date: 2015-05-18
IESG Telechat date: (if known)

Summary: Ready with minor issues.

Major issues:

None.

Minor issues:

* Figure 2: A few things are fuzzy about this figure:

-- The meaning/purpose of the part above the "====", and its
relationship to the rest of the diagram, isn't clear to me. Is it a
legend, explaining the notation for transient vs. finite states?

-- what is the point of the 'preparing' state? There is no way in, and
the only way out is to shutdown. (I could understand it as a starting
state if there was a path to running.) While it is described later, in
this figure it seems to have no purpose.

-- the 'blocked' and 'crashed' states have no way either in or out.
Surely there must be some path into these states, and some path out (at
least to shutdown or deleted.)

I see from the later definitions that arbitrary state transitions can
be represented. Is Figure 2 intended to normatively constrain the state
transitions? Or does it only provide an overview of "expected"
transitions?

I don't feel I understand the intent sufficiently to suggest changes to
remedy my confusion.

* Section 5

This says "Hypervisors *shall* implement HOST-RESOURCES-MIB." This
sounds normative. If so, 'shall' should be replaced with MUST.

The same issue with 'shall' is present in the 2nd paragraph refering to
virtual machines.

Also in the 2nd paragraph I can't parse or fully understand the last
sentence. ("This document defines the objects of these information.")
Changing 'these' to 'this' would make it grammatical, but still not
very clear. I guess you mean something like: "This document defines the
relationship between the objects visible to virtual machine operators
and those visible to hypervisor operators."

* Section 8 - Security Considerations:

I see no 2119 language in this section, but I see language that sounds
normative to me. E.g., "When SNMPv3 strong security is not used, these
objects ***should*** have access of read-only, not read-write." If
these statements are intended to be normative then please use 2119
language.

The rest leaves me concerned about security. But I will leave it to a
security review to dig into.

Nits/editorial comments:

* The introduction says that this has been derived from "enterprise
specific" MIB modules. But the examples sound more "product-specific"
than enterprise-specific. I guess you mean modules created by the
enterprise producing the product, so maybe it is ok, but it struck me
as odd. (Please feel free to leave this as-is if the usage is
appropriate in context.)

* Page 22, DESCRIPTION of vmHvSoftware:

This says "This value should not include its version, and it should be
included in `vmHvVersion'." IIUC 'and' is the wrong word to use here -
'as' would convey the intended meaning.