Hi,

I am the assigned YANG doctor for this individual submission document.

I have done a first review of this document, focusing mainly on pure
YANG issues.

Here are my comments.

o  service-function-forwarder imports an outdated version of
   ietf-inet-types.  Why is this module imported by revision at all?


o  Obvsiously, if adopted by the IETF, all modules need to be renamed and
   all namespaces change.


o  Why so many modules?  Do you expect them to have independent life
   cycles?  If not, maybe consider using submodules.


o  If adopted by the WG, remove the 'deprecated' defintions.


o  Why do you have special types for all sn-name, sfc-name etc?  If
   there is some important semantics or syntax this makes sense, but
   that does not seem to be the case here.


o  There are a couple of rpcs defined that manipulate the
   configuration.  This is bad design; configuration manipulation is
   defined in the protocol.  These kinds of rpcs are often not
   interoperable.  For example, you have an rpc called
   'delete-service-function'.  Which datastore is affected?  This
   comment applies to 'delete-all-service-function',
   'put-service-function', 'delete-service-function',
   'put-service-function-chains', and possibly
   'instantiate-service-function-chain' (I have no idea what this rpc
   actually does).


o  The rpc 'read-service-function' should also be removed.  The
   protocol handles reading of data better than special rpcs.  For
   example, both NETCONF and RESTCONF support getting a subset of the
   available data.


o  There are a couple of rpcs for manipulating rendered service
   paths.  A rendered service path is config false, and as such they
   cannot be directly manipulated with the generic protocol
   operations; thus it is ok to have special rpcs to write / delete
   them.  However, these rpcs need to be better described.  If I
   create a rendered path, is it expected to survive reboots?  Can it
   be deleted by the server at any time?

   Also, can these rendered paths be created by some other means than
   through these rpcs?  If not, maybe they should be modelled as
   config true instead?


o  How is "rendered-service-path-first-hop" different from
   "rendered-service-path-hop[hop-number=1]?

   I notice that these two structures have somewhat different
   content.


o  Both SF and SFF have a writable leaf called "rest-uri".  This is
   protocol-specific and should be removed.  (Also, what value do you
   expect an operator to write there?)


o  service-locator defines 'other-locator'.  I suggest you remove it.
   When would an operator actually write something there, and what
   would it mean?  I expect vendors/future specs to augment the
   locator-type choice with new cases, rather than using a catch-all
   'other-name' leaf.


o  Suggest you rename list "supported-dataplanelocator-types" to
   "supported-dataplanelocator-type".


o  Suggest you rename list "sff-interfaces" to "sff-interface".


o  The list statistic-by-timestamp has a uint64 timestamp as key:

              description
                  "Date and time of record creation in milliseconds
                  counting from 1.1.1970 00:00:00 UTC (= 0)";

   I suggest using yang:date-and-time instead.  A server that can keep
   the uint64 timestamp can easily convert to it to a
   yang:date-and-time.


o  Suggest you rename container "service-statistic" to
   "service-statistics".


o  In sfc-common you have:

    typedef sft-type-name {
        type string;
        description "Service function type name";
    }

   Is this really a free-form string?  Can an operator put any value
   there?


o  In service-function you have:

    leaf ip-mgmt-address {
      type inet:ip-address;
      description
        "The IP and port used to configure this service-function";
    }

   First, it is not IP and port; just IP.

   Second, this seems to be very underspecified.  Which protocol is
   used?  Which credentials?  What is the real intention of this leaf?

   Samme comment applies to this leaf in other models.


o  In service-function-forwarder you have:

          leaf sff-interface {
            type string;
            description
              "An individual interface on the SFF connected to the
               SF";
          }

   What is this string?


o   In list sfc-service-function you have an optional leaf 'order'.
    What happens if this leaf is not set?  What happens if two entries
    have the saem order?

    YANG has built-in support for ordered lists.  I suggest you make
    this list 'ordered-by user' instead.



/martin