Fix shard phy counter

CHANGELOG.md

@@ -31,6 +31,7 @@ Changelog for NeoFS Node
 - Object left in the storage after unsuccessful meta PUT (#3744)
 - Object left in some shards on fatal broadcast failure (#3744)
 - Deadlock on SN exit in rare GC cases (#3744)
+- SN no longer counts removed parent objects as physical ones in metrics (#3717)
 
 ### Changed
 - Optimized locking info in metabase (#3672)

pkg/local_object_storage/metabase/delete.go

@@ -2,6 +2,7 @@ package meta
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"slices"
 
@@ -25,8 +26,8 @@ type DeleteRes struct {
 	// Actually removed objects. First len(addrs) elements always contain addrs
 	// passed to [DB.Delete], but order is different in general.
 	RemovedObjects []RemovedObject
-	// RawRemoved contains the number of removed raw objects.
-	RawRemoved uint64
+	// PhyRemoved contains the number of removed physical objects.
+	PhyRemoved uint64
 	// AvailableRemoved contains the number of removed available objects.
 	AvailableRemoved uint64
 }
@@ -48,14 +49,14 @@ func (db *DB) Delete(addrs []oid.Address) (DeleteRes, error) {
 		return DeleteRes{}, ErrReadOnlyMode
 	}
 
-	var rawRemoved uint64
+	var phyRemoved uint64
 	var availableRemoved uint64
 	var err error
 	var removed []RemovedObject
 
 	err = db.boltDB.Update(func(tx *bbolt.Tx) error {
 		// We need to clear slice because tx can try to execute multiple times.
-		rawRemoved, availableRemoved, removed, err = db.deleteGroup(tx, addrs)
+		phyRemoved, availableRemoved, removed, err = db.deleteGroup(tx, addrs)
 		return err
 	})
 	if err == nil {
@@ -66,7 +67,7 @@ func (db *DB) Delete(addrs []oid.Address) (DeleteRes, error) {
 		}
 	}
 	return DeleteRes{
-		RawRemoved:       rawRemoved,
+		PhyRemoved:       phyRemoved,
 		AvailableRemoved: availableRemoved,
 		RemovedObjects:   removed,
 	}, err
@@ -79,7 +80,7 @@ func (db *DB) Delete(addrs []oid.Address) (DeleteRes, error) {
 // removed number: objects that were available (without Tombstones, GCMarks
 // non-expired, etc.)
 func (db *DB) deleteGroup(tx *bbolt.Tx, addrs []oid.Address) (uint64, uint64, []RemovedObject, error) {
-	var rawDeleted uint64
+	var phyDeleted uint64
 	var availableDeleted uint64
 	var errorCount int
 	var firstErr error
@@ -90,7 +91,7 @@ func (db *DB) deleteGroup(tx *bbolt.Tx, addrs []oid.Address) (uint64, uint64, []
 	}
 
 	for i := range removedObjs {
-		removed, available, size, err := db.delete(tx, removedObjs[i].Address)
+		isPhy, available, size, err := db.delete(tx, removedObjs[i].Address)
 		if err != nil {
 			errorCount++
 			db.log.Warn("failed to delete object", zap.Stringer("addr", removedObjs[i].Address), zap.Error(err))
@@ -101,8 +102,8 @@ func (db *DB) deleteGroup(tx *bbolt.Tx, addrs []oid.Address) (uint64, uint64, []
 			continue
 		}
 
-		if removed {
-			rawDeleted++
+		if isPhy {
+			phyDeleted++
 			removedObjs[i].PayloadLen = size
 		}
 
@@ -117,8 +118,8 @@ func (db *DB) deleteGroup(tx *bbolt.Tx, addrs []oid.Address) (uint64, uint64, []
 		return 0, 0, nil, fmt.Errorf("deleted %d out of %d objects, first error: %w", success, all, firstErr)
 	}
 
-	if rawDeleted > 0 {
-		err := updateCounter(tx, phy, rawDeleted, false)
+	if phyDeleted > 0 {
+		err := updateCounter(tx, phy, phyDeleted, false)
 		if err != nil {
 			return 0, 0, nil, fmt.Errorf("could not decrease phy object counter: %w", err)
 		}
@@ -131,11 +132,11 @@ func (db *DB) deleteGroup(tx *bbolt.Tx, addrs []oid.Address) (uint64, uint64, []
 		}
 	}
 
-	return rawDeleted, availableDeleted, removedObjs, nil
+	return phyDeleted, availableDeleted, removedObjs, nil
 }
 
 // delete removes object indexes from the metabase.
-// The first return value indicates if an object has been removed. (removing a
+// The first return value indicates if an object is physical. (removing a
 // non-exist object is error-free). The second return value indicates if an
 // object was available before the removal (for calculating the logical object
 // counter). The third return value is removed object payload size.
@@ -149,9 +150,15 @@ func (db *DB) delete(tx *bbolt.Tx, addr oid.Address) (bool, bool, uint64, error)
 
 	removeAvailableObject := inGarbage(metaCursor, addr.Object()) == statusAvailable
 
+	isPhy := true
+
 	payloadSize, err := deleteMetadata(metaCursor, db.log, addr.Container(), addr.Object(), false)
 	if err != nil {
-		return false, false, 0, fmt.Errorf("can't remove metadata indexes: %w", err)
+		if !errors.Is(err, errNonPhy) {
+			return false, false, 0, fmt.Errorf("can't remove metadata indexes: %w", err)
+		}
+
+		isPhy = false
 	}
 
 	// if object is not available, counters have already been handled in
@@ -164,7 +171,7 @@ func (db *DB) delete(tx *bbolt.Tx, addr oid.Address) (bool, bool, uint64, error)
 		}
 	}
 
-	return true, removeAvailableObject, payloadSize, nil
+	return isPhy, removeAvailableObject, payloadSize, nil
 }
 
 // forms list of objects from addrs and their missing parts.

pkg/local_object_storage/metabase/delete_test.go

@@ -36,8 +36,9 @@ func TestDB_Delete(t *testing.T) {
 	require.NoError(t, err)
 
 	// try to remove parent, should be no-op, error-free
-	err = metaDelete(db, parent.Address())
+	res, err := db.Delete([]oid.Address{parent.Address()})
 	require.NoError(t, err)
+	require.Zero(t, res.PhyRemoved)
 
 	// inhume child so it will be on graveyard
 	ts := generateObjectWithCID(t, cnr)
@@ -98,7 +99,7 @@ func TestDB_Delete(t *testing.T) {
 
 		all := 2 + len(ecParts1) + len(ecParts2)
 		require.EqualValues(t, all, res.AvailableRemoved)
-		require.EqualValues(t, all, res.RawRemoved)
+		require.EqualValues(t, all-2, res.PhyRemoved)
 		require.Len(t, res.RemovedObjects, all)
 
 		require.ElementsMatch(t, res.RemovedObjects[:2], []meta.RemovedObject{
@@ -138,7 +139,7 @@ func TestContainerInfo(t *testing.T) {
 	require.NoError(t, err)
 
 	require.Equal(t, uint64(1), res.AvailableRemoved)
-	require.Equal(t, uint64(1), res.RawRemoved)
+	require.Equal(t, uint64(1), res.PhyRemoved)
 	require.Len(t, res.RemovedObjects, 1)
 	require.Equal(t, payloadSize, res.RemovedObjects[0].PayloadLen)
 	require.Equal(t, addr, res.RemovedObjects[0].Address)

pkg/local_object_storage/metabase/errors.go

@@ -12,6 +12,8 @@ import (
 // as removed and should not be returned from the Storage Engine.
 var ErrObjectIsExpired = logicerr.New("object is expired")
 
+var errNonPhy = errors.New("non-phy")
+
 // IsErrRemoved checks if error returned by Shard Exists/Get/Put method
 // corresponds to removed object.
 func IsErrRemoved(err error) bool {

pkg/local_object_storage/metabase/metadata.go

@@ -135,6 +135,7 @@ func PutMetadataForObject(tx *bbolt.Tx, hdr object.Object, phy bool) error {
 	return nil
 }
 
+// returns errNonPhy if isParent is unset and the object is not physical.
 func deleteMetadata(c *bbolt.Cursor, l *zap.Logger, cnr cid.ID, id oid.ID, isParent bool) (uint64, error) {
 	var metaBkt = c.Bucket()
 	var err error
@@ -143,7 +144,7 @@ func deleteMetadata(c *bbolt.Cursor, l *zap.Logger, cnr cid.ID, id oid.ID, isPar
 	if !isParent {
 		v := metaBkt.Get(slices.Concat([]byte{metaPrefixIDAttr}, id[:], []byte(object.FilterPhysical), objectcore.MetaAttributeDelimiter, []byte(binPropMarker)))
 		if v == nil {
-			return 0, nil
+			return 0, errNonPhy
 		}
 	}
 	pref := slices.Concat([]byte{metaPrefixID}, id[:])

pkg/local_object_storage/metabase/version.go

@@ -397,6 +397,12 @@ func migrateFrom8Version(db *DB) error {
 				return fmt.Errorf("deleting %v bucket: %w", name, err)
 			}
 		}
+
+		err = syncCounter(tx, true)
+		if err != nil {
+			return fmt.Errorf("resync object counters: %w", err)
+		}
+
 		return updateVersion(tx, 9)
 	})
 }

pkg/local_object_storage/metabase/version_test.go

@@ -4,19 +4,22 @@ import (
 	"encoding/binary"
 	"errors"
 	"fmt"
+	"math"
 	"os"
 	"path"
 	"path/filepath"
 	"slices"
 	"testing"
 
 	"github.com/nspcc-dev/bbolt"
+	checksumtest "github.com/nspcc-dev/neofs-sdk-go/checksum/test"
 	cid "github.com/nspcc-dev/neofs-sdk-go/container/id"
 	cidtest "github.com/nspcc-dev/neofs-sdk-go/container/id/test"
 	"github.com/nspcc-dev/neofs-sdk-go/object"
 	oid "github.com/nspcc-dev/neofs-sdk-go/object/id"
 	oidtest "github.com/nspcc-dev/neofs-sdk-go/object/id/test"
 	objecttest "github.com/nspcc-dev/neofs-sdk-go/object/test"
+	usertest "github.com/nspcc-dev/neofs-sdk-go/user/test"
 	"github.com/stretchr/testify/require"
 )
 
@@ -323,3 +326,80 @@ func TestMigrate7to8(t *testing.T) {
 	require.Len(t, gCnrs, 1)
 	require.Equal(t, inhumeCnr, gCnrs[0])
 }
+
+func TestMigrate8to9(t *testing.T) {
+	db := newDB(t)
+
+	// store several root phy objects
+	const phyContainerNum = 5
+	const phyObjectsPerContainer = 10
+
+	anyOwner := usertest.ID()
+	anyChecksum := checksumtest.Checksum()
+
+	for range phyContainerNum {
+		cnr := cidtest.ID()
+
+		for range phyObjectsPerContainer {
+			var obj object.Object
+			obj.SetContainerID(cnr)
+			obj.SetID(oidtest.ID())
+			obj.SetOwner(anyOwner)
+			obj.SetPayloadChecksum(anyChecksum)
+
+			require.NoError(t, db.Put(&obj))
+		}
+	}
+
+	// store several phy objects with parent
+	const parentObjects = 3
+	for range parentObjects {
+		var parent object.Object
+		parent.SetContainerID(cidtest.ID())
+		parent.SetID(oidtest.ID())
+		parent.SetOwner(anyOwner)
+		parent.SetPayloadChecksum(anyChecksum)
+
+		var child object.Object
+		child.SetContainerID(parent.GetContainerID())
+		child.SetID(oidtest.ID())
+		child.SetOwner(anyOwner)
+		child.SetPayloadChecksum(anyChecksum)
+		child.SetParent(&parent)
+
+		require.NoError(t, db.Put(&child))
+	}
+
+	// force incorrect counters and previous DB version
+	const garbageCount = 1234567890
+
+	err := db.boltDB.Update(func(tx *bbolt.Tx) error {
+		// reset to zero
+		if err := updateCounter(tx, phy, math.MaxUint64, false); err != nil {
+			return err
+		}
+		if err := updateCounter(tx, phy, garbageCount, true); err != nil {
+			return err
+		}
+
+		bkt, err := tx.CreateBucketIfNotExists([]byte{0x05})
+		if err != nil {
+			return err
+		}
+
+		return bkt.Put([]byte("version"), []byte{0x08, 0, 0, 0, 0, 0, 0, 0})
+	})
+	require.NoError(t, err)
+
+	c, err := db.ObjectCounters()
+	require.NoError(t, err)
+	require.EqualValues(t, garbageCount, c.Phy())
+
+	// migrate
+	require.NoError(t, db.Init())
+
+	// check counters have been corrected
+	c, err = db.ObjectCounters()
+	require.NoError(t, err)
+	require.EqualValues(t, phyContainerNum*phyObjectsPerContainer+parentObjects, c.Phy())
+}

pkg/local_object_storage/shard/delete.go

@@ -52,7 +52,7 @@ func (s *Shard) deleteObjs(addrs []oid.Address) error {
 		}
 	}
 
-	s.decObjectCounterBy(physical, res.RawRemoved)
+	s.decObjectCounterBy(physical, res.PhyRemoved)
 
 	var totalRemovedPayload uint64
 

pkg/local_object_storage/shard/metrics_test.go

@@ -177,6 +177,40 @@ func TestCounters(t *testing.T) {
 		}
 		require.Equal(t, expectedSizes, mm.containerSize)
 		require.Equal(t, totalPayload-int64(totalRemovedpayload), mm.payloadSize)
+
+		t.Run("parent", func(t *testing.T) {
+			cnr := cidtest.ID()
+
+			parent := generateObjectWithCID(cnr)
+
+			child := generateObjectWithCID(cnr)
+			child.SetParent(parent)
+
+			require.NoError(t, sh.Put(child, nil))
+
+			require.EqualValues(t, child.PayloadSize(), mm.containerSize[cnr])
+
+			cnrInfo, err := sh.ContainerInfo(cnr)
+			require.NoError(t, err)
+			require.EqualValues(t, child.PayloadSize(), cnrInfo.StorageSize)
+			require.EqualValues(t, 1, cnrInfo.ObjectsNumber)
+
+			phyBefore := mm.objectCounters[physical]
+			logicNumBefore := mm.objectCounters[logical]
+			sumPldSizeBefore := mm.payloadSize
+
+			require.NoError(t, sh.Delete([]oid.Address{parent.Address()}))
+
+			require.EqualValues(t, child.PayloadSize(), mm.containerSize[cnr])
+			require.Equal(t, phyBefore, mm.objectCounters[physical])
+			require.Equal(t, logicNumBefore, mm.objectCounters[logical])
+			require.Equal(t, sumPldSizeBefore, mm.payloadSize)
+
+			cnrInfo, err = sh.ContainerInfo(cnr)
+			require.NoError(t, err)
+			require.EqualValues(t, child.PayloadSize(), cnrInfo.StorageSize)
+			require.Zero(t, cnrInfo.ObjectsNumber)
+		})
 	})
 }