[#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

@@ -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() {
+		case nodeIsUnderMaintenance:
-			shortage, uncheckedCopies = p.handleMaintenance(ctx, nodes[i], checkedNodes, shortage, uncheckedCopies)
+			shortage--
-		} else {
+			uncheckedCopies++
-			if status := checkedNodes.processStatus(nodes[i]); status.Processed() {
-				if status == nodeHoldsObject {
-					shortage--
-				}
-				if status == nodeIsUnderMaintenance {
-					shortage--
-					uncheckedCopies++
-				}
 
-				nodes = append(nodes[:i], nodes[i+1:]...)
+			p.log.Debug(ctx, logs.PolicerConsiderNodeUnderMaintenanceAsOK,
-				i--
+				zap.String("node", netmap.StringifyPublicKey(nodes[i])))
-				continue
+		case nodeHoldsObject:
-			}
+			shortage--
-
+		case nodeDoesNotHoldObject:
-			callCtx, cancel := context.WithTimeout(ctx, p.headTimeout)
+		case nodeStatusUnknown:
-
+			p.log.Error(ctx, logs.PolicerReceiveObjectHeaderToCheckPolicyCompliance,
-			_, err := p.remoteHeader(callCtx, nodes[i], addr, false)
+				zap.Stringer("object", addr),
-
+				zap.Error(err))
-			cancel()
+		default:
-
+			panic("unreachable")
-			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)
-				}
-			}
 		}
 
 		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
+func (p *Policer) checkStatus(ctx context.Context, addr oid.Address, node netmap.NodeInfo) (nodeProcessStatus, error) {
-//
+	if p.netmapKeys.IsLocalKey(node.PublicKey()) {
-// consider remote nodes under maintenance as problem OK. Such
+		return nodeIsLocal, nil
-// nodes MAY not respond with object, however, this is how we
+	}
-// prevent spam with new replicas.
+	if node.Status().IsMaintenance() {
-// However, additional copies should not be removed in this case,
+		return nodeIsUnderMaintenance, nil
-// 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++
 
-	p.log.Debug(ctx, logs.PolicerConsiderNodeUnderMaintenanceAsOK,
+	callCtx, cancel := context.WithTimeout(ctx, p.headTimeout)
-		zap.String("node", netmap.StringifyPublicKey(node)),
+	_, err := p.remoteHeader(callCtx, node, addr, false)
-	)
+	cancel()
-	return shortage, uncheckedCopies
+
+	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,

pkg/services/policer/nodecache.go

@@ -10,6 +10,7 @@ const (
 	nodeHoldsObject
 	nodeStatusUnknown
 	nodeIsUnderMaintenance
+	nodeIsLocal
 )
 
 func (st nodeProcessStatus) Processed() bool {