Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node: Allow to configure tombsone lifetime #2246

Merged

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Feb 8, 2023

Currently, DELETE service sets tombstone expiration epoch to current epoch + 5. This works less than ideal in private networks where an epoch can be e.g. 10 minutes. In this case, after a node is unavailable for more than 1 hour, already deleted objects have a chance to reappear.

After this commit tombstone lifetime can be configured.

Signed-off-by: Evgenii Stratonikov e.stratonikov@yadro.com

| Parameter | Type | Default value | Description |
|-----------------------------|-------|---------------|------------------------------------------------------------------------------------------------|
| `delete.tombstone_lifetime` | `int` | `5` | Tombstone lifetime for removed objects. |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify units for this parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Currently, DELETE service sets tombstone expiration epoch to
`current epoch + 5`. This works less than ideal in private networks
where an epoch can be e.g. 10 minutes. In this case, after a node is
unavailable for more than 1 hour, already deleted objects have a chance
to reappear.

After this commit tombstone lifetime can be configured.

Signed-off-by: Evgenii Stratonikov <e.stratonikov@yadro.com>
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a network-specific parameter, then it should be a network-wide configuration, isn't it? Otherwise you can have different nodes using different configurations on the same network and it can lead to some surprises.

@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #2246 (22b01cb) into support/v0.35 (dcade98) will increase coverage by 0.01%.
The diff coverage is 62.50%.

@@                Coverage Diff                @@
##           support/v0.35    #2246      +/-   ##
=================================================
+ Coverage          30.99%   31.00%   +0.01%     
=================================================
  Files                383      384       +1     
  Lines              28419    28425       +6     
=================================================
+ Hits                8808     8813       +5     
- Misses             18870    18871       +1     
  Partials             741      741              
Impacted Files Coverage Δ
cmd/neofs-node/config.go 0.00% <0.00%> (ø)
cmd/neofs-node/object.go 0.00% <0.00%> (ø)
cmd/neofs-node/config/object/delete.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fyrchik
Copy link
Contributor Author

fyrchik commented Feb 8, 2023

@roman-khimov I partially agree with you. The case is that this lifetime can be arbitrary, we set it only in the DELETE service for every tombstone. Like with REGULAR objects the expiration epoch of a TOMBSTONE is not restricted.

@roman-khimov
Copy link
Member

I know. Technically anyone can create this object with any desired lifetime, OK. But regular well-behaving nodes doing regular DELETEs will use some value. And it's either node-specific (but why a well-behaving node does need some specific value?) or network-specific. It's quite likely that you'll have some node using the default configuration on any network, how will this affect the network?

@roman-khimov
Copy link
Member

Or take it from another perspective, you have some local configuration now that potentially affects the network globally. Is it a good thing to have?

@fyrchik
Copy link
Contributor Author

fyrchik commented Feb 8, 2023

Ok, I agree, it would probably also make it easier to scale. Other thoughts? @carpawell @acid-ant @realloc

This fix is kinda HOT, so I will check how invasive it is to take a network setting into account here.

@realloc
Copy link

realloc commented Feb 8, 2023

This parameters must be treated as a local node configuration option. While set network-wise it doesn't prevent any node from misbehavior nor affects interoperability.

@fyrchik fyrchik merged commit 00afc57 into nspcc-dev:support/v0.35 Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants