diff --git a/cmd/core/main.go b/cmd/core/main.go index 7734bf159..cffd1420d 100644 --- a/cmd/core/main.go +++ b/cmd/core/main.go @@ -173,10 +173,10 @@ func main() { case "", "false", string(oamcontroller.ApplyOnceOnlyOff): controllerArgs.ApplyMode = oamcontroller.ApplyOnceOnlyOff setupLog.Info("ApplyOnceOnly is disabled") - case "true", oamcontroller.ApplyOnceOnlyOn: + case "true", string(oamcontroller.ApplyOnceOnlyOn): controllerArgs.ApplyMode = oamcontroller.ApplyOnceOnlyOn setupLog.Info("ApplyOnceOnly is enabled, that means workload or trait only apply once if no spec change even they are changed by others") - case oamcontroller.ApplyOnceOnlyForce: + case string(oamcontroller.ApplyOnceOnlyForce): controllerArgs.ApplyMode = oamcontroller.ApplyOnceOnlyForce setupLog.Info("ApplyOnceOnlyForce is enabled, that means workload or trait only apply once if no spec change even they are changed or deleted by others") default: diff --git a/pkg/controller/core.oam.dev/oamruntime_controller.go b/pkg/controller/core.oam.dev/oamruntime_controller.go index 7fd960ab3..d6431e3bc 100644 --- a/pkg/controller/core.oam.dev/oamruntime_controller.go +++ b/pkg/controller/core.oam.dev/oamruntime_controller.go @@ -26,12 +26,12 @@ const ( // ApplyOnceOnlyOn indicates workloads and traits should not be affected // if no spec change is made in the ApplicationConfiguration. - ApplyOnceOnlyOn = "on" + ApplyOnceOnlyOn ApplyOnceOnlyMode = "on" // ApplyOnceOnlyForce is a more strong case for ApplyOnceOnly, the workload // and traits won't be affected if no spec change is made in the ApplicationConfiguration, // even if the workload or trait has been deleted from cluster. - ApplyOnceOnlyForce = "force" + ApplyOnceOnlyForce ApplyOnceOnlyMode = "force" ) // Args args used by controller diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go index a6d7702f9..934ce8468 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go @@ -37,6 +37,7 @@ import ( "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" "github.com/crossplane/crossplane-runtime/pkg/event" + "github.com/crossplane/crossplane-runtime/pkg/fieldpath" "github.com/crossplane/crossplane-runtime/pkg/logging" "github.com/crossplane/crossplane-runtime/pkg/meta" "github.com/crossplane/crossplane-runtime/pkg/resource" @@ -316,6 +317,13 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (result reco log := log.WithValues("kind", e.GetKind(), "name", e.GetName()) record := r.record.WithAnnotations("kind", e.GetKind(), "name", e.GetName()) + err := r.confirmDeleteOnApplyOnceMode(ctx, ac.GetNamespace(), &e) + if err != nil { + log.Debug("confirm component can't be garbage collected", "error", err) + record.Event(ac, event.Warning(reasonCannotGGComponents, err)) + ac.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errGCComponent))) + return reconcile.Result{}, errors.Wrap(r.UpdateStatus(ctx, ac), errUpdateAppConfigStatus) + } if err := r.client.Delete(ctx, &e); resource.IgnoreNotFound(err) != nil { log.Debug("Cannot garbage collect component", "error", err) record.Event(ac, event.Warning(reasonCannotGGComponents, err)) @@ -340,6 +348,41 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (result reco return reconcile.Result{RequeueAfter: waitTime}, nil } +// confirmDeleteOnApplyOnceMode will confirm whether the workload can be delete or not in apply once only enabled mode +// currently only workload replicas with 0 can be delete +func (r *OAMApplicationReconciler) confirmDeleteOnApplyOnceMode(ctx context.Context, namespace string, u *unstructured.Unstructured) error { + if r.applyOnceOnlyMode == core.ApplyOnceOnlyOff { + return nil + } + getU := u.DeepCopy() + err := r.client.Get(ctx, client.ObjectKey{Name: u.GetName(), Namespace: namespace}, getU) + if err != nil { + // no need to check if workload not found + return resource.IgnoreNotFound(err) + } + // only check for workload + if labels := getU.GetLabels(); labels == nil || labels[oam.LabelOAMResourceType] != oam.ResourceTypeWorkload { + return nil + } + paved := fieldpath.Pave(getU.Object) + + // TODO: add more kinds of workload replica check here if needed + // "spec.replicas" maybe not accurate for all kinds of workload, but it work for most of them(including Deployment/StatefulSet/CloneSet). + // For workload which don't align with the `spec.replicas` schema, the check won't work + replicas, err := paved.GetInteger("spec.replicas") + if err != nil { + // it's possible for workload without the `spec.replicas`, it's omitempty + if strings.Contains(err.Error(), "no such field") { + return nil + } + return errors.WithMessage(err, "fail to get 'spec.replicas' from workload") + } + if replicas > 0 { + return errors.Errorf("can't delete workload with replicas %d in apply once only mode", replicas) + } + return nil +} + // UpdateStatus updates v1alpha2.ApplicationConfiguration's Status with retry.RetryOnConflict func (r *OAMApplicationReconciler) UpdateStatus(ctx context.Context, ac *v1alpha2.ApplicationConfiguration, opts ...client.UpdateOption) error { status := ac.DeepCopy().Status @@ -552,9 +595,20 @@ func (fn GarbageCollectorFn) Eligible(namespace string, ws []v1alpha2.WorkloadSt } // IsRevisionWorkload check is a workload is an old revision Workload which shouldn't be garbage collected. -// TODO(wonderflow): Do we have a better way to recognise it's a revisionWorkload which can't be garbage collected by AppConfig? -func IsRevisionWorkload(status v1alpha2.WorkloadStatus) bool { - return strings.HasPrefix(status.Reference.Name, status.ComponentName+"-") +func IsRevisionWorkload(status v1alpha2.WorkloadStatus, w []Workload) bool { + if strings.HasPrefix(status.Reference.Name, status.ComponentName+"-") { + // for compatibility, keep the old way + return true + } + + // check all workload, with same componentName + for _, wr := range w { + if wr.ComponentName == status.ComponentName { + return wr.RevisionEnabled + } + } + // component not found, should be deleted + return false } func eligible(namespace string, ws []v1alpha2.WorkloadStatus, w []Workload) []unstructured.Unstructured { @@ -578,7 +632,7 @@ func eligible(namespace string, ws []v1alpha2.WorkloadStatus, w []Workload) []un eligible := make([]unstructured.Unstructured, 0) for _, s := range ws { - if !applied[s.Reference] && !IsRevisionWorkload(s) { + if !applied[s.Reference] && !IsRevisionWorkload(s, w) { w := &unstructured.Unstructured{} w.SetAPIVersion(s.Reference.APIVersion) w.SetKind(s.Reference.Kind) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go index 047658aa8..b6eed1b85 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go @@ -852,9 +852,18 @@ func TestEligible(t *testing.T) { } func TestIsRevisionWorkload(t *testing.T) { - if true != IsRevisionWorkload(v1alpha2.WorkloadStatus{ComponentName: "compName", Reference: runtimev1alpha1.TypedReference{Name: "compName-rev1"}}) { + if true != IsRevisionWorkload(v1alpha2.WorkloadStatus{ComponentName: "compName", Reference: runtimev1alpha1.TypedReference{Name: "compName-rev1"}}, nil) { t.Error("workloadName has componentName as prefix is revisionWorkload") } + if true != IsRevisionWorkload(v1alpha2.WorkloadStatus{ComponentName: "compName", Reference: runtimev1alpha1.TypedReference{Name: "speciedName"}}, []Workload{{ComponentName: "compName", RevisionEnabled: true}}) { + t.Error("workloadName has componentName same and revisionEnabled is revisionWorkload") + } + if false != IsRevisionWorkload(v1alpha2.WorkloadStatus{ComponentName: "compName", Reference: runtimev1alpha1.TypedReference{Name: "speciedName"}}, []Workload{{ComponentName: "compName", RevisionEnabled: false}}) { + t.Error("workloadName has componentName same and revisionEnabled is false") + } + if false != IsRevisionWorkload(v1alpha2.WorkloadStatus{ComponentName: "compName", Reference: runtimev1alpha1.TypedReference{Name: "speciedName"}}, []Workload{{ComponentName: "compName-notmatch", RevisionEnabled: true}}) { + t.Error("workload with no prefix and no componentName match is not revisionEnabled ") + } } func TestDependency(t *testing.T) { diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/revision_enable_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/revision_enable_test.go index 47dc5a2d7..304de11da 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/revision_enable_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/revision_enable_test.go @@ -32,6 +32,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/pointer" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -824,3 +825,342 @@ var _ = Describe("Component Revision Enabled with apply once only force", func() }) }) + +var _ = Describe("Component Revision Enabled with workloadName set and apply once only force", func() { + const ( + namespace = "revision-and-workload-name-specified" + appName = "revision-apply-once2" + compName = "revision-apply-once-comp2" + specifiedNameBase = "specified-name-base" + specifiedNameV1 = "specified-name-v1" + specifiedNameV2 = "specified-name-v2" + ) + var ( + ctx = context.Background() + wr v1.Deployment + component v1alpha2.Component + appConfig v1alpha2.ApplicationConfiguration + appConfigKey = client.ObjectKey{ + Name: appName, + Namespace: namespace, + } + req = reconcile.Request{NamespacedName: appConfigKey} + ns = corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespace, + }, + } + ) + + BeforeEach(func() {}) + + AfterEach(func() { + // delete the namespace with all its resources + Expect(k8sClient.Delete(ctx, &ns, client.PropagationPolicy(metav1.DeletePropagationForeground))). + Should(SatisfyAny(BeNil(), &util.NotFoundMatcher{})) + }) + + It("revision enabled should create workload with specified name protect delete with replicas larger than 0", func() { + + getDeploy := func(image, name string) *v1.Deployment { + return &v1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: name, + }, + Spec: v1.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{ + "app": compName, + }}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "app": compName, + }}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{ + Name: "wordpress", + Image: image, + Ports: []corev1.ContainerPort{ + { + Name: "wordpress", + ContainerPort: 80, + }, + }, + }, + }}}, + }, + } + } + component = v1alpha2.Component{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "core.oam.dev/v1alpha2", + Kind: "Component", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: compName, + Namespace: namespace, + }, + Spec: v1alpha2.ComponentSpec{ + Workload: runtime.RawExtension{ + Object: getDeploy("wordpress:4.6.1-apache", specifiedNameBase), + }, + }, + } + appConfig = v1alpha2.ApplicationConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: appName, + Namespace: namespace, + }, + } + By("Create namespace") + Eventually( + func() error { + return k8sClient.Create(ctx, &ns) + }, + time.Second*3, time.Millisecond*300).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{})) + + By("Create Component") + Expect(k8sClient.Create(ctx, &component)).Should(Succeed()) + cmpV1 := &v1alpha2.Component{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compName}, cmpV1)).Should(Succeed()) + + By("component handler will automatically create controller revision") + Expect(func() bool { + _, ok := componentHandler.createControllerRevision(cmpV1, cmpV1) + return ok + }()).Should(BeTrue()) + var crList v1.ControllerRevisionList + By("Check controller revision created successfully") + Eventually(func() error { + labels := &metav1.LabelSelector{ + MatchLabels: map[string]string{ + ControllerRevisionComponentLabel: compName, + }, + } + selector, err := metav1.LabelSelectorAsSelector(labels) + if err != nil { + return err + } + err = k8sClient.List(ctx, &crList, &client.ListOptions{ + LabelSelector: selector, + }) + if err != nil { + return err + } + if len(crList.Items) != 1 { + return fmt.Errorf("want only 1 revision created but got %d", len(crList.Items)) + } + return nil + }, time.Second, 300*time.Millisecond).Should(BeNil()) + + By("Create an ApplicationConfiguration") + appConfig = v1alpha2.ApplicationConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: appName, + Namespace: namespace, + }, + Spec: v1alpha2.ApplicationConfigurationSpec{Components: []v1alpha2.ApplicationConfigurationComponent{ + { + RevisionName: compName + "-v1", + }, + }}, + } + + By("Creat appConfig & check successfully") + Expect(k8sClient.Create(ctx, &appConfig)).Should(Succeed()) + Eventually(func() error { + return k8sClient.Get(ctx, appConfigKey, &appConfig) + }, time.Second, 300*time.Millisecond).Should(BeNil()) + + By("Reconcile") + reconciler.applyOnceOnlyMode = "force" + reconcileRetry(reconciler, req) + + By("Check workload created successfully") + var workloadKey1 = client.ObjectKey{Namespace: namespace, Name: specifiedNameBase} + Eventually(func() error { + reconcileRetry(reconciler, req) + return k8sClient.Get(ctx, workloadKey1, &wr) + }, 3*time.Second, 300*time.Millisecond).Should(BeNil()) + By("Check workload should only have 1 generation") + Expect(wr.GetGeneration()).Should(BeEquivalentTo(1)) + + By("Check reconcile again and no error will happen") + reconcileRetry(reconciler, req) + + Expect(k8sClient.Get(ctx, appConfigKey, &appConfig)).Should(BeNil()) + + By("===================================== Start to Upgrade revision of component =========================================") + cmpV2 := &v1alpha2.Component{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compName}, cmpV2)).Should(Succeed()) + cmpV2.Spec.Workload = runtime.RawExtension{ + Object: getDeploy("wordpress:v2", specifiedNameV1), + } + By("Update Component") + Expect(k8sClient.Update(ctx, cmpV2)).Should(Succeed()) + By("component handler will automatically create a ne controller revision") + Expect(func() bool { _, ok := componentHandler.createControllerRevision(cmpV2, cmpV2); return ok }()).Should(BeTrue()) + By("Check controller revision created successfully") + Eventually(func() error { + labels := &metav1.LabelSelector{ + MatchLabels: map[string]string{ + ControllerRevisionComponentLabel: compName, + }, + } + selector, err := metav1.LabelSelectorAsSelector(labels) + if err != nil { + return err + } + err = k8sClient.List(ctx, &crList, &client.ListOptions{ + LabelSelector: selector, + }) + if err != nil { + return err + } + if len(crList.Items) != 2 { + return fmt.Errorf("there should be exactly 2 revision created but got %d", len(crList.Items)) + } + return nil + }, time.Second, 300*time.Millisecond).Should(BeNil()) + + By("Update appConfig & check successfully") + appConfig.Spec.Components[0].RevisionName = compName + "-v2" + + Expect(k8sClient.Update(ctx, &appConfig)).Should(Succeed()) + Eventually(func() error { + return k8sClient.Get(ctx, appConfigKey, &appConfig) + }, time.Second, 300*time.Millisecond).Should(BeNil()) + By("Reconcile for new revision") + reconcileRetry(reconciler, req) + + By("Check new revision workload created successfully") + Eventually(func() error { + reconcileRetry(reconciler, req) + var workloadKey = client.ObjectKey{Namespace: namespace, Name: specifiedNameV1} + return k8sClient.Get(ctx, workloadKey, &wr) + }, time.Second, 300*time.Millisecond).Should(BeNil()) + By("Check the new workload should only have 1 generation") + Expect(wr.GetGeneration()).Should(BeEquivalentTo(1)) + Expect(wr.Spec.Template.Spec.Containers[0].Image).Should(BeEquivalentTo("wordpress:v2")) + + By("Check the new workload should only have 1 generation") + Expect(wr.GetGeneration()).Should(BeEquivalentTo(1)) + + By("Check reconcile again") + reconcileRetry(reconciler, req) + + By("Check appconfig condition should have error") + Eventually(func() string { + reconcileRetry(reconciler, req) + err := k8sClient.Get(ctx, appConfigKey, &appConfig) + if err != nil { + return err.Error() + } + if len(appConfig.Status.Conditions) != 1 { + return "condition len should be 1 but now is " + strconv.Itoa(len(appConfig.Status.Conditions)) + } + By(fmt.Sprintf("Reconcile with condition %v", appConfig.Status.Conditions[0])) + return string(appConfig.Status.Conditions[0].Reason) + }, 3*time.Second, 300*time.Millisecond).Should(BeEquivalentTo("ReconcileError")) + + By("Check the old workload still there") + Eventually(func() error { + reconcileRetry(reconciler, req) + var workloadKey = client.ObjectKey{Namespace: namespace, Name: specifiedNameBase} + return k8sClient.Get(ctx, workloadKey, &wr) + }, time.Second, 300*time.Millisecond).Should(BeNil()) + By("Check the old workload should only have 1 generation") + Expect(wr.GetGeneration()).Should(BeEquivalentTo(1)) + Expect(wr.Spec.Template.Spec.Containers[0].Image).Should(BeEquivalentTo("wordpress:4.6.1-apache")) + + wr.Spec.Replicas = pointer.Int32Ptr(0) + Expect(k8sClient.Update(ctx, &wr)).Should(Succeed()) + + By("Reconcile Again and appconfig condition should not have error") + Eventually(func() string { + By("Once more Reconcile and should not have error") + reconcileRetry(reconciler, req) + err := k8sClient.Get(ctx, appConfigKey, &appConfig) + if err != nil { + return err.Error() + } + if len(appConfig.Status.Conditions) != 1 { + return "condition len should be 1 but now is " + strconv.Itoa(len(appConfig.Status.Conditions)) + } + return string(appConfig.Status.Conditions[0].Reason) + }, 3*time.Second, 300*time.Millisecond).Should(BeEquivalentTo("ReconcileSuccess")) + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: specifiedNameBase}, &wr)).Should(SatisfyAny(util.NotFoundMatcher{})) + + By("===================================== Start to Upgrade revision of component again =========================================") + cmpV3 := &v1alpha2.Component{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compName}, cmpV3)).Should(Succeed()) + cmpV3.Spec.Workload = runtime.RawExtension{ + Object: getDeploy("wordpress:v3", specifiedNameV2), + } + By("Update Component") + Expect(k8sClient.Update(ctx, cmpV3)).Should(Succeed()) + By("component handler will automatically create a ne controller revision") + Expect(func() bool { _, ok := componentHandler.createControllerRevision(cmpV3, cmpV3); return ok }()).Should(BeTrue()) + By("Update the AC and add the revisionEnabled Trait") + appConfig.Spec.Components[0].RevisionName = compName + "-v3" + appConfig.Spec.Components[0].Traits = []v1alpha2.ComponentTrait{ + {Trait: runtime.RawExtension{Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Foo", + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "trait.oam.dev/type": "rollout-revision", + }, + }, + "spec": map[string]interface{}{ + "key": "test3", + }, + }}}}, + } + Expect(k8sClient.Update(ctx, &appConfig)).Should(Succeed()) + Eventually(func() error { + return k8sClient.Get(ctx, appConfigKey, &appConfig) + }, time.Second, 300*time.Millisecond).Should(BeNil()) + Expect(len(appConfig.Spec.Components[0].Traits)).Should(BeEquivalentTo(1)) + + By("Check reconcile again and no error will happen, revisionEnabled will skip delete") + reconcileRetry(reconciler, req) + By("Check appconfig condition should not have error") + Eventually(func() string { + By("Once more Reconcile and should not have error") + reconcileRetry(reconciler, req) + err := k8sClient.Get(ctx, appConfigKey, &appConfig) + if err != nil { + return err.Error() + } + if len(appConfig.Status.Conditions) != 1 { + return "condition len should be 1 but now is " + strconv.Itoa(len(appConfig.Status.Conditions)) + } + return string(appConfig.Status.Conditions[0].Reason) + }, 3*time.Second, 300*time.Millisecond).Should(BeEquivalentTo("ReconcileSuccess")) + + By("Check new revision workload created successfully") + Eventually(func() error { + reconcileRetry(reconciler, req) + var workloadKey = client.ObjectKey{Namespace: namespace, Name: specifiedNameV2} + return k8sClient.Get(ctx, workloadKey, &wr) + }, time.Second, 300*time.Millisecond).Should(BeNil()) + By("Check the new workload should only have 1 generation") + Expect(wr.GetGeneration()).Should(BeEquivalentTo(1)) + Expect(wr.Spec.Template.Spec.Containers[0].Image).Should(BeEquivalentTo("wordpress:v3")) + By("Check the new workload should only have 1 generation") + Expect(wr.GetGeneration()).Should(BeEquivalentTo(1)) + By("Check the old workload still there") + Eventually(func() error { + reconcileRetry(reconciler, req) + var workloadKey = client.ObjectKey{Namespace: namespace, Name: specifiedNameV1} + return k8sClient.Get(ctx, workloadKey, &wr) + }, time.Second, 300*time.Millisecond).Should(BeNil()) + + reconciler.applyOnceOnlyMode = "off" + }) + +})