Skip to content

Commit b818d92

Browse files
alexscjundevalexscjundev
committed
fix: check existing CRs from stored references
Signed-off-by: Alex Jun <aljun@nvidia.com>
1 parent cfa0c3c commit b818d92

5 files changed

Lines changed: 232 additions & 24 deletions

File tree

‎fault-remediation/pkg/crstatus/checker.go‎

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"k8s.io/apimachinery/pkg/runtime/schema"
2323
"sigs.k8s.io/controller-runtime/pkg/client"
2424

25+
"github.com/nvidia/nvsentinel/fault-remediation/pkg/annotation"
2526
"github.com/nvidia/nvsentinel/fault-remediation/pkg/config"
2627
)
2728

@@ -65,31 +66,55 @@ func (c *CRStatusChecker) GetCRState(ctx context.Context, actionName string, crN
6566
return CRStateNotFound
6667
}
6768

69+
resourceRef := annotation.MaintenanceResourceReference{
70+
Namespace: resource.Namespace,
71+
Version: resource.Version,
72+
ApiGroup: resource.ApiGroup,
73+
Kind: resource.Kind,
74+
}
75+
76+
return c.GetCRStateForReference(ctx, crName, resourceRef, resource.CompleteConditionType)
77+
}
78+
79+
func (c *CRStatusChecker) GetCRStateForReference(
80+
ctx context.Context,
81+
crName string,
82+
resourceRef annotation.MaintenanceResourceReference,
83+
completeConditionType string,
84+
) CRState {
6885
if c.dryRun {
69-
slog.InfoContext(ctx, "DRY-RUN: CR doesn't exist (dry-run mode)", "crName", crName, "action", actionName)
86+
slog.InfoContext(ctx, "DRY-RUN: CR doesn't exist (dry-run mode)", "crName", crName)
7087
return CRStateNotFound
7188
}
7289

7390
gvk := schema.GroupVersionKind{
74-
Group: resource.ApiGroup,
75-
Version: resource.Version,
76-
Kind: resource.Kind,
91+
Group: resourceRef.ApiGroup,
92+
Version: resourceRef.Version,
93+
Kind: resourceRef.Kind,
94+
}
95+
if gvk.Group == "" || gvk.Version == "" || gvk.Kind == "" {
96+
slog.WarnContext(ctx, "Stored CR reference is missing GVK, allowing create", "crName", crName)
97+
return CRStateNotFound
7798
}
7899

79100
obj := &unstructured.Unstructured{}
80101
obj.SetGroupVersionKind(gvk)
81102

82-
key := client.ObjectKey{Name: crName, Namespace: resource.Namespace}
103+
key := client.ObjectKey{Name: crName, Namespace: resourceRef.Namespace}
83104

84105
if err := c.client.Get(ctx, key, obj); err != nil {
85106
slog.WarnContext(ctx, "Failed to get CR, allowing create", "crName", crName, "gvk", gvk.String(), "error", err)
86107
return CRStateNotFound
87108
}
88109

89-
return c.checkCondition(obj, resource)
110+
return c.checkConditionType(obj, completeConditionType)
90111
}
91112

92113
func (c *CRStatusChecker) checkCondition(obj *unstructured.Unstructured, resource config.MaintenanceResource) CRState {
114+
return c.checkConditionType(obj, resource.CompleteConditionType)
115+
}
116+
117+
func (c *CRStatusChecker) checkConditionType(obj *unstructured.Unstructured, completeConditionType string) CRState {
93118
status, found, err := unstructured.NestedMap(obj.Object, "status")
94119
if err != nil || !found {
95120
return CRStateInProgress
@@ -100,7 +125,7 @@ func (c *CRStatusChecker) checkCondition(obj *unstructured.Unstructured, resourc
100125
return CRStateInProgress
101126
}
102127

103-
conditionStatus := c.findConditionStatus(conditions, resource.CompleteConditionType)
128+
conditionStatus := c.findConditionStatus(conditions, completeConditionType)
104129

105130
switch conditionStatus {
106131
case "True":

‎fault-remediation/pkg/crstatus/crstatus_interface.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,12 @@ package crstatus
1818

1919
import (
2020
"context"
21+
22+
"github.com/nvidia/nvsentinel/fault-remediation/pkg/annotation"
2123
)
2224

2325
type CRStatusCheckerInterface interface {
2426
ShouldSkipCRCreation(context.Context, string, string) bool
2527
GetCRState(context.Context, string, string) CRState
28+
GetCRStateForReference(context.Context, string, annotation.MaintenanceResourceReference, string) CRState
2629
}

‎fault-remediation/pkg/crstatus/crstatus_test.go‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,14 @@
1515
package crstatus
1616

1717
import (
18+
"context"
1819
"testing"
1920

2021
"github.com/stretchr/testify/assert"
2122
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
23+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
2224

25+
"github.com/nvidia/nvsentinel/fault-remediation/pkg/annotation"
2326
"github.com/nvidia/nvsentinel/fault-remediation/pkg/config"
2427
)
2528

@@ -120,3 +123,44 @@ func TestCheckCondition(t *testing.T) {
120123
})
121124
}
122125
}
126+
127+
func TestGetCRStateForReferenceUsesStoredReference(t *testing.T) {
128+
storedCR := &unstructured.Unstructured{
129+
Object: map[string]any{
130+
"apiVersion": "stored.example.com/v9",
131+
"kind": "StoredMaintenance",
132+
"metadata": map[string]any{
133+
"name": "stored-cr",
134+
"namespace": "stored-namespace",
135+
},
136+
"status": map[string]any{
137+
"conditions": []any{
138+
map[string]any{
139+
"type": "NodeReady",
140+
"status": "True",
141+
},
142+
},
143+
},
144+
},
145+
}
146+
147+
fakeClient := fake.NewClientBuilder().WithObjects(storedCR).Build()
148+
checker := NewCRStatusChecker(fakeClient, map[string]config.MaintenanceResource{
149+
"RESTART_BM": {
150+
ApiGroup: "config.example.com",
151+
Version: "v1",
152+
Kind: "ConfigMaintenance",
153+
Namespace: "config-namespace",
154+
CompleteConditionType: "NodeReady",
155+
},
156+
}, false)
157+
158+
state := checker.GetCRStateForReference(context.Background(), "stored-cr", annotation.MaintenanceResourceReference{
159+
ApiGroup: "stored.example.com",
160+
Version: "v9",
161+
Kind: "StoredMaintenance",
162+
Namespace: "stored-namespace",
163+
}, "NodeReady")
164+
165+
assert.Equal(t, CRStateSucceeded, state)
166+
}

‎fault-remediation/pkg/reconciler/reconciler.go‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1001,7 +1001,24 @@ func (r *FaultRemediationReconciler) evaluateExistingCR(
10011001
nodeName string,
10021002
) existingCRDecision {
10031003
crName := groupState.state.MaintenanceCR
1004-
crState := statusChecker.GetCRState(ctx, groupState.state.ActionName, crName)
1004+
1005+
resourceRef, ok := resourceReferenceFromState(groupState.state)
1006+
if !ok {
1007+
slog.WarnContext(ctx, "Stored remediation state is missing CR GVK, allowing retry",
1008+
"node", nodeName, "crName", crName, "group", groupState.name)
1009+
1010+
return existingCRDecision{shouldCreate: true, removeGroup: true}
1011+
}
1012+
1013+
completeConditionType, ok := r.completeConditionTypeForStoredState(groupState.state)
1014+
if !ok {
1015+
slog.WarnContext(ctx, "Stored remediation state has no completion condition config, allowing retry",
1016+
"node", nodeName, "crName", crName, "group", groupState.name, "action", groupState.state.ActionName)
1017+
1018+
return existingCRDecision{shouldCreate: true, removeGroup: true}
1019+
}
1020+
1021+
crState := statusChecker.GetCRStateForReference(ctx, crName, resourceRef, completeConditionType)
10051022

10061023
switch crState {
10071024
case crstatus.CRStateInProgress:
@@ -1021,6 +1038,42 @@ func (r *FaultRemediationReconciler) evaluateExistingCR(
10211038
}
10221039
}
10231040

1041+
func resourceReferenceFromState(
1042+
groupState annotation.EquivalenceGroupState,
1043+
) (annotation.MaintenanceResourceReference, bool) {
1044+
resourceRef := annotation.MaintenanceResourceReference{
1045+
Namespace: groupState.Namespace,
1046+
Version: groupState.Version,
1047+
ApiGroup: groupState.ApiGroup,
1048+
Kind: groupState.Kind,
1049+
}
1050+
if resourceRef.ApiGroup == "" || resourceRef.Version == "" || resourceRef.Kind == "" {
1051+
return annotation.MaintenanceResourceReference{}, false
1052+
}
1053+
1054+
return resourceRef, true
1055+
}
1056+
1057+
func (r *FaultRemediationReconciler) completeConditionTypeForStoredState(
1058+
groupState annotation.EquivalenceGroupState,
1059+
) (string, bool) {
1060+
if r.Config.RemediationClient == nil {
1061+
return "", false
1062+
}
1063+
1064+
remediationConfig := r.Config.RemediationClient.GetConfig()
1065+
if remediationConfig == nil {
1066+
return "", false
1067+
}
1068+
1069+
resource, exists := remediationConfig.RemediationActions[groupState.ActionName]
1070+
if !exists {
1071+
return "", false
1072+
}
1073+
1074+
return resource.CompleteConditionType, true
1075+
}
1076+
10241077
func (r *FaultRemediationReconciler) evaluateSucceededCR(
10251078
ctx context.Context,
10261079
groupState namedEquivalenceGroupState,

‎fault-remediation/pkg/reconciler/reconciler_test.go‎

Lines changed: 99 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ type MockK8sClient struct {
4545
runLogCollectorJobFn func(ctx context.Context, nodeName string) (ctrl.Result, error)
4646
annotationManagerOverride annotation.NodeAnnotationManagerInterface
4747
mockStatusChecker *mockStatusChecker
48+
configOverride *config.TomlConfig
4849
}
4950

5051
func (m *MockK8sClient) CreateMaintenanceResource(ctx context.Context, healthEventData *events.HealthEventData, groupConfig *common.EquivalenceGroupConfig) (string, error) {
@@ -64,11 +65,12 @@ func (m *MockK8sClient) GetStatusChecker() crstatus.CRStatusCheckerInterface {
6465
}
6566

6667
type mockStatusChecker struct {
67-
getCRStateFn func(context.Context, string, string) crstatus.CRState
68-
shouldSkip []bool
69-
states []crstatus.CRState
70-
stateByCR map[string]crstatus.CRState
71-
callCount int
68+
getCRStateFn func(context.Context, string, string) crstatus.CRState
69+
getCRStateForReferenceFn func(context.Context, string, annotation.MaintenanceResourceReference, string) crstatus.CRState
70+
shouldSkip []bool
71+
states []crstatus.CRState
72+
stateByCR map[string]crstatus.CRState
73+
callCount int
7274
}
7375

7476
func (statusChecker *mockStatusChecker) ShouldSkipCRCreation(context.Context, string, string) bool {
@@ -80,6 +82,23 @@ func (statusChecker *mockStatusChecker) GetCRState(ctx context.Context, actionNa
8082
return statusChecker.getCRStateFn(ctx, actionName, crName)
8183
}
8284

85+
return statusChecker.nextState(crName)
86+
}
87+
88+
func (statusChecker *mockStatusChecker) GetCRStateForReference(
89+
ctx context.Context,
90+
crName string,
91+
resourceRef annotation.MaintenanceResourceReference,
92+
completeConditionType string,
93+
) crstatus.CRState {
94+
if statusChecker.getCRStateForReferenceFn != nil {
95+
return statusChecker.getCRStateForReferenceFn(ctx, crName, resourceRef, completeConditionType)
96+
}
97+
98+
return statusChecker.nextState(crName)
99+
}
100+
101+
func (statusChecker *mockStatusChecker) nextState(crName string) crstatus.CRState {
83102
if statusChecker.stateByCR != nil {
84103
return statusChecker.stateByCR[crName]
85104
}
@@ -105,19 +124,25 @@ func (statusChecker *mockStatusChecker) GetCRState(ctx context.Context, actionNa
105124
}
106125

107126
func (m *MockK8sClient) GetConfig() *config.TomlConfig {
127+
if m.configOverride != nil {
128+
return m.configOverride
129+
}
130+
108131
return &config.TomlConfig{
109132
RemediationActions: map[string]config.MaintenanceResource{
110133
protos.RecommendedAction_RESTART_BM.String(): {
111-
EquivalenceGroup: "restart",
112-
ApiGroup: "janitor.dgxc.nvidia.com",
113-
Version: "v1alpha1",
114-
Kind: "RebootNode",
134+
EquivalenceGroup: "restart",
135+
ApiGroup: "janitor.dgxc.nvidia.com",
136+
Version: "v1alpha1",
137+
Kind: "RebootNode",
138+
CompleteConditionType: "NodeReady",
115139
},
116140
protos.RecommendedAction_COMPONENT_RESET.String(): {
117-
EquivalenceGroup: "restart",
118-
ApiGroup: "janitor.dgxc.nvidia.com",
119-
Version: "v1alpha1",
120-
Kind: "RebootNode",
141+
EquivalenceGroup: "restart",
142+
ApiGroup: "janitor.dgxc.nvidia.com",
143+
Version: "v1alpha1",
144+
Kind: "RebootNode",
145+
CompleteConditionType: "NodeReady",
121146
},
122147
},
123148
}
@@ -184,10 +209,17 @@ func (m *MockNodeAnnotationManager) GetRemediationState(ctx context.Context, nod
184209
if createdAtForGroup, ok := m.createdByGroup[groupName]; ok {
185210
groupCreatedAt = createdAtForGroup
186211
}
212+
actionName := m.actionByGroup[groupName]
213+
if actionName == "" {
214+
actionName = protos.RecommendedAction_RESTART_BM.String()
215+
}
187216
annotationState.EquivalenceGroups[groupName] = annotation.EquivalenceGroupState{
188217
MaintenanceCR: crName,
189218
CreatedAt: groupCreatedAt,
190-
ActionName: m.actionByGroup[groupName],
219+
ActionName: actionName,
220+
ApiGroup: "janitor.dgxc.nvidia.com",
221+
Version: "v1alpha1",
222+
Kind: "RebootNode",
191223
}
192224
}
193225
return annotationState, nil, nil
@@ -1016,11 +1048,15 @@ func TestHandleRemediationEventEnsuresGVKBeforeStatusCheck(t *testing.T) {
10161048
mockK8sClient := &MockK8sClient{
10171049
annotationManagerOverride: mockAnnotationManager,
10181050
mockStatusChecker: &mockStatusChecker{
1019-
getCRStateFn: func(_ context.Context, actionName string, crName string) crstatus.CRState {
1051+
getCRStateForReferenceFn: func(_ context.Context, crName string,
1052+
resourceRef annotation.MaintenanceResourceReference, completeConditionType string) crstatus.CRState {
10201053
statusChecked = true
10211054
assert.True(t, ensureCalled, "GVK annotation should be ensured before CR status checks")
1022-
assert.Equal(t, protos.RecommendedAction_RESTART_BM.String(), actionName)
10231055
assert.Equal(t, "existing-cr", crName)
1056+
assert.Equal(t, "janitor.dgxc.nvidia.com", resourceRef.ApiGroup)
1057+
assert.Equal(t, "v1alpha1", resourceRef.Version)
1058+
assert.Equal(t, "RebootNode", resourceRef.Kind)
1059+
assert.Equal(t, "NodeReady", completeConditionType)
10241060

10251061
return crstatus.CRStateInProgress
10261062
},
@@ -1049,6 +1085,53 @@ func TestHandleRemediationEventEnsuresGVKBeforeStatusCheck(t *testing.T) {
10491085
assert.True(t, statusChecked)
10501086
}
10511087

1088+
func TestEvaluateExistingCRUsesStoredReference(t *testing.T) {
1089+
ctx := context.Background()
1090+
statusChecker := &mockStatusChecker{
1091+
getCRStateForReferenceFn: func(_ context.Context, crName string,
1092+
resourceRef annotation.MaintenanceResourceReference, completeConditionType string) crstatus.CRState {
1093+
assert.Equal(t, "stored-cr", crName)
1094+
assert.Equal(t, "stored.example.com", resourceRef.ApiGroup)
1095+
assert.Equal(t, "v9", resourceRef.Version)
1096+
assert.Equal(t, "StoredMaintenance", resourceRef.Kind)
1097+
assert.Equal(t, "stored-namespace", resourceRef.Namespace)
1098+
assert.Equal(t, "NodeReady", completeConditionType)
1099+
1100+
return crstatus.CRStateInProgress
1101+
},
1102+
}
1103+
mockK8sClient := &MockK8sClient{
1104+
mockStatusChecker: statusChecker,
1105+
configOverride: &config.TomlConfig{
1106+
RemediationActions: map[string]config.MaintenanceResource{
1107+
protos.RecommendedAction_RESTART_BM.String(): {
1108+
ApiGroup: "config.example.com",
1109+
Version: "v1",
1110+
Kind: "ConfigMaintenance",
1111+
Namespace: "config-namespace",
1112+
CompleteConditionType: "NodeReady",
1113+
},
1114+
},
1115+
},
1116+
}
1117+
r := NewFaultRemediationReconciler(nil, nil, nil, ReconcilerConfig{RemediationClient: mockK8sClient}, false)
1118+
1119+
decision := r.evaluateExistingCR(ctx, statusChecker, namedEquivalenceGroupState{
1120+
name: "restart",
1121+
state: annotation.EquivalenceGroupState{
1122+
MaintenanceCR: "stored-cr",
1123+
ActionName: protos.RecommendedAction_RESTART_BM.String(),
1124+
ApiGroup: "stored.example.com",
1125+
Version: "v9",
1126+
Kind: "StoredMaintenance",
1127+
Namespace: "stored-namespace",
1128+
},
1129+
}, time.Now(), "test-node")
1130+
1131+
assert.False(t, decision.shouldCreate)
1132+
assert.Equal(t, "stored-cr", decision.crName)
1133+
}
1134+
10521135
func TestCRBasedDeduplication(t *testing.T) {
10531136
ctx := context.Background()
10541137

0 commit comments

Comments
 (0)