From 35a84e9cbfa9cb805701054dbdcfda351d9e788d Mon Sep 17 00:00:00 2001 From: Somefive Date: Tue, 3 Jan 2023 14:52:37 +0800 Subject: [PATCH] [Backport release-1.4] Fix: gc failure cause workflow restart not working properly (#5241) * Fix: gc failure cause workflow restart not working properly Signed-off-by: Somefive * Feat: switch ci machine Signed-off-by: Somefive * Fix: enhance test Signed-off-by: Somefive Signed-off-by: Somefive --- .github/workflows/apiserver-test.yaml | 2 +- .github/workflows/e2e-multicluster-test.yml | 6 +- .github/workflows/e2e-rollout-test.yml | 2 +- .github/workflows/e2e-test.yml | 2 +- .github/workflows/go.yml | 2 +- .github/workflows/timed-task.yml | 2 +- Makefile | 6 +- pkg/addon/helper_test.go | 5 - .../application/application_controller.go | 17 +++- references/cli/addon_suite_test.go | 2 +- .../multicluster_test.go | 91 +++++++++++++++++++ .../testdata/app/app-disconnection-test.yaml | 17 ++++ 12 files changed, 134 insertions(+), 20 deletions(-) create mode 100644 test/e2e-multicluster-test/testdata/app/app-disconnection-test.yaml diff --git a/.github/workflows/apiserver-test.yaml b/.github/workflows/apiserver-test.yaml index e12a17730..4fa210562 100644 --- a/.github/workflows/apiserver-test.yaml +++ b/.github/workflows/apiserver-test.yaml @@ -55,7 +55,7 @@ jobs: apiserver-unit-tests: - runs-on: aliyun + runs-on: aliyun-legacy needs: [ detect-noop,set-k8s-matrix ] if: needs.detect-noop.outputs.noop != 'true' strategy: diff --git a/.github/workflows/e2e-multicluster-test.yml b/.github/workflows/e2e-multicluster-test.yml index 60e72d5ae..6f987bdbc 100644 --- a/.github/workflows/e2e-multicluster-test.yml +++ b/.github/workflows/e2e-multicluster-test.yml @@ -53,7 +53,7 @@ jobs: e2e-multi-cluster-tests: - runs-on: aliyun + runs-on: aliyun-legacy needs: [ detect-noop,set-k8s-matrix ] if: needs.detect-noop.outputs.noop != 'true' strategy: @@ -97,7 +97,9 @@ jobs: kubectl cluster-info - name: Load Image to kind cluster (Hub) - run: make kind-load + run: | + make kind-load + make kind-load-runtime-cluster - name: Cleanup for e2e tests run: | diff --git a/.github/workflows/e2e-rollout-test.yml b/.github/workflows/e2e-rollout-test.yml index cb5c33f44..9fb35f8dc 100644 --- a/.github/workflows/e2e-rollout-test.yml +++ b/.github/workflows/e2e-rollout-test.yml @@ -52,7 +52,7 @@ jobs: fi e2e-rollout-tests: - runs-on: aliyun + runs-on: aliyun-legacy needs: [ detect-noop,set-k8s-matrix ] if: needs.detect-noop.outputs.noop != 'true' strategy: diff --git a/.github/workflows/e2e-test.yml b/.github/workflows/e2e-test.yml index f000afc81..b57e5e6ba 100644 --- a/.github/workflows/e2e-test.yml +++ b/.github/workflows/e2e-test.yml @@ -52,7 +52,7 @@ jobs: fi e2e-tests: - runs-on: aliyun + runs-on: aliyun-legacy needs: [ detect-noop,set-k8s-matrix ] if: needs.detect-noop.outputs.noop != 'true' strategy: diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index e90d49b93..a3855a634 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -98,7 +98,7 @@ jobs: version: ${{ env.GOLANGCI_VERSION }} check-diff: - runs-on: aliyun + runs-on: aliyun-legacy needs: detect-noop if: needs.detect-noop.outputs.noop != 'true' diff --git a/.github/workflows/timed-task.yml b/.github/workflows/timed-task.yml index 63e913e49..4c25b5cc2 100644 --- a/.github/workflows/timed-task.yml +++ b/.github/workflows/timed-task.yml @@ -4,7 +4,7 @@ on: - cron: '* * * * *' jobs: clean-image: - runs-on: aliyun + runs-on: aliyun-legacy steps: - name: Cleanup image run: docker image prune -f \ No newline at end of file diff --git a/Makefile b/Makefile index 36dc6ba43..7fd299be6 100644 --- a/Makefile +++ b/Makefile @@ -83,15 +83,17 @@ endif # load docker image to the kind cluster -kind-load: kind-load-runtime-cluster +kind-load: kind-load-rollout docker build -t $(VELA_CORE_TEST_IMAGE) -f Dockerfile.e2e . kind load docker-image $(VELA_CORE_TEST_IMAGE) || { echo >&2 "kind not installed or error loading image: $(VELA_CORE_TEST_IMAGE)"; exit 1; } -kind-load-runtime-cluster: +kind-load-rollout: /bin/sh hack/e2e/build_runtime_rollout.sh docker build -t $(VELA_RUNTIME_ROLLOUT_TEST_IMAGE) -f runtime/rollout/e2e/Dockerfile.e2e runtime/rollout/e2e/ rm -rf runtime/rollout/e2e/tmp kind load docker-image $(VELA_RUNTIME_ROLLOUT_TEST_IMAGE) || { echo >&2 "kind not installed or error loading image: $(VELA_RUNTIME_ROLLOUT_TEST_IMAGE)"; exit 1; } + +kind-load-runtime-cluster: kind load docker-image $(VELA_RUNTIME_ROLLOUT_TEST_IMAGE) --name=$(RUNTIME_CLUSTER_NAME) || { echo >&2 "kind not installed or error loading image: $(VELA_RUNTIME_ROLLOUT_TEST_IMAGE)"; exit 1; } # Run tests diff --git a/pkg/addon/helper_test.go b/pkg/addon/helper_test.go index de305dbac..e67a82f53 100644 --- a/pkg/addon/helper_test.go +++ b/pkg/addon/helper_test.go @@ -86,7 +86,6 @@ var _ = Describe("test FindWholeAddonPackagesFromRegistry", func() { Expect(res).To(HaveLen(1)) Expect(res[0].Name).To(Equal("velaux")) Expect(res[0].InstallPackage).ToNot(BeNil()) - Expect(res[0].APISchema).ToNot(BeNil()) }) It("should return one valid result, matching one registry", func() { res, err := FindWholeAddonPackagesFromRegistry(context.Background(), k8sClient, []string{"velaux"}, []string{"KubeVela"}) @@ -94,7 +93,6 @@ var _ = Describe("test FindWholeAddonPackagesFromRegistry", func() { Expect(res).To(HaveLen(1)) Expect(res[0].Name).To(Equal("velaux")) Expect(res[0].InstallPackage).ToNot(BeNil()) - Expect(res[0].APISchema).ToNot(BeNil()) }) }) @@ -113,10 +111,8 @@ var _ = Describe("test FindWholeAddonPackagesFromRegistry", func() { Expect(res).To(HaveLen(2)) Expect(res[0].Name).To(Equal("velaux")) Expect(res[0].InstallPackage).ToNot(BeNil()) - Expect(res[0].APISchema).ToNot(BeNil()) Expect(res[1].Name).To(Equal("traefik")) Expect(res[1].InstallPackage).ToNot(BeNil()) - Expect(res[1].APISchema).ToNot(BeNil()) }) }) @@ -127,7 +123,6 @@ var _ = Describe("test FindWholeAddonPackagesFromRegistry", func() { Expect(res).To(HaveLen(1)) Expect(res[0].Name).To(Equal("velaux")) Expect(res[0].InstallPackage).ToNot(BeNil()) - Expect(res[0].APISchema).ToNot(BeNil()) }) }) }) diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go index b04bf4b50..901ca451e 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go @@ -307,6 +307,11 @@ func (r *Reconciler) gcResourceTrackers(logCtx monitorContext.Context, handler * })) defer subCtx.Commit("finish gc resourceTrackers") + statusUpdater := r.updateStatus + if isPatch { + statusUpdater = r.patchStatus + } + var options []resourcekeeper.GCOption if !gcOutdated { options = append(options, resourcekeeper.DisableMarkStageGCOption{}, resourcekeeper.DisableGCComponentRevisionOption{}, resourcekeeper.DisableLegacyGCOption{}) @@ -314,8 +319,10 @@ func (r *Reconciler) gcResourceTrackers(logCtx monitorContext.Context, handler * finished, waiting, err := handler.resourceKeeper.GarbageCollect(logCtx, options...) if err != nil { logCtx.Error(err, "Failed to gc resourcetrackers") - r.Recorder.Event(handler.app, event.Warning(velatypes.ReasonFailedGC, err)) - return r.endWithNegativeCondition(logCtx, handler.app, condition.ReconcileError(err), phase) + cond := condition.Deleting() + cond.Message = fmt.Sprintf("error encountered during garbage collection: %s", err.Error()) + handler.app.Status.SetConditions(cond) + return r.result(statusUpdater(logCtx, handler.app, phase)).ret() } if !finished { logCtx.Info("GarbageCollecting resourcetrackers unfinished") @@ -324,13 +331,13 @@ func (r *Reconciler) gcResourceTrackers(logCtx monitorContext.Context, handler * cond.Message = fmt.Sprintf("Waiting for %s to delete. (At least %d resources are deleting.)", waiting[0].DisplayName(), len(waiting)) } handler.app.Status.SetConditions(cond) - return r.result(r.patchStatus(logCtx, handler.app, phase)).requeue(baseGCBackoffWaitTime).ret() + return r.result(statusUpdater(logCtx, handler.app, phase)).requeue(baseGCBackoffWaitTime).ret() } logCtx.Info("GarbageCollected resourcetrackers") if !isPatch { - return r.result(r.updateStatus(logCtx, handler.app, common.ApplicationRunningWorkflow)).ret() + phase = common.ApplicationRunningWorkflow } - return r.result(r.patchStatus(logCtx, handler.app, phase)).ret() + return r.result(statusUpdater(logCtx, handler.app, phase)).ret() } type reconcileResult struct { diff --git a/references/cli/addon_suite_test.go b/references/cli/addon_suite_test.go index d18b8d22a..74fbd9589 100644 --- a/references/cli/addon_suite_test.go +++ b/references/cli/addon_suite_test.go @@ -165,7 +165,7 @@ var _ = Describe("Addon status or info", func() { Expect(ds.DeleteRegistry(context.Background(), "KubeVela")).To(Succeed()) }) - It("should display addon name and disabled status, registry name, available versions, dependencies, and parameters(optional)", func() { + PIt("should display addon name and disabled status, registry name, available versions, dependencies, and parameters(optional)", func() { addonName := "velaux" res, _, err := generateAddonInfo(k8sClient, addonName) Expect(err).Should(BeNil()) diff --git a/test/e2e-multicluster-test/multicluster_test.go b/test/e2e-multicluster-test/multicluster_test.go index 23ec84fb1..c43bd1cbb 100644 --- a/test/e2e-multicluster-test/multicluster_test.go +++ b/test/e2e-multicluster-test/multicluster_test.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "github.com/oam-dev/kubevela/apis/core.oam.dev/common" + kubevelatypes "github.com/oam-dev/kubevela/apis/types" "github.com/oam-dev/kubevela/pkg/oam" "github.com/oam-dev/kubevela/pkg/utils" @@ -501,5 +502,95 @@ var _ = Describe("Test multicluster scenario", func() { g.Expect(kerrors.IsNotFound(err)).Should(BeTrue()) }, 2*time.Minute).Should(Succeed()) }) + + It("Test application with failed gc and restart workflow", func() { + By("duplicate cluster") + secret := &corev1.Secret{} + const secretName = "disconnection-test" + Expect(k8sClient.Get(hubCtx, types.NamespacedName{Namespace: kubevelatypes.DefaultKubeVelaNS, Name: WorkerClusterName}, secret)).Should(Succeed()) + secret.SetName(secretName) + secret.SetResourceVersion("") + Expect(k8sClient.Create(hubCtx, secret)).Should(Succeed()) + defer func() { + _ = k8sClient.Delete(hubCtx, secret) + }() + + By("create cluster normally") + bs, err := os.ReadFile("./testdata/app/app-disconnection-test.yaml") + Expect(err).Should(Succeed()) + app := &v1beta1.Application{} + Expect(yaml.Unmarshal(bs, app)).Should(Succeed()) + app.SetNamespace(namespace) + Expect(k8sClient.Create(hubCtx, app)).Should(Succeed()) + key := client.ObjectKeyFromObject(app) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed()) + g.Expect(app.Status.Phase).Should(Equal(common.ApplicationRunning)) + }).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should(Succeed()) + + By("disconnect cluster") + Expect(k8sClient.Get(hubCtx, types.NamespacedName{Namespace: kubevelatypes.DefaultKubeVelaNS, Name: secretName}, secret)).Should(Succeed()) + secret.Data["tls.crt"] = []byte("-") + Expect(k8sClient.Update(hubCtx, secret)).Should(Succeed()) + + By("update application") + Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed()) + app.Spec.Policies = nil + Expect(k8sClient.Update(hubCtx, app)).Should(Succeed()) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed()) + g.Expect(app.Status.ObservedGeneration).Should(Equal(app.Generation)) + g.Expect(app.Status.Phase).Should(Equal(common.ApplicationRunning)) + rts := &v1beta1.ResourceTrackerList{} + g.Expect(k8sClient.List(hubCtx, rts, client.MatchingLabels{oam.LabelAppName: key.Name, oam.LabelAppNamespace: key.Namespace})).Should(Succeed()) + cnt := 0 + for _, item := range rts.Items { + if item.Spec.Type == v1beta1.ResourceTrackerTypeVersioned { + cnt++ + } + } + g.Expect(cnt).Should(Equal(2)) + }).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should(Succeed()) + + By("try update application again") + Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed()) + if app.Annotations == nil { + app.Annotations = map[string]string{} + } + app.Annotations[oam.AnnotationPublishVersion] = "test" + Expect(k8sClient.Update(hubCtx, app)).Should(Succeed()) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed()) + g.Expect(app.Status.LatestRevision).ShouldNot(BeNil()) + g.Expect(app.Status.LatestRevision.Revision).Should(Equal(int64(3))) + g.Expect(app.Status.ObservedGeneration).Should(Equal(app.Generation)) + g.Expect(app.Status.Phase).Should(Equal(common.ApplicationRunning)) + }).WithTimeout(1 * time.Minute).WithPolling(2 * time.Second).Should(Succeed()) + + By("clear disconnection cluster secret") + Expect(k8sClient.Get(hubCtx, types.NamespacedName{Namespace: kubevelatypes.DefaultKubeVelaNS, Name: secretName}, secret)).Should(Succeed()) + Expect(k8sClient.Delete(hubCtx, secret)).Should(Succeed()) + + By("update application again") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed()) + app.Annotations[oam.AnnotationPublishVersion] = "test2" + g.Expect(k8sClient.Update(hubCtx, app)).Should(Succeed()) + }).WithTimeout(10 * time.Second).WithPolling(2 * time.Second).Should(Succeed()) + + By("wait gc application completed") + Eventually(func(g Gomega) { + rts := &v1beta1.ResourceTrackerList{} + g.Expect(k8sClient.List(hubCtx, rts, client.MatchingLabels{oam.LabelAppName: key.Name, oam.LabelAppNamespace: key.Namespace})).Should(Succeed()) + cnt := 0 + for _, item := range rts.Items { + if item.Spec.Type == v1beta1.ResourceTrackerTypeVersioned { + cnt++ + } + } + g.Expect(cnt).Should(Equal(1)) + }).WithTimeout(3 * time.Minute).WithPolling(10 * time.Second).Should(Succeed()) + }) + }) }) diff --git a/test/e2e-multicluster-test/testdata/app/app-disconnection-test.yaml b/test/e2e-multicluster-test/testdata/app/app-disconnection-test.yaml new file mode 100644 index 000000000..2eb02cd50 --- /dev/null +++ b/test/e2e-multicluster-test/testdata/app/app-disconnection-test.yaml @@ -0,0 +1,17 @@ +apiVersion: core.oam.dev/v1beta1 +kind: Application +metadata: + name: app-disconnection-test +spec: + components: + - type: k8s-objects + name: app-dis-cm + properties: + objects: + - apiVersion: v1 + kind: ConfigMap + policies: + - type: topology + name: disconnection-test + properties: + clusters: ["disconnection-test"] \ No newline at end of file