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

Some deprecated or non-described fields are made mandatory in Yang #594

Open
roshan-joyce-fujitsu opened this issue Jul 17, 2024 · 16 comments
Assignees

Comments

@roshan-joyce-fujitsu
Copy link
Collaborator

Please see https://github.com/Open-Network-Models-and-Interfaces-ONMI/TAPI/blob/v2.5.0/YANG/tapi-topology.yang#L701
Because it says min-elements 1, a data store that enforces Yang rules will mandate that there is at least one validation-mechanism specified in the list in each Link. However, there is no definition of validation-mechanism in RIA.

Please see https://github.com/Open-Network-Models-and-Interfaces-ONMI/TAPI/blob/v2.5.0/YANG/tapi-topology.yang#L528
Because it says min-elements 2, a data store that enforces Yang rules will mandate that there is at least one entry in the transitioned-layer-protocol-name list in each Link, even if it is not representing a tranisitonal-link. Please note that transitional-link concept has been deprecated in the RIA.

I think we need to remove the min-elements constraint from these list definitions.

CC: @amazzini

@amazzini
Copy link
Collaborator

The UML2YANG tool translated 1 to 1 and 1 to 0..1 compositions into "uses" statement, which is essentially mandatory, hence the optionality is lost. This may imply the need of a systematic review, to remove some "min-elements" which contradicts main statement.

@nigel-r-davis
Copy link
Collaborator

nigel-r-davis commented Jan 28, 2025

Fully agree. We need to remove the min-elements in these cases.

Going further, based on our discussion on the TAPI call today, no properties should be mandatory in the YANG. The RIA should state the conditions under which a particular property is needed (for compliance).

We should discuss this on a call and then take the necessary action prior to the next delivery.

@nigel-r-davis
Copy link
Collaborator

nigel-r-davis commented Feb 11, 2025

We agreed to fix the specific properties identified here.

We also need to review the YANG to identify how many more cases there are. If there are sufficiently few we could consider fixing them all.

@bcjohnso99 I suggest we attempt do the fix identified here in 2.6. I can review the YANG and identify where all the changes need to be. We could then review the list on a call shortly. If excessive or controversial, perhaps we back off and do in 2.7.

@nigel-r-davis
Copy link
Collaborator

nigel-r-davis commented Feb 18, 2025

Agreed to review the YANG and identify where all the changes need to be. Then review the list on the call 4 March.

If excessive or controversial, perhaps we back off and do in 2.7. There appear to be approx 22 so may be able to deal with.

@nigel-r-davis
Copy link
Collaborator

nigel-r-davis commented Mar 1, 2025

@bcjohnso99, (Kishore has vanished from the list again :( ): We should review this on the call 4 March.

The specific issue highlighted by @roshan-joyce-fujitsu and explained by @amazzini is that the UML to YANG tool codes a specific structure in UML to uses in YANG. The structure in UML has a natural optionality such that a mandatory element below this structure is not mandatory overall. This indirection is not present in the YANG.

There were more cases of this "uses" problem in Topology YANG than highligh in this issue. In all cases the min-elements has been removed. The full set I have changed in a draft YANG are:

  • layer-protocol-transition-pac / transitioned-layer-protocol-name (min-elements 2;)
  • risk-parameter-pac / risk-characteristic (min-elements 1)
  • risk-characteristic / risk-identifier-list (min-elements 1)
  • transfer-cost-pac /cost-characteristic (min-elements 1)
  • transfer-timing-pac / latency-characteristic (min-elements 1)
  • validation-pac / validation-mechanism (min-elements 1)

In all cases, I need to check the UML and in some cases will need to fix it (e.g., where there is min-elements 2)

On the meaning of validation-mechanism, I have expanded the text to add three sentences (in bold below) to the one present in the original YANG. As follows:

  • "Provides details of the specific validation mechanism(s) used to confirm the presence of an intended topological entity. Validation mechanism examples include a specific discovery protocol, a test carried out a commissioning, information from the plan. There is no formal format for the string. Considering each example, the string could be 'discovery protocol', 'commissioning test' or 'plan data'."

This will require an adjustment to the UML to align. I have not yet made the UML adjustment highlighted here.

There are other cases of min-elements in topology.yang:

  • link / layer-protocol-name (min-elements 1) - This seems rational, although it could be argued that a link under certain circumstances, e.g., abstract planning, a link has no relevant protocol. We can make a choice whether to keep this or not.
  • link / node-edge-point (min-elements 2) - There are cases where a link may have only one known node edgepoint. IETF RFC8345 allows a link to have no termination points. I suggest that we remove this constraint. See also tapi-connectivity.yang in separate comment below.
  • node / layer-proptocol-name (min-elements 1) - We should make the same choice as we do for link.
  • topology / layer-protocol-name (min -elements 1) - We should make the same choice as we do for link.
  • inter-rule-group / rule (min-elements 1) - This is protected by a list in node that can have zero elements, so it does not impose a rule on all nodes. Where there is a group, the must be a rule. This seems rational. We could make a choice to remove this restriction as the structure is quite extensive and there may be cases where there is an unstated rule.
  • inter-rule-group / associated-node-rule-group (min-elements 2) - An inter-rule-group is used to state a rule between two rule-groups, so this limit does seem fundamental to the definition. It is possible that a group is used to associate the group to itself and during transition of the rule system there may be one group only, however, this is extremely unlikely. It seems that this limit could stay.
  • node-rule-group / rule (min-elements 1) - This is the same as inter-rule-group (above) and the decision should be the same.

There are also min-elements statements in common (2), connectivity (2), equipment (1), oam (2), path-computation (4) and streaming (1) which I plan to detail in this issue.

@nigel-r-davis
Copy link
Collaborator

nigel-r-davis commented Mar 3, 2025

Moving on to the other models:

tapi-common.yang

  • service-interface-point has a supported-cep-layer-protocol-qualifier (min-elements 1) - This does not seem unreasonable as a SIP without supported CEPs is not useful.
    • However, it is possible that at some lifecycle stages there are actually no supported CEPs.
    • I also note that there is a supported-payload-structure which does not have a min-elements criteria. Clearly, there must also be a payload structure.
    • I propose we remove the limit.
  • time-interval (min-elements 1, max-elements 5) - This apparently complies with ITU-T Q.821. This seems a very specific constraint!
    • time-interval is not used in common but is used in tapi-oam.yang in current-data (which is now deprecated) and pm-data-pac (used in current-data and history-data).
    • In my opinion, it seems overly constraining to limit this field to the Q.821 definition as other sources may want a broader definition.
    • The direct usage in current appears to only require one value.
    • The usage in history data is via the pm-data-pac and is defined as "The granularity period or measurement interval time.". This appears to only need one value.
    • I propose we remove the limit.

tapi-connectivity.yang

  • There are several constraints, all of which relate to the ends of a "pipe".
    • connection / connection-end-point-ref (min-elements 2)
    • connectivity-service / end-point (min-elements 2)
    • route / connection-end-point-ref (min-elements 2)
    • All of the above have the same criteria as link / node-edge-point in topology discussed earlier. I suggest the same action here.

tapi-equipment.yang

  • access-port / connector-pin (min-elements 1) - used as part of the key, so needs to be mandatory and 1 (should really also be max-elements 1)
  • abstract-strand / connector-pin (max-elements 2) - used as part of the key, so needs to be mandatory and 2 (should really also be min-elements 1??)
  • abstract-strand / spliced-strand (max-elements 2) - whilst a splice is normally between two strands, there could be cases of more. I suggest we remove this limit. It is difficult to see how a splice can have one strand (or no strands) so a min-elements of 2 could be argued, however, this is similar to the link case and should be dealt with in the same way (which I suggest is to remove the limit)

tapi-notification.yang

  • notification / target-object-name (min-elements 1) - But target-object-identifier (UUID) is not mandatory.
    • note that in streaming the equivalents are not mandatory
    • I propose we leave this as it is and consider it in the broader notification discussions in 2.7 timeframe.

tapi-oam.yang

  • oam-profile / pm-parameter-config (min-elements 1) - oam-profile appears to only be used for PM threshold information which is made mandatory by the min-elements 1.
    • This appears to be in conflict with the comment on oam-profile which indicates that "The OamProfile allows centralization of OAM provisioning aspects, e.g. the PM parameters and their threshold values.". The "e.g." strongly suggests that other properties are to be added and hence pm-parameter-config should not be mandatory.
    • Note that
      • common / profile may use oam-profile (by conditional augment)
      • pm-threshold-data-ref uses oan-profile-ref (but pm-threshold-data-ref does not appear to be used??
    • I propose we remove this constraint

tapi-path-computation

  • path / link (min-elements 1) - the path also has node-edge-points (with no definition of purpose - there is limited definition in the RIA (something to work on in 2.7/2.8 perhaps)) which have no restriction. It is possible that a path could be between two ports of the same device and hence only have node-edge-points and no links (as there are no defined links on the boundary of the network).
    • I propose we leave the constraint as is and readdress it when we tackle path computation in 2.7/2.8

tapi-streaming.yang

  • log-record-header / full-log-record-offset-id (min-elements 1) - this is the fundamental identifier of the stream record and needs to be provided. I propose we leave this as is.

tapi-virtual-network.yang

  • there are several min-elements. I propose we do not change these as the model is in an early draft form with many issues.

@nigel-r-davis
Copy link
Collaborator

nigel-r-davis commented Mar 3, 2025

tapi-digital-otn.yang has several max-elements that I need to explore. All other models are covered above.

@nigel-r-davis
Copy link
Collaborator

Note that some of the fixes will cause changes to the UML. These changes are simple and the UML can be manually aligned with the YANG. Once we have agreed the changes, I will take an action to align the UML.

None of the fixes related to this issue will cause changes to the OAS. <-- Need to confirm with the team.

@nigel-r-davis
Copy link
Collaborator

tapi-digital-otn
-There are the following constraints

  • odu-termination-and-client-adaption-pac / odu-tcm-mep (max-elements 6)
  • odu-ctp-pac / odu-mip (max-elements 2)
  • odu-ctp-pac / odu-tcm-mep (max-elements 12)
  • odu-ctp-pac / odu-tcm-mip (max-elements 12)
  • Whilst the standard for TCM currently constrains the measures in this way, it seems excessive to bound them in TAPI. I propose that we remove all four limits. We should discuss on the call. I will implement whatever the group agrees

@bcjohnso99 We should discuss on the call 4 March.

@bcjohnso99 bcjohnso99 added the 2.6 label Mar 4, 2025
@nigel-r-davis
Copy link
Collaborator

nigel-r-davis commented Mar 4, 2025

Agreed that we should do a more aggresive removal of min-elements and max-elements so for example we will remove it for link /layer-protocol-name... remove all for second bullet list.

4 March : Agreed to keep the tcm mep and mip max-elements in otn. Hence, no changes to tapi-digital-otn.yang.

@nigel-r-davis
Copy link
Collaborator

I have fixed the transitionedLayerProtocolName string in UML to * (from 2..*).

nigel-r-davis referenced this issue Mar 5, 2025
The specific issue is that the UML to YANG tool codes a specific structure in UML to uses in YANG. The structure in UML has a natural optionality such that a mandatory element below this structure is not mandatory overall. This indirection is not present in the YANG.

The cases of this "uses" problem in the topology model have all had the min-elements removed (from tapi-topology.yang):

- layer-protocol-transition-pac / transitioned-layer-protocol-name (min-elements 2;)

- risk-parameter-pac / risk-characteristic (min-elements 1)

- risk-characteristic / risk-identifier-list (min-elements 1)

- transfer-cost-pac /cost-characteristic (min-elements 1)

- transfer-timing-pac / latency-characteristic (min-elements 1)

- validation-pac / validation-mechanism (min-elements 1)

The UML has also been updated (converting 2..* and 1..* to 0..*).
nigel-r-davis referenced this issue Mar 5, 2025
The specific issue is that the UML to YANG tool codes a specific structure in UML to uses in YANG. The structure in UML has a natural optionality such that a mandatory element below this structure is not mandatory overall. This indirection is not present in the YANG.

The cases of this "uses" problem in the topology model have all had the min-elements removed (from tapi-topology.yang):

- layer-protocol-transition-pac / transitioned-layer-protocol-name (min-elements 2;)

- risk-parameter-pac / risk-characteristic (min-elements 1)

- risk-characteristic / risk-identifier-list (min-elements 1)

- transfer-cost-pac /cost-characteristic (min-elements 1)

- transfer-timing-pac / latency-characteristic (min-elements 1)

- validation-pac / validation-mechanism (min-elements 1)

The UML has also been updated (converting 2..* and 1..* to 0..*).
@nigel-r-davis
Copy link
Collaborator

nigel-r-davis commented Mar 5, 2025

My earlier comment

  • "tapi-equipment.yang... access-port / connector-pin (min-elements 1) - used as part of the key, so needs to be mandatory and 1 (should really also be max-elements 1)..... abstract-strand / connector-pin (max-elements 2) - used as part of the key, so needs to be mandatory and 2 (should really also be min-elements 1??)"

Was not correct as neither are parts of key. On that basis there is no reason for the min-elements. Considering the positioning on the call, I will remove these min-elements.

Note that the key for connector-pin list (which is read only) is a list of elements where some elements are conditional. Whilst this looks reasonable conceptually, it is not clear whether this is YANG conformant. As this has been there for several releases without causing an issue reports, it is probably OK (and certainly OK for 2.6). We should readdress this in 2.7.

@bcjohnso99: We need to add this final part for 2.7. To be discussed on the call 11 March.
@bcjohnso99: We need to add work items for each of the items I deferred in comments above.

nigel-r-davis added a commit to nigel-r-davis/TAPI that referenced this issue Mar 6, 2025
Continuing with fixes for issue Open-Network-Models-and-Interfaces-ONMI#594

max-elements and min-elements statements removed (as agreed) as follows:
tapi-common.yang

- service-interface-point has a supported-cep-layer-protocol-qualifier (min-elements 1)

- time-interval (min-elements 1, max-elements 5)

tapi-connectivity.yang

- connection / connection-end-point-ref (min-elements 2)

- connectivity-service / end-point (min-elements 2)

- route / connection-end-point-ref (min-elements 2)

tapi-equipment.yang

- access-port / connector-pin (min-elements 1)

- abstract-strand / connector-pin (max-elements 2)

- abstract-strand / spliced-strand (max-elements 2))

tapi-oam.yang

- oam-profile / pm-parameter-config (min-elements 1)
@nigel-r-davis
Copy link
Collaborator

nigel-r-davis commented Mar 6, 2025

I have completed the agreed (and corrected) changes to the YANG and UML for this issue (as described above). I have also updated the comment for validation-machanism in the YAML (found in 4 places and corrected in all). The changes are currently in https://github.com/nigel-r-davis/TAPI develop.

Note that I have not changed the .tree as it has no indication of min-elements/max-elements. Each list is simply "*".

I will complete a pull request once all other changes agreed at meeting 4 March have been done.

nigel-r-davis added a commit to nigel-r-davis/TAPI that referenced this issue Mar 6, 2025
The comment in topology.yaml has been updated to align with the YANG/UML. This is also part of Open-Network-Models-and-Interfaces-ONMI#594.

Note: no changes are required to be made to the YAML for other parts of Open-Network-Models-and-Interfaces-ONMI#594 because the YAML does not (appear to) constrain number of elements of a list.
@nigel-r-davis
Copy link
Collaborator

nigel-r-davis commented Mar 6, 2025

I have just found one min elements statement in the RIA which I have removed.

There are no references to validation-mechanism in the RIA. We should consider documenting this in 2.7.

The pm-parameter-config had a statement in the RIA

Image

I have modified this to conditional and added a lead in "Where the profile is used to control PMs, the profile...".

Image

and

Image

We will need to review use of profiles in general in detail in 2.7. A careful review of OAM leading to some refinement is also necessary in 2.7 as OAM was still work in progress.

I am going to do a single update to the RIA will all changes captured from this and other issues.

@bcjohnso99 We need to collect any actions for 2.7 from the various 2.6 issues and create new issues to track through 2.7. We should do a pass through the issues once 2.6 delivery to TST is completed.

@roshan-joyce-fujitsu
Copy link
Collaborator Author

Hi @nigel-r-davis , @bcjohnso99

I am fine with the yang changes made so far.
Thank you.

@nigel-r-davis
Copy link
Collaborator

There are other YANG changes underway covered by other issues. This can be moved to approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants