From 431eb4d4068935253e33232c332337a79307ba2c Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Fri, 17 Feb 2023 19:54:35 +0000 Subject: [PATCH] code review --- ...nfig.porch.kpt.dev_packagevariantsets.yaml | 71 ++++++++++++++++++- .../api/v1alpha1/packagevariantset_types.go | 4 +- .../api/v1alpha1/zz_generated.deepcopy.go | 10 +-- .../packagevariantset/fake_client.go | 27 +++++-- .../packagevariantset_controller.go | 54 ++++++++++---- .../packagevariantset_controller_test.go | 12 ++-- 6 files changed, 142 insertions(+), 36 deletions(-) diff --git a/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariantsets.yaml b/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariantsets.yaml index 76f9f57df8..1757dcc0dc 100644 --- a/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariantsets.yaml +++ b/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariantsets.yaml @@ -245,10 +245,75 @@ spec: status: description: PackageVariantSetStatus defines the observed state of PackageVariantSet properties: - validationErrors: - description: 'TODO: Move this to conditions.' + conditions: + description: Conditions describes the reconciliation state of the + object. items: - type: string + description: "Condition contains details for one aspect of the current + state of this API Resource. --- This struct is intended for direct + use as an array at the field path .status.conditions. For example, + \n type FooStatus struct{ // Represents the observations of a + foo's current state. // Known .status.conditions.type are: \"Available\", + \"Progressing\", and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge + // +listType=map // +listMapKey=type Conditions []metav1.Condition + `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\" + protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields }" + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition + transitioned from one status to another. This should be when + the underlying condition changed. If that is not known, then + using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: message is a human readable message indicating + details about the transition. This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: observedGeneration represents the .metadata.generation + that the condition was set based upon. For instance, if .metadata.generation + is currently 12, but the .status.conditions[x].observedGeneration + is 9, the condition is out of date with respect to the current + state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: reason contains a programmatic identifier indicating + the reason for the condition's last transition. Producers + of specific condition types may define expected values and + meanings for this field, and whether the values are considered + a guaranteed API. The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + --- Many .condition.type values are consistent across resources + like Available, but because arbitrary conditions can be useful + (see .node.status.conditions), the ability to deconflict is + important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object type: array type: object type: object diff --git a/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go b/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go index 0be7604a0a..45260bcd35 100644 --- a/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go +++ b/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go @@ -132,8 +132,8 @@ type ValueOrFromField struct { // PackageVariantSetStatus defines the observed state of PackageVariantSet type PackageVariantSetStatus struct { - // TODO: Move this to conditions. - ValidationErrors []string `json:"validationErrors,omitempty"` + // Conditions describes the reconciliation state of the object. + Conditions []metav1.Condition `json:"conditions,omitempty"` } //+kubebuilder:object:root=true diff --git a/porch/controllers/packagevariantsets/api/v1alpha1/zz_generated.deepcopy.go b/porch/controllers/packagevariantsets/api/v1alpha1/zz_generated.deepcopy.go index 785b7ec9f0..b880c46947 100644 --- a/porch/controllers/packagevariantsets/api/v1alpha1/zz_generated.deepcopy.go +++ b/porch/controllers/packagevariantsets/api/v1alpha1/zz_generated.deepcopy.go @@ -199,10 +199,12 @@ func (in *PackageVariantSetSpec) DeepCopy() *PackageVariantSetSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PackageVariantSetStatus) DeepCopyInto(out *PackageVariantSetStatus) { *out = *in - if in.ValidationErrors != nil { - in, out := &in.ValidationErrors, &out.ValidationErrors - *out = make([]string, len(*in)) - copy(*out, *in) + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]v1.Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } } diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go index 8114b30c18..4ea508b9b8 100644 --- a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go @@ -25,24 +25,29 @@ import ( ) type fakeClient struct { - output []string + objects []client.Object client.Client } var _ client.Client = &fakeClient{} func (f *fakeClient) Create(_ context.Context, obj client.Object, _ ...client.CreateOption) error { - f.output = append(f.output, fmt.Sprintf("creating object: %s", obj.GetName())) + f.objects = append(f.objects, obj) return nil } func (f *fakeClient) Delete(_ context.Context, obj client.Object, _ ...client.DeleteOption) error { - f.output = append(f.output, fmt.Sprintf("deleting object: %s", obj.GetName())) + var newObjects []client.Object + for _, old := range f.objects { + if obj.GetName() != old.GetName() { + newObjects = append(newObjects, old) + } + } + f.objects = newObjects return nil } func (f *fakeClient) List(_ context.Context, obj client.ObjectList, _ ...client.ListOption) error { - f.output = append(f.output, fmt.Sprintf("listing objects")) podList := `apiVersion: v1 kind: PodList metadata: @@ -93,12 +98,20 @@ items: repo: dn-2 package: dn-2` + var err error switch v := obj.(type) { case *unstructured.UnstructuredList: - return yaml.Unmarshal([]byte(podList), v) + err = yaml.Unmarshal([]byte(podList), v) + for _, o := range v.Items { + f.objects = append(f.objects, o.DeepCopy()) + } case *pkgvarapi.PackageVariantList: - return yaml.Unmarshal([]byte(pvList), v) + err = yaml.Unmarshal([]byte(pvList), v) + for _, o := range v.Items { + f.objects = append(f.objects, o.DeepCopy()) + } default: - return nil + return fmt.Errorf("unsupported type") } + return err } diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go index c3362dfa9c..9d122661cb 100644 --- a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go @@ -38,6 +38,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -80,13 +81,21 @@ func (r *PackageVariantSetReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{}, nil } + var conditions []metav1.Condition + if errs := validatePackageVariantSet(pvs); len(errs) > 0 { - pvs.Status.ValidationErrors = nil for _, validationErr := range errs { if validationErr.Error() != "" { - pvs.Status.ValidationErrors = append(pvs.Status.ValidationErrors, validationErr.Error()) + conditions = append(conditions, metav1.Condition{ + Type: "Valid", + Status: "False", + Reason: "ValidationFailed", + Message: validationErr.Error(), + LastTransitionTime: metav1.Now(), + }) } } + pvs.Status.Conditions = conditions statusUpdateErr := r.Client.Status().Update(ctx, pvs) return ctrl.Result{}, statusUpdateErr } @@ -102,7 +111,20 @@ func (r *PackageVariantSetReconciler) Reconcile(ctx context.Context, req ctrl.Re downstreams, err := r.unrollDownstreamTargets(ctx, pvs, upstream.Package) if err != nil { - return ctrl.Result{}, err + if strings.Contains(err.Error(), "unable to list specified objects in cluster") { + conditions = append(conditions, metav1.Condition{ + Type: "ObjectsListed", + Status: "False", + Reason: "Error", + Message: err.Error(), + LastTransitionTime: metav1.Now(), + }) + pvs.Status.Conditions = conditions + statusUpdateErr := r.Client.Status().Update(ctx, pvs) + return ctrl.Result{}, statusUpdateErr + } else { + return ctrl.Result{}, err + } } if err := r.ensurePackageVariants(ctx, upstream, downstreams, pvs); err != nil { @@ -231,7 +253,7 @@ func (r *PackageVariantSetReconciler) unrollDownstreamTargets(ctx context.Contex pvs *api.PackageVariantSet, upstreamPackageName string) ([]*pkgvarapi.Downstream, error) { var result []*pkgvarapi.Downstream - for _, target := range pvs.Spec.Targets { + for i, target := range pvs.Spec.Targets { switch { case target.Package != nil: // an explicit repo/package name pair @@ -249,6 +271,9 @@ func (r *PackageVariantSetReconciler) unrollDownstreamTargets(ctx context.Contex client.MatchingLabelsSelector{Selector: selector}); err != nil { return nil, err } + if len(repoList.Items) == 0 { + klog.Warningf("no repositories selected by spec.targets[%d]", i) + } pkgs, err := r.repositorySet(&target, upstreamPackageName, &repoList) if err != nil { return nil, fmt.Errorf("error when selecting repository set: %v", err) @@ -261,6 +286,9 @@ func (r *PackageVariantSetReconciler) unrollDownstreamTargets(ctx context.Contex if err != nil { return nil, err } + if len(selectedObjects) == 0 { + klog.Warningf("no objects selected by spec.targets[%d]", i) + } pkgs, err := r.objectSet(&target, upstreamPackageName, selectedObjects) if err != nil { return nil, fmt.Errorf("error when selecting object set: %v", err) @@ -343,15 +371,16 @@ func (r *PackageVariantSetReconciler) getSelectedObjects(ctx context.Context, se Kind: selector.Kind, }) - labelSelector, err := metav1.LabelSelectorAsSelector(selector.Labels) - if err != nil { - return nil, err + opts := []client.ListOption{client.InNamespace(selector.Namespace)} + if selector.Labels != nil { + labelSelector, err := metav1.LabelSelectorAsSelector(selector.Labels) + if err != nil { + return nil, err + } + opts = append(opts, client.MatchingLabelsSelector{Selector: labelSelector}) } - - if err := r.Client.List(ctx, uList, - client.InNamespace(selector.Namespace), - client.MatchingLabelsSelector{Selector: labelSelector}); err != nil { - return nil, fmt.Errorf("unable to list objects in cluster: %v", err) + if err := r.Client.List(ctx, uList, opts...); err != nil { + return nil, fmt.Errorf("unable to list specified objects in cluster: %v", err) } for _, u := range uList.Items { @@ -446,7 +475,6 @@ func (r *PackageVariantSetReconciler) fetchValue(value *api.ValueOrFromField, func (r *PackageVariantSetReconciler) ensurePackageVariants(ctx context.Context, upstream *pkgvarapi.Upstream, downstreams []*pkgvarapi.Downstream, pvs *api.PackageVariantSet) error { - var pvList pkgvarapi.PackageVariantList if err := r.Client.List(ctx, &pvList, client.InNamespace(pvs.Namespace), diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go index dd659ccb5d..aea9de00fc 100644 --- a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go @@ -353,13 +353,11 @@ func TestEnsurePackageVariants(t *testing.T) { {Repo: "dn-1", Package: "dn-1"}, {Repo: "dn-3", Package: "dn-3"}, } - pvs := &api.PackageVariantSet{ObjectMeta: metav1.ObjectMeta{Name: "my-pvs"}} - fc := &fakeClient{} reconciler := &PackageVariantSetReconciler{Client: fc} - require.NoError(t, reconciler.ensurePackageVariants(context.Background(), upstream, downstreams, pvs)) - require.Equal(t, []string{"listing objects", - "deleting object: my-pv-2", - "creating object: my-pvs-59bfcf77a6b032a656d78b14e2d92fa8c1e978a3", - }, fc.output) + require.NoError(t, reconciler.ensurePackageVariants(context.Background(), upstream, downstreams, + &api.PackageVariantSet{ObjectMeta: metav1.ObjectMeta{Name: "my-pvs"}})) + require.Equal(t, 2, len(fc.objects)) + require.Equal(t, "my-pv-1", fc.objects[0].GetName()) + require.Equal(t, "my-pvs-28ace69e71f644931cd8cc1e8e9388f4de486901", fc.objects[1].GetName()) }