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

libflux: support ancestor paths as alternative to URI in flux_open(3) and flux_open_ex(3) #6573

Merged
merged 7 commits into from
Jan 24, 2025

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Jan 23, 2025

This PR adds support for path-like shorthand arguments to flux_open(3) and flux_open_ex(3) to refer easily to ancestor Flux instances using a familiar filesystem idiom. The string ".." refers to the parent, "../.." its parent, and so on. "/" can be used to refer directly to the root instance, and for completeness "." indicates the current instance (and is therefore ignored as a path element). It is not an error to specify ".." at the root, so specifying more ".." elements than parents just returns a handle to the root instance.

I did my best in the documentation, but feel free to make suggestions.

Comment on lines 174 to 176
/* Return local URI from either FLUX_URI environment variable, or
* if that is not set, the builting config. Caller must free result.
*/
Copy link
Member

Choose a reason for hiding this comment

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

builting -> builtin

@chu11
Copy link
Member

chu11 commented Jan 23, 2025

I think this is a great idea. One high level comment looking at the tests (and haven't looked through the meat of the code yet), I would have thought "paths" like // and /. would work, i.e. they would return the root instance, but you have them listed as bad args.

Perhaps a simple documentation and/or code tweak would be fine. In the docs you use the words "path is composed of ...", which doesn't feel true if // or /. don't work.

Perhaps just make this simpler. The valid inputs are . or / or .. or a series of .. separated by / and that's it. So don't allow Test(".././..", 1), to work. The only valid input after a slash is ... I don't know if this makes the code easier or not, but it feels like it would make the code simpler?

@grondo
Copy link
Contributor Author

grondo commented Jan 23, 2025

I would have thought "paths" like // and /. would work, i.e. they would return the root instance, but you have them listed as bad args.

It doesn't make sense to support true "absolute paths" here (like provided by flux-uri(1) in #6562, so / with no other characters is just a special case to return the root instance. Seems like // is probably an input error and should return an error.

Perhaps a simple documentation and/or code tweak would be fine. In the docs you use the words "path is composed of ...", which doesn't feel true if // or /. don't work.

This is the documentation:

  • One or more ".." separated by "/" refer to a parent instance
    (possibly multiple levels up the hierarchy)
  • "." indicates the current instance
  • A single slash ("/") indicates the root instance
  • ".." at the root is not an error, it just returns a handle to the root
    instance

This is pretty explicit that a single slash is the root instance and only .. separated by / refers to ancestors. Perhaps we could remove support for . if this seems confusing.

@chu11
Copy link
Member

chu11 commented Jan 23, 2025

The sentence I was referring to was:

This string is composed of the elements "/", "..", and "." with familiar path-like rules, including:

The way I read it was "I can put a bunch of these together" and the examples below say / and . are special cases.

Maybe it's just me ... obviously this is more about "what corner cases shall me allow" vs "what is sensible". Obviously // and /. aren't sensible and it's more than valid to reject them.

@grondo
Copy link
Contributor Author

grondo commented Jan 23, 2025

Ah, good point. I can see that now. I wonder if I should just delete that line.

@grondo
Copy link
Contributor Author

grondo commented Jan 24, 2025

Thanks, I've reworded to:

ANCESTOR PATHS
       As an alternative to a URI, the flux_open()  and  flux_open_ex()  func‐
       tions  also  take a path-like string indicating that a handle to an an‐
       cestor instance should be opened. This  string  follows  the  following
       rules:

          • One or more ".." separated by "/" refer to a parent instance (pos‐
            sibly multiple levels up the hierarchy)

          • "." indicates the current instance

          • A single slash ("/") indicates the root instance

          • ".." at the root is not an error, it just returns a handle to  the
            root instance

A bit better @chu11? Happy to take any other suggestions.

@chu11
Copy link
Member

chu11 commented Jan 24, 2025

@grondo yup, i think that's a bit more clear saying "only do this"

Copy link
Member

@chu11 chu11 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just some small nits. Also, note the typo I made in a comment earlier, still seems to be there.

Comment on lines 143 to 147
def test_pathlike_bad_arg(self):
for arg in ("/.", "//", "../f"):
with self.assertRaises(OSError, msg=f"arg={arg}"):
h = flux.Flux(arg)
print(h.attr_get("instance-level"))
Copy link
Member

Choose a reason for hiding this comment

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

could add a test for bad stuff in middle of path, i.e. ../blah/..

Comment on lines 305 to 309
out:
free (copy);
errno = EINVAL;
return rc;
}
Copy link
Member

Choose a reason for hiding this comment

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

it seems a tad strange to set errno on a successful return. maybe move to the "goto out" part above.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Problem: The code to obtain the default URI when no URI is specified
is embedded in `flux_open_ex(3)` and can't be reused.

Move this code to a new internal function. As a side effect, the value
of the default URI is always copied, so rename the local variable
containing the allocated memory.
Problem: It is awkward for users using the C or Python API to open
a Flux handle to an ancestor instance from within a child job. The
caller must first open a local handle, then fetch the `parent-uri`
attribute, close the local handle, and finally open a new handle to
the parent. These steps have to be repeated for traversing further
up the hierarchy. A simplified approach is needed to improve the
user experience.

Simplify access to ancestor Flux instances by enabling relative paths
in `flux_open(3)`. Use "/" for the root instance, ".." for the parent,
and "../.." for its parent, etc.  A single "." represents the current
instance and is ignored in path elements. For consistency with the
filesystem idiom, ".." at the top level is not an error and connects
to the root instance.

Examples:
 - `flux_open ("/")` returns a handle to the root instance
 - `flux_open (".")` returns a handle to the current instance, it is
   equivalent to `flux_open (NULL)`.
 - `flux_open ("..")` returns a handle to the parent
 - `flux_open ("../..")` returns a handle to the parent
Problem: t3100-flux-in-flux.t has a test that occasionally fails
in CI or `-j` heavy `make check`.

Extend the timeout in this test.

Fixes flux-framework#6571
Problem: There are no tests of `flux_open(3)`'s handling of
relative ancestor paths.

Add a couple tests to t3100-flux-in-flux.t to test the handling
of these path-like arguments.
Problem: An extra backtick appears in flux_open.rst.

Remove it.
Problem: The fact that `flux_open(3)` and `flux_open_ex(3)` can
take path-like arguments in place of a URI is not documented.

Document them in `flux_open(3)`.
Problem: The arguments to `flux.Flux()` are not documented.

Document the args and reference `flux_open(3)` so they appear in the
Python docs.
@grondo
Copy link
Contributor Author

grondo commented Jan 24, 2025

Thanks @chu11! Addressed all the comments (including the typo from earlier) and will set MWP.

@mergify mergify bot merged commit 01cad95 into flux-framework:master Jan 24, 2025
35 checks passed
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 83.15789% with 16 lines in your changes missing coverage. Please review.

Project coverage is 79.46%. Comparing base (16f7233) to head (e97e094).
Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
src/common/libflux/handle.c 83.15% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6573      +/-   ##
==========================================
- Coverage   79.47%   79.46%   -0.02%     
==========================================
  Files         531      531              
  Lines       88312    88400      +88     
==========================================
+ Hits        70184    70245      +61     
- Misses      18128    18155      +27     
Files with missing lines Coverage Δ
src/bindings/python/flux/core/handle.py 98.66% <ø> (ø)
src/common/libflux/handle.c 76.76% <83.15%> (+0.96%) ⬆️

... and 9 files with indirect coverage changes

@grondo grondo deleted the uri-pathlike branch January 24, 2025 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants