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

Fix bug where project name not set in user-agent #977

Merged

Conversation

joereuss12
Copy link
Contributor

Project name, if set, will now show up in the user-agent header for requests. This PR also removes some old unused legacy code of the "payload" object (the only piece of the object still used was the project name). Also includes a unit test checking that the client properly sets this header.

@joereuss12 joereuss12 requested a review from haoming29 March 22, 2024 14:51
@joereuss12 joereuss12 linked an issue Mar 22, 2024 that may be closed by this pull request
1 task
@haoming29
Copy link
Contributor

Running the code locally, I still get grab as the appinfo field in xrootd monitoring packet, but the client-side tests look good to me

DEBUG[2024-03-25T14:48:58Z] MonPacket: Received an appinfo packet        
DEBUG[2024-03-25T14:48:58Z] GetSIDRest inputs: http/unknown.1:32828963699019@6ee28c5df997
grab 

@joereuss12
Copy link
Contributor Author

Running the code locally, I still get grab as the appinfo field in xrootd monitoring packet, but the client-side tests look good to me

DEBUG[2024-03-25T14:48:58Z] MonPacket: Received an appinfo packet        
DEBUG[2024-03-25T14:48:58Z] GetSIDRest inputs: http/unknown.1:32828963699019@6ee28c5df997
grab 

Are there multiple of those appinfo fields returned? When I run it I get quite a few returned to me with some being what is expected. I can play around with this more tho if that is not the case

@haoming29
Copy link
Contributor

Running the code locally, I still get grab as the appinfo field in xrootd monitoring packet, but the client-side tests look good to me

DEBUG[2024-03-25T14:48:58Z] MonPacket: Received an appinfo packet        
DEBUG[2024-03-25T14:48:58Z] GetSIDRest inputs: http/unknown.1:32828963699019@6ee28c5df997
grab 

Are there multiple of those appinfo fields returned? When I run it I get quite a few returned to me with some being what is expected. I can play around with this more tho if that is not the case

There are, but the only one that seems to relate to the file transfer is the grab, others have go-http-client. Also, it's possible that I didn't get the correct mock of .job.id

@joereuss12
Copy link
Contributor Author

Yeah, I don't think it will show up unless the project name is set correctly. You can set an env variable called _CONDOR_JOB_AD to a file or having a .job.ad file that contains ProjectName = <project name>. However, I think we should have something show if it is not set, at least with the Pelican version no matter what.

@joereuss12 joereuss12 force-pushed the client-wrong-user-agent-branch branch from 5630d54 to cb348fc Compare March 27, 2024 20:19
@joereuss12
Copy link
Contributor Author

@haoming29 Give this a go now. I was seeing grab too locally but with this one line change it seems to be giving me the correct User-Agent now.

Copy link
Contributor

@haoming29 haoming29 left a comment

Choose a reason for hiding this comment

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

The revised version gave me pelican-client/7.6.1-next project/some project name as the user agent header read by the xrootd, which should be what we expect. Give the green light here. For the comment on the localcache, feel free address it or not.

Thanks for fixing this!

client/handle_http.go Outdated Show resolved Hide resolved
local_cache/local_cache.go Outdated Show resolved Hide resolved
@haoming29
Copy link
Contributor

Yeah, I don't think it will show up unless the project name is set correctly. You can set an env variable called _CONDOR_JOB_AD to a file or having a .job.ad file that contains ProjectName = <project name>. However, I think we should have something show if it is not set, at least with the Pelican version no matter what.

I found that the catch here is that I only get a non-empty project name if the ProjectName field in the job ad is a quoted string like ProjectName = "some project name". If I simply pass something like ProjectName = NRAO-project888, pelican client won't recognize it as a valid project name and ignored it. I'm not sure the syntax of job ad file but this is something that we should be aware of.

Project name, if set, will now show up in the user-agent header for
requests. This PR also removes some old unused legacy code of the
"payload" object (the only piece of the object still used was the
project name). Also includes a unit test checking that the client
properly sets this header.
@joereuss12 joereuss12 force-pushed the client-wrong-user-agent-branch branch from b88d328 to ee5dff9 Compare March 29, 2024 19:26
@joereuss12
Copy link
Contributor Author

Alright, made those suggestions as well as allowed the ProjectName to be surrounded by "" or not and still work. Just going to let the tests run just in case before I merge.

@haoming29 haoming29 merged commit 2105b05 into PelicanPlatform:main Mar 29, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pelican client has wrong user-agent header when requesting object
2 participants