Skip to content

Commit

Permalink
Refactor NodeDisruption code
Browse files Browse the repository at this point in the history
- The main logic of the code is unchanged and covered by tests at 80%
- Remove "resolver" and replace it with a Reconciler.
- SingleNodeDisruptionReconciler is a naming pattern seen in other operators
  • Loading branch information
geobeau committed Oct 26, 2023
1 parent 623c9ca commit 2888cef
Show file tree
Hide file tree
Showing 3 changed files with 201 additions and 175 deletions.
43 changes: 43 additions & 0 deletions internal/controller/budget.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,46 @@ type Budget interface {
// Get the name, namespace and kind of bduget
GetNamespacedName() nodedisruptionv1alpha1.NamespacedName
}

// GetAllBudgetsInSync fetch all the budgets from Kubernetes and synchronise them
func GetAllBudgetsInSync(ctx context.Context, k8sClient client.Client) ([]Budget, error) {
opts := []client.ListOption{}
budgets := []Budget{}

applicationDisruptionBudgets := &nodedisruptionv1alpha1.ApplicationDisruptionBudgetList{}
err := k8sClient.List(ctx, applicationDisruptionBudgets, opts...)
if err != nil {
return budgets, err
}
for _, adb := range applicationDisruptionBudgets.Items {
adbResolver := ApplicationDisruptionBudgetResolver{
ApplicationDisruptionBudget: adb.DeepCopy(),
Client: k8sClient,
Resolver: resolver.Resolver{Client: k8sClient},
}
budgets = append(budgets, &adbResolver)
}

nodeDisruptionBudget := &nodedisruptionv1alpha1.NodeDisruptionBudgetList{}
err = k8sClient.List(ctx, nodeDisruptionBudget, opts...)
if err != nil {
return budgets, err
}
for _, ndb := range nodeDisruptionBudget.Items {
ndbResolver := NodeDisruptionBudgetResolver{
NodeDisruptionBudget: ndb.DeepCopy(),
Client: k8sClient,
Resolver: resolver.Resolver{Client: k8sClient},
}
budgets = append(budgets, &ndbResolver)
}

for _, budget := range budgets {
err := budget.Sync(ctx)
if err != nil {
return budgets, err
}
}

return budgets, nil
}
62 changes: 41 additions & 21 deletions internal/controller/budget_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ func (m *MockBudget) Sync(context.Context) error {
}

// Check if the budget would be impacted by an operation on the provided set of nodes
func (m *MockBudget) IsImpacted(controller.NodeDisruption) bool {
func (m *MockBudget) IsImpacted(resolver.NodeSet) bool {
return m.impacted
}

// Return the number of disruption allowed considering a list of current node disruptions
func (m *MockBudget) TolerateDisruption(controller.NodeDisruption) bool {
func (m *MockBudget) TolerateDisruption(resolver.NodeSet) bool {
return m.tolerate
}

Expand All @@ -57,9 +57,16 @@ func (m *MockBudget) GetNamespacedName() nodedisruptionv1alpha1.NamespacedName {
return m.name
}

func TestValidationNoImpactedBudget(t *testing.T) {
resolver := controller.NodeDisruptionResolver{Client: fake.NewClientBuilder().Build()}
func TestValidateWithBudgetConstraintsNoImpactedBudget(t *testing.T) {
nodes := []string{"node-dummy-0", "node-dummy-1"}
reconciler := controller.SingleNodeDisruptionReconciler{
Client: fake.NewClientBuilder().Build(),
NodeDisruption: nodedisruptionv1alpha1.NodeDisruption{
Status: nodedisruptionv1alpha1.NodeDisruptionStatus{
DisruptedNodes: nodes,
},
},
}

budget1 := MockBudget{
name: nodedisruptionv1alpha1.NamespacedName{Namespace: "test1", Name: "test1"},
Expand All @@ -76,19 +83,21 @@ func TestValidationNoImpactedBudget(t *testing.T) {
}

budgets := []controller.Budget{&budget1, &budget2}

disruption := controller.NodeDisruption{controller.NewNodeSetFromStringList(nodes)}
anyFailed, statuses := resolver.DoValidateDisruption(context.Background(), budgets, disruption)
anyFailed, statuses := reconciler.ValidateWithBudgetConstraints(context.Background(), budgets)
assert.False(t, anyFailed)
assert.Equal(t, len(statuses), 0)
}

func TestValidationImpactedAllOk(t *testing.T) {
resolver := controller.NodeDisruptionResolver{
Client: fake.NewClientBuilder().Build(),
NodeDisruption: &nodedisruptionv1alpha1.NodeDisruption{},
}
nodes := []string{"node-dummy-0", "node-dummy-1"}
reconciler := controller.SingleNodeDisruptionReconciler{
Client: fake.NewClientBuilder().Build(),
NodeDisruption: nodedisruptionv1alpha1.NodeDisruption{
Status: nodedisruptionv1alpha1.NodeDisruptionStatus{
DisruptedNodes: nodes,
},
},
}

budget1 := MockBudget{
name: nodedisruptionv1alpha1.NamespacedName{Namespace: "test1", Name: "test1"},
Expand All @@ -106,15 +115,21 @@ func TestValidationImpactedAllOk(t *testing.T) {

budgets := []controller.Budget{&budget1, &budget2}

disruption := controller.NodeDisruption{controller.NewNodeSetFromStringList(nodes)}
anyFailed, statuses := resolver.DoValidateDisruption(context.Background(), budgets, disruption)
anyFailed, statuses := reconciler.ValidateWithBudgetConstraints(context.Background(), budgets)
assert.False(t, anyFailed)
assert.Equal(t, len(statuses), 2)
}

func TestValidationFailAtDisruption(t *testing.T) {
resolver := controller.NodeDisruptionResolver{Client: fake.NewClientBuilder().Build()}
func TestValidateWithBudgetConstraintsFailAtDisruption(t *testing.T) {
nodes := []string{"node-dummy-0", "node-dummy-1"}
reconciler := controller.SingleNodeDisruptionReconciler{
Client: fake.NewClientBuilder().Build(),
NodeDisruption: nodedisruptionv1alpha1.NodeDisruption{
Status: nodedisruptionv1alpha1.NodeDisruptionStatus{
DisruptedNodes: nodes,
},
},
}

budget1 := MockBudget{
name: nodedisruptionv1alpha1.NamespacedName{Namespace: "test1", Name: "test1"},
Expand All @@ -132,8 +147,7 @@ func TestValidationFailAtDisruption(t *testing.T) {

budgets := []controller.Budget{&budget1, &budget2}

disruption := controller.NodeDisruption{controller.NewNodeSetFromStringList(nodes)}
anyFailed, statuses := resolver.DoValidateDisruption(context.Background(), budgets, disruption)
anyFailed, statuses := reconciler.ValidateWithBudgetConstraints(context.Background(), budgets)
assert.True(t, anyFailed)
assert.Equal(t, len(statuses), 1)
assert.False(t, statuses[0].Ok)
Expand All @@ -143,9 +157,16 @@ func TestValidationFailAtDisruption(t *testing.T) {
assert.False(t, budget2.healthChecked, "Health check should not be made if disruption check failed")
}

func TestValidationFailAtHealth(t *testing.T) {
resolver := controller.NodeDisruptionResolver{Client: fake.NewClientBuilder().Build()}
func TestValidateWithBudgetConstraintsFailAtHealth(t *testing.T) {
nodes := []string{"node-dummy-0", "node-dummy-1"}
reconciler := controller.SingleNodeDisruptionReconciler{
Client: fake.NewClientBuilder().Build(),
NodeDisruption: nodedisruptionv1alpha1.NodeDisruption{
Status: nodedisruptionv1alpha1.NodeDisruptionStatus{
DisruptedNodes: nodes,
},
},
}

budget1 := MockBudget{
name: nodedisruptionv1alpha1.NamespacedName{Namespace: "test1", Name: "test1"},
Expand All @@ -163,8 +184,7 @@ func TestValidationFailAtHealth(t *testing.T) {

budgets := []controller.Budget{&budget1, &budget2}

disruption := controller.NodeDisruption{controller.NewNodeSetFromStringList(nodes)}
anyFailed, statuses := resolver.DoValidateDisruption(context.Background(), budgets, disruption)
anyFailed, statuses := reconciler.ValidateWithBudgetConstraints(context.Background(), budgets)
assert.True(t, anyFailed)
assert.False(t, statuses[0].Ok)
assert.NotEmpty(t, statuses[0].Reason, "Rejected budget should provide a reason")
Expand Down
Loading

0 comments on commit 2888cef

Please sign in to comment.