From 61b79ef6e13ee3cfb5add2b178540ce7a3acf7e2 Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Thu, 20 Jun 2024 17:58:41 +0200 Subject: [PATCH] Fixes error display and improves user experience in gipoller When the gitpoller job returns an error it is added to the `gitrepo` conditions so it can be properly displayed by the UI. It also reschedules the gitpoller job when the `gitrepo` spec is changed so job is inmediatelly executed. Signed-off-by: Xavi Garcia --- pkg/durations/durations.go | 17 +++++++++++- pkg/git/poll/gitrepopolljob.go | 3 +++ pkg/git/poll/gitrepopolljob_test.go | 7 +++++ pkg/git/poll/handler.go | 9 +++++-- pkg/git/poll/handler_test.go | 40 +++++++++++++++++++++++------ 5 files changed, 65 insertions(+), 11 deletions(-) diff --git a/pkg/durations/durations.go b/pkg/durations/durations.go index 5809ecb0c3..5b8f0e9886 100644 --- a/pkg/durations/durations.go +++ b/pkg/durations/durations.go @@ -1,6 +1,10 @@ package durations -import "time" +import ( + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) const ( AgentRegistrationRetry = time.Minute * 1 @@ -27,3 +31,14 @@ const ( TriggerSleep = time.Second * 5 DefaultCpuPprofPeriod = time.Minute ) + +// Equal reports whether the duration t is equal to u. +func Equal(t *metav1.Duration, u *metav1.Duration) bool { + if t == nil && u == nil { + return true + } + if t != nil && u != nil { + return t.Duration == u.Duration + } + return false +} diff --git a/pkg/git/poll/gitrepopolljob.go b/pkg/git/poll/gitrepopolljob.go index 3fe05b2018..0bc31f2439 100644 --- a/pkg/git/poll/gitrepopolljob.go +++ b/pkg/git/poll/gitrepopolljob.go @@ -5,6 +5,7 @@ import ( "context" "fmt" + "github.com/rancher/fleet/internal/cmd/controller/grutil" v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" "github.com/reugn/go-quartz/quartz" "golang.org/x/sync/semaphore" @@ -66,6 +67,8 @@ func (j *GitRepoPollJob) fetchLatestCommitAndUpdateStatus(ctx context.Context) { commit, err := j.fetcher.LatestCommit(ctx, &j.GitRepo, j.client) if err != nil { logger.Error(err, "error fetching commit", "gitrepo", j.GitRepo) + nsName := types.NamespacedName{Name: j.GitRepo.Name, Namespace: j.GitRepo.Namespace} + _ = grutil.UpdateErrorStatus(ctx, j.client, nsName, j.GitRepo.Status, err) return } if j.GitRepo.Status.Commit != commit { diff --git a/pkg/git/poll/gitrepopolljob_test.go b/pkg/git/poll/gitrepopolljob_test.go index bbf200fa8f..59bdbe5e74 100644 --- a/pkg/git/poll/gitrepopolljob_test.go +++ b/pkg/git/poll/gitrepopolljob_test.go @@ -86,6 +86,13 @@ var _ = Describe("GitRepoPollJob tests", func() { err := client.Get(ctx, types.NamespacedName{Name: gitRepo.Name, Namespace: gitRepo.Namespace}, &updatedGitRepo) Expect(err).ToNot(HaveOccurred()) Expect(updatedGitRepo.Status.Commit).To(Equal(gitRepo.Status.Commit)) + errorFound := false + for _, c := range updatedGitRepo.Status.Conditions { + if c.Message == "Some error" { + errorFound = true + } + } + Expect(errorFound).To(BeTrue()) }) }) }) diff --git a/pkg/git/poll/handler.go b/pkg/git/poll/handler.go index df55337e76..f94a2e79e5 100644 --- a/pkg/git/poll/handler.go +++ b/pkg/git/poll/handler.go @@ -6,6 +6,7 @@ import ( "time" v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + "github.com/rancher/fleet/pkg/durations" "github.com/rancher/fleet/pkg/git" "github.com/reugn/go-quartz/quartz" @@ -77,9 +78,13 @@ func (h *Handler) AddOrModifyGitRepoPollJob(ctx context.Context, gitRepo v1alpha return } previousInterval := gitRepoPollJob.GitRepo.Spec.PollingInterval + previousGeneration := gitRepoPollJob.GitRepo.Generation gitRepoPollJob.GitRepo = gitRepo - if gitRepo.Spec.PollingInterval != previousInterval { - // ignoring the error because we're going to schedule immediately + if (previousGeneration != gitRepo.Generation) || + !durations.Equal(gitRepo.Spec.PollingInterval, previousInterval) { + // Spec or polling interval changed + // Reschedule so the job is immediately executed + // (otherwise it'll wait until next timeout) _ = h.scheduler.DeleteJob(gitRepoPollKey) h.scheduleJob(ctx, gitRepoPollKey, gitRepo, true) } diff --git a/pkg/git/poll/handler_test.go b/pkg/git/poll/handler_test.go index d7c73a8762..d1889fff9d 100644 --- a/pkg/git/poll/handler_test.go +++ b/pkg/git/poll/handler_test.go @@ -40,7 +40,7 @@ var _ = Describe("Gitrepo pooling tests", func() { client = fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(&gitRepo).WithStatusSubresource(&gitRepo).Build() }) DescribeTable("Gitrepo pooling tests", - func(pollingInterval time.Duration, disablePolling bool, + func(pollingInterval time.Duration, disablePolling bool, changeSpec bool, schedulerCalls func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration), fetcherCalls func()) { if pollingInterval != 0 { @@ -49,6 +49,9 @@ var _ = Describe("Gitrepo pooling tests", func() { gitRepo.Spec.PollingInterval = nil } gitRepo.Spec.DisablePolling = disablePolling + if changeSpec { + gitRepo.Generation = 2 + } handler := Handler{ scheduler: scheduler, @@ -63,13 +66,13 @@ var _ = Describe("Gitrepo pooling tests", func() { }, Entry("GitRepo is not present and disablePolling = true", - 1*time.Second, true, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { + 1*time.Second, true, false, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { key := GitRepoPollKey(gitRepo) scheduler.EXPECT().GetScheduledJob(key).Return(nil, quartz.ErrJobNotFound).Times(1) }, func() {}), Entry("Gitrepo is not present, not setting pollingInterval and disablePolling=false", - 0*time.Second, false, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { + 0*time.Second, false, false, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { key := GitRepoPollKey(gitRepo) scheduler.EXPECT().GetScheduledJob(key).Return(nil, quartz.ErrJobNotFound).Times(1) @@ -84,7 +87,7 @@ var _ = Describe("Gitrepo pooling tests", func() { ), Entry("Gitrepo is not present, setting pollingInterval to a specific value and disablePolling=false", - 1999*time.Second, false, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { + 1999*time.Second, false, false, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { key := GitRepoPollKey(gitRepo) scheduler.EXPECT().GetScheduledJob(key).Return(nil, quartz.ErrJobNotFound).Times(1) @@ -99,7 +102,7 @@ var _ = Describe("Gitrepo pooling tests", func() { ), Entry("gitrepo present, same polling interval, disablePolling=true", - 10*time.Second, true, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { + 10*time.Second, true, false, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { key := GitRepoPollKey(gitRepo) job := NewGitRepoPollJob(client, fetcher, gitRepo) jobDetail := quartz.NewJobDetail(job, key) @@ -115,7 +118,7 @@ var _ = Describe("Gitrepo pooling tests", func() { ), Entry("gitrepo present, different polling interval, disablePolling=true", - 10*time.Second, true, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { + 10*time.Second, true, false, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { key := GitRepoPollKey(gitRepo) job := NewGitRepoPollJob(client, fetcher, gitRepo) jobDetail := quartz.NewJobDetail(job, key) @@ -131,7 +134,7 @@ var _ = Describe("Gitrepo pooling tests", func() { ), Entry("gitrepo present, same polling interval, disablePolling=false", - 10*time.Second, false, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { + 10*time.Second, false, false, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { key := GitRepoPollKey(gitRepo) job := NewGitRepoPollJob(client, fetcher, gitRepo) jobDetail := quartz.NewJobDetail(job, key) @@ -145,7 +148,7 @@ var _ = Describe("Gitrepo pooling tests", func() { ), Entry("gitrepo present, different polling interval, disablePolling=false", - 1999*time.Second, false, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { + 1999*time.Second, false, false, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { gitRepoCopy := gitRepo gitRepoCopy.Spec.PollingInterval = &metav1.Duration{Duration: 10 * time.Second} key := GitRepoPollKey(gitRepoCopy) @@ -164,6 +167,27 @@ var _ = Describe("Gitrepo pooling tests", func() { fetcher.EXPECT().LatestCommit(gomock.Any(), gomock.Any(), gomock.Any()).Return("commit", nil).Times(1) }, ), + + Entry("gitrepo present, same polling interval, disablePolling=false, generation changed", + 10*time.Second, false, true, func(gitRepo v1alpha1.GitRepo, pollingInterval time.Duration) { + gitRepoCopy := gitRepo + gitRepoCopy.Generation = 1 + key := GitRepoPollKey(gitRepoCopy) + job := NewGitRepoPollJob(client, fetcher, gitRepoCopy) + jobDetail := quartz.NewJobDetail(job, key) + schedJob := &mocks.MockScheduledJob{ + Detail: jobDetail, + TriggerDuration: 10 * time.Second, + } + // gets the job and rechedules + scheduler.EXPECT().GetScheduledJob(key).Return(schedJob, nil).Times(1) + scheduler.EXPECT().DeleteJob(key).Return(nil).Times(1) + trigger := quartz.NewSimpleTrigger(10 * time.Second) + scheduler.EXPECT().ScheduleJob(jobDetail, trigger).Return(nil).Times(1) + }, func() { + fetcher.EXPECT().LatestCommit(gomock.Any(), gomock.Any(), gomock.Any()).Return("commit", nil).Times(1) + }, + ), ) })