From d966c16a068edd3ced12c72a9f68057fffd31887 Mon Sep 17 00:00:00 2001 From: stefanprodan Date: Tue, 11 Aug 2020 13:12:01 +0300 Subject: [PATCH 1/3] Implement Ready condition for image repos --- api/v1alpha1/condition_types.go | 67 +++++++++++++++ api/v1alpha1/imagerepository_types.go | 46 +++++++++-- api/v1alpha1/zz_generated.deepcopy.go | 25 +++++- ...e.toolkit.fluxcd.io_imagerepositories.yaml | 46 ++++++++--- controllers/imagerepository_controller.go | 81 +++++++++++++------ 5 files changed, 218 insertions(+), 47 deletions(-) create mode 100644 api/v1alpha1/condition_types.go diff --git a/api/v1alpha1/condition_types.go b/api/v1alpha1/condition_types.go new file mode 100644 index 0000000..ca7150a --- /dev/null +++ b/api/v1alpha1/condition_types.go @@ -0,0 +1,67 @@ +/* +Copyright 2020 The Flux CD contributors. + +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 v1alpha1 + +import ( + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// Condition contains condition information for a toolkit resource. +type Condition struct { + // Type of the condition, currently ('Ready'). + // +required + Type string `json:"type"` + + // Status of the condition, one of ('True', 'False', 'Unknown'). + // +required + Status corev1.ConditionStatus `json:"status"` + + // LastTransitionTime is the timestamp corresponding to the last status + // change of this condition. + // +required + LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty"` + + // Reason is a brief machine readable explanation for the condition's last + // transition. + // +required + Reason string `json:"reason,omitempty"` + + // Message is a human readable description of the details of the last + // transition, complementing reason. + // +optional + Message string `json:"message,omitempty"` +} + +const ( + // ReadyCondition records the last reconciliation result. + ReadyCondition string = "Ready" +) + +const ( + // ReconciliationSucceededReason represents the fact that the reconciliation of the resource has succeeded. + ReconciliationSucceededReason string = "ReconciliationSucceeded" + + // ReconciliationFailedReason represents the fact that the reconciliation of the resource has failed. + ReconciliationFailedReason string = "ReconciliationFailed" + + // ProgressingReason represents the fact that a reconciliation is underway. + ProgressingReason string = "Progressing" + + // SuspendedReason represents the fact that the reconciliation is suspended. + SuspendedReason string = "Suspended" +) diff --git a/api/v1alpha1/imagerepository_types.go b/api/v1alpha1/imagerepository_types.go index 40280af..bd12927 100644 --- a/api/v1alpha1/imagerepository_types.go +++ b/api/v1alpha1/imagerepository_types.go @@ -17,6 +17,7 @@ limitations under the License. package v1alpha1 import ( + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -40,18 +41,47 @@ type ScanResult struct { // ImageRepositoryStatus defines the observed state of ImageRepository type ImageRepositoryStatus struct { + // +optional + Conditions []Condition `json:"conditions,omitempty"` + + // ObservedGeneration is the last reconciled generation. + // +optional + ObservedGeneration int64 `json:"observedGeneration,omitempty"` + // CannonicalName is the name of the image repository with all the // implied bits made explicit; e.g., `docker.io/library/alpine` // rather than `alpine`. - CanonicalImageName string `json:"canonicalImageName,omitempty"` - // LastError is the error from last reconciliation, or empty if - // reconciliation was successful. - LastError string `json:"lastError"` - // LastScanTime records the last time the repository was - // successfully scanned. // +optional - LastScanTime *metav1.Time `json:"lastScanTime,omitempty"` - LastScanResult ScanResult `json:"lastScanResult,omitempty"` + CanonicalImageName string `json:"canonicalImageName,omitempty"` + + // LastScanResult contains the number of fetched tags. + // +optional + LastScanResult ScanResult `json:"lastScanResult,omitempty"` +} + +// SetImageRepositoryReadiness sets the ready condition with the given status, reason and message. +func SetImageRepositoryReadiness(ir ImageRepository, status corev1.ConditionStatus, reason, message string) ImageRepository { + ir.Status.Conditions = []Condition{ + { + Type: ReadyCondition, + Status: status, + LastTransitionTime: metav1.Now(), + Reason: reason, + Message: message, + }, + } + ir.Status.ObservedGeneration = ir.ObjectMeta.Generation + return ir +} + +func GetLastTransitionTime(ir ImageRepository) *metav1.Time { + for _, condition := range ir.Status.Conditions { + if condition.Type == ReadyCondition { + return &condition.LastTransitionTime + } + } + + return nil } // +kubebuilder:object:root=true diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index c496bf9..4ac1d67 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -25,6 +25,22 @@ import ( runtime "k8s.io/apimachinery/pkg/runtime" ) +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Condition) DeepCopyInto(out *Condition) { + *out = *in + in.LastTransitionTime.DeepCopyInto(&out.LastTransitionTime) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Condition. +func (in *Condition) DeepCopy() *Condition { + if in == nil { + return nil + } + out := new(Condition) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ImagePolicy) DeepCopyInto(out *ImagePolicy) { *out = *in @@ -218,9 +234,12 @@ func (in *ImageRepositorySpec) DeepCopy() *ImageRepositorySpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ImageRepositoryStatus) DeepCopyInto(out *ImageRepositoryStatus) { *out = *in - if in.LastScanTime != nil { - in, out := &in.LastScanTime, &out.LastScanTime - *out = (*in).DeepCopy() + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } out.LastScanResult = in.LastScanResult } diff --git a/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml b/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml index 153a774..329d6d3 100644 --- a/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml +++ b/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml @@ -60,24 +60,48 @@ spec: all the implied bits made explicit; e.g., `docker.io/library/alpine` rather than `alpine`. type: string - lastError: - description: LastError is the error from last reconciliation, or empty - if reconciliation was successful. - type: string + conditions: + items: + description: Condition contains condition information for a toolkit + resource. + properties: + lastTransitionTime: + description: LastTransitionTime is the timestamp corresponding + to the last status change of this condition. + format: date-time + type: string + message: + description: Message is a human readable description of the + details of the last transition, complementing reason. + type: string + reason: + description: Reason is a brief machine readable explanation + for the condition's last transition. + type: string + status: + description: Status of the condition, one of ('True', 'False', + 'Unknown'). + type: string + type: + description: Type of the condition, currently ('Ready'). + type: string + required: + - status + - type + type: object + type: array lastScanResult: + description: LastScanResult contains the number of fetched tags. properties: tagCount: type: integer required: - tagCount type: object - lastScanTime: - description: LastScanTime records the last time the repository was - successfully scanned. - format: date-time - type: string - required: - - lastError + observedGeneration: + description: ObservedGeneration is the last reconciled generation. + format: int64 + type: integer type: object type: object served: true diff --git a/controllers/imagerepository_controller.go b/controllers/imagerepository_controller.go index 757e705..23ef5dd 100644 --- a/controllers/imagerepository_controller.go +++ b/controllers/imagerepository_controller.go @@ -18,13 +18,14 @@ package controllers import ( "context" + "fmt" "strings" "time" "github.com/go-logr/logr" "github.com/google/go-containerregistry/pkg/name" "github.com/google/go-containerregistry/pkg/v1/remote" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" kuberecorder "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" @@ -72,55 +73,85 @@ func (r *ImageRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, er ref, err := name.ParseReference(imageRepo.Spec.Image) if err != nil { - imageRepo.Status.LastError = err.Error() - if err := r.Status().Update(ctx, &imageRepo); err != nil { - return ctrl.Result{}, err + status := imagev1alpha1.SetImageRepositoryReadiness( + imageRepo, + corev1.ConditionFalse, + imagev1alpha1.ReconciliationFailedReason, + err.Error(), + ) + if err := r.Status().Update(ctx, &status); err != nil { + return ctrl.Result{Requeue: true}, err } log.Error(err, "Unable to parse image name", "imageName", imageRepo.Spec.Image) - return ctrl.Result{}, nil + return ctrl.Result{}, err } - canonicalName := ref.Context().String() - imageRepo.Status.CanonicalImageName = canonicalName + imageRepo.Status.CanonicalImageName = ref.Context().String() now := time.Now() - ok, when := r.shouldScan(&imageRepo, now) + ok, when := r.shouldScan(imageRepo, now) if ok { ctx, cancel := context.WithTimeout(ctx, scanTimeout) defer cancel() - tags, err := remote.ListWithContext(ctx, ref.Context()) // TODO: auth - if err != nil { - imageRepo.Status.LastError = err.Error() - if err = r.Status().Update(ctx, &imageRepo); err != nil { - return ctrl.Result{}, err - } + + reconciledRepo, reconcileErr := r.scan(ctx, imageRepo, ref) + if err = r.Status().Update(ctx, &reconciledRepo); err != nil { + return ctrl.Result{Requeue: true}, err } - imageRepo.Status.LastScanTime = &metav1.Time{Time: now} - imageRepo.Status.LastScanResult.TagCount = len(tags) - imageRepo.Status.LastError = "" - // share the information in the database - r.Database.SetTags(canonicalName, tags) - log.Info("successful scan", "tag count", len(tags)) - if err = r.Status().Update(ctx, &imageRepo); err != nil { - return ctrl.Result{}, err + if reconcileErr != nil { + return ctrl.Result{RequeueAfter: when}, reconcileErr + } else { + log.Info(fmt.Sprintf("reconciliation finished in %s, next run in %s", + time.Now().Sub(now).String(), + when), + ) } } + return ctrl.Result{RequeueAfter: when}, nil } +func (r *ImageRepositoryReconciler) scan(ctx context.Context, imageRepo imagev1alpha1.ImageRepository, ref name.Reference) (imagev1alpha1.ImageRepository, error) { + canonicalName := ref.Context().String() + + // TODO: implement auth + tags, err := remote.ListWithContext(ctx, ref.Context()) + if err != nil { + return imagev1alpha1.SetImageRepositoryReadiness( + imageRepo, + corev1.ConditionFalse, + imagev1alpha1.ReconciliationFailedReason, + err.Error(), + ), err + } + + // TODO: add context and error handling to database ops + r.Database.SetTags(canonicalName, tags) + + imageRepo.Status.LastScanResult.TagCount = len(tags) + return imagev1alpha1.SetImageRepositoryReadiness( + imageRepo, + corev1.ConditionTrue, + imagev1alpha1.ReconciliationSucceededReason, + fmt.Sprintf("successful scan, found %v tags", len(tags)), + ), nil +} + // shouldScan takes an image repo and the time now, and says whether // the repository should be scanned now, and how long to wait for the // next scan. -func (r *ImageRepositoryReconciler) shouldScan(repo *imagev1alpha1.ImageRepository, now time.Time) (bool, time.Duration) { +func (r *ImageRepositoryReconciler) shouldScan(repo imagev1alpha1.ImageRepository, now time.Time) (bool, time.Duration) { scanInterval := defaultScanInterval if repo.Spec.ScanInterval != nil { scanInterval = repo.Spec.ScanInterval.Duration } - if repo.Status.LastScanTime == nil { + lastTransitionTime := imagev1alpha1.GetLastTransitionTime(repo) + if lastTransitionTime == nil { return true, scanInterval } + // when recovering, it's possible that the resource has a last // scan time, but there's no records because the database has been // dropped and created again. @@ -132,7 +163,7 @@ func (r *ImageRepositoryReconciler) shouldScan(repo *imagev1alpha1.ImageReposito return true, scanInterval } - when := scanInterval - now.Sub(repo.Status.LastScanTime.Time) + when := scanInterval - now.Sub(lastTransitionTime.Time) if when < time.Second { return true, scanInterval } From 4a546eb97cfd9f4ab8c0b74d06d1b55f06a1faec Mon Sep 17 00:00:00 2001 From: stefanprodan Date: Thu, 13 Aug 2020 18:45:49 +0300 Subject: [PATCH 2/3] Add image URL invalid reason --- api/v1alpha1/condition_types.go | 3 +++ controllers/imagerepository_controller.go | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/api/v1alpha1/condition_types.go b/api/v1alpha1/condition_types.go index ca7150a..5783970 100644 --- a/api/v1alpha1/condition_types.go +++ b/api/v1alpha1/condition_types.go @@ -59,6 +59,9 @@ const ( // ReconciliationFailedReason represents the fact that the reconciliation of the resource has failed. ReconciliationFailedReason string = "ReconciliationFailed" + // ImageURLInvalidReason represents the fact that a given repository has an invalid image URL. + ImageURLInvalidReason string = "ImageURLInvalid" + // ProgressingReason represents the fact that a reconciliation is underway. ProgressingReason string = "Progressing" diff --git a/controllers/imagerepository_controller.go b/controllers/imagerepository_controller.go index 23ef5dd..e93bbc8 100644 --- a/controllers/imagerepository_controller.go +++ b/controllers/imagerepository_controller.go @@ -76,14 +76,14 @@ func (r *ImageRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, er status := imagev1alpha1.SetImageRepositoryReadiness( imageRepo, corev1.ConditionFalse, - imagev1alpha1.ReconciliationFailedReason, + imagev1alpha1.ImageURLInvalidReason, err.Error(), ) if err := r.Status().Update(ctx, &status); err != nil { return ctrl.Result{Requeue: true}, err } log.Error(err, "Unable to parse image name", "imageName", imageRepo.Spec.Image) - return ctrl.Result{}, err + return ctrl.Result{}, nil } imageRepo.Status.CanonicalImageName = ref.Context().String() @@ -100,7 +100,7 @@ func (r *ImageRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, er } if reconcileErr != nil { - return ctrl.Result{RequeueAfter: when}, reconcileErr + return ctrl.Result{Requeue: true}, reconcileErr } else { log.Info(fmt.Sprintf("reconciliation finished in %s, next run in %s", time.Now().Sub(now).String(), From 155f6e948de1fdba511fa7fc43e50c6d66714c75 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Thu, 20 Aug 2020 14:45:43 +0100 Subject: [PATCH 3/3] Be consistent about returning an error It's mooted in https://github.com/fluxcd/toolkit/discussions/164 that a distinct metric is used for not completing reconciliation, as opposed to an unexpected error. Until that discussion has run its course, we should just do what the other controllers do, and that's returning an error when the controller is unable to reconcile to completion. This also adds a comment noting the purpose of the redundant `Requeue: true` fields, for the avoidance of confusion later. --- controllers/imagerepository_controller.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/controllers/imagerepository_controller.go b/controllers/imagerepository_controller.go index e93bbc8..05837b1 100644 --- a/controllers/imagerepository_controller.go +++ b/controllers/imagerepository_controller.go @@ -64,8 +64,14 @@ type ImageRepositoryReconciler struct { func (r *ImageRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { ctx := context.Background() + // NB: In general, if an error is returned then controller-runtime + // will requeue the request with back-off. In the following this + // is usually made explicit by _also_ returning + // `ctrl.Result{Requeue: true}`. + var imageRepo imagev1alpha1.ImageRepository if err := r.Get(ctx, req.NamespacedName, &imageRepo); err != nil { + // _Might_ get requeued return ctrl.Result{}, client.IgnoreNotFound(err) } @@ -83,7 +89,7 @@ func (r *ImageRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, er return ctrl.Result{Requeue: true}, err } log.Error(err, "Unable to parse image name", "imageName", imageRepo.Spec.Image) - return ctrl.Result{}, nil + return ctrl.Result{Requeue: true}, err } imageRepo.Status.CanonicalImageName = ref.Context().String()