KirillovDenis/feature/use_chi_instead_of_gorilla #149

Merged
alexvanin merged 4 commits from dkirillov/frostfs-s3-gw:KirillovDenis/feature/use_chi_instead_of_gorilla into master 2023-07-11 13:53:47 +00:00
Member

Reopen to not forget about this draft

1 min, 10 buckets, writers 300 VUs, readers 300 VUs

scenario gorilla chi
1mb write 447 MB/s 463 MB/s
1mb read 607 MB/s 648 MB/s
16mb write 513 MB/s 503 MB/s
16mb read 724 MB/s 656 MB/s
Reopen to not forget about this draft 1 min, 10 buckets, writers 300 VUs, readers 300 VUs | scenario | gorilla | chi | |------------|----------|----------| | 1mb write | 447 MB/s | 463 MB/s | | 1mb read | 607 MB/s | 648 MB/s | | 16mb write | 513 MB/s | 503 MB/s | | 16mb read | 724 MB/s | 656 MB/s |
dkirillov self-assigned this 2023-06-21 14:41:49 +00:00
dkirillov added 2 commits 2023-06-21 14:41:51 +00:00
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
[#XX] Add custom bucket domain based router
Some checks failed
DCO check / Commits Check (pull_request) Failing after 2s
Tests / Lint (pull_request) Failing after 2s
Tests / Tests (1.17) (pull_request) Failing after 7s
Tests / Coverage (pull_request) Failing after 9s
Tests / Tests (1.19.x) (pull_request) Failing after 2s
Tests / Tests (1.18.x) (pull_request) Failing after 2s
CodeQL / Analyze (go) (pull_request) Failing after 2s
Builds / Build CLI (pull_request) Failing after 2s
Builds / Build Docker image (pull_request) Has been skipped
16c89b14c1
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
dkirillov added 120 commits 2023-07-05 09:26:12 +00:00
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Contains debug logs for switching
connections in pool.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Aleksey Pastukhov <a.pastukhov@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Due to source code relocation from GitHub.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
This parameter enables parsing xml body without
xmlns="http://s3.amazonaws.com/doc/2006-03-01/" attribute
for CompleteMultipartUpload requests

Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Periodic white space XML writer sends XML header
and white spaces to the io.Writer.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Such functions should be used together with periodic white space
XML writer.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
This mechanism is used by Amazon S3 to keep client's
connection alive while object is being constructed from
the upload parts.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Reduce code duplication for error handling

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Add bug report and feature request templates

Signed-off-by: Liza <e.chichindaeva@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Since we have pkg 'internal/frostfs/services/tree' that is downloading
during build we cannot export any package that is depended on it.

Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
We shouldn't create delete marker if:
1. object doesn't exist at all
2. last version is already a delete marker

Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Log error instead of failing when multiple unversioned nodes are found

Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
[#122] Fix linter warnings
All checks were successful
Builds (1.19)
DCO
Builds (1.20)
Lint
Tests (1.19)
Tests (1.20)
81e860481d
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
When object exists in tree but missing in storage, we can't remove
bucket. While storage node does not sync tree service and object
service, the only way to delete such broken bucket is to ignore
'object not found' error, clear cache and do not include missing
objects in the listing result.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
[#78] Add test of bucket removal with object not found error
All checks were successful
DCO
Builds (1.20)
Tests (1.19)
Tests (1.20)
Builds (1.19)
Lint
868edfdb31
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Don't use escaping when presign url.
Escape manually before.

Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Add computing actual object size during calculating hash on put.
Use this actual value to save in tree and cache

Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
[#84] add tracing support
All checks were successful
Builds (1.20)
Builds (1.19)
DCO
Tests (1.19)
Tests (1.20)
Lint
4e1fd9589b
Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
For slow actions runners.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
[#124] Add govulncheck in CI
All checks were successful
Builds (1.20)
Builds (1.19)
DCO
Tests (1.19)
Tests (1.20)
Vulncheck
Lint
c75add64ec
Check dependency issues on every PR.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Codeownders feature isn't supported yet in forgejo nor
gitea. Looking for github.com/go-gitea/gitea/pull/24910

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Replace changelog history before the fork
with the link to the fork source.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
The only thing supported in .forgejo dir is a workflows.
It might be useful to keep .github dir for repo mirrors.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
[#133] Update docs
All checks were successful
Builds (1.20)
Builds (1.19)
DCO
Tests (1.19)
Tests (1.20)
Vulncheck
Lint
19c89b38e6
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
[#103] Return 504 http code on timeout errors
All checks were successful
/ Vulncheck (pull_request) Successful in 1m42s
/ Lint (pull_request) Successful in 3m31s
/ Tests (1.19) (pull_request) Successful in 2m57s
/ Tests (1.20) (pull_request) Successful in 3m4s
/ Builds (1.19) (pull_request) Successful in 2m35s
/ Builds (1.20) (pull_request) Successful in 2m39s
/ DCO (pull_request) Successful in 1m7s
462589fc0c
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Previous SDK implementation had implicit gRPC interceptor
for tracing. Now pool constructors allow any dial options,
so gateway should manually pass tracing interceptors from
observability package.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
[#135] authmate: Update docs
All checks were successful
/ Vulncheck (pull_request) Successful in 1m30s
/ Builds (1.19) (pull_request) Successful in 2m37s
/ Builds (1.20) (pull_request) Successful in 2m31s
/ DCO (pull_request) Successful in 3m6s
/ Lint (pull_request) Successful in 2m46s
/ Tests (1.19) (pull_request) Successful in 5m26s
/ Tests (1.20) (pull_request) Successful in 2m36s
dfc4476afd
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
[#111] Use request scope logger
Some checks failed
/ Lint (pull_request) Failing after 32s
/ Tests (1.19) (pull_request) Failing after 32s
/ Tests (1.20) (pull_request) Failing after 32s
/ Builds (1.19) (pull_request) Failing after 32s
/ Builds (1.20) (pull_request) Failing after 32s
/ Vulncheck (pull_request) Failing after 57s
/ DCO (pull_request) Failing after 1m0s
23593eee3d
Store child zap logger with request scope fields into context.
Request scoped fields: request_id, api/method, bucket, object

Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
[#106] Add chunk uploading
All checks were successful
/ DCO (pull_request) Successful in 1m4s
/ Vulncheck (pull_request) Successful in 1m50s
/ Builds (1.19) (pull_request) Successful in 9m1s
/ Builds (1.20) (pull_request) Successful in 2m20s
/ Lint (pull_request) Successful in 10m19s
/ Tests (1.19) (pull_request) Successful in 2m45s
/ Tests (1.20) (pull_request) Successful in 3m19s
614d703726
Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
[#143] Fix transformToS3Error function
All checks were successful
/ Builds (1.19) (pull_request) Successful in 3m11s
/ Builds (1.20) (pull_request) Successful in 2m57s
/ DCO (pull_request) Successful in 4m7s
/ Lint (pull_request) Successful in 2m25s
/ Tests (1.19) (pull_request) Successful in 3m9s
/ Tests (1.20) (pull_request) Successful in 3m18s
/ Vulncheck (pull_request) Successful in 1m15s
9df8695463
Unwrap error before checking for s3 error

Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
[#131] Update docs
All checks were successful
/ Builds (1.19) (pull_request) Successful in 3m3s
/ Builds (1.20) (pull_request) Successful in 2m44s
/ DCO (pull_request) Successful in 3m59s
/ Vulncheck (pull_request) Successful in 1m34s
/ Lint (pull_request) Successful in 3m41s
/ Tests (1.19) (pull_request) Successful in 3m9s
/ Tests (1.20) (pull_request) Successful in 2m36s
2cbe3b9a27
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
[#155] metrics: Use default registerer for app metrics
All checks were successful
/ DCO (pull_request) Successful in 3m45s
/ Lint (pull_request) Successful in 3m29s
/ Tests (1.19) (pull_request) Successful in 2m44s
/ Tests (1.20) (pull_request) Successful in 3m19s
/ Vulncheck (pull_request) Successful in 4m40s
/ Builds (1.19) (pull_request) Successful in 2m58s
/ Builds (1.20) (pull_request) Successful in 1m47s
499f4c6495
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
dkirillov force-pushed KirillovDenis/feature/use_chi_instead_of_gorilla from aaab1138ab to 3620357d4a 2023-07-05 14:06:08 +00:00 Compare
dkirillov changed title from WIP: KirillovDenis/feature/use_chi_instead_of_gorilla to KirillovDenis/feature/use_chi_instead_of_gorilla 2023-07-05 14:20:31 +00:00
dkirillov requested review from storage-services-developers 2023-07-05 14:20:43 +00:00
dkirillov requested review from storage-services-committers 2023-07-05 14:20:43 +00:00
alexvanin reviewed 2023-07-10 08:58:18 +00:00
@ -19,6 +19,7 @@ import (
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/data"
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/errors"
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer"
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware"
Owner

Is it dependency requirement or a bit of refactoring for cleaner code?

Is it dependency requirement or a bit of refactoring for cleaner code?
Author
Member

It's consequence of the refactoring. api/middleware now contains ReqInfo

It's consequence of the refactoring. `api/middleware` now contains `ReqInfo`
alexvanin marked this conversation as resolved
@ -0,0 +43,4 @@
BucketResolveFunc func(ctx context.Context, bucket string) (*data.BucketInfo, error)
// cidResolveFunc is a func to resolve CID in Stats handler.
cidResolveFunc func(ctx context.Context, reqInfo *ReqInfo) (cnrID string)
Owner

By the way, is it necessary to have a separate type for these functions? Are they being used in tests or can they?

By the way, is it necessary to have a separate type for these functions? Are they being used in tests or can they?
Author
Member

I moved this to separate type just for convenience (for the sake of readability in Stats function)

I moved this to separate type just for convenience (for the sake of readability in `Stats` function)
alexvanin marked this conversation as resolved
@ -0,0 +37,4 @@
// HeadersMatch adds a matcher for header values.
// It accepts a sequence of key/value pairs. Values may define variables.
// Panics if number of parameters is not even.
// Supports only exact matching.
Owner

Did you consider support of regexp values like in mux?
There are couple of good regexp checks in the mux configration, e.g. "partNumber", "{partNumber:[0-9]+}". Or we are going to parse number anyway in the handler so it doesn't matter much?

Did you consider support of regexp values like in mux? There are couple of good regexp checks in the mux configration, e.g. `"partNumber", "{partNumber:[0-9]+}"`. Or we are going to parse number anyway in the handler so it doesn't matter much?
Author
Member

I wanted to add the minimal and simple filters that we need. And yes, we will parse such numbers further anyway

I wanted to add the minimal and simple filters that we need. And yes, we will parse such numbers further anyway
alexvanin marked this conversation as resolved
@ -0,0 +124,4 @@
if !r.URL.Query().Has(query.Key) || queryVal != "" && query.Value != queryVal {
continue LOOP
}
}
Owner

I expect this to work faster than mux parsing because there is no regex matching. Maybe we can try to benchmark it just for internal research.

I expect this to work faster than mux parsing because there is no regex matching. Maybe we can try to benchmark it just for internal research.
cmd/s3-gw/app.go Outdated
@ -496,0 +503,4 @@
throttleOps := middleware.ThrottleOpts{
Limit: a.settings.maxClient.count,
BacklogTimeout: a.settings.maxClient.deadline,
}
Owner

It does work the same as it was implemented manually for the mux or is there some difference? Based on the naming, throttler and limiter might work a bit differently.

It does work the same as it was implemented manually for the mux or is there some difference? Based on the naming, throttler and limiter might work a bit differently.
Author
Member

When I checked code of middleware.ThrottleOpts previously, it seemed to be almost the same. I'll check it again

When I checked code of `middleware.ThrottleOpts` previously, it seemed to be almost the same. I'll check it again
Author
Member

// Throttle is a middleware that limits number of currently processed requests
// at a time across all users. Note: Throttle is not a rate-limiter per user,
// instead it just puts a ceiling on the number of currentl in-flight requests
// being processed from the point from where the Throttle middleware is mounted.

So it's what we need. There are some difference though:

  • It returns http.StatusTooManyRequests error, our previous implementation returns http.StatusServiceUnavailable
  • It also has some limit for requests that are waiting (in current implementation the same number as for in-flight requests). If we recieve request but all limits (in-flights and waiting are exausted) client gets error without waiting
// Throttle is a middleware that limits number of currently processed requests // at a time across all users. Note: Throttle is not a rate-limiter per user, // instead it just puts a ceiling on the number of currentl in-flight requests // being processed from the point from where the Throttle middleware is mounted. So it's what we need. There are some difference though: * It returns `http.StatusTooManyRequests` error, our previous implementation returns ` http.StatusServiceUnavailable` * It also has some limit for requests that are waiting (in current implementation the same number as for in-flight requests). If we recieve request but all limits (in-flights and waiting are exausted) client gets error without waiting
alexvanin marked this conversation as resolved
dkirillov force-pushed KirillovDenis/feature/use_chi_instead_of_gorilla from 3620357d4a to a3ae0004b6 2023-07-10 09:56:02 +00:00 Compare
alexvanin approved these changes 2023-07-10 14:32:10 +00:00
ironbee approved these changes 2023-07-11 07:36:14 +00:00
alexvanin merged commit 0edae789b8 into master 2023-07-11 13:53:46 +00:00
alexvanin referenced this pull request from a commit 2023-07-11 13:53:47 +00:00
alexvanin deleted branch KirillovDenis/feature/use_chi_instead_of_gorilla 2023-07-11 13:53:47 +00:00
alexvanin referenced this pull request from a commit 2023-07-11 14:29:26 +00:00
alexvanin added this to the v0.28.0 milestone 2023-07-12 09:43:10 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-s3-gw#149
No description provided.