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

Add mount-point type #1168

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

Add mount-point type #1168

wants to merge 4 commits into from

Conversation

dplore
Copy link
Member

@dplore dplore commented Aug 22, 2024

Change Scope

  • Add filesystem type as /system/mount-points/mount-point/type
  • This change is backwards compatible
  • The operational use case is to give a human a hint as to the type of storage device in use for troubleshooting and diagnostics purposes. Example storage types might include flash, HDD, temporary/ramdisk or network based.

Platform Implementations

@dplore dplore requested a review from a team as a code owner August 22, 2024 00:15
@OpenConfigBot
Copy link

OpenConfigBot commented Aug 22, 2024

No major YANG version changes in commit d447a80

@earies
Copy link
Contributor

earies commented Aug 23, 2024

Regarding mount-points (and a few other items that happen to be modeled under /system today), I'd be curious to get your thoughts of a few things since this is being touched. First I'll place the contents of this subtree including this patch here to illustrate:

module: openconfig-system
  +--rw system
     +--ro mount-points
        +--ro mount-point* [name]
           +--ro name     -> ../state/name
           +--ro state
              +--ro name?                string
              +--ro storage-component?   -> /oc-platform:components/component/name
              +--ro size?                uint64
              +--ro available?           uint64
              +--ro utilized?            uint64
              +--ro type?                string

Now, the first is anything under /system related to resources has primarily been designed to assume the active CONTROLLER_CARD... more of an assumption. In this case there is at least a child leaf that is a leafref to a component however our list key here is a string name. Assume you want to convey both primary/secondary controller cards, one has to come up w/ a unique naming scheme now (or if you can convey this information from other components of the distributed system). A proper naming scheme to account for uniqueness would be to encode the component name thus likely duplicating what the leafref is also doing.

For storage mount-points in general, I think it would be helpful to map each leaf value/description to something like the output of the df command. In some cases this is self explanatory, in others not as much. For this patch, I'll assume that the type maps to what is under the Filesystem column whereas the key name is associated to Mounted on - would you agree?

But overall, I'm of the opinion we consider the possible relocation of this hierarchy to /components/component/storage as it can be unique to each component and categorized as such vs. attempting the inverse.

The same holds true for CPU, memory, processes, utilization (some of which has some level of duplication already)

@dplore
Copy link
Member Author

dplore commented Aug 26, 2024

Yes, the type maps to linux df's "Filesystem" column and the key name maps to df's "Mounted on"

/system is indeed meant to represent to global state of the device. It seems no notion of redundant controller cards was ever explicitly factored into it's creation. To date we've expected the primary controller card's state to be transparently reflected as part of /system. So a management system does not need to know there are redundant cards when looking at this tree. So /system/mount-points is expected to reflect the PRIMARY CONTROLLER_CARD values.

It does seem more complete to also have a way to inspect the properties of the SECONDARY CONTROLLER_CARD in general. So far I we haven't had this use case. This could be evaluated in a separate pull request

Copy link

@s19nal s19nal left a comment

Choose a reason for hiding this comment

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

as reviewed on OC operators meeting - this looks good to me

@dplore dplore added the last-call PR that is in final review before merging. label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
last-call PR that is in final review before merging. non-breaking
Projects
Status: last-call
Development

Successfully merging this pull request may close these issues.

4 participants