Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check mplementation report by Roque in Opsawg Mailing list #110

Closed
oscargdd opened this issue May 21, 2020 · 5 comments · Fixed by #175
Closed

Check mplementation report by Roque in Opsawg Mailing list #110

oscargdd opened this issue May 21, 2020 · 5 comments · Fixed by #175
Assignees
Labels
enhancement New feature or request

Comments

@oscargdd
Copy link
Collaborator

message in mailing list: https://mailarchive.ietf.org/arch/msg/opsawg/8u62vbE8GcnQeS9PNPPsPR5Y-LA/

Major: Why importing the "ietf-l3vpn-svc" module?

I think this is a major problem as it violates the architecture in Section 4 and the note in section 5 that states: "Note that the use of L3NM within a Service Provider does assume nor preclude exposing the VPN service via L3SM.".

If there is no dependencies with L3SM, we should not force the network controller to support it, neither its libraries. I noted that the SVC data model is mainly important to have a common library of types, identities and features. However, the correct implementation would be a separate YANG module for these libraries that both L3SM and L3NM import from.

All in all, importing the SVC module breaks the separation between the two layers. If we would like to re-use types, identities, features definitions, a separate module for them should be defined.

2) Minor: RD Semantic:
The document proposes:

    leaf rd {
      type union {
      type rt-types:route-distinguisher;
       type empty;
    }
}

Leafs have an implicit empty state (unless you use the default statement) but the authors are trying to give a specific meaning to the empty state by using a union statement. I believe a much natural representation would be:

    leaf rd {
      type rt-types:route-distinguisher;
    }

    leaf auto-assign-rd {
      when "not(../rd)";
      type boolean;
    }
@oscargdd oscargdd changed the title Check Review by Roque in Opsawg Mailing list Check mplementation report by Roque in Opsawg Mailing list May 21, 2020
@oscargdd
Copy link
Collaborator Author

After the discussions, it seems reasonable to have a separate Yang module to contain the types. The suggestion is to write the module to cover the four service models (client service models, l3sm, l2sm and Network service models, l2nm, l3nm). It seems reasonable to include this module in l3nm draft instead of creating a new one to avoid dependencies.

@sbarguil sbarguil added the enhancement New feature or request label May 21, 2020
@oscargdd
Copy link
Collaborator Author

Waiting for more feedback in mailing list per OPSA WG Chairs request

@boucadair
Copy link
Collaborator

After the discussions, it seems reasonable to have a separate Yang module to contain the types. The suggestion is to write the module to cover the four service models (client service models, l3sm, l2sm and Network service models, l2nm, l3nm). It seems reasonable to include this module in l3nm draft instead of creating a new one to avoid dependencies.

Looks good to me.

We need to include a note that L3SM defines some of the types in this common module, but a new module is defined to better fit rfc8407#section-4.12 and rfc8407#section-4.13.

@sbarguil
Copy link
Collaborator

#32

@boucadair
Copy link
Collaborator

@oscargdd: I think this one can be closed. The rd question is discussed in #114.

@oscargdd oscargdd linked a pull request Jul 23, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants