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

Check if HDF5 "file" is a DAOS object #2021

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Conversation

brtnfld
Copy link
Contributor

@brtnfld brtnfld commented Jun 16, 2021

PR to skip the POXIS calls used in reading magic number. These POSIX calls can not be used for a DAOS object.

Intel DAOS team feedback is requested as we might be able to remove the HDF5 calls.

This solution assumes Unified Namespace and getfattr is available.

@brtnfld brtnfld requested a review from WardF as a code owner June 16, 2021 15:06
@brtnfld
Copy link
Contributor Author

brtnfld commented Jun 16, 2021

We might refrain from using getfattr if we modify H5Fis_accessible to return the VOL access type instead. Investigating.

Copy link

@christgau christgau left a comment

Choose a reason for hiding this comment

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

Wouldn't it be better to directly call getxattr(2) from sys/xattr.h here instead of calling a command which has to be in the path (sure, it is very likely the case) and could potentially do something else than returning file attributes? In the end, getfattr does same call to retrieve the attributes.... at least on a CentOS 7 installation.

@DennisHeimbigner
Copy link
Collaborator

I do not think this code is in the proper location. I think it should be part of the later
file reading checks.

@brtnfld
Copy link
Contributor Author

brtnfld commented Jun 18, 2021

I do not think this code is in the proper location. I think it should be part of the later
file reading checks.

I think it has to be before phase 7 because that looks like the first place you start to read the magic number. Where do you suggest?

@brtnfld
Copy link
Contributor Author

brtnfld commented Jun 18, 2021

I do not think this code is in the proper location. I think it should be part of the later
file reading checks.

Wouldn't it be better to directly call getxattr(2) from sys/xattr.h here instead of calling a command which has to be in the path (sure, it is very likely the case) and could potentially do something else than returning file attributes? In the end, getfattr does same call to retrieve the attributes.... at least on a CentOS 7 installation.

Yes, that would be better.

@DennisHeimbigner
Copy link
Collaborator

FYI, I am working on this again.

@DennisHeimbigner
Copy link
Collaborator

If there is some way to modify this to avoid using popen, it would be appreciated.

@brtnfld
Copy link
Contributor Author

brtnfld commented Aug 4, 2021

Let me see what I can do, the eventual plan is to have H5Fis_accessible return if it is a DAOS container.

@WardF WardF added this to the 4.9.1 milestone Mar 10, 2022
@edwardhartnett
Copy link
Contributor

At the EGU I learned of some large data producers wanting to use DAOS for some of their very large data sets. I think it would be great of netCDF handled DAOS better.

I don't think this should hold up the long-awaited 4.9.0 release, but it would be great if this could be figured out for the 4.9.1 release...

@DennisHeimbigner
Copy link
Collaborator

To date, we have no solution.

@edwardhartnett
Copy link
Contributor

@WardF can we get this merged? I'd love to test it, and I think it's an important feature. I note that it's scheduled for the 4.9.1 release, so if we can get it in I can test it in the next release candidate...

@brtnfld
Copy link
Contributor Author

brtnfld commented Nov 4, 2022

Here is the response I received from Intel:

You can use the extended attribute, check for its existence:
/** extended attribute name that will container the UNS info */
#define DUNS_XATTR_NAME "user.daos"

Ideally you would use that macro, or the duns and call duns_resolve_path(), but you can’t do that I assume in this case since you would need to build netcdf with daos, right?
You can just call lgetxattr on the path with that name and I would think it should be OK. If we change the actual name, one would need to update netCDF but I don’t see that happening.

@WardF WardF modified the milestones: 4.9.1, 4.9.2 Feb 13, 2023
@WardF WardF modified the milestones: 4.9.2, 4.9.3 May 16, 2023
Copy link
Collaborator

@DennisHeimbigner DennisHeimbigner left a comment

Choose a reason for hiding this comment

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

I see numerous issues with this PR. Most importantly, the DAOS check
appears to be in the wrong place in the code. Should it not be in openmagic?
Oh well, we can fix it later.

@WardF
Copy link
Member

WardF commented May 22, 2024

@DennisHeimbigner it has not been merged, so now would be a good time to fix it too :)

@DennisHeimbigner
Copy link
Collaborator

Well let me take a look.

@DennisHeimbigner
Copy link
Collaborator

This is coming back to me. It makes me uneasy from a security point of view to use popen
because it in effect is executing a shell script of sorts as part of the netcdf-c library.
I really wish there was another, more direct, way to test for DAOS.

@DennisHeimbigner
Copy link
Collaborator

Also, there is not test case for this. Is there any open DAOS based server against we can test?

@dopplershift
Copy link
Member

There's a getxattr() api that can (and probably should) be used instead.

@brtnfld
Copy link
Contributor Author

brtnfld commented May 23, 2024

I think this only needs to be done now if dfuse is not being used, as a dfuse netcdf file should appear as a POSIX file. This addressed the case of unified name space being used, which seems less common now.

@christgau
Copy link

I think this only needs to be done now if dfuse is not being used, as a dfuse netcdf file should appear as a POSIX file.

True.

This addressed the case of unified name space being used, which seems less common now.

But still is a valid use case. Supporting this appears to be lightweight in terms of code. I would highly appreciate it to be available (checking extended file attributes directly, instead via an popen call).

@christgau
Copy link

Is there any indicator which DAOS version this branch is developed against?

@brtnfld
Copy link
Contributor Author

brtnfld commented May 28, 2024

At the time, it would have been DAOS 2.0

@DennisHeimbigner
Copy link
Collaborator

Attached is an revised version of dinfermodel.c for you to test. It isolates the DAOS code
and makes it a bit more conformant to the dinfermodel.c coding style.
It also implements the daos check using the API calls listxattr and getxattr
(see dinfermodel.c#isdaoscontainer.c).

Please see it works for your tests.
dinfermodel.c.txt

@brtnfld
Copy link
Contributor Author

brtnfld commented Jun 25, 2024

Your patch works and correctly detects the daos file with the daos-vol, which was tested on Google Cloud. I see other testing errors, but I don't think they relate to this issue.

@WardF WardF self-assigned this Jun 25, 2024
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

6 participants