#8183: Add S3 provider for Hetzner object storage #8551
Conversation
621b7ad to
124ddad
Compare
124ddad to
0c13880
Compare
I'm not particularly happy about altering the integration tests just for one provider. The azureblob backend has a similar problem too and what it does is check the create error (hopefully it is speci 8000 fic enough) and sleep and retry. Could we use that technique here too? |
|
Very understandable. Note that this PR creates a new S3 provider, not a separate backend like azureblob. The proposal to retry would require changing the behavior for all S3 providers, or special-casing Hetzner Object Storage. Or maybe a new quirk? Are any of these choices acceptable, or do we need to look at a different approach? Hetzner returns // Current code
case "BucketAlreadyExists", "BucketNameUnavailable":
if f.opt.UseAlreadyExists.Value {
// We can trust BucketAlreadyExists to mean not owned by us, so make it non retriable
err = fserrors.NoRetryError(err)
} else {
// We can't trust BucketAlreadyExists to mean not owned by us, so ignore it
err = nil
}
}I've implemented the retry logic by mimicking azureblob With this change, all tests pass except one: Test I cannot see a way for Hetzner Object Storage to handle Next steps
|
0c13880 to
d59b30f
Compare
|
I've pushed an update backing out the changes to the integration tests, and adding a retry for |
Absolutely. That is what quirks are for. Make sure you name it for what it does, not fixHetzner.
Good question! We could just decide that one test failure is ok. Does it work if you just run that test with the -run flag? If so the integration tester would run tes tests, get one failure, and retry that test.
The AWS SDK reties 10 times for most things anyway which is why we only use 2 by default. You could just have a bit of manual logic, or make a new pacer, whichever you fancy. I think if the tests pass today it probably isn't worth worrying about too much though. |
d59b30f to
707c142
Compare
Nope: because an
Sounds good. YAGNI! I've pushed an update that adds the quirk. Given we're willing to accept one test failure, I think this is ready for review! |
707c142 to
3c24a28
Compare
What is the purpose of this change?
This PR closes issue #8183: adding an S3 provider for Hetzner Object Storage.
Was the change discussed in an issue or in the forum before?
#8183
Notes
Hetzner test performance
Hetzner only offers European regions for their object storage.
When I ran the integration tests from my home in North Carolina, or my VPS in Virginia, they blew past the 10-minute test timeout (especially the chunk tests).
When I used a Hetzner VPS in Finland, they ran well under the limit.
Yay, trans-Atlantic latency!
Hetzner
useAlreadyExistsbehaviorThe existing integration tests appear to assume that if a bucket is being asynchronously deleted, and you request creating a new bucket with the same name, the provider honor the bucket create command (cancelling the deletion?).
Hetzner doesn't do this. You'll get a create error (which the S3
fscode swallows), then the bucket still gets deleted, and then all the tests error out because they try to use a bucket that doesn't exist.I considered waiting until the API reported the bucket name is available for reuse, but we can't count on every provider doing that swiftly. Hetzner, at least, is slow to report that the name is free. If we were recreating the bucket into a different region, AWS is known to take over an hour.
Instead, I altered the tests to use a new bucket name each time they request a bucket. With this change, the tests pass under Hetzner and AWS. I don't know if this breaks any assumptions.
Please let me know if this isn't the right way to handle this!
I considered extracting the
fs.NewFS()+f.mkDir()+ error handling into a reusable function, but it wasn't clear that there was a unified abstraction across the tests. That pattern appeared in several places, some others seemed to want manual control over those steps, and others usedoperations.MkDir()instead off.MkDir(). I tried to keep the change small and concrete until I get feedback otherwise.Checklist