ape: Implement boltdb storage for local overrides #820

Merged
fyrchik merged 1 commit from aarifullin/frostfs-node:feature/804_override_storage into master 2023-12-11 09:41:56 +00:00
Member

Signed-off-by: Airat Arifullin a.arifullin@yadro.com

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
aarifullin changed title from [#XX] ape: Implement boltdb storage for local overrides to WIP: [#XX] ape: Implement boltdb storage for local overrides 2023-11-20 16:36:15 +00:00
aarifullin force-pushed feature/804_override_storage from 5b65f42339 to 042a8a6901 2023-11-21 08:00:03 +00:00 Compare
aarifullin requested review from storage-core-committers 2023-11-21 08:02:43 +00:00
aarifullin requested review from storage-core-developers 2023-11-21 08:02:55 +00:00
aarifullin force-pushed feature/804_override_storage from 042a8a6901 to b8afdb33aa 2023-11-21 08:03:32 +00:00 Compare
aarifullin changed title from WIP: [#XX] ape: Implement boltdb storage for local overrides to [#XX] ape: Implement boltdb storage for local overrides 2023-11-21 08:04:01 +00:00
aarifullin changed title from [#XX] ape: Implement boltdb storage for local overrides to ape: Implement boltdb storage for local overrides 2023-11-21 08:04:21 +00:00
aarifullin force-pushed feature/804_override_storage from b8afdb33aa to 1972f49aee 2023-11-21 08:45:49 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from 1972f49aee to bfc8b2abb4 2023-11-21 09:03:51 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from bfc8b2abb4 to ba45faed1a 2023-11-21 09:27:55 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from ba45faed1a to 4cf617475b 2023-11-21 09:29:25 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from 4cf617475b to ce9d94616f 2023-11-21 12:04:39 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from ce9d94616f to 86613586cd 2023-11-21 12:20:58 +00:00 Compare
fyrchik reviewed 2023-11-22 07:53:24 +00:00
@ -38,0 +40,4 @@
persistentSessionsSubsection = "persistent_sessions"
persistentStateSubsection = "persistent_state"
notificationSubsection = "notification"
localOverrideDatabaseSubsection = "local_override_database"
Owner

It is too generic for a top-level name, can we use sth like persistent_policy_rules?

It is too generic for a top-level name, can we use sth like `persistent_policy_rules`?
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -248,0 +254,4 @@
const (
// PermDefault is a default permission bits for local override storage file.
PermDefault = 0o777
Owner

Why not 0o640?

Why not `0o640`?
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -248,0 +294,4 @@
// MaxBatchDelay returns the value of "max_batch_delay" config parameter.
//
// Returns 0 if the value is not a positive number.
func (l LocalOverrideDatabaseConfig) MaxBatchDelay() time.Duration {
Owner

I don't think these parameters are necessary, putting local overrides is not something we do often.
And we use Update anyway.

I don't think these parameters are necessary, putting local overrides is not something we do often. And we use `Update` anyway.
Author
Member

Removed

Removed
fyrchik marked this conversation as resolved
@ -19,1 +23,3 @@
localChainStorage: make(map[cid.ID]engine.LocalOverrideEngine),
var _ engine.LocalOverrideEngine = (*AccessPolicyEngine)(nil)
func NewAccessPolicyEngine(opts ...func(ape *AccessPolicyEngine)) *AccessPolicyEngine {
Owner
  1. Why is it public?
  2. What is the benefit of using functional options here?
1. Why is it public? 2. What is the benefit of using functional options here?
Author
Member
  1. I've made it private
  2. inmemory implementations are used as default if no options are passed
1. I've made it private 2. `inmemory` implementations are used as default if no options are passed
Owner

I mean why do we use functional options and not some struct?
If path to db is empty we can use in-memory and log. Frankly, I would refuse to start unless we explicitly want in-memory (most likely not).

I mean why do we use _functional_ options and not some struct? If path to db is empty we can use in-memory and log. Frankly, I would refuse to start unless we explicitly want in-memory (most likely not).
Author
Member

If path to db is empty we can use in-memory

I liked the idea. I remove func options at all and pass only two parameters to the constructor that are initialized explicitly

> If path to db is empty we can use in-memory I liked the idea. I remove func options at all and pass only two parameters to the constructor that are initialized explicitly
fyrchik marked this conversation as resolved
@ -20,0 +69,4 @@
return a.localOverrideDatabase
}
func WithChainRouter(router engine.ChainRouter) func(ape *AccessPolicyEngine) {
Owner

Is is used anywhere?

Is is used anywhere?
Author
Member

Removed

Removed
fyrchik marked this conversation as resolved
@ -0,0 +122,4 @@
var value []byte
if value = cbucket.Get(k); value == nil {
chains := []*chain.Chain{c}
marshalled, err := json.Marshal(chains)
Owner

Marshaling inside Update is a bad idea, can we move it outside?

Marshaling inside `Update` is a bad idea, can we move it outside?
Author
Member

You are correct. Placing the unmarshalling within transaction is really bad idea. I've taken these out of Update and View for all methods

You are correct. Placing the unmarshalling within transaction is really bad idea. I've taken these out of `Update` and `View` for all methods
Owner

For View it is not so bad, it blocks only remapping.

For `View` it is not so bad, it blocks only remapping.
fyrchik marked this conversation as resolved
@ -0,0 +141,4 @@
}
}
if !wasReplaced {
chains = append(chains, c)
Owner

What about storing chain by key? This way we have O(1) update and listing can be done with iteration.

What about storing chain by key? This way we have O(1) update and listing can be done with iteration.
Author
Member

OK.

I have made Bucket: Name -> Bucket: Resource -> key (chainID): value (marshalled chain)

OK. I have made `Bucket: Name` -> `Bucket: Resource` -> `key (chainID): value (marshalled chain)`
fyrchik marked this conversation as resolved
@ -0,0 +61,4 @@
}
}
func WithOpenFile(f func(string, int, fs.FileMode) (*os.File, error)) Option {
Owner

What is the usecase for this option?

What is the usecase for this option?
Author
Member

Removed

Removed
fyrchik marked this conversation as resolved
aarifullin force-pushed feature/804_override_storage from 86613586cd to b222542e3b 2023-11-22 10:21:44 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from b222542e3b to 610ad12967 2023-11-22 10:24:19 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from 610ad12967 to 5289ee2651 2023-11-22 11:09:22 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from 5289ee2651 to 0224366c07 2023-11-22 11:11:49 +00:00 Compare
dstepanov-yadro requested changes 2023-11-23 08:26:26 +00:00
@ -962,0 +976,4 @@
c.onShutdown(func() {
if err := ape.LocalOverrideDatabaseCore().Close(); err != nil {
c.log.Info(logs.FrostFSNodeAccessPolicyEngineClosingFailure,

Info -> Warn

Info -> Warn
Author
Member

Fixed

Fixed
dstepanov-yadro marked this conversation as resolved
@ -20,0 +49,4 @@
}
func (a *accessPolicyEngine) MorphRuleChainStorage() engine.MorphRuleChainStorage {
a.mtx.Lock()

I think it must be RLock too.

I think it must be `RLock` too.
Author
Member

I have put a.mtx.Lock() because MorphRuleChainStorage is changable. We may add rules to policy-contract via engine.MorphRuleChainStorage methods.

If it is RLock-ed for adding new rules (write) then chain routing for a request will be incorrect

I have put `a.mtx.Lock()` because `MorphRuleChainStorage` is changable. We may add rules to policy-contract via `engine.MorphRuleChainStorage` methods. If it is `RLock`-ed for adding new rules (write) then chain routing for a request will be incorrect
dstepanov-yadro marked this conversation as resolved
@ -0,0 +143,4 @@
if err != nil {
return err
}
serializedChain = rbuck.Get([]byte(chainID))

Get result must be copied to other slice: returned value is valid only for transaction lifetime.

`Get` result must be copied to other slice: returned value is valid only for transaction lifetime.
Author
Member

You are correct. The doc really says

"The returned value is only valid for the life of the transaction".

Fixed

You are correct. The doc really says > "The returned value is only valid for the life of the transaction". Fixed
dstepanov-yadro marked this conversation as resolved
@ -53,1 +54,4 @@
// LocalOverrideStorageDecorator interface provides methods to decorate LocalOverrideEngine
// interface methods.
type LocalOverrideStorageDecorator interface {

interface looks redundant: using struct can be enough i think. not required to fix.

interface looks redundant: using struct can be enough i think. not required to fix.
Author
Member

If you are talking about using

type accessPolicyEngine struct { /*...*/ }

instead LocalOverrideStorageDecorator, then I can agree your point is fair, but I really do not want to let control service manage database, i.e. provide access to Init, Open, Close and give only LocalStorage methods

If you are talking about using ```go type accessPolicyEngine struct { /*...*/ } ``` instead `LocalOverrideStorageDecorator`, then I can agree your point is fair, but I really do not want to let control service manage database, i.e. provide access to `Init`, `Open`, `Close` and give only `LocalStorage` methods
dstepanov-yadro marked this conversation as resolved
aarifullin force-pushed feature/804_override_storage from 0224366c07 to 377b34a980 2023-11-23 08:53:39 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from 377b34a980 to 41ebec2eeb 2023-11-23 08:57:38 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from 41ebec2eeb to c445638472 2023-11-23 11:57:44 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from c445638472 to 3b5176335f 2023-11-23 11:59:51 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from 3b5176335f to f54884981d 2023-11-23 12:00:57 +00:00 Compare
dstepanov-yadro requested changes 2023-11-23 12:20:06 +00:00
@ -0,0 +179,4 @@
return err
}
return rbuck.ForEach(func(_, serializedChain []byte) error {
serializedChains = append(serializedChains, serializedChain)

This []byte slice requires to be copied too

This `[]byte` slice requires to be copied too
Author
Member

Oh, thanks! Fixed

Oh, thanks! Fixed
dstepanov-yadro marked this conversation as resolved
aarifullin force-pushed feature/804_override_storage from f54884981d to a32551bcb6 2023-11-23 15:00:53 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2023-11-28 07:11:51 +00:00
aarifullin requested review from storage-core-committers 2023-11-28 07:12:05 +00:00
dstepanov-yadro approved these changes 2023-11-28 15:06:38 +00:00
aarifullin force-pushed feature/804_override_storage from a32551bcb6 to 61b01a791f 2023-11-28 15:59:25 +00:00 Compare
fyrchik approved these changes 2023-11-29 06:20:37 +00:00
@ -963,0 +966,4 @@
func initAccessPolicyEngine(_ context.Context, c *cfg) {
var localOverrideDB chainbase.LocalOverrideDatabase
if nodeconfig.PersistentPolicyRules(c.appCfg).Path() == "" {
localOverrideDB = chainbase.NewInmemoryLocalOverrideDatabase()
Owner

Maybe log something in this case? It is probably an error.

Maybe log something in this case? It is probably an error.
Author
Member

I think it's nothing wrong to skip persistent_policy_rules section to skip db initialization and use default inmemory storage - it may be useful for debug. Let's warn a user with c.log.Warn if path is not set and in-memory will be used

I think it's nothing wrong to skip `persistent_policy_rules` section to skip db initialization and use default `inmemory` storage - it may be useful for debug. Let's warn a user with `c.log.Warn` if path is not set and in-memory will be used
aarifullin force-pushed feature/804_override_storage from 61b01a791f to e03b9e0565 2023-11-29 09:10:37 +00:00 Compare
aarifullin requested review from fyrchik 2023-11-29 09:14:33 +00:00
aarifullin requested review from dstepanov-yadro 2023-11-29 09:14:34 +00:00
dstepanov-yadro approved these changes 2023-11-29 15:14:29 +00:00
aarifullin added the
blocked
label 2023-12-05 09:20:54 +00:00
Author
Member

Blocked by #842

Blocked by https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/842
aarifullin force-pushed feature/804_override_storage from e03b9e0565 to e65ee24f4b 2023-12-06 12:04:03 +00:00 Compare
aarifullin changed title from ape: Implement boltdb storage for local overrides to WIP: ape: Implement boltdb storage for local overrides 2023-12-06 12:04:41 +00:00
aarifullin force-pushed feature/804_override_storage from e65ee24f4b to 5c65708357 2023-12-07 11:14:53 +00:00 Compare
aarifullin force-pushed feature/804_override_storage from 5c65708357 to 62dd50934a 2023-12-07 16:01:13 +00:00 Compare
aarifullin removed the
blocked
label 2023-12-07 16:01:31 +00:00
aarifullin changed title from WIP: ape: Implement boltdb storage for local overrides to ape: Implement boltdb storage for local overrides 2023-12-07 16:01:40 +00:00
aarifullin requested review from dstepanov-yadro 2023-12-07 16:05:42 +00:00
Author
Member

For those who reviewed this PR - the pull request has been changed in this file - it's got a such bucket hierarchy:

  • global namespace (Ingress)
  • target type
  • target name
For those who reviewed this PR - the pull request has been changed in this [file](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/820/files#diff-68f1e52934f6af23a0971dea2a981478309a0ff8) - it's got a such bucket hierarchy: - global namespace (`Ingress`) - target type - target name
aarifullin force-pushed feature/804_override_storage from 62dd50934a to 0f45e3d344 2023-12-07 16:08:53 +00:00 Compare
dkirillov approved these changes 2023-12-08 06:47:04 +00:00
dkirillov left a comment
Member

LGTM

LGTM
fyrchik approved these changes 2023-12-08 07:32:58 +00:00
dstepanov-yadro approved these changes 2023-12-08 07:33:21 +00:00
fyrchik merged commit 0f45e3d344 into master 2023-12-11 09:41:56 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
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#820
No description provided.