TrueCloudLab/frostfs-node
21
1
Fork 27
Code Issues 104 Pull requests 4 Projects Releases 2 Activity Actions

Add support EC parameters #1065

Merged
fyrchik merged 3 commits from achuprov/frostfs-node:adm_ec_parameters into master 2024-09-04 19:51:07 +00:00
Conversation 2 Commits 3 Files changed 10 +62 -3
achuprov commented 2024-03-31 18:58:28 +00:00
Member
Copy link
./bin/frostfs-adm --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml morph dump-config
ContainerAliasFee:           0 (int)
EpochDuration:               240 (int)
InnerRingCandidateFee:       10000000000 (int)
MaxECParityCount:            5 (int)
WithdrawFee:                 100000000 (int)
ContainerFee:                0 (int)
HomomorphicHashingDisabled:  false (bool)
MaintenanceModeAllowed:      true (bool)
MaxECDataCount:              16 (int)
MaxObjectSize:               67108864 (int)

 ./bin/frostfs-cli --config /home/achuprov/Documents/work/frostfs-dev-env/wallets/conf.yml  netmap netinfo --rpc-endpoint s02.frostf
s.devenv:8080
Epoch: 13
Network magic: [net 0x3c2d] 15405
Time per block: 1s
FrostFS network configuration (system)
  Container fee: 0
  Epoch duration: 240
  Inner Ring candidate fee: 10000000000
  Maximum object size: 67108864
  Withdrawal fee: 100000000
  Homomorphic hashing disabled: false
  Maintenance mode allowed: true
  Maximum count of data shards: 20
  Maximum count of parity shards: 20
``` ./bin/frostfs-adm --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml morph dump-config ContainerAliasFee: 0 (int) EpochDuration: 240 (int) InnerRingCandidateFee: 10000000000 (int) MaxECParityCount: 5 (int) WithdrawFee: 100000000 (int) ContainerFee: 0 (int) HomomorphicHashingDisabled: false (bool) MaintenanceModeAllowed: true (bool) MaxECDataCount: 16 (int) MaxObjectSize: 67108864 (int) ``` ``` ./bin/frostfs-cli --config /home/achuprov/Documents/work/frostfs-dev-env/wallets/conf.yml netmap netinfo --rpc-endpoint s02.frostf s.devenv:8080 Epoch: 13 Network magic: [net 0x3c2d] 15405 Time per block: 1s FrostFS network configuration (system) Container fee: 0 Epoch duration: 240 Inner Ring candidate fee: 10000000000 Maximum object size: 67108864 Withdrawal fee: 100000000 Homomorphic hashing disabled: false Maintenance mode allowed: true Maximum count of data shards: 20 Maximum count of parity shards: 20 ```
fyrchik requested changes 2024-04-01 06:43:11 +00:00
cmd/frostfs-adm/docs/deploy.md Outdated
@ -34,6 +34,8 @@ alphabet-wallets: /home/user/deploy/alphabet-wallets
network:
max_object_size: 67108864
epoch_duration: 240
max_EC_data_count: 12
fyrchik commented 2024-04-01 06:36:02 +00:00
Owner
Copy link

let's use only_small_letters

let's use `only_small_letters`
achuprov commented 2024-04-01 15:55:53 +00:00
Author
Member
Copy link

fixed

fixed
cmd/frostfs-adm/internal/modules/config/config.go Outdated
@ -107,2 +111,3 @@
Endpoint: "https://neo.rpc.node:30333",
MaxObjectSize: 67108864, // 64 MiB
MaxObjectSize: 67108864, // 64 MiB
MaxECDataCount: 12,
fyrchik commented 2024-04-01 06:36:38 +00:00
Owner
Copy link

The comment woudn't hurt, it is a magic constant, otherwise.

The comment woudn't hurt, it is a magic constant, otherwise.
fyrchik commented 2024-04-01 06:41:44 +00:00
Owner
Copy link

The choice is indeed arbitrary.
The reasons are:

  1. 16-node networks is something we test regularly, 12+4.
  2. 4 parity chunks is more than enough (most policies have less or equal than 3 replicas)
The choice is indeed arbitrary. The reasons are: 1. 16-node networks is something we test regularly, `12+4`. 2. 4 parity chunks is more than enough (most policies have less or equal than 3 replicas)
achuprov commented 2024-04-01 15:56:03 +00:00
Author
Member
Copy link

fixed

fixed
cmd/frostfs-adm/internal/modules/morph/config/config.go Outdated
@ -106,2 +107,4 @@
bw := io.NewBufBinWriter()
c, err := helper.GetN3Client(viper.GetViper())
fyrchik commented 2024-04-01 06:43:06 +00:00
Owner
Copy link

This is a lot of code for a simple parameter addition, what is the reason?

This is a lot of code for a simple parameter addition, what is the reason?
acid-ant requested changes 2024-04-01 08:44:54 +00:00
cmd/frostfs-adm/internal/commonflags/flags.go Outdated
@ -26,6 +26,8 @@ const (
ContractsURLFlagDesc = "URL to archive with compiled FrostFS contracts"
EpochDurationInitFlag = "network.epoch_duration"
MaxObjectSizeInitFlag = "network.max_object_size"
MaxECDataCountinitFlag = "network.max_ec_data_count"
acid-ant commented 2024-04-01 08:42:37 +00:00
Member
Copy link

MaxECDataCountinitFlag -> MaxECDataCountFlag

MaxECDataCountinitFlag -> MaxECDataCountFlag
achuprov commented 2024-04-01 15:56:54 +00:00
Author
Member
Copy link

fixed

fixed
acid-ant marked this conversation as resolved
cmd/frostfs-cli/modules/netmap/netinfo.go Outdated
@ -46,3 +46,3 @@
cmd.Printf(format, "Withdrawal fee", netInfo.WithdrawalFee())
cmd.Printf(format, "Homomorphic hashing disabled", netInfo.HomomorphicHashingDisabled())
cmd.Printf(format, "Maintenance mode allowed", netInfo.MaintenanceModeAllowed())
cmd.Printf(format, "Maximum count of data shards", netInfo.MaxECDataCount())
acid-ant commented 2024-04-01 08:44:18 +00:00
Member
Copy link

Why have you removed cmd.Printf(format, "Maintenance mode allowed"...?

Why have you removed `cmd.Printf(format, "Maintenance mode allowed"...`?
achuprov commented 2024-04-01 15:57:01 +00:00
Author
Member
Copy link

fixed

fixed
acid-ant marked this conversation as resolved
achuprov force-pushed adm_ec_parameters from ccf3103451 to d5bdd48413 2024-04-01 15:48:41 +00:00 Compare
achuprov requested review from storage-core-committers 2024-04-01 15:55:43 +00:00
achuprov requested review from storage-core-developers 2024-04-01 15:55:48 +00:00
acid-ant requested changes 2024-04-01 19:03:40 +00:00
cmd/frostfs-adm/internal/commonflags/flags.go Outdated
@ -27,2 +27,4 @@
EpochDurationInitFlag = "network.epoch_duration"
MaxObjectSizeInitFlag = "network.max_object_size"
MaxECDataCountFlag = "network.max_ec_data_count"
MaxECParityCountConfig = "network.max_ec_parity_count"
acid-ant commented 2024-04-01 19:02:07 +00:00
Member
Copy link

Config -> Flag.

Config -> Flag.
acid-ant marked this conversation as resolved
cmd/frostfs-adm/internal/modules/config/config_test.go Outdated
@ -27,6 +27,8 @@ func TestGenerateConfigExample(t *testing.T) {
require.Equal(t, "https://neo.rpc.node:30333", v.GetString("rpc-endpoint"))
require.Equal(t, filepath.Join(appDir, "alphabet-wallets"), v.GetString("alphabet-wallets"))
require.Equal(t, 67108864, v.GetInt("network.max_object_size"))
require.Equal(t, 12, v.GetInt("network.max_EC_data_count"))
acid-ant commented 2024-04-01 18:59:22 +00:00
Member
Copy link

Let's use only_small_letters here too.

Let's use only_small_letters here too.
acid-ant marked this conversation as resolved
achuprov force-pushed adm_ec_parameters from d5bdd48413 to 0b7ab2eb02 2024-04-02 15:16:05 +00:00 Compare
achuprov force-pushed adm_ec_parameters from 0b7ab2eb02 to a2719fe3e9 2024-04-02 15:18:35 +00:00 Compare
acid-ant approved these changes 2024-04-05 07:15:35 +00:00
fyrchik approved these changes 2024-04-05 07:35:38 +00:00
fyrchik commented 2024-04-05 07:35:58 +00:00
Owner
Copy link

Please, look at staticcheck warnings

Please, look at staticcheck warnings
acid-ant requested changes 2024-04-05 07:39:31 +00:00
cmd/frostfs-adm/internal/modules/morph/config/config.go Outdated
@ -108,0 +114,4 @@
prm := make(map[string]any)
for k, v := range m {
switch k {
case netmap.MaxECDataCountConfig, netmap.MaxECParityCountConfig:
acid-ant commented 2024-04-05 07:39:04 +00:00
Member
Copy link

Why we need to modify parameters for EC here?

Why we need to modify parameters for EC here?
👍 1
achuprov commented 2024-04-05 08:06:58 +00:00
Author
Member
Copy link

There's a rule that the total of MaxECDataCount and MaxECParityCount can't be more than 256. If a user only picks one of these, we need to get the other from the network.

There's a rule that the total of `MaxECDataCount` and `MaxECParityCount` can't be more than 256. If a user only picks one of these, we need to get the other from the network.
acid-ant commented 2024-04-05 09:34:27 +00:00
Member
Copy link

If it is requirement to avoid setting one value without another, maybe it is better to set it only in pair?
In this case, we can skip getting the previous values and avoid iteration over the whole map for editing two keys.

If it is requirement to avoid setting one value without another, maybe it is better to set it only in pair? In this case, we can skip getting the previous values and avoid iteration over the whole map for editing two keys.
achuprov commented 2024-04-05 09:53:26 +00:00
Author
Member
Copy link

This will simplify the code, but this behavior may not be obvious to users

This will simplify the code, but this behavior may not be obvious to users
fyrchik commented 2024-04-05 09:59:46 +00:00
Owner
Copy link

I have no objections to setting both: but we better still have 2 separate parameters, they just need to be provided simultaneously.

I have no objections to setting both: but we better still have 2 separate parameters, they just need to be provided simultaneously.
👍 1
acid-ant commented 2024-04-05 10:04:23 +00:00
Member
Copy link

What is the reason to set configs for EC one by one? Because we have ability to skip check for 256 by calling contract method directly.

What is the reason to set configs for EC one by one? Because we have ability to skip check for `256` by calling contract method directly.
fyrchik commented 2024-04-05 11:20:44 +00:00
Owner
Copy link

Because this is how they are stored in the netmap contract.

Because this is how they are stored in the netmap contract.
achuprov commented 2024-04-08 08:34:29 +00:00
Author
Member
Copy link

fixed

fixed
acid-ant marked this conversation as resolved
achuprov force-pushed adm_ec_parameters from a2719fe3e9 to e52f6eeba8 2024-04-05 08:32:19 +00:00 Compare
achuprov force-pushed adm_ec_parameters from e52f6eeba8 to c51dd39b80 2024-04-08 08:09:45 +00:00 Compare
fyrchik approved these changes 2024-04-08 08:52:09 +00:00
cmd/frostfs-adm/internal/modules/morph/config/config.go Outdated
@ -131,0 +141,4 @@
var sumEC int64
_, okData := args[netmap.MaxECDataCountConfig]
_, okParity := args[netmap.MaxECParityCountConfig]
if (okData || okParity) && !(okData && okParity) {
fyrchik commented 2024-04-08 08:51:52 +00:00
Owner
Copy link

This is okData == okParity (which is also quite readable IMO: their existence status must be the same).

This is `okData == okParity` (which is also quite readable IMO: their existence status must be the same).
achuprov commented 2024-04-08 10:28:36 +00:00
Author
Member
Copy link

fixed

fixed
fyrchik commented 2024-04-08 08:52:31 +00:00
Owner
Copy link

Please, rebase on master, gofumpt commits should already be there.

Please, rebase on master, `gofumpt` commits should already be there.
achuprov force-pushed adm_ec_parameters from c51dd39b80 to 727f6bb0b8 2024-04-08 09:15:11 +00:00 Compare
achuprov force-pushed adm_ec_parameters from 727f6bb0b8 to 92569b9bbf 2024-04-08 09:28:44 +00:00 Compare
acid-ant approved these changes 2024-04-08 10:38:36 +00:00
aarifullin approved these changes 2024-04-08 10:42:47 +00:00
fyrchik merged commit 92569b9bbf into master 2024-04-09 07:03:41 +00:00
fyrchik referenced this pull request from a commit 2024-04-09 07:03:44 +00:00
[#1065] adm: refactor dump-config
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
acid-ant
aarifullin
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 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-node#1065
No description provided.
Delete branch "achuprov/frostfs-node:adm_ec_parameters"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?