Skip to content

Expand mount-point/state/type to union of string and enum #1226

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

mrevang
Copy link

@mrevang mrevang commented Nov 15, 2024

Change Scope

  • This change is being made to add more structure to the content of mount-point/state/type. This will enable more predictable content for programming against.
  • The underlying type is being updated from a string to a union (indentyref/string), so it's not backwards compatible. In practice, a string will ultimately still be decoded, so it is backward compatible in that regard.

Platform Implementations

  • Before mount-point/state/type was added to the standard, it was a Google specific augment.

@mrevang mrevang requested a review from a team as a code owner November 15, 2024 21:03
@dplore
Copy link
Member

dplore commented Nov 15, 2024

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented Nov 15, 2024

Major YANG version changes in commit 4d31308:
openconfig-system.yang: 2.3.0 -> 3.0.0

@dplore
Copy link
Member

dplore commented Nov 15, 2024

/gcbrun

type string;
type union {
type string;
type enumeration {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few overall comments:

  • Depending how this is looked at, it is a backwards incompatible change as you are ultimately changing the underlying YANG type which can have other effects outside of the encoded value representation
  • I would propose if doing this to take the enum declaration out into a reusable typedef or better yet a set of expansible identities (and use an identityref rather)
  • Reference to /proc/filesystems is useful here and there is also an implicit assumption of underlying OS in doing this (this already exists in other spots too)
  • IMO drop the union - just keep an identityref then and implementations can augment in expanded identities to the base if necessary. If you order the union as such then the rule of thumb is that the value matches the string as first match everytime as both are encoded identically

But if this is looked at as a mapping of the first column of something like df output (Filesystem) then you have a mish-mash of what can be represented here (This is likely what each implemtation is doing today) meaning alot of different string values (or expanded identities which then don't make as much sense imo)

If this is to be the type from something like the output of mounts then corresponds directly w/ the list you might see from /proc/filesystems

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we also need the enumeration to be listed above the string in the union definition (I appreciate that you are suggesting that the union isn't needed at all), otherwise everything would always match against the string, regardless of the enum values?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evan made this change for us (moved the indentityref before the string)

@dplore
Copy link
Member

dplore commented Dec 3, 2024

Thanks for the comments. We reviewed in the Dec 3 operators meeting and there is some rough consensus that making this change to this very new leaf as an indentify-ref like @earies suggests is cleaner and still supports the operational use case. We have good precedence of adding more identity refs as needed in areas like transceiver types.

@mrevang could you revise this remove the string entirely and become an identity-ref?

@dplore
Copy link
Member

dplore commented Jul 8, 2025

Thanks for the comments. We reviewed in the Dec 3 operators meeting and there is some rough consensus that making this change to this very new leaf as an indentify-ref like @earies suggests is cleaner and still supports the operational use case. We have good precedence of adding more identity refs as needed in areas like transceiver types.

@mrevang could you revise this remove the string entirely and become an identity-ref?

The new yang would look like:

    leaf type {
      type identityref {
        base oc-fs-types:OPENCONFIG_FS_MOUNT;
      }

Then in a new file, such as release/models/system/openconfig-fs-types.yang, add identities for all the desired filesystem types. These identities will be encoded the same as strings (should anyone have implemented it)

  identity OPENCONFIG_FS_MOUNT {
    description
      "Base identity for system filesystem mounting point types. 
      Derived identities are based on the linux mount api fs_type.";
    reference
      "https://docs.kernel.org/filesystems/mount_api.html";
  }

  identity UNKNOWN {
    base OPENCONFIG_FS_MOUNT;
    description
      "Indicates that the mount type could not be determined.
      Use of this identity SHOULD be avoided.";
  }

  identity CGROUP_FS {
    base OPENCONFIG_FS_MOUNT;
    description
      "cgroup filesystem type.";
  }

  identity DEBUGFS_FS {
    base OPENCONFIG_FS_MOUNT;
    description
      "debugfs filesystem type.";
  }

and so on

@mrevang mrevang requested a review from mike-albano as a code owner July 9, 2025 02:40
@dplore
Copy link
Member

dplore commented Jul 9, 2025

/gcbrun

@mrevang
Copy link
Author

mrevang commented Jul 9, 2025

Thanks all for the comments and suggestions. I've removed the union, and replaced the string with an identifyref.

@dplore
Copy link
Member

dplore commented Jul 10, 2025

/gcbrun

@dplore
Copy link
Member

dplore commented Jul 10, 2025

/gcbrun

@mrevang mrevang changed the base branch from master to lag-ethernet-cfg July 10, 2025 17:45
@mrevang mrevang changed the base branch from lag-ethernet-cfg to master July 10, 2025 17:45
@dplore dplore self-assigned this Jul 10, 2025
@dplore dplore moved this from Waiting for author to Ready to discuss in OC Operator Review Jul 10, 2025
@dplore
Copy link
Member

dplore commented Jul 10, 2025

/gcbrun

@dplore
Copy link
Member

dplore commented Jul 10, 2025

This will be reviewed at the next OC Operators meeting on July 15, 2025

@dplore
Copy link
Member

dplore commented Jul 11, 2025

/gcbrun

@ElodinLaarz
Copy link
Contributor

Discussed at the OC Operators Meeting on July 15th. Seems generally fine. One suggestion is to change the description Please prefer the identifyref over the string to something of the form The string type exists to support backwards compatibility, ... to better reflect the desire for deprecating the string field.

@ElodinLaarz ElodinLaarz moved this from Ready to discuss to last-call in OC Operator Review Jul 15, 2025
@mrevang
Copy link
Author

mrevang commented Jul 15, 2025

Discussed at the OC Operators Meeting on July 15th. Seems generally fine. One suggestion is to change the description Please prefer the identifyref over the string to something of the form The string type exists to support backwards compatibility, ... to better reflect the desire for deprecating the string field.

Thanks for the suggestion, updated accordingly.

@mrevang mrevang closed this Jul 15, 2025
@mrevang mrevang reopened this Jul 15, 2025
@dplore
Copy link
Member

dplore commented Jul 16, 2025

/gcbrun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: last-call
Development

Successfully merging this pull request may close these issues.

6 participants