From 2e019617a9d3dec56fd4e5b808bf0a53fe985c8a Mon Sep 17 00:00:00 2001 From: Kensei Nakada Date: Wed, 4 Oct 2023 13:10:49 +0900 Subject: [PATCH] introduce tortoise condition and FailedToReconcile condition in it --- api/v1beta1/tortoise_types.go | 36 +++++ .../autoscaling.mercari.com_tortoises.yaml | 38 +++++ controllers/tortoise_controller.go | 6 +- ...ion_tortoises.autoscaling.mercari.com.yaml | 33 +++++ ...ion_tortoises.autoscaling.mercari.com.yaml | 33 +++++ pkg/tortoise/tortoise.go | 40 +++++ pkg/tortoise/tortoise_test.go | 137 +++++++++++++++++- 7 files changed, 319 insertions(+), 4 deletions(-) diff --git a/api/v1beta1/tortoise_types.go b/api/v1beta1/tortoise_types.go index df845693..40157fb4 100644 --- a/api/v1beta1/tortoise_types.go +++ b/api/v1beta1/tortoise_types.go @@ -247,10 +247,46 @@ type HPATargetUtilizationRecommendationPerContainer struct { } type Conditions struct { + // TortoiseConditions is the condition of this tortoise. + // +patchMergeKey=type + // +patchStrategy=merge + // +listType=map + // +listMapKey=type + // +optional + TortoiseConditions []TortoiseCondition `json:"tortoiseConditions" protobuf:"bytes,1,name=tortoiseConditions"` + // ContainerRecommendationFromVPA is the condition of container recommendation from VPA, which is observed last time. // +optional ContainerRecommendationFromVPA []ContainerRecommendationFromVPA `json:"containerRecommendationFromVPA,omitempty" protobuf:"bytes,1,opt,name=containerRecommendationFromVPA"` } +// TortoiseConditionType are the valid conditions of a Tortoise. +type TortoiseConditionType string + +const ( + // TortoiseConditionTypeFailedToReconcile means tortoise failed to reconcile due to some reasons. + TortoiseConditionTypeFailedToReconcile TortoiseConditionType = "FailedToReconcile" +) + +type TortoiseCondition struct { + // Type is the type of the condition. + Type TortoiseConditionType `json:"type" protobuf:"bytes,1,name=type"` + // Status is the status of the condition. (True, False, Unknown) + Status v1.ConditionStatus `json:"status" protobuf:"bytes,2,name=status"` + // The last time this condition was updated. + LastUpdateTime metav1.Time `json:"lastUpdateTime,omitempty" protobuf:"bytes,6,opt,name=lastUpdateTime"` + // lastTransitionTime is the last time the condition transitioned from + // one status to another + // +optional + LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty" protobuf:"bytes,3,opt,name=lastTransitionTime"` + // reason is the reason for the condition's last transition. + // +optional + Reason string `json:"reason,omitempty" protobuf:"bytes,4,opt,name=reason"` + // message is a human-readable explanation containing details about + // the transition + // +optional + Message string `json:"message,omitempty" protobuf:"bytes,5,opt,name=message"` +} + type ContainerRecommendationFromVPA struct { // ContainerName is the name of target container. ContainerName string `json:"containerName" protobuf:"bytes,1,name=containerName"` diff --git a/config/crd/bases/autoscaling.mercari.com_tortoises.yaml b/config/crd/bases/autoscaling.mercari.com_tortoises.yaml index 83f40740..e67dbf3c 100644 --- a/config/crd/bases/autoscaling.mercari.com_tortoises.yaml +++ b/config/crd/bases/autoscaling.mercari.com_tortoises.yaml @@ -501,6 +501,8 @@ spec: conditions: properties: containerRecommendationFromVPA: + description: ContainerRecommendationFromVPA is the condition of + container recommendation from VPA, which is observed last time. items: properties: containerName: @@ -545,6 +547,42 @@ spec: - recommendation type: object type: array + tortoiseConditions: + description: TortoiseConditions is the condition of this tortoise. + items: + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition + transitioned from one status to another + format: date-time + type: string + lastUpdateTime: + description: The last time this condition was updated. + format: date-time + type: string + message: + description: message is a human-readable explanation containing + details about the transition + type: string + reason: + description: reason is the reason for the condition's last + transition. + type: string + status: + description: Status is the status of the condition. (True, + False, Unknown) + type: string + type: + description: Type is the type of the condition. + type: string + required: + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map type: object recommendations: properties: diff --git a/controllers/tortoise_controller.go b/controllers/tortoise_controller.go index d114c80b..df5856f8 100644 --- a/controllers/tortoise_controller.go +++ b/controllers/tortoise_controller.go @@ -84,8 +84,10 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ return ctrl.Result{}, err } defer func() { - if reterr != nil { - r.EventRecorder.Event(tortoise, "Warning", "ReconcileError", reterr.Error()) + tortoise = r.TortoiseService.RecordReconciliationFailure(tortoise, reterr, now) + _, err = r.TortoiseService.UpdateTortoiseStatus(ctx, tortoise, now) + if err != nil { + logger.Error(err, "update Tortoise status", "tortoise", req.NamespacedName) } }() diff --git a/manifests/crd/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml b/manifests/crd/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml index 72773184..d5c480ec 100644 --- a/manifests/crd/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml +++ b/manifests/crd/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml @@ -399,6 +399,7 @@ spec: conditions: properties: containerRecommendationFromVPA: + description: ContainerRecommendationFromVPA is the condition of container recommendation from VPA, which is observed last time. items: properties: containerName: @@ -440,6 +441,38 @@ spec: - recommendation type: object type: array + tortoiseConditions: + description: TortoiseConditions is the condition of this tortoise. + items: + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition transitioned from one status to another + format: date-time + type: string + lastUpdateTime: + description: The last time this condition was updated. + format: date-time + type: string + message: + description: message is a human-readable explanation containing details about the transition + type: string + reason: + description: reason is the reason for the condition's last transition. + type: string + status: + description: Status is the status of the condition. (True, False, Unknown) + type: string + type: + description: Type is the type of the condition. + type: string + required: + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map type: object recommendations: properties: diff --git a/manifests/default/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml b/manifests/default/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml index 7f470f1f..defcf2c1 100644 --- a/manifests/default/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml +++ b/manifests/default/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml @@ -399,6 +399,7 @@ spec: conditions: properties: containerRecommendationFromVPA: + description: ContainerRecommendationFromVPA is the condition of container recommendation from VPA, which is observed last time. items: properties: containerName: @@ -440,6 +441,38 @@ spec: - recommendation type: object type: array + tortoiseConditions: + description: TortoiseConditions is the condition of this tortoise. + items: + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition transitioned from one status to another + format: date-time + type: string + lastUpdateTime: + description: The last time this condition was updated. + format: date-time + type: string + message: + description: message is a human-readable explanation containing details about the transition + type: string + reason: + description: reason is the reason for the condition's last transition. + type: string + status: + description: Status is the status of the condition. (True, False, Unknown) + type: string + type: + description: Type is the type of the condition. + type: string + required: + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map type: object recommendations: properties: diff --git a/pkg/tortoise/tortoise.go b/pkg/tortoise/tortoise.go index 4e9c02f7..1d088044 100644 --- a/pkg/tortoise/tortoise.go +++ b/pkg/tortoise/tortoise.go @@ -320,3 +320,43 @@ func (s *Service) updateLastTimeUpdateTortoise(tortoise *v1beta1.Tortoise, now t s.lastTimeUpdateTortoise[client.ObjectKeyFromObject(tortoise)] = now } + +func (s *Service) RecordReconciliationFailure(t *v1beta1.Tortoise, err error, now time.Time) *v1beta1.Tortoise { + if err != nil { + s.recorder.Event(t, "Warning", "ReconcileError", err.Error()) + for i := range t.Status.Conditions.TortoiseConditions { + if t.Status.Conditions.TortoiseConditions[i].Type == v1beta1.TortoiseConditionTypeFailedToReconcile { + // TODO: have a clear reason and utilize it to have a better reconciliation next. + // For example, in some cases, the reconciliation may keep failing until people fix some problems manually. + t.Status.Conditions.TortoiseConditions[i].Reason = "ReconcileError" + t.Status.Conditions.TortoiseConditions[i].Message = err.Error() + t.Status.Conditions.TortoiseConditions[i].Status = corev1.ConditionTrue + t.Status.Conditions.TortoiseConditions[i].LastTransitionTime = metav1.NewTime(now) + t.Status.Conditions.TortoiseConditions[i].LastUpdateTime = metav1.NewTime(now) + return t + } + } + // add as a new condition if not found. + t.Status.Conditions.TortoiseConditions = append(t.Status.Conditions.TortoiseConditions, v1beta1.TortoiseCondition{ + Type: v1beta1.TortoiseConditionTypeFailedToReconcile, + Status: corev1.ConditionTrue, + Reason: "ReconcileError", + Message: err.Error(), + LastTransitionTime: metav1.NewTime(now), + LastUpdateTime: metav1.NewTime(now), + }) + return t + } + + for i := range t.Status.Conditions.TortoiseConditions { + if t.Status.Conditions.TortoiseConditions[i].Type == v1beta1.TortoiseConditionTypeFailedToReconcile { + t.Status.Conditions.TortoiseConditions[i].Reason = "" + t.Status.Conditions.TortoiseConditions[i].Message = "" + t.Status.Conditions.TortoiseConditions[i].Status = corev1.ConditionFalse + t.Status.Conditions.TortoiseConditions[i].LastTransitionTime = metav1.NewTime(now) + t.Status.Conditions.TortoiseConditions[i].LastUpdateTime = metav1.NewTime(now) + return t + } + } + return t +} diff --git a/pkg/tortoise/tortoise_test.go b/pkg/tortoise/tortoise_test.go index 974aa132..bee58fe9 100644 --- a/pkg/tortoise/tortoise_test.go +++ b/pkg/tortoise/tortoise_test.go @@ -2,22 +2,24 @@ package tortoise import ( "context" + "errors" + "reflect" "testing" "time" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/mercari/tortoise/api/v1beta1" appv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" v1 "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" + "k8s.io/client-go/tools/record" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - - "github.com/mercari/tortoise/api/v1beta1" ) func TestService_updateUpperRecommendation(t *testing.T) { @@ -775,3 +777,134 @@ func TestService_UpdateTortoiseStatus(t *testing.T) { }) } } + +func TestService_RecordReconciliationFailure(t *testing.T) { + now := time.Now() + type args struct { + t *v1beta1.Tortoise + err error + } + tests := []struct { + name string + args args + want *v1beta1.Tortoise + }{ + { + name: "success reconciliation", + args: args{ + t: &v1beta1.Tortoise{ + Status: v1beta1.TortoiseStatus{ + Conditions: v1beta1.Conditions{ + TortoiseConditions: []v1beta1.TortoiseCondition{ + { + Type: v1beta1.TortoiseConditionTypeFailedToReconcile, + Status: corev1.ConditionTrue, + Message: "failed to reconcile", + Reason: "ReconcileError", + LastUpdateTime: metav1.NewTime(now.Add(-1 * time.Minute)), + LastTransitionTime: metav1.NewTime(now.Add(-1 * time.Minute)), + }, + }, + }, + }, + }, + err: nil, + }, + want: &v1beta1.Tortoise{ + Status: v1beta1.TortoiseStatus{ + Conditions: v1beta1.Conditions{ + TortoiseConditions: []v1beta1.TortoiseCondition{ + { + Type: v1beta1.TortoiseConditionTypeFailedToReconcile, + Status: corev1.ConditionFalse, + Message: "", + Reason: "", + LastUpdateTime: metav1.NewTime(now), + LastTransitionTime: metav1.NewTime(now), + }, + }, + }, + }, + }, + }, + { + name: "failed reconciliation and tortoise doens't have TortoiseConditionTypeFailedToReconcile", + args: args{ + t: &v1beta1.Tortoise{ + Status: v1beta1.TortoiseStatus{ + Conditions: v1beta1.Conditions{ + TortoiseConditions: []v1beta1.TortoiseCondition{ + // TortoiseConditionTypeFailedToReconcile isn't recorded yet. + }, + }, + }, + }, + err: errors.New("failed to reconcile"), + }, + want: &v1beta1.Tortoise{ + Status: v1beta1.TortoiseStatus{ + Conditions: v1beta1.Conditions{ + TortoiseConditions: []v1beta1.TortoiseCondition{ + { + Type: v1beta1.TortoiseConditionTypeFailedToReconcile, + Status: corev1.ConditionTrue, + Message: "failed to reconcile", + Reason: "ReconcileError", + LastUpdateTime: metav1.NewTime(now), + LastTransitionTime: metav1.NewTime(now), + }, + }, + }, + }, + }, + }, + { + name: "failed reconciliation and tortoise has TortoiseConditionTypeFailedToReconcile", + args: args{ + t: &v1beta1.Tortoise{ + Status: v1beta1.TortoiseStatus{ + Conditions: v1beta1.Conditions{ + TortoiseConditions: []v1beta1.TortoiseCondition{ + { + Type: v1beta1.TortoiseConditionTypeFailedToReconcile, + Status: corev1.ConditionFalse, + Message: "", + Reason: "", + LastUpdateTime: metav1.NewTime(now.Add(-1 * time.Minute)), + LastTransitionTime: metav1.NewTime(now.Add(-1 * time.Minute)), + }, + }, + }, + }, + }, + err: errors.New("failed to reconcile"), + }, + want: &v1beta1.Tortoise{ + Status: v1beta1.TortoiseStatus{ + Conditions: v1beta1.Conditions{ + TortoiseConditions: []v1beta1.TortoiseCondition{ + { + Type: v1beta1.TortoiseConditionTypeFailedToReconcile, + Status: corev1.ConditionTrue, + Message: "failed to reconcile", + Reason: "ReconcileError", + LastUpdateTime: metav1.NewTime(now), + LastTransitionTime: metav1.NewTime(now), + }, + }, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &Service{ + recorder: record.NewFakeRecorder(10), + } + if got := s.RecordReconciliationFailure(tt.args.t, tt.args.err, now); !reflect.DeepEqual(got, tt.want) { + t.Errorf("Service.RecordReconciliationFailure() = %v, want %v", got, tt.want) + } + }) + } +}