Skip to content

Commit 2823f76

Browse files
authored
fix: Clean up nolint directives marked as TODO - Part 1 (#828)
1 parent d7b6b85 commit 2823f76

File tree

7 files changed

+400
-225
lines changed

7 files changed

+400
-225
lines changed

fault-quarantine/pkg/breaker/breaker.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,7 @@ func (b *slidingWindowBreaker) calculateBackoffDelay(attempt int,
405405
}
406406

407407
// Safe conversion: attempt is guaranteed to be [0, 30] at this point
408-
safeAttempt := uint(attempt) //nolint:gosec // Range validated above
408+
safeAttempt := uint(attempt)
409409
multiplier := int64(1 << safeAttempt) // 2^attempt as integer
410410
delay := time.Duration(int64(initialDelay) * multiplier)
411411

fault-remediation/pkg/config/config.go

Lines changed: 42 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -141,44 +141,63 @@ not a use-case for it.
141141
Additionally, the ImpactedEntityScope must be included in EntityTypeToResourceNames (meaning that partial draining is
142142
enabled for that entity).
143143
*/
144-
// nolint:cyclop
145144
func (c *TomlConfig) validateEquivalenceGroup(actionName string, resource MaintenanceResource) error {
146145
if len(resource.EquivalenceGroup) == 0 {
147146
return fmt.Errorf("action '%s' must have a non-empty EquivalenceGroup", actionName)
148147
}
149148

150149
for _, group := range resource.SupersedingEquivalenceGroups {
151-
foundGroup := false
150+
if err := validateOneSupersedingGroup(actionName, group, resource, c.RemediationActions); err != nil {
151+
return err
152+
}
153+
}
152154

153-
for _, maintenanceResource := range c.RemediationActions {
154-
if group == maintenanceResource.EquivalenceGroup {
155-
if len(maintenanceResource.ImpactedEntityScope) != 0 {
156-
return fmt.Errorf("supersedingEquivalenceGroup %s cannot have an impactedEntityScopes: %s",
157-
maintenanceResource.EquivalenceGroup, maintenanceResource.ImpactedEntityScope)
158-
}
155+
return validateResourceImpactedEntityScope(actionName, resource)
156+
}
159157

160-
foundGroup = true
161-
}
162-
}
158+
func validateOneSupersedingGroup(actionName, group string, resource MaintenanceResource,
159+
remediationActions map[string]MaintenanceResource,
160+
) error {
161+
if group == resource.EquivalenceGroup {
162+
return fmt.Errorf("action '%s': SupersedingEquivalenceGroup cannot include the EquivalenceGroup itself: %s",
163+
actionName, group)
164+
}
165+
166+
foundGroup := false
163167

164-
if !foundGroup {
165-
return fmt.Errorf("superseding EquivalenceGroup %s must be defined in config", group)
168+
for _, maintenanceResource := range remediationActions {
169+
if group != maintenanceResource.EquivalenceGroup {
170+
continue
166171
}
167172

168-
if group == resource.EquivalenceGroup {
169-
return fmt.Errorf("SupersedingEquivalenceGroup cannot include the EquivalenceGroup itself: %s", group)
173+
if len(maintenanceResource.ImpactedEntityScope) != 0 {
174+
return fmt.Errorf("action '%s': superseding EquivalenceGroup %s cannot have an ImpactedEntityScope defined: %s",
175+
actionName, maintenanceResource.EquivalenceGroup, maintenanceResource.ImpactedEntityScope)
170176
}
177+
178+
foundGroup = true
171179
}
172180

173-
if len(resource.ImpactedEntityScope) != 0 {
174-
if actionName != protos.RecommendedAction_COMPONENT_RESET.String() {
175-
return fmt.Errorf("action '%s' cannot have an ImpactedEntityScope defined", actionName)
176-
}
181+
if !foundGroup {
182+
return fmt.Errorf("action '%s': superseding EquivalenceGroup %s must be defined in config",
183+
actionName, group)
184+
}
177185

178-
if _, ok := model.EntityTypeToResourceNames[resource.ImpactedEntityScope]; !ok {
179-
return fmt.Errorf("impacted entity for action does not support partial draining: %s",
180-
resource.ImpactedEntityScope)
181-
}
186+
return nil
187+
}
188+
189+
func validateResourceImpactedEntityScope(actionName string, resource MaintenanceResource) error {
190+
if len(resource.ImpactedEntityScope) == 0 {
191+
return nil
192+
}
193+
194+
if actionName != protos.RecommendedAction_COMPONENT_RESET.String() {
195+
return fmt.Errorf("action '%s' cannot have an ImpactedEntityScope defined", actionName)
196+
}
197+
198+
if _, ok := model.EntityTypeToResourceNames[resource.ImpactedEntityScope]; !ok {
199+
return fmt.Errorf("action '%s' cannot use ImpactedEntityScope '%s': entity type does not support partial draining",
200+
actionName, resource.ImpactedEntityScope)
182201
}
183202

184203
return nil

fault-remediation/pkg/config/config_test.go

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,15 @@ func TestTomlConfig_Validate(t *testing.T) {
4444
expectError bool
4545
errorSubstr string
4646
}{
47+
{
48+
name: "empty template mountPath should be rejected",
49+
config: TomlConfig{
50+
Template: Template{MountPath: ""},
51+
RemediationActions: map[string]MaintenanceResource{},
52+
},
53+
expectError: true,
54+
errorSubstr: "template mountPath must be non-empty",
55+
},
4756
{
4857
name: "valid config with matching templates",
4958
config: TomlConfig{
@@ -140,6 +149,7 @@ func TestTomlConfig_Validate(t *testing.T) {
140149
},
141150
},
142151
expectError: true,
152+
errorSubstr: "must have a non-empty EquivalenceGroup",
143153
},
144154
{
145155
name: "missing SupersedingEquivalenceGroup should be rejected",
@@ -155,23 +165,29 @@ func TestTomlConfig_Validate(t *testing.T) {
155165
},
156166
},
157167
expectError: true,
168+
errorSubstr: "must be defined in config",
158169
},
159170
{
160171
name: "SupersedingEquivalenceGroup cannot include the EquivalenceGroup",
161172
config: TomlConfig{
162173
Template: Template{MountPath: tempDir},
163174
RemediationActions: map[string]MaintenanceResource{
164-
"COMPONENT_RESET": {
175+
"ACTION_A": {
176+
TemplateFileName: "template-a.yaml",
177+
Scope: "Cluster",
178+
EquivalenceGroup: "restart",
179+
},
180+
"ACTION_B": {
165181
TemplateFileName: "template-b.yaml",
166182
Scope: "Namespaced",
167183
Namespace: "test-namespace",
168184
EquivalenceGroup: "reset",
169185
SupersedingEquivalenceGroups: []string{"restart", "reset"},
170-
ImpactedEntityScope: "GPU_UUID",
171186
},
172187
},
173188
},
174189
expectError: true,
190+
errorSubstr: "cannot include the EquivalenceGroup itself",
175191
},
176192
{
177193
name: "Non-supported ImpactedEntityScope should be rejected",
@@ -187,32 +203,33 @@ func TestTomlConfig_Validate(t *testing.T) {
187203
},
188204
},
189205
expectError: true,
206+
errorSubstr: "does not support partial draining",
190207
},
191208
{
192209
name: "SupersedingEquivalenceGroup cannot have an ImpactedEntityScope",
193210
config: TomlConfig{
194211
Template: Template{MountPath: tempDir},
195212
RemediationActions: map[string]MaintenanceResource{
196-
"ACTION_A": {
197-
TemplateFileName: "template-a.yaml",
198-
Scope: "Cluster",
213+
"COMPONENT_RESET": {
214+
TemplateFileName: "template-b.yaml",
215+
Scope: "Namespaced",
216+
Namespace: "test-namespace",
199217
EquivalenceGroup: "restart",
200-
ImpactedEntityScope: "PCI",
218+
ImpactedEntityScope: "GPU_UUID",
201219
},
202220
"ACTION_B": {
203-
TemplateFileName: "template-b.yaml",
204-
Scope: "Namespaced",
205-
Namespace: "test-namespace",
221+
TemplateFileName: "template-a.yaml",
222+
Scope: "Cluster",
206223
EquivalenceGroup: "reset",
207224
SupersedingEquivalenceGroups: []string{"restart"},
208-
ImpactedEntityScope: "GPU_UUID",
209225
},
210226
},
211227
},
212228
expectError: true,
229+
errorSubstr: "cannot have an ImpactedEntityScope defined",
213230
},
214231
{
215-
name: "SupersedingEquivalenceGroup cannot have an ImpactedEntityScope",
232+
name: "SupersedingEquivalenceGroup cannot have an ImpactedEntityScope (COMPONENT_RESET)",
216233
config: TomlConfig{
217234
Template: Template{MountPath: tempDir},
218235
RemediationActions: map[string]MaintenanceResource{
@@ -233,6 +250,7 @@ func TestTomlConfig_Validate(t *testing.T) {
233250
},
234251
},
235252
expectError: true,
253+
errorSubstr: "cannot have an ImpactedEntityScope defined",
236254
},
237255
{
238256
name: "Only the COMPONENT_RESET action can have an ImpactedEntityScope",
@@ -255,6 +273,7 @@ func TestTomlConfig_Validate(t *testing.T) {
255273
},
256274
},
257275
expectError: true,
276+
errorSubstr: "cannot have an ImpactedEntityScope defined",
258277
},
259278
}
260279

fault-remediation/pkg/initializer/init.go

Lines changed: 75 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -48,32 +48,21 @@ type Components struct {
4848
FaultRemediationReconciler reconciler.FaultRemediationReconciler
4949
}
5050

51-
// nolint: cyclop // todo
5251
func InitializeAll(
5352
ctx context.Context,
5453
params InitializationParams,
5554
ctrlruntimeClient ctrlruntimeClient.Client,
5655
) (*Components, error) {
5756
slog.Info("Starting fault remediation module initialization")
5857

59-
tokenConfig, err := storeconfig.TokenConfigFromEnv("fault-remediation")
58+
tokenConfig, tomlConfig, err := loadTokenAndTomlConfig(params)
6059
if err != nil {
61-
return nil, fmt.Errorf("failed to load token configuration: %w", err)
60+
return nil, err
6261
}
6362

6463
builder := client.GetPipelineBuilder()
6564
pipeline := builder.BuildQuarantinedAndDrainedNodesPipeline()
6665

67-
var tomlConfig config.TomlConfig
68-
if err := configmanager.LoadTOMLConfig(params.TomlConfigPath, &tomlConfig); err != nil {
69-
return nil, fmt.Errorf("error while loading the toml Config: %w", err)
70-
}
71-
72-
// Validate the configuration for consistency
73-
if err := tomlConfig.Validate(); err != nil {
74-
return nil, fmt.Errorf("configuration validation failed: %w", err)
75-
}
76-
7766
if params.DryRun {
7867
slog.Info("Running in dry-run mode")
7968
}
@@ -82,56 +71,25 @@ func InitializeAll(
8271
slog.Info("Log collector enabled")
8372
}
8473

85-
remediationClient, err := remediation.NewRemediationClient(
86-
ctrlruntimeClient,
87-
params.DryRun,
88-
tomlConfig,
89-
)
74+
remediationClient, stateManager, err := initRemediationAndStateManager(params.Config, ctrlruntimeClient,
75+
params.DryRun, tomlConfig)
9076
if err != nil {
91-
return nil, fmt.Errorf("error while initializing remediation client: %w", err)
77+
return nil, err
9278
}
9379

94-
kubeClient, err := kubernetes.NewForConfig(params.Config)
95-
if err != nil {
96-
return nil, fmt.Errorf("error init kube client for state manager: %w", err)
97-
}
98-
99-
stateManager := statemanager.NewStateManager(kubeClient)
100-
10180
slog.Info("Successfully initialized client")
10281

103-
// Load datastore configuration
104-
datastoreConfig, err := datastore.LoadDatastoreConfig()
82+
ds, watcherInstance, healthEventStore, datastoreConfig, err := initDatastoreAndWatcher(ctx, pipeline)
10583
if err != nil {
106-
return nil, fmt.Errorf("failed to load datastore configuration: %w", err)
84+
return nil, err
10785
}
10886

109-
// Convert store Config types to the types expected by reconciler
11087
clientTokenConfig := client.TokenConfig{
11188
ClientName: tokenConfig.ClientName,
11289
TokenDatabase: tokenConfig.TokenDatabase,
11390
TokenCollection: tokenConfig.TokenCollection,
11491
}
11592

116-
ds, err := datastore.NewDataStore(ctx, *datastoreConfig)
117-
if err != nil {
118-
return nil, fmt.Errorf("error initializing datastore: %w", err)
119-
}
120-
121-
// Create watcher using the factory pattern
122-
watcherConfig := watcher.WatcherConfig{
123-
Pipeline: pipeline,
124-
CollectionName: "HealthEvents",
125-
}
126-
127-
watcherInstance, err := watcher.CreateChangeStreamWatcher(ctx, ds, watcherConfig)
128-
if err != nil {
129-
return nil, fmt.Errorf("error initializing change stream watcher: %w", err)
130-
}
131-
132-
// Get the HealthEventStore for document operations
133-
healthEventStore := ds.HealthEventStore()
134-
13593
reconcilerCfg := reconciler.ReconcilerConfig{
13694
DataStoreConfig: *datastoreConfig,
13795
TokenConfig: clientTokenConfig,
@@ -150,3 +108,71 @@ func InitializeAll(
150108
ds, watcherInstance, healthEventStore, reconcilerCfg, params.DryRun),
151109
}, nil
152110
}
111+
112+
func loadTokenAndTomlConfig(params InitializationParams) (*storeconfig.TokenConfig, *config.TomlConfig, error) {
113+
tokenConfig, err := storeconfig.TokenConfigFromEnv("fault-remediation")
114+
if err != nil {
115+
return nil, nil, fmt.Errorf("failed to load token configuration: %w", err)
116+
}
117+
118+
var tomlConfig config.TomlConfig
119+
if err := configmanager.LoadTOMLConfig(params.TomlConfigPath, &tomlConfig); err != nil {
120+
return nil, nil, fmt.Errorf("error while loading the toml Config: %w", err)
121+
}
122+
123+
if err := tomlConfig.Validate(); err != nil {
124+
return nil, nil, fmt.Errorf("configuration validation failed: %w", err)
125+
}
126+
127+
return &tokenConfig, &tomlConfig, nil
128+
}
129+
130+
func initRemediationAndStateManager(
131+
restConfig *rest.Config,
132+
ctrlruntimeClient ctrlruntimeClient.Client,
133+
dryRun bool,
134+
tomlConfig *config.TomlConfig,
135+
) (*remediation.FaultRemediationClient, statemanager.StateManager, error) {
136+
remediationClient, err := remediation.NewRemediationClient(ctrlruntimeClient, dryRun, *tomlConfig)
137+
if err != nil {
138+
return nil, nil, fmt.Errorf("error while initializing remediation client: %w", err)
139+
}
140+
141+
kubeClient, err := kubernetes.NewForConfig(restConfig)
142+
if err != nil {
143+
return nil, nil, fmt.Errorf("error init kube client for state manager: %w", err)
144+
}
145+
146+
stateManager := statemanager.NewStateManager(kubeClient)
147+
148+
return remediationClient, stateManager, nil
149+
}
150+
151+
func initDatastoreAndWatcher(
152+
ctx context.Context,
153+
pipeline datastore.Pipeline,
154+
) (datastore.DataStore, datastore.ChangeStreamWatcher, datastore.HealthEventStore, *datastore.DataStoreConfig, error) {
155+
datastoreConfig, err := datastore.LoadDatastoreConfig()
156+
if err != nil {
157+
return nil, nil, nil, nil, fmt.Errorf("failed to load datastore configuration: %w", err)
158+
}
159+
160+
ds, err := datastore.NewDataStore(ctx, *datastoreConfig)
161+
if err != nil {
162+
return nil, nil, nil, nil, fmt.Errorf("error initializing datastore: %w", err)
163+
}
164+
165+
watcherConfig := watcher.WatcherConfig{
166+
Pipeline: pipeline,
167+
CollectionName: "HealthEvents",
168+
}
169+
170+
watcherInstance, err := watcher.CreateChangeStreamWatcher(ctx, ds, watcherConfig)
171+
if err != nil {
172+
return nil, nil, nil, nil, fmt.Errorf("error initializing change stream watcher: %w", err)
173+
}
174+
175+
healthEventStore := ds.HealthEventStore()
176+
177+
return ds, watcherInstance, healthEventStore, datastoreConfig, nil
178+
}

0 commit comments

Comments
 (0)