Skip to content

Commit

Permalink
refactor: optimize allInjected and allRecovered states (chaos-mes…
Browse files Browse the repository at this point in the history
…h#4199)

* refactor: allInjected and allRecovered conditions

Signed-off-by: Yue Yang <[email protected]>

* chore: update

Signed-off-by: Yue Yang <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Yue Yang <[email protected]>

* chore: add tests

Signed-off-by: Yue Yang <[email protected]>

* Update condition_suite_test.go

Signed-off-by: Yue Yang <[email protected]>

* Update controller_test.go

Signed-off-by: Yue Yang <[email protected]>

* fix: remove unnecessary else

Signed-off-by: Yue Yang <[email protected]>

* chore: add comments

Signed-off-by: Yue Yang <[email protected]>

* Update ci.yml

Signed-off-by: Yue Yang <[email protected]>

---------

Signed-off-by: Yue Yang <[email protected]>
  • Loading branch information
g1eny0ung authored Nov 1, 2023
1 parent 4e3ee29 commit c2d7fa5
Show file tree
Hide file tree
Showing 6 changed files with 181 additions and 46 deletions.
2 changes: 0 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,6 @@ jobs:
if: matrix.job == 'test'
with:
files: ./cover.out
verbose: true
fail_ci_if_error: true
ui:
needs: changes
if: ${{ needs.changes.outputs.ui == 'true' }}
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ For more information and how-to, see [RFC: Keep A Changelog](https://github.com/
- Update ginkgo to 2.12.0 [#4190](https://github.com/chaos-mesh/chaos-mesh/pull/4190)
- Update k8s controller-runtime dependency [#4198](https://github.com/chaos-mesh/chaos-mesh/pull/4198)
- Automatically remove the token from the dashboard when it expires [#4193](https://github.com/chaos-mesh/chaos-mesh/pull/4193)
- Optimize `allInjected` and `allRecovered` states when targets are not selected [#4199](https://github.com/chaos-mesh/chaos-mesh/pull/4199)

### Deprecated

Expand Down
28 changes: 28 additions & 0 deletions controllers/common/condition/condition_suite_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright 2023 Chaos Mesh Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

package condition_test

import (
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)

func TestCondition(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Condition Suite")
}
111 changes: 68 additions & 43 deletions controllers/common/condition/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ type StatusAndReason struct {
}

func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
obj := r.Object.DeepCopyObject().(v1alpha1.InnerObjectWithSelector)
if err := r.Client.Get(context.TODO(), req.NamespacedName, obj); err != nil {
obj := r.Object.DeepCopyObject().(v1alpha1.InnerObject)
if err := r.Client.Get(ctx, req.NamespacedName, obj); err != nil {
if apierrors.IsNotFound(err) {
r.Log.Info("chaos not found")
} else {
Expand All @@ -70,44 +70,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
}
}

newConditionMap := make(map[v1alpha1.ChaosConditionType]StatusAndReason)
if obj.GetStatus().Experiment.Records != nil {
newConditionMap[v1alpha1.ConditionSelected] = StatusAndReason{
Status: corev1.ConditionTrue,
}
} else {
newConditionMap[v1alpha1.ConditionSelected] = StatusAndReason{
Status: corev1.ConditionFalse,
}
}

allInjected := corev1.ConditionTrue
allRecovered := corev1.ConditionTrue
for _, record := range obj.GetStatus().Experiment.Records {
if record.Phase != v1alpha1.NotInjected {
allRecovered = corev1.ConditionFalse
}

if record.Phase != v1alpha1.Injected {
allInjected = corev1.ConditionFalse
}
}
newConditionMap[v1alpha1.ConditionAllInjected] = StatusAndReason{
Status: allInjected,
}
newConditionMap[v1alpha1.ConditionAllRecovered] = StatusAndReason{
Status: allRecovered,
}

if obj.IsPaused() {
newConditionMap[v1alpha1.ConditionPaused] = StatusAndReason{
Status: corev1.ConditionTrue,
}
} else {
newConditionMap[v1alpha1.ConditionPaused] = StatusAndReason{
Status: corev1.ConditionFalse,
}
}
newConditionMap := diffConditions(obj)

if !reflect.DeepEqual(newConditionMap, conditionMap) {
conditions := make([]v1alpha1.ChaosCondition, 0, 5)
Expand All @@ -120,15 +83,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
}

r.Log.Info("updating conditions", "conditions", conditions)
obj := r.Object.DeepCopyObject().(v1alpha1.InnerObjectWithSelector)
obj := r.Object.DeepCopyObject().(v1alpha1.InnerObject)

if err := r.Client.Get(context.TODO(), req.NamespacedName, obj); err != nil {
if err := r.Client.Get(ctx, req.NamespacedName, obj); err != nil {
r.Log.Error(err, "unable to get chaos")
return err
}

obj.GetStatus().Conditions = conditions
return r.Client.Update(context.TODO(), obj)
return r.Client.Update(ctx, obj)
}

return nil
Expand All @@ -139,5 +102,67 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
r.Recorder.Eventf(obj, "Normal", "Failed", "Failed to update conditions: %s", updateError.Error())
return ctrl.Result{}, nil
}

return ctrl.Result{}, nil
}

func diffConditions(obj v1alpha1.InnerObject) (newConditionMap map[v1alpha1.ChaosConditionType]StatusAndReason) {
records := obj.GetStatus().Experiment.Records
newConditionMap = make(map[v1alpha1.ChaosConditionType]StatusAndReason)

if records != nil {
newConditionMap[v1alpha1.ConditionSelected] = StatusAndReason{
Status: corev1.ConditionTrue,
}
} else {
newConditionMap[v1alpha1.ConditionSelected] = StatusAndReason{
Status: corev1.ConditionFalse,
}
}

// If records is `nil`, we don't need to check the `allInjected` and `allRecovered` conditions.
allInjected := corev1.ConditionFalse
if records != nil && every(records, func(record *v1alpha1.Record) bool {
return record.Phase == v1alpha1.Injected
}) {
allInjected = corev1.ConditionTrue
}

allRecovered := corev1.ConditionFalse
if records != nil && every(records, func(record *v1alpha1.Record) bool {
return record.Phase == v1alpha1.NotInjected
}) {
allRecovered = corev1.ConditionTrue
}

newConditionMap[v1alpha1.ConditionAllInjected] = StatusAndReason{
Status: allInjected,
}
newConditionMap[v1alpha1.ConditionAllRecovered] = StatusAndReason{
Status: allRecovered,
}

if obj.IsPaused() {
newConditionMap[v1alpha1.ConditionPaused] = StatusAndReason{
Status: corev1.ConditionTrue,
}
} else {
newConditionMap[v1alpha1.ConditionPaused] = StatusAndReason{
Status: corev1.ConditionFalse,
}
}

return
}

// every returns true if all elements in the given slice satisfy the given condition.
//
// In this package, we use it to check if all records are injected or recovered.
func every[T any](arr []T, condition func(T) bool) bool {
for _, item := range arr {
if !condition(item) {
return false
}
}
return true
}
77 changes: 77 additions & 0 deletions controllers/common/condition/controller_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// Copyright 2023 Chaos Mesh Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

package condition

import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/chaos-mesh/chaos-mesh/api/v1alpha1"
)

var _ = Describe("Test condition controller", func() {
reconciler := Reconciler{
Object: &v1alpha1.PodChaos{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
v1alpha1.PauseAnnotationKey: "true",
},
},
},
}

Context("Test diffConditions", func() {
It("selected/allInjected/allRecovered state should be false when records is empty", func() {
newConditionMap := diffConditions(reconciler.Object.DeepCopyObject().(v1alpha1.InnerObject))

Expect(newConditionMap[v1alpha1.ConditionSelected].Status).To(Equal(corev1.ConditionFalse))
Expect(newConditionMap[v1alpha1.ConditionAllInjected].Status).To(Equal(corev1.ConditionFalse))
Expect(newConditionMap[v1alpha1.ConditionAllRecovered].Status).To(Equal(corev1.ConditionFalse))
})

It("Paused state should be true when pause annotation is true", func() {
obj := reconciler.Object.DeepCopyObject().(v1alpha1.InnerObject)
obj.SetAnnotations(map[string]string{
v1alpha1.PauseAnnotationKey: "true",
})
newConditionMap := diffConditions(obj)

Expect(newConditionMap[v1alpha1.ConditionPaused].Status).To(Equal(corev1.ConditionTrue))
})

It("AllInjected state should be true when all records are injected", func() {
obj := reconciler.Object.DeepCopyObject().(v1alpha1.InnerObject)
obj.GetStatus().Experiment.Records = append(obj.GetStatus().Experiment.Records, &v1alpha1.Record{
Phase: v1alpha1.Injected,
})
newConditionMap := diffConditions(obj)

Expect(newConditionMap[v1alpha1.ConditionAllInjected].Status).To(Equal(corev1.ConditionTrue))
})

It("AllRecovered state should be true when all records are recovered", func() {
obj := reconciler.Object.DeepCopyObject().(v1alpha1.InnerObject)
obj.GetStatus().Experiment.Records = append(obj.GetStatus().Experiment.Records, &v1alpha1.Record{
Phase: v1alpha1.NotInjected,
})
newConditionMap := diffConditions(obj)

Expect(newConditionMap[v1alpha1.ConditionAllRecovered].Status).To(Equal(corev1.ConditionTrue))
})
})
})
8 changes: 7 additions & 1 deletion controllers/common/step.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,11 @@ import (
)

func AllSteps() []pipeline.PipelineStep {
return []pipeline.PipelineStep{finalizers.InitStep, desiredphase.Step, condition.Step, records.Step, finalizers.CleanStep}
return []pipeline.PipelineStep{
finalizers.InitStep,
desiredphase.Step,
condition.Step,
records.Step,
finalizers.CleanStep,
}
}

0 comments on commit c2d7fa5

Please sign in to comment.