Skip to content

Commit ad7b881

Browse files
committed
feat: add max remediation attempts per equivalence group
This addresses issue #1543 by implementing a configurable limit on the number of remediation attempts per equivalence group. Changes: - Add MaxRetryAttempts config field to TomlConfig - Add RetryCount field to EquivalenceGroupState (persists in node annotation) - Increment retry count on each remediation attempt - Skip remediation when retry limit is exceeded - Add tests for retry count tracking and persistence The retry counter survives pod restarts because it's stored in the node's annotation rather than in-memory state. Fixes #1543 Signed-off-by: Billard <82095453+iacker@users.noreply.github.com>
1 parent 42672ae commit ad7b881

5 files changed

Lines changed: 209 additions & 1 deletion

File tree

fault-remediation/pkg/annotation/annotation.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,11 +104,19 @@ func (m *NodeAnnotationManager) UpdateRemediationState(ctx context.Context, node
104104
return err
105105
}
106106

107+
// Increment retry count if this group already exists
108+
existingGroup, exists := state.EquivalenceGroups[group]
109+
retryCount := 0
110+
if exists {
111+
retryCount = existingGroup.RetryCount + 1
112+
}
113+
107114
// Update state for the group
108115
state.EquivalenceGroups[group] = EquivalenceGroupState{
109116
MaintenanceCR: crName,
110117
CreatedAt: time.Now().UTC(),
111118
ActionName: actionName,
119+
RetryCount: retryCount,
112120
}
113121

114122
// Marshal to JSON
@@ -131,7 +139,8 @@ func (m *NodeAnnotationManager) UpdateRemediationState(ctx context.Context, node
131139
slog.InfoContext(ctx, "Updated remediation state annotation for node",
132140
"node", nodeName,
133141
"group", group,
134-
"crName", crName)
142+
"crName", crName,
143+
"retryCount", retryCount)
135144

136145
return nil
137146
})

fault-remediation/pkg/annotation/annotation_interface.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,4 +47,8 @@ type EquivalenceGroupState struct {
4747
// Action that created the CR (e.g., "RESTART_BM")
4848
// Required to look up the corresponding MaintenanceResource from the TomlConfig
4949
ActionName string `json:"actionName"`
50+
51+
// RetryCount tracks how many remediation attempts have been made for this equivalence group.
52+
// Survives pod restarts as it's persisted in the node annotation.
53+
RetryCount int `json:"retryCount,omitempty"`
5054
}
Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
// Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package annotation
16+
17+
import (
18+
"context"
19+
"testing"
20+
21+
"github.com/stretchr/testify/assert"
22+
"github.com/stretchr/testify/require"
23+
corev1 "k8s.io/api/core/v1"
24+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
25+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
26+
)
27+
28+
func TestRetryCountIncrementsOnUpdate(t *testing.T) {
29+
ctx := context.Background()
30+
nodeName := "test-node"
31+
groupName := "test-group"
32+
33+
node := &corev1.Node{
34+
ObjectMeta: metav1.ObjectMeta{
35+
Name: nodeName,
36+
Annotations: map[string]string{},
37+
},
38+
}
39+
40+
client := fake.NewClientBuilder().WithObjects(node).Build()
41+
annotationManager := NodeAnnotationManager{client: client}
42+
43+
// First update - retry count should be 0
44+
err := annotationManager.UpdateRemediationState(ctx, nodeName, groupName, "cr-1", "RESTART_BM")
45+
require.NoError(t, err)
46+
47+
state, _, err := annotationManager.GetRemediationState(ctx, nodeName)
48+
require.NoError(t, err)
49+
assert.Equal(t, 0, state.EquivalenceGroups[groupName].RetryCount, "First attempt should have retry count 0")
50+
51+
// Second update - retry count should be 1
52+
err = annotationManager.UpdateRemediationState(ctx, nodeName, groupName, "cr-2", "RESTART_BM")
53+
require.NoError(t, err)
54+
55+
state, _, err = annotationManager.GetRemediationState(ctx, nodeName)
56+
require.NoError(t, err)
57+
assert.Equal(t, 1, state.EquivalenceGroups[groupName].RetryCount, "Second attempt should have retry count 1")
58+
59+
// Third update - retry count should be 2
60+
err = annotationManager.UpdateRemediationState(ctx, nodeName, groupName, "cr-3", "RESTART_BM")
61+
require.NoError(t, err)
62+
63+
state, _, err = annotationManager.GetRemediationState(ctx, nodeName)
64+
require.NoError(t, err)
65+
assert.Equal(t, 2, state.EquivalenceGroups[groupName].RetryCount, "Third attempt should have retry count 2")
66+
}
67+
68+
func TestRetryCountIndependentPerGroup(t *testing.T) {
69+
ctx := context.Background()
70+
nodeName := "test-node"
71+
72+
node := &corev1.Node{
73+
ObjectMeta: metav1.ObjectMeta{
74+
Name: nodeName,
75+
Annotations: map[string]string{},
76+
},
77+
}
78+
79+
client := fake.NewClientBuilder().WithObjects(node).Build()
80+
annotationManager := NodeAnnotationManager{client: client}
81+
82+
// Create group-1 twice
83+
err := annotationManager.UpdateRemediationState(ctx, nodeName, "group-1", "cr-1", "RESTART_BM")
84+
require.NoError(t, err)
85+
err = annotationManager.UpdateRemediationState(ctx, nodeName, "group-1", "cr-2", "RESTART_BM")
86+
require.NoError(t, err)
87+
88+
// Create group-2 once
89+
err = annotationManager.UpdateRemediationState(ctx, nodeName, "group-2", "cr-3", "COMPONENT_RESET")
90+
require.NoError(t, err)
91+
92+
state, _, err := annotationManager.GetRemediationState(ctx, nodeName)
93+
require.NoError(t, err)
94+
95+
assert.Equal(t, 1, state.EquivalenceGroups["group-1"].RetryCount, "group-1 should have retry count 1")
96+
assert.Equal(t, 0, state.EquivalenceGroups["group-2"].RetryCount, "group-2 should have retry count 0")
97+
}
98+
99+
func TestRetryCountPersistsAcrossPodRestarts(t *testing.T) {
100+
ctx := context.Background()
101+
nodeName := "test-node"
102+
groupName := "test-group"
103+
104+
node := &corev1.Node{
105+
ObjectMeta: metav1.ObjectMeta{
106+
Name: nodeName,
107+
Annotations: map[string]string{},
108+
},
109+
}
110+
111+
client := fake.NewClientBuilder().WithObjects(node).Build()
112+
113+
// Simulate first pod session
114+
manager1 := NodeAnnotationManager{client: client}
115+
err := manager1.UpdateRemediationState(ctx, nodeName, groupName, "cr-1", "RESTART_BM")
116+
require.NoError(t, err)
117+
118+
// Simulate pod restart - create new annotation manager instance
119+
manager2 := NodeAnnotationManager{client: client}
120+
err = manager2.UpdateRemediationState(ctx, nodeName, groupName, "cr-2", "RESTART_BM")
121+
require.NoError(t, err)
122+
123+
// Verify retry count persisted
124+
state, _, err := manager2.GetRemediationState(ctx, nodeName)
125+
require.NoError(t, err)
126+
assert.Equal(t, 1, state.EquivalenceGroups[groupName].RetryCount,
127+
"Retry count should persist across pod restarts")
128+
}

fault-remediation/pkg/config/config.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,10 @@ type TomlConfig struct {
8585

8686
// Common configuration
8787
UpdateRetry UpdateRetry `toml:"updateRetry"`
88+
89+
// MaxRetryAttempts is the maximum number of remediation attempts per equivalence group
90+
// before marking the remediation as failed. Zero means unlimited retries (legacy behavior).
91+
MaxRetryAttempts int `toml:"maxRetryAttempts"`
8892
}
8993

9094
// Validate checks the configuration for consistency and completeness.

fault-remediation/pkg/reconciler/reconciler.go

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1065,6 +1065,19 @@ func (r *FaultRemediationReconciler) handleRemediationEvent(
10651065
nodeName)
10661066
}
10671067

1068+
// Check if we've exceeded the maximum retry attempts
1069+
maxRetries := r.Config.RemediationClient.GetConfig().MaxRetryAttempts
1070+
if maxRetries > 0 {
1071+
res, err, done := r.trySkipMaxRetriesExceeded(ctx, nodeName, groupConfig.EffectiveEquivalenceGroup,
1072+
maxRetries, eventWithToken, watcherInstance, healthEventStore)
1073+
if done {
1074+
span.SetAttributes(
1075+
attribute.String("fault_remediation.status", "max_retries_exceeded"),
1076+
)
1077+
return res, err
1078+
}
1079+
}
1080+
10681081
result, err := r.runLogCollectorAndRemediate(ctx, healthEvent, healthEventWithStatus, eventWithToken,
10691082
watcherInstance, healthEventStore, groupConfig, nodeName)
10701083
if err != nil {
@@ -1161,6 +1174,56 @@ func (r *FaultRemediationReconciler) trySkipResolvedEvent(
11611174
return result, err, true
11621175
}
11631176

1177+
// trySkipMaxRetriesExceeded returns (result, err, true) when the retry count for this
1178+
// equivalence group has reached or exceeded the configured maximum; otherwise (zero, nil, false).
1179+
func (r *FaultRemediationReconciler) trySkipMaxRetriesExceeded(
1180+
ctx context.Context,
1181+
nodeName string,
1182+
effectiveEquivalenceGroup string,
1183+
maxRetries int,
1184+
eventWithToken datastore.EventWithToken,
1185+
watcherInstance datastore.ChangeStreamWatcher,
1186+
healthEventStore datastore.HealthEventStore,
1187+
) (ctrl.Result, error, bool) {
1188+
remediationState, _, err := r.annotationManager.GetRemediationState(ctx, nodeName)
1189+
if err != nil {
1190+
if apierrors.IsNotFound(err) {
1191+
// Node does not exist, not a retry limit issue
1192+
return ctrl.Result{}, nil, false
1193+
}
1194+
slog.ErrorContext(ctx, "Failed to get remediation state for retry check",
1195+
"node", nodeName,
1196+
"error", err)
1197+
return ctrl.Result{}, fmt.Errorf("failed to get remediation state for retry check: %w", err), true
1198+
}
1199+
1200+
groupState, exists := remediationState.EquivalenceGroups[effectiveEquivalenceGroup]
1201+
if !exists {
1202+
// First attempt for this group
1203+
return ctrl.Result{}, nil, false
1204+
}
1205+
1206+
if groupState.RetryCount >= maxRetries {
1207+
slog.WarnContext(ctx, "Maximum retry attempts exceeded for equivalence group",
1208+
"node", nodeName,
1209+
"group", effectiveEquivalenceGroup,
1210+
"retryCount", groupState.RetryCount,
1211+
"maxRetries", maxRetries)
1212+
1213+
metrics.EventsProcessed.WithLabelValues(metrics.CRStatusSkipped, nodeName).Inc()
1214+
1215+
// Mark as failed since we cannot remediate
1216+
if err := r.updateNodeRemediatedStatus(ctx, healthEventStore, eventWithToken, false); err != nil {
1217+
return ctrl.Result{}, err, true
1218+
}
1219+
1220+
result, err := r.markProcessedOrError(ctx, watcherInstance, eventWithToken, nodeName)
1221+
return result, err, true
1222+
}
1223+
1224+
return ctrl.Result{}, nil, false
1225+
}
1226+
11641227
// handleEventCoveredByExistingCR routes a shouldCreate=false decision: an event behind a
11651228
// still-in-progress CR is requeued, an event covered by a terminal CR is finalized.
11661229
func (r *FaultRemediationReconciler) handleEventCoveredByExistingCR(

0 commit comments

Comments
 (0)