From 54e549707f86cec070a144f6489484df64af91a8 Mon Sep 17 00:00:00 2001 From: Istvan Kispal Date: Thu, 20 Jun 2024 22:01:48 +0200 Subject: [PATCH 1/2] Expose kpt package rendering errors via the status of PackageVariant. --- .../packagevariant_controller.go | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go b/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go index b20aaabb..10e38f05 100644 --- a/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go +++ b/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go @@ -16,6 +16,7 @@ package packagevariant import ( "context" + "encoding/json" "flag" "fmt" "strconv" @@ -345,10 +346,9 @@ func (r *PackageVariantReconciler) ensurePackageVariant(ctx context.Context, } if changed { // Save the updated PackageRevisionResources - if err = r.Update(ctx, prr); err != nil { + if err = r.updatePackageResources(ctx, prr, pv); err != nil { return nil, err } - klog.Infoln(fmt.Sprintf("package variant %q applied mutations to package revision %q", pv.Name, newPR.Name)) } return []*porchapi.PackageRevision{newPR}, nil @@ -427,10 +427,9 @@ func (r *PackageVariantReconciler) findAndUpdateExistingRevisions(ctx context.Co } // Save the updated PackageRevisionResources - if err := r.Update(ctx, prr); err != nil { + if err := r.updatePackageResources(ctx, prr, pv); err != nil { return nil, err } - klog.Infoln(fmt.Sprintf("package variant %q updated package revision %q for new mutations", pv.Name, downstream.Name)) } } return downstreams, nil @@ -1050,3 +1049,34 @@ func isPackageVariantFunc(fn *fn.SubObject, pvName string) (bool, error) { func generatePVFuncName(funcName, pvName string, pos int) string { return fmt.Sprintf("%s.%s.%s.%d", PackageVariantFuncPrefix, pvName, funcName, pos) } + +func (r *PackageVariantReconciler) updatePackageResources(ctx context.Context, prr *porchapi.PackageRevisionResources, pv *api.PackageVariant) error { + if err := r.Update(ctx, prr); err != nil { + return err + } + // save render status into a condition + // TODO: add renderStatus field to DownstreamTarget and use that instead + c := metav1.Condition{ + Type: prr.Name + "/Render", + } + details := "" + jsonBytes, err := json.Marshal(prr.Status.RenderStatus.Result) + if err != nil { + details = fmt.Sprintf("JSON marshal error: %v", err) + } else { + details = string(jsonBytes) + } + + if prr.Status.RenderStatus.Err != "" { + c.Status = "False" + c.Reason = "Error" + c.Message = fmt.Sprintf("%s, details: %s", prr.Status.RenderStatus.Err, details) + } else { + c.Status = "True" + c.Reason = "Success" + c.Message = details + } + meta.SetStatusCondition(&pv.Status.Conditions, c) + klog.Infoln(fmt.Sprintf("package variant %q applied mutations to package revision %q", pv.Name, prr.Name)) + return nil +} From aa7c67356297d5880308c33df1624bc790825d64 Mon Sep 17 00:00:00 2001 From: Istvan Kispal Date: Thu, 20 Jun 2024 22:01:48 +0200 Subject: [PATCH 2/2] Move downstream render status to the `downstreamTargets` field --- Makefile | 45 +++-- .../config.porch.kpt.dev_packagevariants.yaml | 156 ++++++++++++++++++ .../api/v1alpha1/packagevariant_types.go | 4 +- .../api/v1alpha1/zz_generated.deepcopy.go | 5 +- .../packagevariant_controller.go | 57 +++---- 5 files changed, 212 insertions(+), 55 deletions(-) diff --git a/Makefile b/Makefile index 693d4275..dc3e4e76 100644 --- a/Makefile +++ b/Makefile @@ -144,7 +144,7 @@ generate-api: KUBE_VERBOSE=2 $(CURDIR)/scripts/generate-api.sh .PHONY: generate -generate: generate-api +generate: generate-api ## Generate CRDs, other K8s manifests and helper go code @for f in $(MODULES); do (cd $$f; echo "Generating $$f"; go generate -v ./...) || exit 1; done .PHONY: tidy @@ -239,6 +239,27 @@ deploy: deployment-config .PHONY: push-and-deploy push-and-deploy: push-images deploy +.PHONY: run-in-kind +run-in-kind: IMAGE_REPO=porch-kind +run-in-kind: IMAGE_TAG=test +run-in-kind: load-images-to-kind deployment-config deploy-current-config ## Build and deploy porch into a kind cluster + +.PHONY: run-in-kind-no-server +run-in-kind-no-server: IMAGE_REPO=porch-kind +run-in-kind-no-server: IMAGE_TAG=test +run-in-kind-no-server: SKIP_PORCHSERVER_BUILD=true +run-in-kind-no-server: load-images-to-kind deployment-config-no-server deploy-current-config ## Build and deploy porch without the porch-server into a kind cluster + +.PHONY: run-in-kind-no-controller +run-in-kind-no-controller: IMAGE_REPO=porch-kind +run-in-kind-no-controller: IMAGE_TAG=test +run-in-kind-no-controller: SKIP_CONTROLLER_BUILD=true +run-in-kind-no-controller: load-images-to-kind deployment-config-no-controller deploy-current-config ## Build and deploy porch without the controllers into a kind cluster + +.PHONY: destroy +destroy: ## Deletes all porch resources installed by the last run-in-kind-* command + kpt live destroy $(DEPLOYPORCHCONFIGDIR) + .PHONY: deployment-config deployment-config: ## Generate a porch deployment kpt package into $(DEPLOYPORCHCONFIGDIR) rm -rf $(DEPLOYPORCHCONFIGDIR) || true @@ -308,28 +329,6 @@ deploy-current-config: ## Deploy the configuration that is currently in $(DEPLOY @kubectl rollout status deployment porch-server --namespace porch-system 2>/dev/null || true @echo "Done." -.PHONY: run-in-kind -run-in-kind: IMAGE_REPO=porch-kind -run-in-kind: IMAGE_TAG=test -run-in-kind: load-images-to-kind deployment-config deploy-current-config ## Build and deploy porch into a kind cluster - -.PHONY: run-in-kind-no-server -run-in-kind-no-server: IMAGE_REPO=porch-kind -run-in-kind-no-server: IMAGE_TAG=test -run-in-kind-no-server: SKIP_PORCHSERVER_BUILD=true -run-in-kind-no-server: load-images-to-kind deployment-config-no-server deploy-current-config ## Build and deploy porch without the porch-server into a kind cluster - -.PHONY: run-in-kind-no-controller -run-in-kind-no-controller: IMAGE_REPO=porch-kind -run-in-kind-no-controller: IMAGE_TAG=test -run-in-kind-no-controller: SKIP_CONTROLLER_BUILD=true -run-in-kind-no-controller: load-images-to-kind deployment-config-no-controller deploy-current-config ## Build and deploy porch without the controllers into a kind cluster - -.PHONY: destroy -destroy: ## Deletes all porch resources installed by the last run-in-kind-* command - kpt live destroy $(DEPLOYPORCHCONFIGDIR) - - PKG=gitea-dev .PHONY: deploy-gitea-dev-pkg deploy-gitea-dev-pkg: diff --git a/controllers/config/crd/bases/config.porch.kpt.dev_packagevariants.yaml b/controllers/config/crd/bases/config.porch.kpt.dev_packagevariants.yaml index e5e89180..7b6b6e64 100644 --- a/controllers/config/crd/bases/config.porch.kpt.dev_packagevariants.yaml +++ b/controllers/config/crd/bases/config.porch.kpt.dev_packagevariants.yaml @@ -416,6 +416,162 @@ spec: properties: name: type: string + renderStatus: + description: |- + RenderStatus represents the result of performing render operation + on a package resources. + properties: + error: + type: string + result: + description: ResultList contains aggregated results from + multiple functions + properties: + apiVersion: + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources + type: string + exitCode: + description: ExitCode is the exit code of kpt command + type: integer + items: + description: Items contain a list of function result + items: + description: Result contains the structured result + from an individual function + properties: + exec: + description: |- + ExecPath is the the absolute os-specific path to the executable file + If user provides an executable file with commands, ExecPath should + contain the entire input string. + type: string + exitCode: + description: ExitCode is the exit code from running + the function + type: integer + image: + description: |- + Image is the full name of the image that generates this result + Image and Exec are mutually exclusive + type: string + results: + description: Results is the list of results for + the function + items: + description: ResultItem defines a validation + result + properties: + field: + description: Field is a reference to the + field in a resource this result refers + to + properties: + currentValue: + description: CurrentValue is the current + field value + type: string + path: + description: Path is the field path. + This field is required. + type: string + proposedValue: + description: ProposedValue is the proposed + value of the field to fix an issue. + type: string + type: object + file: + description: File references a file containing + the resource this result refers to + properties: + index: + description: |- + Index is the index into the file containing the resource + (i.e. if there are multiple resources in a single file) + type: integer + path: + description: |- + Path is relative path to the file containing the resource. + This field is required. + type: string + type: object + message: + description: Message is a human readable + message. This field is required. + type: string + resourceRef: + description: |- + ResourceRef is a reference to a resource. + Required fields: apiVersion, kind, name. + properties: + apiVersion: + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources + type: string + kind: + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds + type: string + name: + description: Name is the metadata.name + field of a Resource + type: string + namespace: + description: Namespace is the metadata.namespace + field of a Resource + type: string + type: object + severity: + description: Severity is the severity of + this result + type: string + tags: + additionalProperties: + type: string + description: |- + Tags is an unstructured key value map stored with a result that may be set + by external tools to store and retrieve arbitrary metadata + type: object + type: object + type: array + stderr: + description: |- + TODO(droot): This is required for making structured results subpackage aware. + Enable this once test harness supports filepath based assertions. + Pkg is OS specific Absolute path to the package. + Pkg string `yaml:"pkg,omitempty"` + Stderr is the content in function stderr + type: string + required: + - exitCode + type: object + type: array + kind: + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds + type: string + metadata: + type: object + required: + - exitCode + type: object + required: + - error + type: object type: object type: array type: object diff --git a/controllers/packagevariants/api/v1alpha1/packagevariant_types.go b/controllers/packagevariants/api/v1alpha1/packagevariant_types.go index 9ee498a7..7b5562e7 100644 --- a/controllers/packagevariants/api/v1alpha1/packagevariant_types.go +++ b/controllers/packagevariants/api/v1alpha1/packagevariant_types.go @@ -15,6 +15,7 @@ package v1alpha1 import ( + porchapi "github.com/nephio-project/porch/api/porch/v1alpha1" kptfilev1 "github.com/nephio-project/porch/pkg/kpt/api/kptfile/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -107,7 +108,8 @@ type PackageVariantStatus struct { } type DownstreamTarget struct { - Name string `json:"name,omitempty"` + Name string `json:"name,omitempty"` + RenderStatus porchapi.RenderStatus `json:"renderStatus,omitempty"` } //+kubebuilder:object:root=true diff --git a/controllers/packagevariants/api/v1alpha1/zz_generated.deepcopy.go b/controllers/packagevariants/api/v1alpha1/zz_generated.deepcopy.go index a56ac380..a2846628 100644 --- a/controllers/packagevariants/api/v1alpha1/zz_generated.deepcopy.go +++ b/controllers/packagevariants/api/v1alpha1/zz_generated.deepcopy.go @@ -42,6 +42,7 @@ func (in *Downstream) DeepCopy() *Downstream { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DownstreamTarget) DeepCopyInto(out *DownstreamTarget) { *out = *in + in.RenderStatus.DeepCopyInto(&out.RenderStatus) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DownstreamTarget. @@ -239,7 +240,9 @@ func (in *PackageVariantStatus) DeepCopyInto(out *PackageVariantStatus) { if in.DownstreamTargets != nil { in, out := &in.DownstreamTargets, &out.DownstreamTargets *out = make([]DownstreamTarget, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } } diff --git a/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go b/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go index 10e38f05..b435e35d 100644 --- a/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go +++ b/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go @@ -16,7 +16,6 @@ package packagevariant import ( "context" - "encoding/json" "flag" "fmt" "strconv" @@ -59,7 +58,7 @@ const ( workspaceNamePrefix = "packagevariant-" ConditionTypeStalled = "Stalled" // whether or not the packagevariant object is making progress or not - ConditionTypeReady = "Ready" // whether or notthe reconciliation succeded + ConditionTypeReady = "Ready" // whether or not the reconciliation succeeded requeueDuration = 30 * time.Second ) @@ -698,12 +697,24 @@ func (r *PackageVariantReconciler) updateDraft(ctx context.Context, } func setTargetStatusConditions(pv *api.PackageVariant, targets []*porchapi.PackageRevision) { - pv.Status.DownstreamTargets = nil + downstreams := []api.DownstreamTarget{} + // keep downstream status when possible for _, t := range targets { - pv.Status.DownstreamTargets = append(pv.Status.DownstreamTargets, api.DownstreamTarget{ - Name: t.GetName(), - }) + found := false + for _, d := range pv.Status.DownstreamTargets { + if d.Name == t.Name { + found = true + downstreams = append(downstreams, d) + break + } + } + if !found { + downstreams = append(downstreams, api.DownstreamTarget{ + Name: t.GetName(), + }) + } } + pv.Status.DownstreamTargets = downstreams meta.SetStatusCondition(&pv.Status.Conditions, metav1.Condition{ Type: ConditionTypeReady, Status: "True", @@ -897,7 +908,7 @@ func ensurePackageContext(pv *api.PackageVariant, err = cm.SetNestedField(data, "data") if err != nil { - return fmt.Errorf("could not set package conext data: %w", err) + return fmt.Errorf("could not set package context data: %w", err) } prr.Spec.Resources["package-context.yaml"] = cm.String() return nil @@ -1054,29 +1065,15 @@ func (r *PackageVariantReconciler) updatePackageResources(ctx context.Context, p if err := r.Update(ctx, prr); err != nil { return err } - // save render status into a condition - // TODO: add renderStatus field to DownstreamTarget and use that instead - c := metav1.Condition{ - Type: prr.Name + "/Render", - } - details := "" - jsonBytes, err := json.Marshal(prr.Status.RenderStatus.Result) - if err != nil { - details = fmt.Sprintf("JSON marshal error: %v", err) - } else { - details = string(jsonBytes) - } - - if prr.Status.RenderStatus.Err != "" { - c.Status = "False" - c.Reason = "Error" - c.Message = fmt.Sprintf("%s, details: %s", prr.Status.RenderStatus.Err, details) - } else { - c.Status = "True" - c.Reason = "Success" - c.Message = details + for i, target := range pv.Status.DownstreamTargets { + if target.Name == prr.Name { + pv.Status.DownstreamTargets[i].RenderStatus = prr.Status.RenderStatus + return nil + } } - meta.SetStatusCondition(&pv.Status.Conditions, c) - klog.Infoln(fmt.Sprintf("package variant %q applied mutations to package revision %q", pv.Name, prr.Name)) + pv.Status.DownstreamTargets = append(pv.Status.DownstreamTargets, api.DownstreamTarget{ + Name: prr.Name, + RenderStatus: prr.Status.RenderStatus, + }) return nil }