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 <[email protected]>
rancher · Jun 21, 2024 · 61b79ef · 61b79ef
1 parent 8b11049
commit 61b79ef
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 11 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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)
+			},
+		),
 	)
 })