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

Storage system query support #280

Merged
merged 9 commits into from
Mar 15, 2021
Merged

Conversation

shanedsnyder
Copy link
Contributor

Based off of #265 (thanks @rhc54 )

Work in progress as we sort out PMIX_STORAGE_TYPE attribute.

Signed-off-by: Ralph Castain <[email protected]>
@kathrynmohror
Copy link
Collaborator

Please use emoji reactions ON THIS COMMENT to indicate your position on this proposal.

  • You do not need to vote on every proposal
  • If you have no opinion, don't vote - that is also useful data
  • If you've already commented on this issue, please still vote so
    we know your current thoughts
  • Not all proposals solve exactly the same problem, so we may end
    up accepting proposals that appear to have some overlap
    This is not a binding majority-rule vote, but it will be a very
    significant input into the corresponding ASC decision.

Here are the meanings for the emojis:

  • Hooray or Rocket: I support this so strongly that I
    want to be an advocate for it
  • Heart: I think this is an ideal solution
  • Thumbs up: I'd be happy with this solution
  • Confused: I'd rather we not do this, but I can tolerate it
  • Thumbs down: I'd be actively unhappy, and may even consider
    other technologies instead
    If you want to explain in more detail, feel free to add another
    comment, but please also vote on this comment.

Chap_API_Storage.tex Show resolved Hide resolved
Chap_API_Storage.tex Show resolved Hide resolved
Chap_API_Storage.tex Show resolved Hide resolved
Chap_API_Storage.tex Outdated Show resolved Hide resolved
Copy link
Contributor

@SteVwonder SteVwonder left a comment

Choose a reason for hiding this comment

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

A few inline comments below. One general question is what error codes will be returned if the storage system doesn't know how to respond to a specific BW, Capacity, or IOPs query?

Chap_API_Storage.tex Outdated Show resolved Hide resolved
Chap_API_Storage.tex Outdated Show resolved Hide resolved
Chap_API_Storage.tex Outdated Show resolved Hide resolved
@shanedsnyder
Copy link
Contributor Author

A few inline comments below. One general question is what error codes will be returned if the storage system doesn't know how to respond to a specific BW, Capacity, or IOPs query?

@SteVwonder We are just assuming that storage systems and/or the PMIx server will return PMIX_ERR_NOT_SUPPORTED for unsupported storage system attributes similar to how different PMIx implementations can return this for unsupported function calls.

Chap_API_Storage.tex Outdated Show resolved Hide resolved
@jjhursey jjhursey added this to the PMIx v5 Standard milestone Sep 24, 2020
Chap_API_Storage.tex Outdated Show resolved Hide resolved
Chap_API_Storage.tex Outdated Show resolved Hide resolved
@rhc54 rhc54 mentioned this pull request Oct 1, 2020
@rhc54
Copy link
Member

rhc54 commented Oct 1, 2020

@shanedsnyder Please remember to add the "Signed-off-by: Name <email>" line to your commit(s) so they can be committed!

@jjhursey jjhursey added Eligible Eligible for consideration by ASC RFC Request for Comment labels Oct 1, 2020
@jjhursey
Copy link
Member

jjhursey commented Oct 1, 2020

ASC Meeting Oct. 1, 2020

  • Good feedback and discussion on this ticket. WG will take this back and update this PR.
  • Decided to use the "Provisional Acceptance" timeline/process for this ticket. (See governance document for exact procedure)
    • This means that the authors may continue to work on this PR and re-present it in the next quarterly meeting.
    • Well before the next meeting post another straw poll and PDF once it is ready for review.
    • Chairs will re-add the "Eligible" label and add it to the agenda.
    • In the next meeting they may present another reading and, if successful, vote.
    • If all goes well, then this would be included in the next minor release marked as "provisional"

@jjhursey jjhursey removed the Eligible Eligible for consideration by ASC label Oct 1, 2020
@jjhursey jjhursey added the Eligible Eligible for consideration by ASC label Jan 19, 2021
@jjhursey
Copy link
Member

1Q 2021 ASC Meeting (Feb. 18, 2021)

  • Revision Exception vote passed: 10 yes / 0 no / 2 abstain / 2 absent
  • This PR enters a 2 week comment period. If no objection then it will be merged for the next minor release.

@jjhursey jjhursey added the Accepted as Provisional ASC vote passed. Accepted as Provisional! label Feb 18, 2021
@jjhursey jjhursey mentioned this pull request Feb 26, 2021
@jjhursey jjhursey mentioned this pull request Mar 5, 2021
@jjhursey
Copy link
Member

jjhursey commented Mar 5, 2021

It looks like some of the commits were not signed off. You will need to interactive rebase and update the commit messages. That'll like rewrite history, but that's ok. Let me know if you need a hand with that.

Once that is done then we will need to add the provisional markers (pending PR #338), but the release managers for v4 can help with that.

@rhc54
Copy link
Member

rhc54 commented Mar 5, 2021

@jjhursey We can't accept this yet because there is no prototype implementation - we still have not seen the final PR for OpenPMIx. AFAIK, that is a requirement even for provisional acceptance, yes?

@jjhursey
Copy link
Member

jjhursey commented Mar 5, 2021

An implementation is not required (looking at 1.3.3 of the governance doc), but anyone can request one as a reason to hold the merge. I'm ok with holding merging this until we get one.

@rhc54
Copy link
Member

rhc54 commented Mar 5, 2021

Somehow, that just seems like an oversight - nobody should be able to put something in the Standard that has never been implemented, at least as a prototype. We have no way of knowing if the proposal is even feasible. Should definitely amend that section to correct this.

@jjhursey
Copy link
Member

jjhursey commented Mar 5, 2021

I'm open to it. Do you want to file an issue in the governance repo for discussion?

@shanedsnyder
Copy link
Contributor Author

It looks like some of the commits were not signed off. You will need to interactive rebase and update the commit messages. That'll like rewrite history, but that's ok. Let me know if you need a hand with that.

Just fixed this.

@shanedsnyder
Copy link
Contributor Author

FYI, what I have for initial implementation is here: openpmix/openpmix#2086

Do we need implementation for every single attribute? Some of these aren't going to be practical in the short-term... The bandwidth and IOPS attributes obviously are going to require storage system APIs to provide the info -- Lustre has said that's something they could provide in the future, but I don't know that anything is available now. Similarly, the attributes related to storage medium and storage accessibility are going to require system-specific knowledge that I'm not sure how we'd support in OpenPMIx.

I understand the desire to want things implemented before accepting into the standard just really wish this issue was raised sooner, as we've potentially wasted a lot of time now. If we need to focus on things that can be implemented today, I suspect we're going to be really limited in what we can provide.

@rhc54
Copy link
Member

rhc54 commented Mar 7, 2021

I don't think you have to implement support for everything. I just want to see that the concept has been vetted. What you posted should suffice for that purpose, and I appreciate it.

@jjhursey
Copy link
Member

jjhursey commented Mar 8, 2021

I think there is a balance we need to strike with implementation requirements. Thanks Ralph for opening https://github.com/pmix/pmix-standard/issues/341 for discussion. We have a monthly call this week I'll add it to the agenda for discussion.

@rhc54
Copy link
Member

rhc54 commented Mar 9, 2021

@pmix/asc-chairs @pmix/asc-secretaries I believe this is ready to be committed, having met the review/vote as well as the implementation requirements I had requested.

Thanks @shanedsnyder for all the hard work!

@abouteiller abouteiller self-requested a review March 11, 2021 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted as Provisional ASC vote passed. Accepted as Provisional! Eligible Eligible for consideration by ASC RFC Request for Comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants