From 5a5721048cd617fb51bb9f407da5c35d53271a21 Mon Sep 17 00:00:00 2001 From: qiaozp <47812250+chivalryq@users.noreply.github.com> Date: Fri, 11 Aug 2023 12:50:22 +0800 Subject: [PATCH] Refactor: organize appHandler, remove unused flags (#6254) --- cmd/core/app/options/options.go | 1 - .../core.oam.dev/oamruntime_controller.go | 4 - .../application/application_controller.go | 55 +++--- .../core.oam.dev/v1beta1/application/apply.go | 21 +- .../v1beta1/application/apply_test.go | 2 +- .../v1beta1/application/assemble/assemble.go | 182 ------------------ .../assemble/assemble_suite_test.go | 96 --------- .../v1beta1/application/dispatcher.go | 2 +- .../v1beta1/application/generator.go | 26 +-- .../v1beta1/application/generator_test.go | 10 +- .../v1beta1/application/revision.go | 22 +-- .../v1beta1/application/revision_test.go | 36 ++-- 12 files changed, 90 insertions(+), 367 deletions(-) diff --git a/cmd/core/app/options/options.go b/cmd/core/app/options/options.go index f8328f3e2..bcc65d1d6 100644 --- a/cmd/core/app/options/options.go +++ b/cmd/core/app/options/options.go @@ -81,7 +81,6 @@ func NewCoreOptions() *CoreOptions { DefRevisionLimit: 20, AutoGenWorkloadDefinition: true, ConcurrentReconciles: 4, - EnableCompatibility: false, IgnoreAppWithoutControllerRequirement: false, IgnoreDefinitionWithoutControllerRequirement: false, }, diff --git a/pkg/controller/core.oam.dev/oamruntime_controller.go b/pkg/controller/core.oam.dev/oamruntime_controller.go index 009e24bf9..bbc19da3f 100644 --- a/pkg/controller/core.oam.dev/oamruntime_controller.go +++ b/pkg/controller/core.oam.dev/oamruntime_controller.go @@ -46,9 +46,6 @@ type Args struct { // AutoGenWorkloadDefinition indicates whether automatic generated workloadDefinition which componentDefinition refers to AutoGenWorkloadDefinition bool - // EnableCompatibility indicates that will change some functions of controller to adapt to multiple platforms, such as asi. - EnableCompatibility bool - // IgnoreAppWithoutControllerRequirement indicates that application controller will not process the app without 'app.oam.dev/controller-version-require' annotation. IgnoreAppWithoutControllerRequirement bool @@ -66,7 +63,6 @@ func (a *Args) AddFlags(fs *pflag.FlagSet, c *Args) { "definition-revision-limit is the maximum number of component/trait definition useless revisions that will be maintained, if the useless revisions exceed this number, older ones will be GCed first.The default value is 20.") fs.BoolVar(&a.AutoGenWorkloadDefinition, "autogen-workload-definition", c.AutoGenWorkloadDefinition, "Automatic generated workloadDefinition which componentDefinition refers to.") fs.IntVar(&a.ConcurrentReconciles, "concurrent-reconciles", c.ConcurrentReconciles, "concurrent-reconciles is the concurrent reconcile number of the controller. The default value is 4") - fs.BoolVar(&a.EnableCompatibility, "enable-asi-compatibility", c.EnableCompatibility, "enable compatibility for asi") fs.BoolVar(&a.IgnoreAppWithoutControllerRequirement, "ignore-app-without-controller-version", c.IgnoreAppWithoutControllerRequirement, "If true, application controller will not process the app without 'app.oam.dev/controller-version-require' annotation") fs.BoolVar(&a.IgnoreDefinitionWithoutControllerRequirement, "ignore-definition-without-controller-version", c.IgnoreDefinitionWithoutControllerRequirement, "If true, trait/component/workflowstep definition controller will not process the definition without 'definition.oam.dev/controller-version-require' annotation") } diff --git a/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go b/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go index 0f5709b6d..cb7a7772c 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go @@ -95,7 +95,6 @@ type Reconciler struct { type options struct { appRevisionLimit int concurrentReconciles int - disableStatusUpdate bool ignoreAppNoCtrlReq bool controllerVersion string } @@ -141,7 +140,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu logCtx.AddTag("publish_version", app.GetAnnotations()[oam.AnnotationPublishVersion]) appParser := appfile.NewApplicationParser(r.Client, r.pd) - handler, err := NewAppHandler(logCtx, r, app, appParser) + handler, err := NewAppHandler(logCtx, r, app) if err != nil { return r.endWithNegativeCondition(logCtx, app, condition.ReconcileError(err), common.ApplicationStarting) } @@ -179,14 +178,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu app.Status.SetConditions(condition.ReadyCondition("Revision")) r.Recorder.Event(app, event.Normal(velatypes.ReasonRevisoned, velatypes.MessageRevisioned)) - if err := handler.UpdateAppLatestRevisionStatus(logCtx); err != nil { + if err := handler.UpdateAppLatestRevisionStatus(logCtx, r.patchStatus); err != nil { logCtx.Error(err, "Failed to update application status") return r.endWithNegativeCondition(logCtx, app, condition.ReconcileError(err), common.ApplicationRendering) } logCtx.Info("Successfully apply application revision") if err := handler.ApplyPolicies(logCtx, appFile); err != nil { - logCtx.Error(err, "[Handle ApplyPolicies]") + logCtx.Error(err, "[handle ApplyPolicies]") r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedApply, err)) return r.endWithNegativeCondition(logCtx, app, condition.ErrorCondition(common.PolicyCondition.String(), errors.WithMessage(err, "ApplyPolices")), common.ApplicationPolicyGenerating) } @@ -440,23 +439,26 @@ func (r *Reconciler) endWithNegativeCondition(ctx context.Context, app *v1beta1. return r.result(fmt.Errorf("object level reconcile error, type: %q, msg: %q", string(condition.Type), condition.Message)).ret() } +// Application status can be updated by two methods: patch and update. +type method int + +const ( + patch = iota + update +) + +type statusPatcher func(ctx context.Context, app *v1beta1.Application, phase common.ApplicationPhase) error + func (r *Reconciler) patchStatus(ctx context.Context, app *v1beta1.Application, phase common.ApplicationPhase) error { - app.Status.Phase = phase - updateObservedGeneration(app) - if oldApp, ok := originalAppFrom(ctx); ok && oldApp != nil && equality.Semantic.DeepEqual(oldApp.Status, app.Status) { - return nil - } - ctx, cancel := ctrlrec.NewReconcileTerminationContext(ctx) - defer cancel() - if err := r.Status().Patch(ctx, app, client.Merge); err != nil { - // set to -1 to re-run workflow if status is failed to patch - executor.StepStatusCache.Store(fmt.Sprintf("%s-%s", app.Name, app.Namespace), -1) - return err - } - return nil + return r.writeStatusByMethod(ctx, patch, app, phase) } func (r *Reconciler) updateStatus(ctx context.Context, app *v1beta1.Application, phase common.ApplicationPhase) error { + return r.writeStatusByMethod(ctx, update, app, phase) +} + +func (r *Reconciler) writeStatusByMethod(ctx context.Context, method method, app *v1beta1.Application, phase common.ApplicationPhase) error { + // pre-check if the status is changed app.Status.Phase = phase updateObservedGeneration(app) if oldApp, ok := originalAppFrom(ctx); ok && oldApp != nil && equality.Semantic.DeepEqual(oldApp.Status, app.Status) { @@ -464,15 +466,17 @@ func (r *Reconciler) updateStatus(ctx context.Context, app *v1beta1.Application, } ctx, cancel := ctrlrec.NewReconcileTerminationContext(ctx) defer cancel() - if !r.disableStatusUpdate { - return r.Status().Update(ctx, app) + var f func() error + switch method { + case patch: + f = func() error { return r.Status().Patch(ctx, app, client.Merge) } + case update: + f = func() error { return r.Status().Update(ctx, app) } + default: + // Should never happen + panic("unknown method") } - obj, err := app.Unstructured() - if err != nil { - return err - } - if err := r.Status().Update(ctx, obj); err != nil { - // set to -1 to re-run workflow if status is failed to update + if err := f(); err != nil { executor.StepStatusCache.Store(fmt.Sprintf("%s-%s", app.Name, app.Namespace), -1) return err } @@ -648,7 +652,6 @@ func timeReconcile(app *v1beta1.Application) func() { func parseOptions(args core.Args) options { return options{ - disableStatusUpdate: args.EnableCompatibility, appRevisionLimit: args.AppRevisionLimit, concurrentReconciles: args.ConcurrentReconciles, ignoreAppNoCtrlReq: args.IgnoreAppWithoutControllerRequirement, diff --git a/pkg/controller/core.oam.dev/v1beta1/application/apply.go b/pkg/controller/core.oam.dev/v1beta1/application/apply.go index 0caa0e4a7..d90c771ed 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/apply.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/apply.go @@ -20,6 +20,7 @@ import ( "context" "sync" + "github.com/kubevela/workflow/pkg/cue/packages" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" @@ -45,7 +46,9 @@ import ( // AppHandler handles application reconcile type AppHandler struct { - r *Reconciler + client.Client + pd *packages.PackageDiscover + app *v1beta1.Application currentAppRev *v1beta1.ApplicationRevision latestAppRev *v1beta1.ApplicationRevision @@ -57,13 +60,12 @@ type AppHandler struct { services []common.ApplicationComponentStatus appliedResources []common.ClusterObjectReference deletedResources []common.ClusterObjectReference - parser *appfile.Parser mu sync.Mutex } // NewAppHandler create new app handler -func NewAppHandler(ctx context.Context, r *Reconciler, app *v1beta1.Application, parser *appfile.Parser) (*AppHandler, error) { +func NewAppHandler(ctx context.Context, r *Reconciler, app *v1beta1.Application) (*AppHandler, error) { if ctx, ok := ctx.(monitorContext.Context); ok { subCtx := ctx.Fork("create-app-handler", monitorContext.DurationMetric(func(v float64) { metrics.AppReconcileStageDurationHistogram.WithLabelValues("create-app-handler").Observe(v) @@ -75,10 +77,10 @@ func NewAppHandler(ctx context.Context, r *Reconciler, app *v1beta1.Application, return nil, errors.Wrapf(err, "failed to create resourceKeeper") } return &AppHandler{ - r: r, + Client: r.Client, + pd: r.pd, app: app, resourceKeeper: resourceHandler, - parser: parser, }, nil } @@ -247,7 +249,7 @@ func (h *AppHandler) collectTraitHealthStatus(comp *appfile.Component, tr *appfi pCtx.SetCtx(pkgmulticluster.WithCluster(pCtx.GetCtx(), pkgmulticluster.Local)) } _accessor := util.NewApplicationResourceNamespaceAccessor(h.app.Namespace, traitOverrideNamespace) - templateContext, err := tr.GetTemplateContext(pCtx, h.r.Client, _accessor) + templateContext, err := tr.GetTemplateContext(pCtx, h.Client, _accessor) if err != nil { return common.ApplicationTraitStatus{}, nil, errors.WithMessagef(err, "app=%s, comp=%s, trait=%s, get template context error", appName, comp.Name, tr.Name) } @@ -271,10 +273,10 @@ func (h *AppHandler) collectWorkloadHealthStatus(ctx context.Context, comp *appf ) if comp.CapabilityCategory == types.TerraformCategory { var configuration terraforv1beta2.Configuration - if err := h.r.Client.Get(ctx, client.ObjectKey{Name: comp.Name, Namespace: accessor.Namespace()}, &configuration); err != nil { + if err := h.Client.Get(ctx, client.ObjectKey{Name: comp.Name, Namespace: accessor.Namespace()}, &configuration); err != nil { if kerrors.IsNotFound(err) { var legacyConfiguration terraforv1beta1.Configuration - if err := h.r.Client.Get(ctx, client.ObjectKey{Name: comp.Name, Namespace: accessor.Namespace()}, &legacyConfiguration); err != nil { + if err := h.Client.Get(ctx, client.ObjectKey{Name: comp.Name, Namespace: accessor.Namespace()}, &legacyConfiguration); err != nil { return false, nil, nil, errors.WithMessagef(err, "app=%s, comp=%s, check health error", appName, comp.Name) } isHealth = setStatus(status, legacyConfiguration.Status.ObservedGeneration, legacyConfiguration.Generation, @@ -287,7 +289,7 @@ func (h *AppHandler) collectWorkloadHealthStatus(ctx context.Context, comp *appf appRev.Name, configuration.Status.Apply.State, configuration.Status.Apply.Message) } } else { - templateContext, err := comp.GetTemplateContext(comp.Ctx, h.r.Client, accessor) + templateContext, err := comp.GetTemplateContext(comp.Ctx, h.Client, accessor) if err != nil { return false, nil, nil, errors.WithMessagef(err, "app=%s, comp=%s, get template context error", appName, comp.Name) } @@ -389,6 +391,7 @@ func setStatus(status *common.ApplicationComponentStatus, observedGeneration, ge } // ApplyPolicies will render policies into manifests from appfile and dispatch them +// Note the builtin policy like apply-once, shared-resource, etc. is not handled here. func (h *AppHandler) ApplyPolicies(ctx context.Context, af *appfile.Appfile) error { if ctx, ok := ctx.(monitorContext.Context); ok { subCtx := ctx.Fork("apply-policies", monitorContext.DurationMetric(func(v float64) { diff --git a/pkg/controller/core.oam.dev/v1beta1/application/apply_test.go b/pkg/controller/core.oam.dev/v1beta1/application/apply_test.go index 57cd154ea..ace63ee0c 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/apply_test.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/apply_test.go @@ -224,7 +224,7 @@ var _ = Describe("Test deleter resource", func() { }, }, } - h, err := NewAppHandler(ctx, reconciler, &v1beta1.Application{ObjectMeta: metav1.ObjectMeta{Name: "example", Namespace: "default"}}, nil) + h, err := NewAppHandler(ctx, reconciler, &v1beta1.Application{ObjectMeta: metav1.ObjectMeta{Name: "example", Namespace: "default"}}) Expect(err).Should(Succeed()) h.appliedResources = appliedRsc Expect(h.Delete(ctx, "", common.WorkflowResourceCreator, &u)) diff --git a/pkg/controller/core.oam.dev/v1beta1/application/assemble/assemble.go b/pkg/controller/core.oam.dev/v1beta1/application/assemble/assemble.go index dc519ef3e..4b225f534 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/assemble/assemble.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/assemble/assemble.go @@ -18,47 +18,15 @@ package assemble import ( "github.com/pkg/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/klog/v2" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" "github.com/oam-dev/kubevela/apis/types" - "github.com/oam-dev/kubevela/pkg/appfile" "github.com/oam-dev/kubevela/pkg/oam" "github.com/oam-dev/kubevela/pkg/oam/util" ) -// NewAppManifests create a AppManifests -func NewAppManifests(appRevision *v1beta1.ApplicationRevision, parser *appfile.Parser) *AppManifests { - return &AppManifests{AppRevision: appRevision, parser: parser} -} - -// AppManifests contains configuration to assemble resources recorded in the ApplicationRevision. -// 'Assemble' means expand Application(Component and Trait) into K8s resource and get them ready to go, to be emitted -// into K8s -type AppManifests struct { - AppRevision *v1beta1.ApplicationRevision - WorkloadOptions []WorkloadOption - - componentManifests []*types.ComponentManifest - appName string - appNamespace string - appLabels map[string]string - appAnnotations map[string]string - appOwnerRef *metav1.OwnerReference - - assembledWorkloads map[string]*unstructured.Unstructured - assembledTraits map[string][]*unstructured.Unstructured - // key is workload reference, values are the references of scopes the workload belongs to - skipWorkloadApplyComp map[string]bool - - finalized bool - err error - - parser *appfile.Parser -} - // WorkloadOption will be applied to each workloads AFTER it has been assembled by generic rules shown below: // 1) use component name as workload name // 2) use application namespace as workload namespace if unspecified @@ -75,78 +43,6 @@ type WorkloadOption interface { ApplyToWorkload(*unstructured.Unstructured, *v1beta1.ComponentDefinition, []*unstructured.Unstructured) error } -// WithWorkloadOption add a WorkloadOption to plug in custom logic applied to each workload -func (am *AppManifests) WithWorkloadOption(wo WorkloadOption) *AppManifests { - if am.WorkloadOptions == nil { - am.WorkloadOptions = make([]WorkloadOption, 0) - } - am.WorkloadOptions = append(am.WorkloadOptions, wo) - return am -} - -// WithComponentManifests set component manifests with the given one -func (am *AppManifests) WithComponentManifests(componentManifests []*types.ComponentManifest) *AppManifests { - am.componentManifests = componentManifests - return am -} - -// AssembledManifests do assemble and merge all assembled resources(except referenced scopes) into one array -// The result guarantee the order of resources as defined in application originally. -// If it contains more than one component, the resources are well-orderred and also grouped. -// For example, if app = comp1 (wl1 + trait1 + trait2) + comp2 (wl2 + trait3 +trait4), -// the result is [wl1, trait1, trait2, wl2, trait3, trait4] -func (am *AppManifests) AssembledManifests() ([]*unstructured.Unstructured, error) { - if !am.finalized { - am.assemble() - } - if am.err != nil { - return nil, am.err - } - r := make([]*unstructured.Unstructured, 0) - for compName, wl := range am.assembledWorkloads { - skipApplyWorkload := false - ts := am.assembledTraits[compName] - for _, t := range ts { - r = append(r, t.DeepCopy()) - if v := t.GetLabels()[oam.LabelManageWorkloadTrait]; v == "true" { - skipApplyWorkload = true - } - } - if !skipApplyWorkload { - r = append(r, wl.DeepCopy()) - } else { - klog.InfoS("assemble meet a managedByTrait workload, so skip apply it", - "namespace", am.AppRevision.Namespace, "appRev", am.AppRevision.Name) - } - } - return r, nil -} - -// GroupAssembledManifests do assemble and return all resources grouped by components -func (am *AppManifests) GroupAssembledManifests() ( - map[string]*unstructured.Unstructured, - map[string][]*unstructured.Unstructured, - error) { - if !am.finalized { - am.assemble() - } - if am.err != nil { - return nil, nil, am.err - } - workloads := make(map[string]*unstructured.Unstructured) - for k, wl := range am.assembledWorkloads { - workloads[k] = wl.DeepCopy() - } - traits := make(map[string][]*unstructured.Unstructured) - for k, ts := range am.assembledTraits { - traits[k] = make([]*unstructured.Unstructured, len(ts)) - for i, t := range ts { - traits[k][i] = t.DeepCopy() - } - } - return workloads, traits, nil -} - // checkAutoDetectComponent will check if the standardWorkload is empty, // currently only Helm-based component is possible to be auto-detected // TODO implement auto-detect mechanism @@ -154,35 +50,6 @@ func checkAutoDetectComponent(wl *unstructured.Unstructured) bool { return wl == nil || (len(wl.GetAPIVersion()) == 0 && len(wl.GetKind()) == 0) } -func (am *AppManifests) assemble() { - if err := am.complete(); err != nil { - am.finalizeAssemble(err) - return - } - - klog.InfoS("Assemble manifests for application", "name", am.appName, "revision", am.AppRevision.GetName()) - if err := am.validate(); err != nil { - am.finalizeAssemble(err) - return - } - for _, comp := range am.componentManifests { - klog.InfoS("Assemble manifests for component", "name", comp.Name) - wl, traits, err := PrepareBeforeApply(comp, am.AppRevision, am.WorkloadOptions) - if err != nil { - am.finalizeAssemble(err) - return - } - if wl == nil { - klog.Warningf("component without specify workloadDef can not attach traits currently") - continue - } - - am.assembledWorkloads[comp.Name] = wl - am.assembledTraits[comp.Name] = traits - } - am.finalizeAssemble(nil) -} - // PrepareBeforeApply will prepare for some necessary info before apply func PrepareBeforeApply(comp *types.ComponentManifest, appRev *v1beta1.ApplicationRevision, workloadOpt []WorkloadOption) (*unstructured.Unstructured, []*unstructured.Unstructured, error) { if checkAutoDetectComponent(comp.StandardWorkload) { @@ -211,55 +78,6 @@ func PrepareBeforeApply(comp *types.ComponentManifest, appRev *v1beta1.Applicati return wl, assembledTraits, nil } -func (am *AppManifests) complete() error { - if len(am.componentManifests) == 0 { - var err error - af, err := am.parser.GenerateAppFileFromRevision(am.AppRevision) - if err != nil { - return errors.WithMessage(err, "fail to generate appfile from revision for app manifests complete") - } - am.componentManifests, err = af.GenerateComponentManifests() - if err != nil { - return errors.WithMessage(err, "fail to complete manifests as generate from app revision failed") - } - } - am.appNamespace = am.AppRevision.GetNamespace() - am.appLabels = am.AppRevision.GetLabels() - am.appName = am.AppRevision.GetLabels()[oam.LabelAppName] - am.appAnnotations = am.AppRevision.GetAnnotations() - am.appOwnerRef = metav1.GetControllerOf(am.AppRevision) - - am.assembledWorkloads = make(map[string]*unstructured.Unstructured) - am.assembledTraits = make(map[string][]*unstructured.Unstructured) - am.skipWorkloadApplyComp = make(map[string]bool) - return nil -} - -func (am *AppManifests) finalizeAssemble(err error) { - am.finalized = true - if err == nil { - klog.InfoS("Successfully assemble manifests for application", "name", am.appName, "revision", am.AppRevision.GetName(), "namespace", am.appNamespace) - return - } - klog.ErrorS(err, "Failed assembling manifests for application", "name", am.appName, "revision", am.AppRevision.GetName()) - am.err = errors.WithMessagef(err, "cannot assemble resources' manifests for application %q", am.appName) -} - -// AssembleOptions is highly coulped with AppRevision, should check the AppRevision provides all info -// required by AssembleOptions -func (am *AppManifests) validate() error { - if am.appOwnerRef == nil { - return errors.New("AppRevision must have an Application as owner") - } - if len(am.AppRevision.Labels[oam.LabelAppName]) == 0 { - return errors.New("AppRevision must have app name in the label") - } - if len(am.AppRevision.Labels[oam.LabelAppRevisionHash]) == 0 { - return errors.New("AppRevision must have revision hash in the label") - } - return nil -} - func assembleWorkload(compName string, wl *unstructured.Unstructured, labels map[string]string, resources []*unstructured.Unstructured, appRev *v1beta1.ApplicationRevision, wop []WorkloadOption) (*unstructured.Unstructured, error) { // use component name as workload name if workload name is not specified diff --git a/pkg/controller/core.oam.dev/v1beta1/application/assemble/assemble_suite_test.go b/pkg/controller/core.oam.dev/v1beta1/application/assemble/assemble_suite_test.go index 38ae667a4..eb1db9e32 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/assemble/assemble_suite_test.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/assemble/assemble_suite_test.go @@ -15,99 +15,3 @@ limitations under the License. */ package assemble - -import ( - "os" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - "sigs.k8s.io/yaml" - - "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" - "github.com/oam-dev/kubevela/pkg/oam" -) - -var _ = Describe("Test Assemble Options", func() { - It("test assemble", func() { - var ( - compName = "test-comp" - namespace = "default" - ) - - appRev := &v1beta1.ApplicationRevision{} - b, err := os.ReadFile("./testdata/apprevision.yaml") - /* appRevision test data is generated based on below application - apiVersion: core.oam.dev/v1beta1 - kind: Application - metadata: - name: test-assemble - spec: - components: - - name: test-comp - type: webservice - properties: - image: crccheck/hello-world - port: 8000 - traits: - - type: ingress - properties: - domain: localhost - http: - "/": 8000 - */ - Expect(err).Should(BeNil()) - err = yaml.Unmarshal(b, appRev) - Expect(err).Should(BeNil()) - - ao := NewAppManifests(appRev, appParser) - workloads, traits, err := ao.GroupAssembledManifests() - Expect(err).Should(BeNil()) - - By("Verify amount of result resources") - allResources, err := ao.AssembledManifests() - Expect(err).Should(BeNil()) - Expect(len(allResources)).Should(Equal(3)) - - By("Verify amount of result grouped resources") - Expect(len(workloads)).Should(Equal(1)) - Expect(len(traits[compName])).Should(Equal(2)) - - By("Verify workload metadata (name, namespace, labels, annotations, ownerRef)") - wl := workloads[compName] - Expect(wl.GetName()).Should(Equal(compName)) - Expect(wl.GetNamespace()).Should(Equal(namespace)) - labels := wl.GetLabels() - labelKeys := make([]string, 0, len(labels)) - for k := range labels { - labelKeys = append(labelKeys, k) - } - Expect(labelKeys).Should(ContainElements( - oam.LabelAppName, - oam.LabelAppRevision, - oam.LabelAppRevisionHash, - oam.LabelAppComponent, - oam.LabelAppComponentRevision, - oam.WorkloadTypeLabel, - oam.LabelOAMResourceType)) - Expect(len(wl.GetAnnotations())).Should(Equal(1)) - - By("Verify trait metadata (name, namespace, labels, annotations, ownerRef)") - trait := traits[compName][0] - Expect(trait.GetName()).Should(ContainSubstring(compName)) - Expect(trait.GetNamespace()).Should(Equal(namespace)) - labels = trait.GetLabels() - labelKeys = make([]string, 0, len(labels)) - for k := range labels { - labelKeys = append(labelKeys, k) - } - Expect(labelKeys).Should(ContainElements( - oam.LabelAppName, - oam.LabelAppRevision, - oam.LabelAppRevisionHash, - oam.LabelAppComponent, - oam.LabelAppComponentRevision, - oam.TraitTypeLabel, - oam.LabelOAMResourceType)) - Expect(len(wl.GetAnnotations())).Should(Equal(1)) - }) -}) diff --git a/pkg/controller/core.oam.dev/v1beta1/application/dispatcher.go b/pkg/controller/core.oam.dev/v1beta1/application/dispatcher.go index b286169b3..45614d475 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/dispatcher.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/dispatcher.go @@ -179,7 +179,7 @@ func (h *AppHandler) generateDispatcher(appRev *v1beta1.ApplicationRevision, rea traitType = splitName } } - stageType, err = getTraitDispatchStage(h.r.Client, traitType, appRev) + stageType, err = getTraitDispatchStage(h.Client, traitType, appRev) if err != nil { return nil, err } diff --git a/pkg/controller/core.oam.dev/v1beta1/application/generator.go b/pkg/controller/core.oam.dev/v1beta1/application/generator.go index 3951c61fd..31ba7aef9 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/generator.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/generator.go @@ -95,37 +95,37 @@ func (h *AppHandler) GenerateApplicationSteps(ctx monitorContext.Context, oam.LabelAppNamespace: app.Namespace, } handlerProviders := providers.NewProviders() - kube.Install(handlerProviders, h.r.Client, appLabels, &kube.Handlers{ + kube.Install(handlerProviders, h.Client, appLabels, &kube.Handlers{ Apply: h.Dispatch, Delete: h.Delete, }) - configprovider.Install(handlerProviders, h.r.Client, func(ctx context.Context, resources []*unstructured.Unstructured, applyOptions []apply.ApplyOption) error { + configprovider.Install(handlerProviders, h.Client, func(ctx context.Context, resources []*unstructured.Unstructured, applyOptions []apply.ApplyOption) error { for _, res := range resources { res.SetLabels(util.MergeMapOverrideWithDst(res.GetLabels(), appLabels)) } return h.resourceKeeper.Dispatch(ctx, resources, applyOptions) }) - oamProvider.Install(handlerProviders, app, af, h.r.Client, h.applyComponentFunc( + oamProvider.Install(handlerProviders, app, af, h.Client, h.applyComponentFunc( appParser, appRev, af), h.renderComponentFunc(appParser, appRev, af)) pCtx := velaprocess.NewContext(generateContextDataFromApp(app, appRev.Name)) renderer := func(ctx context.Context, comp common.ApplicationComponent) (*appfile.Component, error) { return appParser.ParseComponentFromRevisionAndClient(ctx, comp, appRev) } - multiclusterProvider.Install(handlerProviders, h.r.Client, app, af, + multiclusterProvider.Install(handlerProviders, h.Client, app, af, h.applyComponentFunc(appParser, appRev, af), h.checkComponentHealth(appParser, appRev, af), renderer) terraformProvider.Install(handlerProviders, app, renderer) - query.Install(handlerProviders, h.r.Client, nil) + query.Install(handlerProviders, h.Client, nil) instance := generateWorkflowInstance(af, app) executor.InitializeWorkflowInstance(instance) runners, err := generator.GenerateRunners(ctx, instance, wfTypes.StepGeneratorOptions{ Providers: handlerProviders, - PackageDiscover: h.r.pd, + PackageDiscover: h.pd, ProcessCtx: pCtx, - TemplateLoader: template.NewWorkflowStepTemplateRevisionLoader(appRev, h.r.Client.RESTMapper()), - Client: h.r.Client, + TemplateLoader: template.NewWorkflowStepTemplateRevisionLoader(appRev, h.Client.RESTMapper()), + Client: h.Client, StepConvertor: map[string]func(step workflowv1alpha1.WorkflowStep) (workflowv1alpha1.WorkflowStep, error){ wfTypes.WorkflowStepTypeApplyComponent: func(lstep workflowv1alpha1.WorkflowStep) (workflowv1alpha1.WorkflowStep, error) { copierStep := lstep.DeepCopy() @@ -319,7 +319,7 @@ func (h *AppHandler) renderComponentFunc(appParser *appfile.Parser, appRev *v1be if err != nil { return nil, nil, err } - return renderComponentsAndTraits(h.r.Client, manifest, appRev, clusterName, overrideNamespace) + return renderComponentsAndTraits(h.Client, manifest, appRev, clusterName, overrideNamespace) } } @@ -335,7 +335,7 @@ func (h *AppHandler) checkComponentHealth(appParser *appfile.Parser, appRev *v1b } wl.Ctx.SetCtx(auth.ContextWithUserInfo(ctx, h.app)) - readyWorkload, readyTraits, err := renderComponentsAndTraits(h.r.Client, manifest, appRev, clusterName, overrideNamespace) + readyWorkload, readyTraits, err := renderComponentsAndTraits(h.Client, manifest, appRev, clusterName, overrideNamespace) if err != nil { return false, nil, nil, err } @@ -378,7 +378,7 @@ func (h *AppHandler) applyComponentFunc(appParser *appfile.Parser, appRev *v1bet } wl.Ctx.SetCtx(auth.ContextWithUserInfo(ctx, h.app)) - readyWorkload, readyTraits, err := renderComponentsAndTraits(h.r.Client, manifest, appRev, clusterName, overrideNamespace) + readyWorkload, readyTraits, err := renderComponentsAndTraits(h.Client, manifest, appRev, clusterName, overrideNamespace) if err != nil { return nil, nil, false, err } @@ -414,7 +414,7 @@ func (h *AppHandler) applyComponentFunc(appParser *appfile.Parser, appRev *v1bet if DisableResourceApplyDoubleCheck { return readyWorkload, readyTraits, isHealth, nil } - workload, traits, err := getComponentResources(auth.ContextWithUserInfo(ctx, h.app), manifest, wl.SkipApplyWorkload, h.r.Client) + workload, traits, err := getComponentResources(auth.ContextWithUserInfo(ctx, h.app), manifest, wl.SkipApplyWorkload, h.Client) return workload, traits, isHealth, err } } @@ -457,7 +457,7 @@ func (h *AppHandler) prepareWorkloadAndManifests(ctx context.Context, ctxData.Cluster = cluster } // cluster info are secrets stored in the control plane cluster - ctxData.ClusterVersion = multicluster.GetVersionInfoFromObject(pkgmulticluster.WithCluster(ctx, types.ClusterLocalName), h.r.Client, ctxData.Cluster) + ctxData.ClusterVersion = multicluster.GetVersionInfoFromObject(pkgmulticluster.WithCluster(ctx, types.ClusterLocalName), h.Client, ctxData.Cluster) ctxData.CompRevision, _ = ctrlutil.ComputeSpecHash(comp) }) if err != nil { diff --git a/pkg/controller/core.oam.dev/v1beta1/application/generator_test.go b/pkg/controller/core.oam.dev/v1beta1/application/generator_test.go index b527c3c0f..06a38a85b 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/generator_test.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/generator_test.go @@ -113,7 +113,7 @@ var _ = Describe("Test Application workflow generator", func() { _, err = af.GeneratePolicyManifests(context.Background()) Expect(err).Should(BeNil()) - handler, err := NewAppHandler(ctx, reconciler, app, appParser) + handler, err := NewAppHandler(ctx, reconciler, app) Expect(err).Should(Succeed()) logCtx := monitorContext.NewTraceContext(ctx, "") @@ -157,7 +157,7 @@ var _ = Describe("Test Application workflow generator", func() { _, err = af.GeneratePolicyManifests(context.Background()) Expect(err).Should(BeNil()) - handler, err := NewAppHandler(ctx, reconciler, app, appParser) + handler, err := NewAppHandler(ctx, reconciler, app) Expect(err).Should(Succeed()) logCtx := monitorContext.NewTraceContext(ctx, "") @@ -199,7 +199,7 @@ var _ = Describe("Test Application workflow generator", func() { af, err := appParser.GenerateAppFile(ctx, app) Expect(err).Should(BeNil()) - handler, err := NewAppHandler(ctx, reconciler, app, appParser) + handler, err := NewAppHandler(ctx, reconciler, app) Expect(err).Should(Succeed()) logCtx := monitorContext.NewTraceContext(ctx, "") @@ -241,7 +241,7 @@ var _ = Describe("Test Application workflow generator", func() { af, err := appParser.GenerateAppFile(ctx, app) Expect(err).Should(BeNil()) - handler, err := NewAppHandler(ctx, reconciler, app, appParser) + handler, err := NewAppHandler(ctx, reconciler, app) Expect(err).Should(Succeed()) logCtx := monitorContext.NewTraceContext(ctx, "") @@ -280,7 +280,7 @@ var _ = Describe("Test Application workflow generator", func() { af, err := appParser.GenerateAppFile(ctx, app) Expect(err).Should(BeNil()) - handler, err := NewAppHandler(ctx, reconciler, app, appParser) + handler, err := NewAppHandler(ctx, reconciler, app) Expect(err).Should(Succeed()) logCtx := monitorContext.NewTraceContext(ctx, "") diff --git a/pkg/controller/core.oam.dev/v1beta1/application/revision.go b/pkg/controller/core.oam.dev/v1beta1/application/revision.go index 5e01a0437..cc51c312c 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/revision.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/revision.go @@ -114,14 +114,14 @@ func (h *AppHandler) createResourcesConfigMap(ctx context.Context, ConfigMapKeyPolicy: string(util.MustJSONMarshal(policies)), }, } - err := h.r.Client.Get(ctx, client.ObjectKey{Name: appRev.Name, Namespace: appRev.Namespace}, &corev1.ConfigMap{}) + err := h.Client.Get(ctx, client.ObjectKey{Name: appRev.Name, Namespace: appRev.Namespace}, &corev1.ConfigMap{}) if err == nil { return nil } if err != nil && !apierrors.IsNotFound(err) { return err } - return h.r.Client.Create(ctx, cm) + return h.Client.Create(ctx, cm) } // SprintComponentManifest formats and returns the resulting string. @@ -283,7 +283,7 @@ func (h *AppHandler) getLatestAppRevision(ctx context.Context) error { } latestRevName := h.app.Status.LatestRevision.Name latestAppRev := &v1beta1.ApplicationRevision{} - if err := h.r.Get(ctx, client.ObjectKey{Name: latestRevName, Namespace: h.app.Namespace}, latestAppRev); err != nil { + if err := h.Get(ctx, client.ObjectKey{Name: latestRevName, Namespace: h.app.Namespace}, latestAppRev); err != nil { klog.ErrorS(err, "Failed to get latest app revision", "appRevisionName", latestRevName) return errors.Wrapf(err, "fail to get latest app revision %s", latestRevName) } @@ -403,7 +403,7 @@ func (h *AppHandler) currentAppRevIsNew(ctx context.Context) (bool, bool, error) return false, false, nil } - revs, err := GetAppRevisions(ctx, h.r.Client, h.app.Name, h.app.Namespace) + revs, err := GetAppRevisions(ctx, h.Client, h.app.Name, h.app.Namespace) if err != nil { klog.ErrorS(err, "Failed to list app revision", "appName", h.app.Name) return false, false, errors.Wrap(err, "failed to list app revision") @@ -567,9 +567,9 @@ func (h *AppHandler) FinalizeAndApplyAppRevision(ctx context.Context) error { sharding.PropagateScheduledShardIDLabel(h.app, appRev) gotAppRev := &v1beta1.ApplicationRevision{} - if err := h.r.Get(ctx, client.ObjectKey{Name: appRev.Name, Namespace: appRev.Namespace}, gotAppRev); err != nil { + if err := h.Get(ctx, client.ObjectKey{Name: appRev.Name, Namespace: appRev.Namespace}, gotAppRev); err != nil { if apierrors.IsNotFound(err) { - return h.r.Create(ctx, appRev) + return h.Create(ctx, appRev) } return err } @@ -588,12 +588,12 @@ func (h *AppHandler) FinalizeAndApplyAppRevision(ctx context.Context) error { appRev.Spec.Compression.SetType(compression.Zstd) } - return h.r.Update(ctx, appRev) + return h.Update(ctx, appRev) } // UpdateAppLatestRevisionStatus only call to update app's latest revision status after applying manifests successfully // otherwise it will override previous revision which is used during applying to do GC jobs -func (h *AppHandler) UpdateAppLatestRevisionStatus(ctx context.Context) error { +func (h *AppHandler) UpdateAppLatestRevisionStatus(ctx context.Context, patchStatus statusPatcher) error { if DisableAllApplicationRevision { return nil } @@ -614,7 +614,7 @@ func (h *AppHandler) UpdateAppLatestRevisionStatus(ctx context.Context) error { Revision: int64(revNum), RevisionHash: h.currentRevHash, } - if err := h.r.patchStatus(ctx, h.app, common.ApplicationRendering); err != nil { + if err := patchStatus(ctx, h.app, common.ApplicationRendering); err != nil { klog.InfoS("Failed to update the latest appConfig revision to status", "application", klog.KObj(h.app), "latest revision", revName, "err", err) return err @@ -635,13 +635,13 @@ func (h *AppHandler) UpdateApplicationRevisionStatus(ctx context.Context, appRev // Versioned the context backend values. if wfStatus.ContextBackend != nil { var cm corev1.ConfigMap - if err := h.r.Client.Get(ctx, ktypes.NamespacedName{Namespace: wfStatus.ContextBackend.Namespace, Name: wfStatus.ContextBackend.Name}, &cm); err != nil { + if err := h.Client.Get(ctx, ktypes.NamespacedName{Namespace: wfStatus.ContextBackend.Namespace, Name: wfStatus.ContextBackend.Name}, &cm); err != nil { klog.Error(err, "[UpdateApplicationRevisionStatus] failed to load the context values", "ApplicationRevision", appRev.Name) } appRev.Status.WorkflowContext = cm.Data } - if err := h.r.Client.Status().Update(ctx, appRev); err != nil { + if err := h.Client.Status().Update(ctx, appRev); err != nil { if logCtx, ok := ctx.(monitorContext.Context); ok { logCtx.Error(err, "[UpdateApplicationRevisionStatus] failed to update application revision status", "ApplicationRevision", appRev.Name) } else { diff --git a/pkg/controller/core.oam.dev/v1beta1/application/revision_test.go b/pkg/controller/core.oam.dev/v1beta1/application/revision_test.go index a9bec583d..c2aa04794 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/revision_test.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/revision_test.go @@ -117,7 +117,7 @@ var _ = Describe("test generate revision ", func() { appRevision2 = *appRevision1.DeepCopy() appRevision2.Name = "appRevision2" - _handler, err := NewAppHandler(ctx, reconciler, &app, nil) + _handler, err := NewAppHandler(ctx, reconciler, &app) Expect(err).Should(Succeed()) handler = _handler }) @@ -201,12 +201,12 @@ var _ = Describe("test generate revision ", func() { Expect(handler.PrepareCurrentAppRevision(ctx, generatedAppfile)).Should(Succeed()) Expect(handler.FinalizeAndApplyAppRevision(ctx)).Should(Succeed()) Expect(handler.ProduceArtifacts(context.Background(), comps, nil)).Should(Succeed()) - Expect(handler.UpdateAppLatestRevisionStatus(ctx)).Should(Succeed()) + Expect(handler.UpdateAppLatestRevisionStatus(ctx, reconciler.patchStatus)).Should(Succeed()) curApp := &v1beta1.Application{} Eventually( func() error { - return handler.r.Get(ctx, + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: app.Name}, curApp) }, @@ -216,7 +216,7 @@ var _ = Describe("test generate revision ", func() { curAppRevision := &v1beta1.ApplicationRevision{} Eventually( func() error { - return handler.r.Get(ctx, + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: curApp.Status.LatestRevision.Name}, curAppRevision) }, @@ -241,7 +241,7 @@ var _ = Describe("test generate revision ", func() { Expect(handler.ProduceArtifacts(context.Background(), comps, nil)).Should(Succeed()) Eventually( func() error { - return handler.r.Get(ctx, + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: app.Name}, curApp) }, @@ -254,7 +254,7 @@ var _ = Describe("test generate revision ", func() { curAppRevision = &v1beta1.ApplicationRevision{} Eventually( func() error { - return handler.r.Get(ctx, + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: lastRevision}, curAppRevision) }, @@ -278,10 +278,10 @@ var _ = Describe("test generate revision ", func() { Expect(handler.PrepareCurrentAppRevision(ctx, generatedAppfile)).Should(Succeed()) Expect(handler.FinalizeAndApplyAppRevision(ctx)).Should(Succeed()) Expect(handler.ProduceArtifacts(context.Background(), comps, nil)).Should(Succeed()) - Expect(handler.UpdateAppLatestRevisionStatus(ctx)).Should(Succeed()) + Expect(handler.UpdateAppLatestRevisionStatus(ctx, reconciler.patchStatus)).Should(Succeed()) Eventually( func() error { - return handler.r.Get(ctx, + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: app.Name}, curApp) }, @@ -295,7 +295,7 @@ var _ = Describe("test generate revision ", func() { curAppRevision = &v1beta1.ApplicationRevision{} Eventually( func() error { - return handler.r.Get(ctx, + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: curApp.Status.LatestRevision.Name}, curAppRevision) }, @@ -322,10 +322,10 @@ var _ = Describe("test generate revision ", func() { Expect(handler.PrepareCurrentAppRevision(ctx, generatedAppfile)).Should(Succeed()) Expect(handler.FinalizeAndApplyAppRevision(ctx)).Should(Succeed()) Expect(handler.ProduceArtifacts(context.Background(), comps, nil)).Should(Succeed()) - Expect(handler.UpdateAppLatestRevisionStatus(ctx)).Should(Succeed()) + Expect(handler.UpdateAppLatestRevisionStatus(ctx, reconciler.patchStatus)).Should(Succeed()) Eventually( func() error { - return handler.r.Get(ctx, + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: app.Name}, curApp) }, @@ -339,7 +339,7 @@ var _ = Describe("test generate revision ", func() { curAppRevision = &v1beta1.ApplicationRevision{} Eventually( func() error { - return handler.r.Get(ctx, + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: curApp.Status.LatestRevision.Name}, curAppRevision) }, @@ -366,19 +366,19 @@ var _ = Describe("test generate revision ", func() { Expect(handler.PrepareCurrentAppRevision(ctx, generatedAppfile)).Should(Succeed()) Expect(handler.FinalizeAndApplyAppRevision(ctx)).Should(Succeed()) Expect(handler.ProduceArtifacts(context.Background(), comps, nil)).Should(Succeed()) - Expect(handler.UpdateAppLatestRevisionStatus(ctx)).Should(Succeed()) + Expect(handler.UpdateAppLatestRevisionStatus(ctx, reconciler.patchStatus)).Should(Succeed()) curApp := &v1beta1.Application{} Eventually( func() error { - return handler.r.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: app.Name}, curApp) + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: app.Name}, curApp) }, time.Second*10, time.Millisecond*500).Should(BeNil()) Expect(curApp.Status.LatestRevision.Revision).Should(BeEquivalentTo(1)) By("Verify the created appRevision is exactly what it is") curAppRevision := &v1beta1.ApplicationRevision{} Eventually( func() error { - return handler.r.Get(ctx, + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: curApp.Status.LatestRevision.Name}, curAppRevision) }, @@ -399,7 +399,7 @@ var _ = Describe("test generate revision ", func() { Expect(handler.ProduceArtifacts(context.Background(), comps, nil)).Should(Succeed()) Eventually( func() error { - return handler.r.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: app.Name}, curApp) + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: app.Name}, curApp) }, time.Second*10, time.Millisecond*500).Should(BeNil()) // no new revision should be created Expect(curApp.Status.LatestRevision.Name).Should(Equal(lastRevision)) @@ -409,7 +409,7 @@ var _ = Describe("test generate revision ", func() { curAppRevision = &v1beta1.ApplicationRevision{} Eventually( func() error { - return handler.r.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: lastRevision}, curAppRevision) + return handler.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: lastRevision}, curAppRevision) }, time.Second*5, time.Millisecond*500).Should(BeNil()) Expect(err).Should(Succeed()) Expect(curAppRevision.GetLabels()[oam.LabelAppRevisionHash]).Should(Equal(appHash1)) @@ -734,7 +734,7 @@ status: {} Expect(k8sClient.Create(ctx, &apprev)).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{})) // prepare handler - _handler, err := NewAppHandler(ctx, reconciler, &app, nil) + _handler, err := NewAppHandler(ctx, reconciler, &app) Expect(err).Should(Succeed()) handler = _handler