From 0bcbeb26b2b79d210d4b99360e1ad58ae809faab Mon Sep 17 00:00:00 2001
From: Evgenii Stratonikov <e.stratonikov@yadro.com>
Date: Fri, 17 Jan 2025 12:38:44 +0300
Subject: [PATCH] [#1605] policer: Simplify processRepNodes() checks

Current flow is hard to reason about, #1601 is a notorious example of
accidental complexity.
1. Remove multiple nested ifs, use depth=1.
2. Process each status exactly once, hopefully preventing bugs like
   #1601.

Signed-off-by: Evgenii Stratonikov <e.stratonikov@yadro.com>
---
 pkg/services/policer/check.go     | 106 ++++++++++++++----------------
 pkg/services/policer/nodecache.go |   1 +
 2 files changed, 52 insertions(+), 55 deletions(-)

diff --git a/pkg/services/policer/check.go b/pkg/services/policer/check.go
index 7ac5fc9e0..3e536f105 100644
--- a/pkg/services/policer/check.go
+++ b/pkg/services/policer/check.go
@@ -117,50 +117,40 @@ func (p *Policer) processRepNodes(ctx context.Context, requirements *placementRe
 		default:
 		}
 
-		if p.netmapKeys.IsLocalKey(nodes[i].PublicKey()) {
+		var err error
+		st := checkedNodes.processStatus(nodes[i])
+		if !st.Processed() {
+			st, err = p.checkStatus(ctx, addr, nodes[i])
+			checkedNodes.set(nodes[i], st)
+			if st == nodeDoesNotHoldObject {
+				// 1. This is the first time the node is encountered (`!st.Processed()`).
+				// 2. The node does not hold object (`st == nodeDoesNotHoldObject`).
+				// So we leave the node in the list and skip its removal
+				// at the end of the loop body.
+				continue
+			}
+		}
+
+		switch st {
+		case nodeIsLocal:
 			requirements.needLocalCopy = true
 
 			shortage--
-		} else if nodes[i].Status().IsMaintenance() {
-			shortage, uncheckedCopies = p.handleMaintenance(ctx, nodes[i], checkedNodes, shortage, uncheckedCopies)
-		} else {
-			if status := checkedNodes.processStatus(nodes[i]); status.Processed() {
-				if status == nodeHoldsObject {
-					shortage--
-				}
-				if status == nodeIsUnderMaintenance {
-					shortage--
-					uncheckedCopies++
-				}
+		case nodeIsUnderMaintenance:
+			shortage--
+			uncheckedCopies++
 
-				nodes = append(nodes[:i], nodes[i+1:]...)
-				i--
-				continue
-			}
-
-			callCtx, cancel := context.WithTimeout(ctx, p.headTimeout)
-
-			_, err := p.remoteHeader(callCtx, nodes[i], addr, false)
-
-			cancel()
-
-			if err == nil {
-				shortage--
-				checkedNodes.set(nodes[i], nodeHoldsObject)
-			} else {
-				if client.IsErrObjectNotFound(err) {
-					checkedNodes.set(nodes[i], nodeDoesNotHoldObject)
-					continue
-				} else if client.IsErrNodeUnderMaintenance(err) {
-					shortage, uncheckedCopies = p.handleMaintenance(ctx, nodes[i], checkedNodes, shortage, uncheckedCopies)
-				} else {
-					p.log.Error(ctx, logs.PolicerReceiveObjectHeaderToCheckPolicyCompliance,
-						zap.Stringer("object", addr),
-						zap.Error(err),
-					)
-					checkedNodes.set(nodes[i], nodeStatusUnknown)
-				}
-			}
+			p.log.Debug(ctx, logs.PolicerConsiderNodeUnderMaintenanceAsOK,
+				zap.String("node", netmap.StringifyPublicKey(nodes[i])))
+		case nodeHoldsObject:
+			shortage--
+		case nodeDoesNotHoldObject:
+		case nodeStatusUnknown:
+			p.log.Error(ctx, logs.PolicerReceiveObjectHeaderToCheckPolicyCompliance,
+				zap.Stringer("object", addr),
+				zap.Error(err))
+		default:
+			panic("unreachable")
 		}
 
 		nodes = append(nodes[:i], nodes[i+1:]...)
@@ -170,22 +160,28 @@ func (p *Policer) processRepNodes(ctx context.Context, requirements *placementRe
 	p.handleProcessNodesResult(ctx, addr, requirements, nodes, checkedNodes, shortage, uncheckedCopies)
 }
 
-// handleMaintenance handles node in maintenance mode and returns new shortage and uncheckedCopies values
-//
-// consider remote nodes under maintenance as problem OK. Such
-// nodes MAY not respond with object, however, this is how we
-// prevent spam with new replicas.
-// However, additional copies should not be removed in this case,
-// because we can remove the only copy this way.
-func (p *Policer) handleMaintenance(ctx context.Context, node netmap.NodeInfo, checkedNodes nodeCache, shortage uint32, uncheckedCopies int) (uint32, int) {
-	checkedNodes.set(node, nodeIsUnderMaintenance)
-	shortage--
-	uncheckedCopies++
+func (p *Policer) checkStatus(ctx context.Context, addr oid.Address, node netmap.NodeInfo) (nodeProcessStatus, error) {
+	if p.netmapKeys.IsLocalKey(node.PublicKey()) {
+		return nodeIsLocal, nil
+	}
+	if node.Status().IsMaintenance() {
+		return nodeIsUnderMaintenance, nil
+	}
 
-	p.log.Debug(ctx, logs.PolicerConsiderNodeUnderMaintenanceAsOK,
-		zap.String("node", netmap.StringifyPublicKey(node)),
-	)
-	return shortage, uncheckedCopies
+	callCtx, cancel := context.WithTimeout(ctx, p.headTimeout)
+	_, err := p.remoteHeader(callCtx, node, addr, false)
+	cancel()
+
+	if err == nil {
+		return nodeHoldsObject, nil
+	}
+	if client.IsErrObjectNotFound(err) {
+		return nodeDoesNotHoldObject, nil
+	}
+	if client.IsErrNodeUnderMaintenance(err) {
+		return nodeIsUnderMaintenance, nil
+	}
+	return nodeStatusUnknown, err
 }
 
 func (p *Policer) handleProcessNodesResult(ctx context.Context, addr oid.Address, requirements *placementRequirements,
diff --git a/pkg/services/policer/nodecache.go b/pkg/services/policer/nodecache.go
index 53b64d3fa..c2157de5d 100644
--- a/pkg/services/policer/nodecache.go
+++ b/pkg/services/policer/nodecache.go
@@ -10,6 +10,7 @@ const (
 	nodeHoldsObject
 	nodeStatusUnknown
 	nodeIsUnderMaintenance
+	nodeIsLocal
 )
 
 func (st nodeProcessStatus) Processed() bool {