[#70] Add arrays of copies numbers for location constraints #99

Merged
alexvanin merged 1 commit from ironbee/frostfs-s3-gw:add-arrays-for-copies-numbers into master 2023-05-03 13:04:02 +00:00
Contributor

Signed-off-by: Artem Tataurov a.tataurov@yadro.com

Signed-off-by: Artem Tataurov <a.tataurov@yadro.com>
ironbee requested review from dkirillov 2023-04-24 23:51:03 +00:00
ironbee force-pushed add-arrays-for-copies-numbers from 3c3a53b925 to 7675c938ed 2023-04-25 11:30:35 +00:00 Compare
ironbee changed title from WIP: Add arrays of copies numbers for location constraints to Add arrays of copies numbers for location constraints 2023-04-25 11:31:05 +00:00
Member

Please fix commit message and link this PR to appropriate issue

Please fix commit message and link this PR to appropriate issue
dkirillov reviewed 2023-04-26 06:40:39 +00:00
@ -0,0 +7,4 @@
)
func TestCopiesNumberPicker(t *testing.T) {
Member

Unnecessary empty line

Unnecessary empty line
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:05:12 +00:00
@ -154,0 +170,4 @@
if len(vector) == 0 {
l.Warn("skipping the empty vector of copies numbers", zap.Int("index", i))
continue
Member

Why do we use this? Probably we can use break (the same as for empty constraint)

Why do we use this? Probably we can use `break` (the same as for empty constraint)
Author
Contributor

My thoughts were:

  1. Empty constraint: something went wrong while we wrote the config thus we break the loop to avoid adding damaged settings.
  2. Articulated constrained, empty vector: this is not necessarily an emergency situation. We could have emptied the vector to fill it in later in future. Thus the code skips the whole constraint + vector pair and goes on parsing further constraints.

I will add the break.

My thoughts were: 1) Empty constraint: something went wrong while we wrote the config thus we break the loop to avoid adding damaged settings. 2) Articulated constrained, empty vector: this is not necessarily an emergency situation. We could have emptied the vector to fill it in later in future. Thus the code skips the whole constraint + vector pair and goes on parsing further constraints. I will add the break.
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:07:28 +00:00
@ -83,6 +83,7 @@ const ( // Settings.
// Policy.
cfgPolicyDefault = "placement_policy.default"
cfgPolicyRegionMapFile = "placement_policy.region_mapping"
cfgCopiesNumbers = "placement_policy.copies_numbers"
Member

This new config param we should mention in docs/configuration.md and in config examples

This new config param we should mention in `docs/configuration.md` and in config examples
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:13:00 +00:00
@ -81,0 +110,4 @@
copiesNumbersSplit := strings.Split(copiesNumbersStr, ",")
for i := range copiesNumbersSplit {
item := strings.ReplaceAll(copiesNumbersSplit[i], " ", "")
Member

Can we drop this? It seems we can require that header must have format 1,2,3

Can we drop this? It seems we can require that header must have format `1,2,3`
Author
Contributor

Is there a validation somewhere or we need to implement it?

Is there a validation somewhere or we need to implement it?
Member

My suggestion was accepting only headers without spaces between numbers (X-Amz-Meta-Frostfs-Copies-Number: 1,2,3 rather than X-Amz-Meta-Frostfs-Copies-Number: 1, 2, 3). But if we support both it's ok

My suggestion was accepting only headers without spaces between numbers (`X-Amz-Meta-Frostfs-Copies-Number: 1,2,3` rather than `X-Amz-Meta-Frostfs-Copies-Number: 1, 2, 3`). But if we support both it's ok
Author
Contributor

We support both cases, yes.

We support both cases, yes.
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:14:50 +00:00
@ -0,0 +40,4 @@
})
t.Run("pick copies number from metadata", func(t *testing.T) {
metadata["frostfs-copies-number"] = "7, 8, 9"
Member

We should also test 1,2,3 (without spaces)

We should also test `1,2,3` (without spaces)
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:16:00 +00:00
@ -32,6 +35,7 @@ type (
DefaultMaxAge int
NotificatorEnabled bool
CopiesNumber uint32
Member

Let's rename this to DefaultCopiesNumbers and make it []uint32

Let's rename this to `DefaultCopiesNumbers` and make it `[]uint32`
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:18:55 +00:00
@ -150,3 +150,3 @@
}
p.CopiesNumber, err = getCopiesNumberOrDefault(p.Header, h.cfg.CopiesNumber)
copiesNumbers, err := h.pickCopiesNumbers(p.Header, bktInfo.LocationConstraint)
Member

We can use:

p.CopiesNumber, err = h.pickCopiesNumbers(p.Header, bktInfo.LocationConstraint)
We can use: ``` p.CopiesNumber, err = h.pickCopiesNumbers(p.Header, bktInfo.LocationConstraint) ```
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:19:26 +00:00
@ -54,2 +52,4 @@
Reader: r.Body,
}
copiesNumbers, err := h.pickCopiesNumbers(parseMetadata(r), bktInfo.LocationConstraint)
Member

The same

The [same](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/99/files#issuecomment-7411)
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:19:44 +00:00
@ -171,0 +177,4 @@
Encryption: encryptionParams,
}
copiesNumbers, err := h.pickCopiesNumbers(metadata, dstBktInfo.LocationConstraint)
Member

The same

The [same](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/99/files#issuecomment-7411)
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:20:05 +00:00
@ -121,3 +121,3 @@
CopiesNumber: h.cfg.CopiesNumber,
}
copiesNumbers, err := h.pickCopiesNumbers(parseMetadata(r), bktInfo.LocationConstraint)
Member

The same

The [same](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/99/files#issuecomment-7411)
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:20:15 +00:00
@ -235,2 +227,4 @@
Encryption: encryptionParams,
}
copiesNumbers, err := h.pickCopiesNumbers(metadata, bktInfo.LocationConstraint)
Member

The same

The [same](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/99/files#issuecomment-7411)
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:22:18 +00:00
@ -146,3 +146,3 @@
CopiesNumber: h.cfg.CopiesNumber,
}
copiesNumbers, err := h.pickCopiesNumbers(parseMetadata(r), bktInfo.LocationConstraint)
Member

The same

The [same](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/99/files#issuecomment-7411)
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-26 08:24:14 +00:00
@ -226,0 +230,4 @@
NewLock: lock,
}
copiesNumbers, ok := h.cfg.CopiesNumbers[bktInfo.LocationConstraint]
Member

Why don't we use h.pickCopiesNumbers ?

Why don't we use `h.pickCopiesNumbers` ?
dkirillov marked this conversation as resolved
ironbee force-pushed add-arrays-for-copies-numbers from 7675c938ed to 788096e4a1 2023-04-26 17:08:12 +00:00 Compare
ironbee force-pushed add-arrays-for-copies-numbers from 788096e4a1 to e3d972e501 2023-04-26 18:00:17 +00:00 Compare
dkirillov requested review from storage-services-committers 2023-04-27 06:21:25 +00:00
dkirillov requested review from storage-services-developers 2023-04-27 06:21:25 +00:00
dkirillov changed title from Add arrays of copies numbers for location constraints to [#70] Add arrays of copies numbers for location constraints 2023-04-27 06:24:48 +00:00
dkirillov reviewed 2023-04-27 06:31:38 +00:00
@ -154,0 +164,4 @@
vector32[i] = uint32(vector[i])
}
if constraint == "" {
Member

We can write:

if constraint == "" || len(vector) == 0 {
	break
}
We can write: ``` if constraint == "" || len(vector) == 0 { break } ```
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-27 06:32:16 +00:00
@ -128,2 +128,4 @@
# Path to container policy mapping. The same as '--container-policy' flag for authmate
region_mapping: /path/to/container/policy.json
# Array of locations constraints and their vectors of copies numbers
copies_numbers:
Member

Let's add the same example to config/config.env

Let's add the same example to `config/config.env`
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-27 06:38:00 +00:00
@ -288,0 +285,4 @@
|------------------|--------------------------------------------------|---------------|---------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `default` | `string` | yes | `REP 3` | Default policy of placing containers in FrostFS. If a user sends a request `CreateBucket` and doesn't define policy for placing of a container in FrostFS, the S3 Gateway will put the container with default policy. |
| `region_mapping` | `string` | yes | | Path to file that maps aws `LocationContraint` values to FrostFS placement policy. The similar to `--container-policy` flag in `frostfs-s3-authmate` util, see in [docs](./authmate.md#containers-policy) |
| `copies_numbers` | `[]`[Copies numbers](#copies_numbers-subsection) | no | | Array of configured location constraints and their copies numbers. |
Member

Can we provide some example above, like:

placement_policy:
  default: REP 3
  region_mapping: /path/to/mapping/rules.json
  copies_numbers:
    - location_constraint: one
      vector:
        - 1
        - 2
Can we provide some example above, like: ```yaml placement_policy: default: REP 3 region_mapping: /path/to/mapping/rules.json copies_numbers: - location_constraint: one vector: - 1 - 2 ```
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-27 06:40:37 +00:00
@ -0,0 +7,4 @@
)
func TestCopiesNumberPicker(t *testing.T) {
var locCosntraints = map[string][]uint32{}
Member

Typo. Should be var locConstraints = ...

Typo. Should be `var locConstraints = ...`
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-04-27 06:43:22 +00:00
@ -0,0 +26,4 @@
metadata["somekey1"] = "5, 6, 7"
expectedCopiesNumbers := []uint32{1}
actualCopiesNumbers, _ := h.pickCopiesNumbers(metadata, locationConstraint2)
Member

Why don't we check error?

actualCopiesNumbers, err := h.pickCopiesNumbers(metadata, locationConstraint2)
require.NoError(t, err)
Why don't we check error? ``` actualCopiesNumbers, err := h.pickCopiesNumbers(metadata, locationConstraint2) require.NoError(t, err) ```
dkirillov marked this conversation as resolved
ironbee force-pushed add-arrays-for-copies-numbers from e3d972e501 to 8286fe27ce 2023-04-27 14:25:18 +00:00 Compare
ironbee force-pushed add-arrays-for-copies-numbers from 8286fe27ce to 07ae24b97d 2023-04-27 14:38:10 +00:00 Compare
ironbee force-pushed add-arrays-for-copies-numbers from 07ae24b97d to 8af2432415 2023-04-27 14:40:02 +00:00 Compare
ironbee force-pushed add-arrays-for-copies-numbers from 8af2432415 to 438ba1ab58 2023-04-27 15:06:58 +00:00 Compare
dkirillov reviewed 2023-05-02 06:40:38 +00:00
@ -154,0 +157,4 @@
for i := 0; ; i++ {
key := cfgCopiesNumbers + "." + strconv.Itoa(i) + "."
constraint := v.GetString(key + "location_constraint")
vector := v.GetIntSlice(key + "vector")
Member

It seems here we should use v.GetStringSlice otherwise we cannot properly get vector if it passed via environment variables

It seems here we should use `v.GetStringSlice` otherwise we cannot properly get vector if it passed via environment variables
dkirillov marked this conversation as resolved
ironbee force-pushed add-arrays-for-copies-numbers from 438ba1ab58 to 983d77a9c8 2023-05-02 14:42:29 +00:00 Compare
dkirillov reviewed 2023-05-03 09:32:17 +00:00
@ -154,0 +160,4 @@
vector := v.GetStringSlice(key + "vector")
vector32 := make([]uint32, len(vector))
for i := range vector {
Member

Let's use j to avoid confusing with i from outer loop

Let's use `j` to avoid confusing with `i` from outer loop
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-05-03 09:35:50 +00:00
@ -154,0 +164,4 @@
parsedValue, err := strconv.ParseUint(vector[i], 10, 32)
if err != nil {
l.Error("cannot parse copies numbers", zap.Error(err))
break
Member

Actually, this breaks only inner loop, but this isn't enough.
We should use break LOOP where LOOP is a label right above the outer loop.

Actually, this breaks only inner loop, but this isn't enough. We should use `break LOOP` where `LOOP` is a label right above the outer loop.
dkirillov marked this conversation as resolved
ironbee force-pushed add-arrays-for-copies-numbers from 983d77a9c8 to e487ee5b7d 2023-05-03 10:48:39 +00:00 Compare
dkirillov approved these changes 2023-05-03 11:11:44 +00:00
alexvanin approved these changes 2023-05-03 13:03:51 +00:00
alexvanin merged commit e487ee5b7d into master 2023-05-03 13:04:02 +00:00
alexvanin deleted branch add-arrays-for-copies-numbers 2023-05-03 13:04:02 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-services-developers
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#99
No description provided.