TrueCloudLab/frostfs-node
20
1
Fork 28
Code Issues 111 Pull requests 4 Projects Releases 2 Activity Actions

adm/frostfsid: Add 'set-kv' #1634

Merged
dstepanov-yadro merged 2 commits from achuprov/frostfs-node:feat/frostfs_adm_add_set-kv into master 2025-02-10 16:27:51 +00:00
Conversation 0 Commits 2 Files changed 2 +79
2 changed files with 79 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

77
cmd/frostfs-adm/internal/modules/morph/frostfsid/frostfsid.go
View file

@ -1,6 +1,7 @@
package frostfsid
import (
"errors"
"fmt"
"math/big"
"sort"
@ -38,6 +39,11 @@ const (
groupIDFlag = "group-id"
rootNamespacePlaceholder = "<root>"
keyFlag = "key"
keyDescFlag = "Key for storing a value in the subject's KV storage"
valueFlag = "value"
valueDescFlag = "Value to be stored in the subject's KV storage"
)
var (
@ -151,6 +157,23 @@ var (
},
Run: frostfsidListGroupSubjects,
}
frostfsidSetKVCmd = &cobra.Command{
Use: "set-kv",
Short: "Store a key-value pair in the subject's KV storage",
PreRun: func(cmd *cobra.Command, _ []string) {
_ = viper.BindPFlag(commonflags.EndpointFlag, cmd.Flags().Lookup(commonflags.EndpointFlag))
},
Run: frostfsidSetKV,
}
frostfsidDeleteKVCmd = &cobra.Command{
Use: "delete-kv",
Short: "Delete a value from the subject's KV storage",
PreRun: func(cmd *cobra.Command, _ []string) {
_ = viper.BindPFlag(commonflags.EndpointFlag, cmd.Flags().Lookup(commonflags.EndpointFlag))
},
Run: frostfsidDeleteKV,
}
)
func initFrostfsIDCreateNamespaceCmd() {
@ -236,6 +259,21 @@ func initFrostfsIDListGroupSubjectsCmd() {
frostfsidListGroupSubjectsCmd.Flags().Bool(includeNamesFlag, false, "Whether include subject name (require additional requests)")
}
func initFrostfsIDSetKVCmd() {
Cmd.AddCommand(frostfsidSetKVCmd)
frostfsidSetKVCmd.Flags().StringP(commonflags.EndpointFlag, commonflags.EndpointFlagShort, "", commonflags.EndpointFlagDesc)
frostfsidSetKVCmd.Flags().String(subjectAddressFlag, "", "Subject address")
frostfsidSetKVCmd.Flags().String(keyFlag, "", keyDescFlag)
frostfsidSetKVCmd.Flags().String(valueFlag, "", valueDescFlag)
}
func initFrostfsIDDeleteKVCmd() {
achuprov marked this conversation as resolved Outdated
fyrchik commented 2025-02-07 09:49:47 +00:00
Outdated
Review
Copy link

Why do we need this line? Its seems generic and unrelated to the "well-known" keys.

Why do we need this line? Its seems generic and unrelated to the "well-known" keys.
Cmd.AddCommand(frostfsidDeleteKVCmd)
frostfsidDeleteKVCmd.Flags().StringP(commonflags.EndpointFlag, commonflags.EndpointFlagShort, "", commonflags.EndpointFlagDesc)
frostfsidDeleteKVCmd.Flags().String(subjectAddressFlag, "", "Subject address")
frostfsidDeleteKVCmd.Flags().String(keyFlag, "", keyDescFlag)
}
func frostfsidCreateNamespace(cmd *cobra.Command, _ []string) {
ns := getFrostfsIDNamespace(cmd)
@ -403,6 +441,45 @@ func frostfsidRemoveSubjectFromGroup(cmd *cobra.Command, _ []string) {
commonCmd.ExitOnErr(cmd, "remove subject from group error: %w", err)
}
func frostfsidSetKV(cmd *cobra.Command, _ []string) {
subjectAddress := getFrostfsIDSubjectAddress(cmd)
key, _ := cmd.Flags().GetString(keyFlag)
value, _ := cmd.Flags().GetString(valueFlag)
if key == "" {
commonCmd.ExitOnErr(cmd, "", errors.New("key can't be empty"))
fyrchik commented 2025-02-05 11:13:26 +00:00
Outdated
Review
Copy link

Please, let's have 2 separate commands: set-kv and delete-kv.
The --disable flag has a wrong name and this for loop should not be executed if we want to delete kv.

Please, let's have 2 separate commands: `set-kv` and `delete-kv`. The `--disable` flag has a wrong name and this `for` loop should not be executed if we want to delete kv.
👍 1
achuprov commented 2025-02-06 16:50:14 +00:00
Outdated
Review
Copy link

done

done
}
achuprov marked this conversation as resolved Outdated
fyrchik commented 2025-02-05 11:15:29 +00:00
Outdated
Review
Copy link

This logic should be reverted: we have key and we should check whether it is well-known.
You iterate over all well-known keys and check whether a key is present.
Another problem is that you silently ignore previous key value, the flags should be marked as mutually exclusive.

This logic should be reverted: we _have_ key and we should check whether it is well-known. You iterate over all well-known keys and check whether a key is present. Another problem is that you silently ignore previous key value, the flags should be marked as mutually exclusive.
achuprov commented 2025-02-06 16:50:23 +00:00
Outdated
Review
Copy link

fixed

fixed
ffsid, err := newFrostfsIDClient(cmd)
commonCmd.ExitOnErr(cmd, "init contract client: %w", err)
method, args := ffsid.roCli.SetSubjectKVCall(subjectAddress, key, value)
ffsid.addCall(method, args)
err = ffsid.sendWait()
commonCmd.ExitOnErr(cmd, "set KV: %w", err)
}
func frostfsidDeleteKV(cmd *cobra.Command, _ []string) {
subjectAddress := getFrostfsIDSubjectAddress(cmd)
key, _ := cmd.Flags().GetString(keyFlag)
if key == "" {
aarifullin commented 2025-02-04 15:27:14 +00:00
Outdated
Review
Copy link

I can't figure out what is going on in this part. Why do you set a key to subject's KV storage and then remove it if disable flag is set? I checked the contract's method and deleteSubjectKV literally means deletion but not disabling.
So, we need to make frostfsidSetKV free from deletion and introduce frostfsidDeleteKV and not use disable.
frostfsidSetKV is responsible for setting but not "disabling" (when it deletes in fact) :)

I can't figure out what is going on in this part. Why do you set a key to subject's KV storage and then remove it if `disable` flag is set? I checked the contract's method and `deleteSubjectKV` literally means deletion but not disabling. So, we need to make `frostfsidSetKV` free from deletion and introduce `frostfsidDeleteKV` and not use `disable`. `frostfsidSetKV` is responsible for setting but not "disabling" (when it deletes in fact) :)
achuprov commented 2025-02-05 10:50:56 +00:00
Outdated
Review
Copy link

IMHO, by default, the FrostFS ID KV storage does not contain any options. When we add a key, these options are enabled. This means that deleting an option disables it.

IMHO, by default, the `FrostFS ID KV` storage does not contain any options. When we add a key, these options are enabled. This means that deleting an option disables it.
fyrchik commented 2025-02-05 11:27:06 +00:00
Outdated
Review
Copy link

There are no "options" that are enabled or disabled, there are keys and values, which (according to contract) we can set and delete.

There are no "options" that are enabled or disabled, there are keys and values, which (according to contract) we can `set` and `delete`.
commonCmd.ExitOnErr(cmd, "", errors.New("key can't be empty"))
}
ffsid, err := newFrostfsIDClient(cmd)
commonCmd.ExitOnErr(cmd, "init contract client: %w", err)
method, args := ffsid.roCli.DeleteSubjectKVCall(subjectAddress, key)
ffsid.addCall(method, args)
fyrchik commented 2025-02-07 09:51:25 +00:00
Outdated
Review
Copy link

Empty value almost always is an error, because we forgot.
Let's remove this "wellKnownValue" logic.

Empty value almost always is an error, because we forgot. Let's remove this "wellKnownValue" logic.
achuprov commented 2025-02-07 14:05:53 +00:00
Outdated
Review
Copy link

removed

removed
err = ffsid.sendWait()
commonCmd.ExitOnErr(cmd, "delete KV: %w", err)
}
func frostfsidListGroupSubjects(cmd *cobra.Command, _ []string) {
ns := getFrostfsIDNamespace(cmd)
aarifullin commented 2025-02-07 08:36:53 +00:00
Outdated
Review
Copy link

Let's avoid using can't and failed in error messages. I used to use them too but actually that's unnecessary.
I'd consider just empty key. And failed to set -> set key: %w, failed to delete -> delete key: %w

Let's avoid using `can't` and `failed` in error messages. I used to use them too but actually that's unnecessary. I'd consider just `empty key`. And `failed to set` -> `set key: %w`, `failed to delete` -> `delete key: %w`
achuprov commented 2025-02-10 12:35:00 +00:00
Outdated
Review
Copy link

fixed

fixed
groupID := getFrostfsIDGroupID(cmd)

2
cmd/frostfs-adm/internal/modules/morph/frostfsid/root.go
View file

@ -12,6 +12,8 @@ func init() {
initFrostfsIDAddSubjectToGroupCmd()
initFrostfsIDRemoveSubjectFromGroupCmd()
initFrostfsIDListGroupSubjectsCmd()
initFrostfsIDSetKVCmd()
initFrostfsIDDeleteKVCmd()
initFrostfsIDAddSubjectKeyCmd()
initFrostfsIDRemoveSubjectKeyCmd()
}