-
Notifications
You must be signed in to change notification settings - Fork 27
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
Director advertise origin instead of writebackhost #622
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
package client | ||
|
||
import ( | ||
"bytes" | ||
"context" | ||
"fmt" | ||
"io" | ||
|
@@ -848,16 +849,40 @@ func UploadFile(src string, origDest *url.URL, token string, namespace namespace | |
nonZeroSize = fileInfo.Size() > 0 | ||
} | ||
|
||
// Parse the writeback host as a URL | ||
writebackhostUrl, err := url.Parse(namespace.WriteBackHost) | ||
// call a GET on the director, director will respond with our endpoint | ||
directorUrlStr := param.Federation_DirectorUrl.GetString() | ||
directorUrl, err := url.Parse(directorUrlStr) | ||
if err != nil { | ||
return 0, err | ||
return 0, errors.Wrap(err, "failed to parse director url") | ||
} | ||
directorUrl.Path, err = url.JoinPath("/api/v1.0/director/origin", origDest.Path) | ||
if err != nil { | ||
return 0, errors.Wrap(err, "failed to parse director path for upload") | ||
} | ||
|
||
payload := []byte("forPUT") | ||
req, err := http.NewRequest("GET", directorUrl.String(), bytes.NewBuffer(payload)) | ||
if err != nil { | ||
return 0, errors.Wrap(err, "failed to construct request for director-origin query") | ||
} | ||
|
||
dest := &url.URL{ | ||
Host: writebackhostUrl.Host, | ||
Scheme: "https", | ||
Path: origDest.Path, | ||
client := &http.Client{ | ||
Transport: config.GetTransport(), | ||
CheckRedirect: func(req *http.Request, via []*http.Request) error { | ||
return http.ErrUseLastResponse | ||
}, | ||
} | ||
resp, err := client.Do(req) | ||
if err != nil { | ||
return 0, errors.Wrap(err, "failed to send request to director to obtain upload endpoint") | ||
} | ||
if resp.StatusCode == 405 { | ||
return 0, errors.New("Error 405: No writeable origins were found") | ||
} | ||
defer resp.Body.Close() | ||
dest, err := url.Parse(resp.Header.Get("Location")) | ||
if err != nil { | ||
return 0, errors.Wrap(err, "failed to parse location header from director response") | ||
} | ||
|
||
// Create the wrapped reader and send it to the request | ||
|
@@ -972,10 +997,9 @@ Loop: | |
|
||
} | ||
|
||
var UploadClient = &http.Client{Transport: config.GetTransport()} | ||
|
||
// Actually perform the Put request to the server | ||
func doPut(request *http.Request, responseChan chan<- *http.Response, errorChan chan<- error) { | ||
var UploadClient = &http.Client{Transport: config.GetTransport()} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the impact of this? Shouldn't clients be shared? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved this within the function because when it was a global variable, the transport was not being set correctly. For example, it did not recognize |
||
client := UploadClient | ||
dump, _ := httputil.DumpRequestOut(request, false) | ||
log.Debugf("Dumping request: %s", dump) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this calling
GET
in order to do aPUT
? Shouldn't it just invoke aPUT
and rely on the director to redirect?What's the implications on backward compatibility here? Doesn't this break OSDF integration because it doesn't use the topology information when that's the preferred mechanism?
To avoid backward compat breaks, we should do something similar to what is done with downloads:
0. Based on configuration, perhaps take the topology code branches.
PUT
to the director when doing the namespace resolution.For examples from the analogous
GET
case, see https://github.com/PelicanPlatform/pelican/blob/v7.3.3/client/director.go#L66There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a put? It's not doing an update to the director, is it? It's getting information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbockelm Just want to make sure I am approaching this correctly:
So should I move this logic to around where we query the director/
CreateNsFromDirectorResp
withinclient/main.go
and populatewritebackhost
with the correct value in cases we do not use topology. Then changeuploadfile
to get the destination fromwritebackhost
in the namespace struct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tweak:
I'd say change uploadfile back to this behavior. Basically, revert the changes in that method back to where it was before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, another clarification:
The logic should also change slightly too. Instead of a
GET
with body contents, it should be aPUT
.