Remove use of global fed metadata within client #798

joereuss12 · 2024-02-09T19:17:42Z

Changes include:

Created a local metadata object that store federation metadata per url.
Changed a lot of tests on client commands, we no longer rely on osdf for tests to pass
Made a function in config that discovers a federation but does not populate global config values

joereuss12 · 2024-02-09T19:18:21Z

Still a work in progress. Still need to have the url metadata be stored in a ttl cache and everything could use a little more testing.

bbockelm

First round of feedback on the approach.

client/handle_http.go

client/main.go

config/config.go

bbockelm

Another draft review contained.

Please talk with @haoming29 about how to have the cache expire objects. You'll need to run the Start() method to launch the goroutine and think about what lifetime you'd like it to have.

Do you have precommit setup on your development host? There's a large number of static check issues that are making it into the PR that should be caught by precommit.

bbockelm · 2024-02-20T13:31:30Z

client/handle_http_test.go

 		assert.NoError(t, err)
-		if err == nil && len(transferDetailsUpload) == 2 {


Why delete all these tests? Do we have anything remaining that verifies the transferred files and their contents?

My thought process as well as discussion with Justin was to not fully rely on osg-htc.org for our tests so we should just make sure that everything is correct up to the point of the upload/download. Not super sure if that was the best way to do so or if we should just rely on osg-htc.org for osdf:// cases? The other tests do uploads and downloads and check for sizes of the returned files

client/main.go

bbockelm · 2024-02-20T13:34:42Z

client/main.go

+func newPelicanURL(remoteUrl *url.URL, scheme string) (pelicanURL PelicanURL, err error) {
+	if remoteUrl.Host != "" {
+		if scheme == "osdf" || scheme == "stash" {
+			remoteUrl.Path, err = url.JoinPath(remoteUrl.Host, remoteUrl.Path)


Let's not change the URL provided by the caller.

This section is used when someone does an osdf or stash url but only puts two /'s instead of 3 (hence they have a hostname). What would you like me to do differently? Create our own url like the comment below and modify that url?

bbockelm · 2024-02-20T13:36:10Z

client/main.go

+			log.Debugln("Detected pelican:// url, getting federation metadata from specified host")
+			federationUrl, _ := url.Parse(remoteUrl.String())
+			federationUrl.Scheme = "https"
+			federationUrl.Path = ""


There's other parts of the URL - the query and fragment, for example - that may be passed through.

Instead of taking everything and overwriting a few known attributes, let's create a fresh URL and populate only the parts we need.

client/main.go

bbockelm · 2024-02-20T13:45:05Z

client/main.go

+	// Verify valid scheme
+	err = schemeUnderstood(sourceScheme)
+	if err != nil {
+		return nil, err


Wrap the error message here to make sure the user knows what URL is being considered.

The user should be told the scheme they tried as well as the permitted ones from the err returned from schemeUnderstood:

pelican/client/main.go

Lines 488 to 489 in 144ec34

return fmt.Errorf("Do not understand the destination scheme: %s. Permitted values are %s",

scheme, strings.Join(understoodSchemes, ", "))

client/main.go

config/config.go

joereuss12 · 2024-02-20T16:40:50Z

Do you have precommit setup on your development host? There's a large number of static check issues that are making it into the PR that should be caught by precommit.

The linter was not working for me for a while, I took a few stabs at fixing it but each time was taking a little to long so I put it off. I just updated it here: #829 and it seems to be working now

Changes include: - Created a local metadata object that store federation metadata per url. - Changed a lot of tests on client commands, we no longer rely on osdf for tests to pass - Made a function in config that discovers a federation but does not populate global config values

Changes include: - Addressing comments on PR - Making unit tests for new functions - Fix previous typo in "discovery" - Added ttl cache for federations - Fixed merge conflicts

Changes: - Changed around some of the test logic and improved tests - Improved ttl cache with a custom loader function - Other misc changes addressing PR comments

joereuss12 · 2024-02-21T16:56:16Z

@bbockelm check out these changes and my comments. Let me know if anything needs tweaking. Also, look to see where the cache is created, where the loader function is (right next to cache creation), and where I start and stop the cache. I think the locations are pretty good but let me know what you think.

Was failing because it was doing a pelican prefix and a osdf scheme, causing us to load osdf defaults for metadata instead of our specified metadata we want.

bbockelm · 2024-02-29T19:32:18Z

@joereuss12 - now that the big client refactor has merged, please clear up the conflicts here.

This is v2 of PR PelicanPlatform#798 since the merge conflicts were becoming very tedious. Therefore, I took the changes from that PR and converted it over to a new branch. You can get the changes split up a little better from that PR as well but I will do my best to list them here: Changes include: - Added a ttl cache for storing federation url metadata so we do not have to look up each time - Improved unit tests for client commands, Note: for pelican prefix and osdf:// url scheme, it's hard to test since we use osg-htc.org for metadata lookup. Therefore it is hard to work with fed in a box so I just check for the proper metadata lookup - Added function called discoverUrlFederation which discovers a federation from a url and does not populate global metadata config values - Added a function called schemeUnderstood to take some repeated code within client into one function to check for understood url schemes - Added unit tests for discoverUrlFederation and schemeUnderstood

joereuss12 · 2024-03-01T16:23:18Z

Closed with #878

This is v2 of PR PelicanPlatform#798 since the merge conflicts were becoming very tedious. Therefore, I took the changes from that PR and converted it over to a new branch. You can get the changes split up a little better from that PR as well but I will do my best to list them here: Changes include: - Added a ttl cache for storing federation url metadata so we do not have to look up each time - Improved unit tests for client commands, Note: for pelican prefix and osdf:// url scheme, it's hard to test since we use osg-htc.org for metadata lookup. Therefore it is hard to work with fed in a box so I just check for the proper metadata lookup - Added function called discoverUrlFederation which discovers a federation from a url and does not populate global metadata config values - Added a function called schemeUnderstood to take some repeated code within client into one function to check for understood url schemes - Added unit tests for discoverUrlFederation and schemeUnderstood

bbockelm reviewed Feb 15, 2024

View reviewed changes

joereuss12 force-pushed the cleanup-metadata-client-branch branch 2 times, most recently from f90f82c to 91d6f9c Compare February 19, 2024 17:36

joereuss12 marked this pull request as ready for review February 19, 2024 17:36

joereuss12 force-pushed the cleanup-metadata-client-branch branch from 91d6f9c to 144ec34 Compare February 19, 2024 17:41

bbockelm requested changes Feb 20, 2024

View reviewed changes

joereuss12 added 3 commits February 20, 2024 22:46

Finished: remove use of metadata in client

6f0c53a

Changes include: - Addressing comments on PR - Making unit tests for new functions - Fix previous typo in "discovery" - Added ttl cache for federations - Fixed merge conflicts

Improvements made to removing fed metadata

a6b7f3a

Changes: - Changed around some of the test logic and improved tests - Improved ttl cache with a custom loader function - Other misc changes addressing PR comments

joereuss12 force-pushed the cleanup-metadata-client-branch branch from 144ec34 to a6b7f3a Compare February 21, 2024 16:54

joereuss12 added 2 commits February 21, 2024 15:13

Fix linter issues and missed checks

a51457d

Fix failing integration test

332573f

Was failing because it was doing a pelican prefix and a osdf scheme, causing us to load osdf defaults for metadata instead of our specified metadata we want.

joereuss12 mentioned this pull request Feb 22, 2024

Revisit condor shadow hooks with plugin staging #730

Closed

This was linked to issues Feb 22, 2024

Cleanup use of federation metadata in client / avoid race conditions #744

Closed

Improve test coverage for DiscoverFederation #618

Closed

joereuss12 mentioned this pull request Mar 1, 2024

Remove use of global fed metadata within client #878

Merged

joereuss12 closed this Mar 1, 2024

joereuss12 mentioned this pull request Mar 11, 2024

Fixes to pelican plugin staging #882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove use of global fed metadata within client #798

Remove use of global fed metadata within client #798

joereuss12 commented Feb 9, 2024

joereuss12 commented Feb 9, 2024

bbockelm left a comment

bbockelm left a comment

bbockelm Feb 20, 2024

joereuss12 Feb 20, 2024

bbockelm Feb 20, 2024

joereuss12 Feb 20, 2024

bbockelm Feb 20, 2024

bbockelm Feb 20, 2024

joereuss12 Feb 20, 2024

joereuss12 commented Feb 20, 2024

joereuss12 commented Feb 21, 2024

bbockelm commented Feb 29, 2024

joereuss12 commented Mar 1, 2024

		assert.NoError(t, err)
		if err == nil && len(transferDetailsUpload) == 2 {

	return fmt.Errorf("Do not understand the destination scheme: %s. Permitted values are %s",
	scheme, strings.Join(understoodSchemes, ", "))

Remove use of global fed metadata within client #798

Remove use of global fed metadata within client #798

Conversation

joereuss12 commented Feb 9, 2024

joereuss12 commented Feb 9, 2024

bbockelm left a comment

Choose a reason for hiding this comment

bbockelm left a comment

Choose a reason for hiding this comment

bbockelm Feb 20, 2024

Choose a reason for hiding this comment

joereuss12 Feb 20, 2024

Choose a reason for hiding this comment

bbockelm Feb 20, 2024

Choose a reason for hiding this comment

joereuss12 Feb 20, 2024

Choose a reason for hiding this comment

bbockelm Feb 20, 2024

Choose a reason for hiding this comment

bbockelm Feb 20, 2024

Choose a reason for hiding this comment

joereuss12 Feb 20, 2024

Choose a reason for hiding this comment

joereuss12 commented Feb 20, 2024

joereuss12 commented Feb 21, 2024

bbockelm commented Feb 29, 2024

joereuss12 commented Mar 1, 2024