Skip to content

Add suffix pass-through VOL connector for testing #5477

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

Closed
wants to merge 2 commits into from

Conversation

mattjala
Copy link
Contributor

The suffix pass-through VOL connector is largely a clone of the internal passthrough connector, except that it internally adds a suffix to user-provided filenames, changing the storage destination of files (and the values of external links, for consistency).

The purpose of this connector is to verify that the VOL tools tests can run against connectors which modify how files are stored (e.g. subfiling, REST VOL) without having to skip certain tests due to unsupported features. Other changes to CMake infrastructure to support running the tools tests against connectors will come in later PRs.

Since this VOL isn't intended to be used by users, it's not added to the install directory.

Only these routines have been changed from the skeletons in H5VLpassthru.c:
-file create

  • file open
  • file specific
  • file get
  • link create
  • link get

@mattjala mattjala added Priority - 2. Medium It would be nice to have this in the next release Component - Tools Command-line tools like h5dump, includes high-level tools Type - New Feature labels Apr 23, 2025
@mattjala mattjala self-assigned this Apr 23, 2025
@jhendersonHDF
Copy link
Collaborator

The purpose of this connector is to verify that the VOL tools tests can run against connectors which modify how files are stored (e.g. subfiling, REST VOL) without having to skip certain tests due to unsupported features.

This was previously covered by the DAOS VOL connector (it was the main target for the first cut at VOL tools testing), so I'm not sure that the focus here should be on creating a testing VOL connector that largely does nothing different from the passthrough connector. Adding a suffix to a filename isn't really changing anything about how the files are stored either and seems like an awful lot of maintenance burden since this VOL connector is nearly an entire copy of the passthrough connector. One could make the argument for having this connector stack on top of the passthrough connector and pass through every single operation except for the ones it's trying to do differently, but that's also beginning to complicate the testing infrastructure. My suggestion is that we should resume VOL tools testing with all the available VOLs we currently test once the tools testing is in a usable state, rather than trying to simulate that in this manner since it's not "doing the real thing".

@mattjala
Copy link
Contributor Author

This was previously covered by the DAOS VOL connector (it was the main target for the first cut at VOL tools testing), so I'm not sure that the focus here should be on creating a testing VOL connector that largely does nothing different from the passthrough connector. Adding a suffix to a filename isn't really changing anything about how the files are stored either and seems like an awful lot of maintenance burden since this VOL connector is nearly an entire copy of the passthrough connector. One could make the argument for having this connector stack on top of the passthrough connector and pass through every single operation except for the ones it's trying to do differently, but that's also beginning to complicate the testing infrastructure. My suggestion is that we should resume VOL tools testing with all the available VOLs we currently test once the tools testing is in a usable state, rather than trying to simulate that in this manner since it's not "doing the real thing".

Testing the tools with existing VOLs is a reasonable approach, though we will likely need to set up a somewhat ad-hoc list of tests that each connector should skip due to being expected to fail. It's probably true that the utility this offers as a 'simple' test isn't worth the tradeoff of having to maintain two copies of the passthrough connectors that will likely end up out of sync.

This was initially created for my own testing while working on the tools tests, to detect cases where tools were/were not using the active connector unexpectedly.

@jhendersonHDF
Copy link
Collaborator

though we will likely need to set up a somewhat ad-hoc list of tests that each connector should skip due to being expected to fail

Yes, I expect this will be the case regardless as we don't have a well-defined set of capabilities that have to be supported for particular tools tests yet (though there was a start to putting checks in the tools code to try and cover some of these cases). But I believe the DAOS VOL connector supported everything needed the last we were testing with it. And of course we are already doing this for the API tests until we have the ability to check for unsupported functionality. What would be convenient, and what has been brought up several times before, is to have a complete reference VOL that's very easy to build and actually uses orthogonal storage. As that's not likely to happen in the near future, leveraging the existing VOLs that we have makes the most sense.

My concern is mostly that testing with a passthrough connector doesn't verify much beyond the support for the things that are changed specifically in that connector. To get a full picture of whether things are working you need to implement a lot of other things using truly different storage mechanisms, at which point you're basically on the path to creating a reference VOL in the first place.

It's probably true that the utility this offers as a 'simple' test isn't worth the tradeoff of having to maintain two copies of the passthrough connectors that will likely end up out of sync.

I think more VOLs for testing are great, though for ones like these which are just meant to test a very specific thing I think it generally makes more sense for them to just be built into the testing code as in https://github.com/HDFGroup/hdf5/blob/develop/test/vol.c#L54. That way we don't have to create some new build code for each test VOL and deal with all the complexity that can come with that when trying to build on multiple platforms and so on. For the tools testing this may be a bit more complicated as the code would likely have to be part of the tools library, but maybe that's something we should do. Having a built-in set of testing VOLs that the tools can use could make sense.

)

add_library(suffix_pt_vol_connector
SHARED
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just as an example of the build system complexity you have to keep in mind, it would probably be surprising here if somebody gets a shared library built when they are building the library with static libs only. And then if we skip building this VOL for static lib only builds of the library, we get less testing as opposed to the case where the VOL is built into the code and the tools can load and use it in either case.

@mattjala
Copy link
Contributor Author

I think more VOLs for testing are great, though for ones like these which are just meant to test a very specific thing I think it generally makes more sense for them to just be built into the testing code as in https://github.com/HDFGroup/hdf5/blob/develop/test/vol.c#L54. That way we don't have to create some new build code for each test VOL and deal with all the complexity that can come with that when trying to build on multiple platforms and so on. For the tools testing this may be a bit more complicated as the code would likely have to be part of the tools library, but maybe that's something we should do. Having a built-in set of testing VOLs that the tools can use could make sense.

I agree that it would be better to have this connector built directly into the tools, but there are a couple issues. First, a major purpose of this connector is to test the ability of the (forthcoming) tools cmake test changes to load/disable an environmentally specified connector based on the particular build configuration, and if it's built in then it won't need to be loaded at all. It might still be worth doing to separate testing of tools' actual functionality with connectors from the tests' ability to load connectors.

Second, as of right now the tools tests are essentially just a bunch of commands run by CMake, which doesn't leave an opportunity to set up and enable a test VOL. This seems like it would require adding some new flag to the tools, as not every tool has command line options to specify a VOL connector.

@byrnHDF
Copy link
Contributor

byrnHDF commented Apr 24, 2025

Second, as of right now the tools tests are essentially just a bunch of commands run by CMake, which doesn't leave an opportunity to set up and enable a test VOL. This seems like it would require adding some new flag to the tools, as not every tool has command line options to specify a VOL connector.

CMake does have the setup/teardown properties to have processes run and cleanup test environments. See the s3 PR #5397 for an example

@mattjala
Copy link
Contributor Author

Closing this for now, since doing the tools testing with existing connectors seems more useful and introduces less maintenance requirements. This may eventually be reintroduced as part of the tools library itself, if it proves useful to have an 'in-line' VOL for testing.

@mattjala mattjala closed this Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Tools Command-line tools like h5dump, includes high-level tools Priority - 2. Medium It would be nice to have this in the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants