ape: Implement boltdb storage for local overrides #820
No reviewers
Labels
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
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#820
Loading…
Reference in a new issue
No description provided.
Delete branch "aarifullin/frostfs-node:feature/804_override_storage"
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?
Signed-off-by: Airat Arifullin a.arifullin@yadro.com
[#XX] ape: Implement boltdb storage for local overridesto WIP: [#XX] ape: Implement boltdb storage for local overrides5b65f42339
to042a8a6901
042a8a6901
tob8afdb33aa
WIP: [#XX] ape: Implement boltdb storage for local overridesto [#XX] ape: Implement boltdb storage for local overrides[#XX] ape: Implement boltdb storage for local overridesto ape: Implement boltdb storage for local overridesb8afdb33aa
to1972f49aee
1972f49aee
tobfc8b2abb4
bfc8b2abb4
toba45faed1a
ba45faed1a
to4cf617475b
4cf617475b
toce9d94616f
ce9d94616f
to86613586cd
@ -38,0 +40,4 @@
persistentSessionsSubsection = "persistent_sessions"
persistentStateSubsection = "persistent_state"
notificationSubsection = "notification"
localOverrideDatabaseSubsection = "local_override_database"
It is too generic for a top-level name, can we use sth like
persistent_policy_rules
?Fixed
@ -248,0 +254,4 @@
const (
// PermDefault is a default permission bits for local override storage file.
PermDefault = 0o777
Why not
0o640
?Fixed
@ -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 {
I don't think these parameters are necessary, putting local overrides is not something we do often.
And we use
Update
anyway.Removed
@ -19,1 +23,3 @@
localChainStorage: make(map[cid.ID]engine.LocalOverrideEngine),
var _ engine.LocalOverrideEngine = (*AccessPolicyEngine)(nil)
func NewAccessPolicyEngine(opts ...func(ape *AccessPolicyEngine)) *AccessPolicyEngine {
inmemory
implementations are used as default if no options are passedI 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 liked the idea. I remove func options at all and pass only two parameters to the constructor that are initialized explicitly
@ -20,0 +69,4 @@
return a.localOverrideDatabase
}
func WithChainRouter(router engine.ChainRouter) func(ape *AccessPolicyEngine) {
Is is used anywhere?
Removed
@ -0,0 +122,4 @@
var value []byte
if value = cbucket.Get(k); value == nil {
chains := []*chain.Chain{c}
marshalled, err := json.Marshal(chains)
Marshaling inside
Update
is a bad idea, can we move it outside?You are correct. Placing the unmarshalling within transaction is really bad idea. I've taken these out of
Update
andView
for all methodsFor
View
it is not so bad, it blocks only remapping.@ -0,0 +141,4 @@
}
}
if !wasReplaced {
chains = append(chains, c)
What about storing chain by key? This way we have O(1) update and listing can be done with iteration.
OK.
I have made
Bucket: Name
->Bucket: Resource
->key (chainID): value (marshalled chain)
@ -0,0 +61,4 @@
}
}
func WithOpenFile(f func(string, int, fs.FileMode) (*os.File, error)) Option {
What is the usecase for this option?
Removed
86613586cd
tob222542e3b
b222542e3b
to610ad12967
610ad12967
to5289ee2651
5289ee2651
to0224366c07
@ -962,0 +976,4 @@
c.onShutdown(func() {
if err := ape.LocalOverrideDatabaseCore().Close(); err != nil {
c.log.Info(logs.FrostFSNodeAccessPolicyEngineClosingFailure,
Info -> Warn
Fixed
@ -20,0 +49,4 @@
}
func (a *accessPolicyEngine) MorphRuleChainStorage() engine.MorphRuleChainStorage {
a.mtx.Lock()
I think it must be
RLock
too.I have put
a.mtx.Lock()
becauseMorphRuleChainStorage
is changable. We may add rules to policy-contract viaengine.MorphRuleChainStorage
methods.If it is
RLock
-ed for adding new rules (write) then chain routing for a request will be incorrect@ -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.You are correct. The doc really says
Fixed
@ -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.
If you are talking about using
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 toInit
,Open
,Close
and give onlyLocalStorage
methods0224366c07
to377b34a980
377b34a980
to41ebec2eeb
41ebec2eeb
toc445638472
c445638472
to3b5176335f
3b5176335f
tof54884981d
@ -0,0 +179,4 @@
return err
}
return rbuck.ForEach(func(_, serializedChain []byte) error {
serializedChains = append(serializedChains, serializedChain)
This
[]byte
slice requires to be copied tooOh, thanks! Fixed
f54884981d
toa32551bcb6
a32551bcb6
to61b01a791f
@ -963,0 +966,4 @@
func initAccessPolicyEngine(_ context.Context, c *cfg) {
var localOverrideDB chainbase.LocalOverrideDatabase
if nodeconfig.PersistentPolicyRules(c.appCfg).Path() == "" {
localOverrideDB = chainbase.NewInmemoryLocalOverrideDatabase()
Maybe log something in this case? It is probably an error.
I think it's nothing wrong to skip
persistent_policy_rules
section to skip db initialization and use defaultinmemory
storage - it may be useful for debug. Let's warn a user withc.log.Warn
if path is not set and in-memory will be used61b01a791f
toe03b9e0565
Blocked by #842
e03b9e0565
toe65ee24f4b
ape: Implement boltdb storage for local overridesto WIP: ape: Implement boltdb storage for local overridese65ee24f4b
to5c65708357
5c65708357
to62dd50934a
WIP: ape: Implement boltdb storage for local overridesto ape: Implement boltdb storage for local overridesFor those who reviewed this PR - the pull request has been changed in this file - it's got a such bucket hierarchy:
Ingress
)62dd50934a
to0f45e3d344
LGTM