EC DELETE by cthulhu-rider · Pull Request #3549 · nspcc-dev/neofs-node

cthulhu-rider · 2025-08-28T16:30:28Z

No description provided.

codecov · 2025-08-28T16:32:59Z

Codecov Report

❌ Patch coverage is 78.57143% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.67%. Comparing base (e8348a9) to head (a7c2a54).
⚠️ Report is 12 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/local_object_storage/shard/head.go	56.52%	10 Missing ⚠️
cmd/neofs-lens/internal/meta/get.go	0.00%	7 Missing ⚠️
pkg/local_object_storage/metabase/ec.go	90.32%	2 Missing and 1 partial ⚠️
pkg/local_object_storage/metabase/exists.go	85.00%	3 Missing ⚠️
pkg/local_object_storage/metabase/inhume.go	84.21%	2 Missing and 1 partial ⚠️
pkg/local_object_storage/engine/exists.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3549      +/-   ##
==========================================
+ Coverage   26.51%   26.67%   +0.15%     
==========================================
  Files         653      654       +1     
  Lines       49290    49373      +83     
==========================================
+ Hits        13071    13169      +98     
+ Misses      35181    35158      -23     
- Partials     1038     1046       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cthulhu-rider · 2025-08-29T10:52:13Z

pkg/local_object_storage/metabase/put_test.go

+		childAddr := objAddr
+		childAddr.SetObject(child.GetID())
+
+		// TODO: check why Get returns different header


i'll leave an issue for this

Because there are limits to what Get can restore from the metabase, it's OK.

roman-khimov

This makes a different choice compared to regular big objects, we don't mark children as deleted immediately there, but we handle relations during GC cycle (related to #3551). We need some unified approach.

pkg/local_object_storage/engine/put_test.go

roman-khimov · 2025-09-09T19:19:23Z

pkg/local_object_storage/metabase/put_test.go

+		childAddr := objAddr
+		childAddr.SetObject(child.GetID())
+
+		// TODO: check why Get returns different header


Because there are limits to what Get can restore from the metabase, it's OK.

cthulhu-rider · 2025-09-10T08:38:58Z

This makes a different choice compared to regular big objects, we don't mark children as deleted immediately there, but we handle relations during GC cycle (related to #3551). We need some unified approach.

generally speaking, i expected that i wouldnt need any additional actions and DELETE would work out of the box. All EC parts refer to the parent. And split DELETE works as expected, also building logic around the parent ref. Surprise to me. I'll review again what's missing in the general approach

cthulhu-rider · 2025-09-10T12:11:42Z

ok, all parent-child magic when happens in

neofs-node/pkg/local_object_storage/engine/inhume.go

Line 93 in 64c8ca9

    
           func (e *StorageEngine) inhumeAddr(addr oid.Address, force bool, tombstone *oid.Address, tombExpiration uint64) (bool, error) {

and collecting of child objects is hardly tied to the split scheme. I see two ways:

Unify the obtaining of child objects, making it independent of the root object's encoding (split, EC, etc.). Object ID is input, a full list of child IDs is output. When deleting, only fact of the relationship itself is important, not how it is obtained.
process EC specifically

finally, i'd like to have 1. Maybe @roman-khimov meant this by

We need some unified approach.

but it'd be a quite shaking change, risky. I suggest to start from 2, and generalize to 1 separately. For example, postpone to #3500 where it almost definitely will be needed

roman-khimov · 2025-09-10T12:30:19Z

Think of expired objects,

neofs-node/pkg/local_object_storage/engine/inhume.go

Lines 266 to 271 in 64c8ca9

    
           func (e *StorageEngine) processExpiredObjects(addrs []oid.Address) { 
        
           	err := e.inhume(addrs, false, nil, 0) 
        
           	if err != nil { 
        
           		e.log.Warn("handling expired objects", zap.Error(err)) 
        
           	} 
        
           }

Maybe if we're to fix inhumeAddr() for EC it'd be sufficient.

cthulhu-rider · 2025-09-15T15:53:19Z

reworked closer to split case

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

One less function. One less usage of `getSplitInfo()`. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

One less error to worry about. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

Previous text was outdated. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

cmd/neofs-lens/internal/meta/get.go

roman-khimov · 2025-09-17T16:30:30Z

pkg/local_object_storage/engine/ec_test.go

+
+	var tombPartAddrs []oid.Address
+	for i := range partNum {
+		tombPart, err := iec.FormObjectForECPart(signer, tomb, testutil.RandByteSlice(32), iec.PartInfo{


Why are we splitting tombstones?

i havent made any exceptions yet. But given the lack of benefit in ECing them, better to exclude. Their payload is empty anyway

Exactly. It's header only and splitting it doesn't make much sense to me. Same goes for locks.

fixed this in test. How about excluding tombstones in a separate PR?

Works for me.

pkg/local_object_storage/engine/get.go

roman-khimov · 2025-09-17T16:46:25Z

pkg/local_object_storage/shard/head.go

+					oid.NewAddress(addr.Container(), si.GetLink())}
+			case errors.As(err, &errECParts):
+				if len(errECParts) == 0 {
+					panic(errors.New("empty EC part set"))


Worth a panic?

yeah, to prevent code mistakes. Subsequent logic assumes that it is non-empty

In general, the mechanism is inherited from split scheme. Key changes: - all GET methods return newly added error if EC parent is requested. At the moment, only physically stored EC parts are requested. This will protect against collision with split case and will reveal unadapted places if any. - If some EC parts are stored in the metabase along with the object being removed, they are also marked. This automates their closure and subsequent GC. Closes #3502. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

It is the parent objects that are expiring and deleting in practice. Reading is also performed at their address. Previously, the method checked child status instead. In particular, this led to 404 child status return when parent object was removed. This collided with the part absence on SN: triggered requests from backup SN and resulted in 404 final status. Refs #3502. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

cthulhu-rider mentioned this pull request Aug 29, 2025

Leaking child objects after parent delete in multishard local storage setup #3551

Closed

cthulhu-rider force-pushed the ec-delete branch 4 times, most recently from 21d46d9 to c82ecb9 Compare August 29, 2025 10:48

cthulhu-rider commented Aug 29, 2025

View reviewed changes

cthulhu-rider mentioned this pull request Aug 29, 2025

Metabase can return strange child header #3552

Closed

cthulhu-rider force-pushed the ec-delete branch 2 times, most recently from 870dd1b to 89b58f0 Compare August 29, 2025 10:56

cthulhu-rider marked this pull request as ready for review August 29, 2025 11:16

cthulhu-rider requested review from End-rey, carpawell and roman-khimov as code owners August 29, 2025 11:16

roman-khimov reviewed Sep 9, 2025

View reviewed changes

cthulhu-rider mentioned this pull request Sep 11, 2025

Minor improvements in storage DELETE #3575

Merged

cthulhu-rider force-pushed the ec-delete branch 3 times, most recently from b6fcd84 to 9a513cf Compare September 15, 2025 15:52

cthulhu-rider force-pushed the ec-delete branch 3 times, most recently from 9b6b39b to 687f621 Compare September 16, 2025 11:52

cthulhu-rider added 4 commits September 16, 2025 15:17

storage/metabase: Drop unused getCompat() parameter

4206760

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

storage/metabase: Drop static getCompat() parameter

2ba9236

One less function. One less usage of `getSplitInfo()`. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

storage/metabase: Refactor getSplitInfo() without ErrLackSplitInfo

651ccf6

One less error to worry about. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

storage/metabase: Actualize getSplitInfo() desc

1f5d187

Previous text was outdated. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

storage/metabase: Add more docs about split object behavior

6ecbbab

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

cthulhu-rider force-pushed the ec-delete branch 2 times, most recently from 428e719 to 9135b7f Compare September 16, 2025 12:27

roman-khimov reviewed Sep 17, 2025

View reviewed changes

cthulhu-rider added 2 commits September 18, 2025 13:33

cthulhu-rider force-pushed the ec-delete branch from 9135b7f to a7c2a54 Compare September 18, 2025 10:33

roman-khimov mentioned this pull request Sep 18, 2025

Decouple expired object collector from epoch ticker #3582

Merged

roman-khimov approved these changes Sep 18, 2025

View reviewed changes

roman-khimov merged commit 231cb3d into master Sep 18, 2025
21 of 22 checks passed

roman-khimov deleted the ec-delete branch September 18, 2025 13:13

Conversation

cthulhu-rider commented Aug 28, 2025

Uh oh!

codecov bot commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roman-khimov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cthulhu-rider commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cthulhu-rider commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roman-khimov commented Sep 10, 2025

Uh oh!

cthulhu-rider commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cthulhu-rider Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Aug 28, 2025 •

edited

Loading

roman-khimov left a comment •

edited

Loading

cthulhu-rider commented Sep 10, 2025 •

edited

Loading

cthulhu-rider commented Sep 10, 2025 •

edited

Loading

cthulhu-rider commented Sep 15, 2025 •

edited

Loading

cthulhu-rider Sep 18, 2025 •

edited

Loading