From 7825230a52f05673b43d06bb59be50832a131d13 Mon Sep 17 00:00:00 2001 From: Alejandro Ruiz Date: Wed, 30 Oct 2024 12:09:10 +0100 Subject: [PATCH] Backport #3033 Backport GitRepo status resources improvements --- .../gitops/reconciler/gitjob_controller.go | 2 - .../gitops/reconciler/resourcekey_test.go | 173 ++++++++++++++++++ .../gitops/reconciler/suite_test.go | 16 ++ internal/cmd/controller/grutil/resourcekey.go | 130 +++++++------ internal/cmd/controller/grutil/status.go | 10 +- 5 files changed, 269 insertions(+), 62 deletions(-) create mode 100644 internal/cmd/controller/gitops/reconciler/resourcekey_test.go create mode 100644 internal/cmd/controller/gitops/reconciler/suite_test.go diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go index aeee0389fa..e60e07a146 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go @@ -219,8 +219,6 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return result(repoPolled, gitrepo), grutil.UpdateErrorStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status, err) } - grutil.SetStatusFromResourceKey(ctx, r.Client, gitrepo) - gitrepo.Status.Display.ReadyBundleDeployments = fmt.Sprintf("%d/%d", gitrepo.Status.Summary.Ready, gitrepo.Status.Summary.DesiredReady) diff --git a/internal/cmd/controller/gitops/reconciler/resourcekey_test.go b/internal/cmd/controller/gitops/reconciler/resourcekey_test.go new file mode 100644 index 0000000000..295cd68cec --- /dev/null +++ b/internal/cmd/controller/gitops/reconciler/resourcekey_test.go @@ -0,0 +1,173 @@ +package reconciler + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/rancher/wrangler/v3/pkg/summary" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" +) + +var _ = Describe("Resourcekey", func() { + var ( + gitrepo *fleet.GitRepo + list *fleet.BundleDeploymentList + ) + + BeforeEach(func() { + gitrepo = &fleet.GitRepo{ + Status: fleet.GitRepoStatus{ + Summary: fleet.BundleSummary{ + Ready: 2, + WaitApplied: 1, + }, + }, + } + list = &fleet.BundleDeploymentList{ + Items: []fleet.BundleDeployment{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "bd1", + Labels: map[string]string{ + fleet.RepoLabel: "gitrepo1", + fleet.ClusterLabel: "cluster1", + fleet.ClusterNamespaceLabel: "c-ns1", + }, + }, + Status: fleet.BundleDeploymentStatus{ + Resources: []fleet.BundleDeploymentResource{ + { + Kind: "Deployment", + APIVersion: "v1", + Name: "web", + Namespace: "default", + }, + { + // extra service for one cluster + Kind: "Service", + APIVersion: "v1", + Name: "web-svc", + Namespace: "default", + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "bd1", + Labels: map[string]string{ + fleet.RepoLabel: "gitrepo1", + fleet.ClusterLabel: "cluster2", + fleet.ClusterNamespaceLabel: "c-ns1", + }, + }, + Status: fleet.BundleDeploymentStatus{ + Resources: []fleet.BundleDeploymentResource{ + { + Kind: "Deployment", + APIVersion: "v1", + Name: "web", + Namespace: "default", + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "bd1", + Labels: map[string]string{ + fleet.RepoLabel: "gitrepo1", + fleet.ClusterLabel: "cluster1", + fleet.ClusterNamespaceLabel: "c-ns2", + }, + }, + Status: fleet.BundleDeploymentStatus{ + NonReadyStatus: []fleet.NonReadyStatus{ + { + Kind: "Deployment", + APIVersion: "v1", + Name: "web", + Namespace: "default", + Summary: summary.Summary{ + State: "Pending", + Error: true, + Transitioning: true, + Message: []string{"message1", "message2"}, + }, + }, + }, + Resources: []fleet.BundleDeploymentResource{ + { + Kind: "Deployment", + APIVersion: "v1", + Name: "web", + Namespace: "default", + }, + }, + }, + }, + }, + } + + }) + + It("returns a list", func() { + setResources(list, gitrepo) + + Expect(gitrepo.Status.Resources).To(HaveLen(2)) + Expect(gitrepo.Status.Resources).To(ContainElement(fleet.GitRepoResource{ + APIVersion: "v1", + Kind: "Deployment", + Type: "deployment", + ID: "default/web", + + Namespace: "default", + Name: "web", + + IncompleteState: false, + State: "WaitApplied", + Error: false, + Transitioning: false, + Message: "", + PerClusterState: []fleet.ResourcePerClusterState{ + { + State: "Pending", + ClusterID: "c-ns2/cluster1", + Error: true, + Transitioning: true, + Message: "message1; message2", + Patch: nil, + }, + }, + })) + Expect(gitrepo.Status.Resources).To(ContainElement(fleet.GitRepoResource{ + APIVersion: "v1", + Kind: "Service", + Type: "service", + ID: "default/web-svc", + + Namespace: "default", + Name: "web-svc", + + IncompleteState: false, + State: "WaitApplied", + Error: false, + Transitioning: false, + Message: "", + PerClusterState: []fleet.ResourcePerClusterState{}, + })) + + Expect(gitrepo.Status.ResourceErrors).To(BeEmpty()) + + Expect(gitrepo.Status.ResourceCounts.Ready).To(Equal(0)) + Expect(gitrepo.Status.ResourceCounts.DesiredReady).To(Equal(4)) + Expect(gitrepo.Status.ResourceCounts.WaitApplied).To(Equal(3)) + Expect(gitrepo.Status.ResourceCounts.Modified).To(Equal(0)) + Expect(gitrepo.Status.ResourceCounts.Orphaned).To(Equal(0)) + Expect(gitrepo.Status.ResourceCounts.Missing).To(Equal(0)) + Expect(gitrepo.Status.ResourceCounts.Unknown).To(Equal(0)) + Expect(gitrepo.Status.ResourceCounts.NotReady).To(Equal(1)) + }) +}) diff --git a/internal/cmd/controller/gitops/reconciler/suite_test.go b/internal/cmd/controller/gitops/reconciler/suite_test.go new file mode 100644 index 0000000000..82ce4c5725 --- /dev/null +++ b/internal/cmd/controller/gitops/reconciler/suite_test.go @@ -0,0 +1,16 @@ +package reconciler + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestGitOpsReconciler(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "GitOps Reconciler Suite") +} + +var _ = BeforeSuite(func() { +}) diff --git a/internal/cmd/controller/grutil/resourcekey.go b/internal/cmd/controller/grutil/resourcekey.go index 2163cd4df4..96463eef17 100644 --- a/internal/cmd/controller/grutil/resourcekey.go +++ b/internal/cmd/controller/grutil/resourcekey.go @@ -1,55 +1,72 @@ package grutil import ( - "context" "encoding/json" "sort" "strings" - "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" - "sigs.k8s.io/controller-runtime/pkg/client" ) -func SetStatusFromResourceKey(ctx context.Context, c client.Client, gitrepo *fleet.GitRepo) { - state := bundleErrorState(gitrepo.Status.Summary) - gitrepo.Status.Resources, gitrepo.Status.ResourceErrors = fromResourceKey(ctx, c, gitrepo.Namespace, gitrepo.Name, state) - gitrepo.Status = countResources(gitrepo.Status) +func setResources(list *fleet.BundleDeploymentList, gitrepo *fleet.GitRepo) { + s := summaryState(gitrepo.Status.Summary) + r, errors := fromResources(list, s) + gitrepo.Status.ResourceErrors = errors + gitrepo.Status.ResourceCounts = countResources(r) + gitrepo.Status.Resources = merge(r) } -func bundleErrorState(summary fleet.BundleSummary) string { - bundleErrorState := "" +// merge takes a list of GitRepo resources and deduplicates resources deployed to multiple clusters, +// ensuring that for such resources, the output contains a single resource entry with a field summarizing +// its status on each cluster. +func merge(resources []fleet.GitRepoResource) []fleet.GitRepoResource { + merged := map[string]fleet.GitRepoResource{} + for _, resource := range resources { + key := key(resource) + if existing, ok := merged[key]; ok { + existing.PerClusterState = append(existing.PerClusterState, resource.PerClusterState...) + merged[key] = existing + } else { + merged[key] = resource + } + } + + var result []fleet.GitRepoResource + for _, resource := range merged { + result = append(result, resource) + } + + sort.Slice(result, func(i, j int) bool { + return key(result[i]) < key(result[j]) + }) + return result +} + +func key(resource fleet.GitRepoResource) string { + return resource.Type + "/" + resource.ID +} + +func summaryState(summary fleet.BundleSummary) string { if summary.WaitApplied > 0 { - bundleErrorState = "WaitApplied" + return "WaitApplied" } if summary.ErrApplied > 0 { - bundleErrorState = "ErrApplied" + return "ErrApplied" } - return bundleErrorState + return "" } -// fromResourceKey lists all bundledeployments for this GitRepo and returns a list of -// GitRepoResource states for all resources +// fromResources inspects all bundledeployments for this GitRepo and returns a list of +// GitRepoResources and error messages. // // It populates gitrepo status resources from bundleDeployments. BundleDeployment.Status.Resources is the list of deployed resources. -func fromResourceKey(ctx context.Context, c client.Client, namespace, name string, bundleErrorState string) ([]fleet.GitRepoResource, []string) { +func fromResources(list *fleet.BundleDeploymentList, summaryState string) ([]fleet.GitRepoResource, []string) { var ( resources []fleet.GitRepoResource errors []string ) - bdList := &fleet.BundleDeploymentList{} - err := c.List(ctx, bdList, client.MatchingLabels{ - fleet.RepoLabel: name, - fleet.BundleNamespaceLabel: namespace, - }) - if err != nil { - errors = append(errors, err.Error()) - return resources, errors - } - - for _, bd := range bdList.Items { - bd := bd // fix gosec warning regarding "Implicit memory aliasing in for loop" + for _, bd := range list.Items { bdResources := bundleDeploymentResources(bd) incomplete, err := addState(bd, bdResources) @@ -60,32 +77,29 @@ func fromResourceKey(ctx context.Context, c client.Client, namespace, name strin } } - for k, state := range bdResources { - resource := toResourceState(k, state, incomplete, bundleErrorState) + for k, perCluster := range bdResources { + resource := toResourceState(k, perCluster, incomplete, summaryState) resources = append(resources, resource) } } sort.Strings(errors) - sort.Slice(resources, func(i, j int) bool { - return resources[i].Type+"/"+resources[i].ID < resources[j].Type+"/"+resources[j].ID - }) return resources, errors } -func toResourceState(k fleet.ResourceKey, state []fleet.ResourcePerClusterState, incomplete bool, bundleErrorState string) fleet.GitRepoResource { +func toResourceState(k fleet.ResourceKey, perCluster []fleet.ResourcePerClusterState, incomplete bool, summaryState string) fleet.GitRepoResource { resource := fleet.GitRepoResource{ APIVersion: k.APIVersion, Kind: k.Kind, Namespace: k.Namespace, Name: k.Name, IncompleteState: incomplete, - PerClusterState: state, + PerClusterState: perCluster, } resource.Type, resource.ID = toType(resource) - for _, state := range state { + for _, state := range perCluster { resource.State = state.State resource.Error = state.Error resource.Transitioning = state.Transitioning @@ -93,22 +107,23 @@ func toResourceState(k fleet.ResourceKey, state []fleet.ResourcePerClusterState, break } + // fallback to state from gitrepo summary if resource.State == "" { if resource.IncompleteState { - if bundleErrorState != "" { - resource.State = bundleErrorState + if summaryState != "" { + resource.State = summaryState } else { resource.State = "Unknown" } - } else if bundleErrorState != "" { - resource.State = bundleErrorState + } else if summaryState != "" { + resource.State = summaryState } else { resource.State = "Ready" } } - sort.Slice(state, func(i, j int) bool { - return state[i].ClusterID < state[j].ClusterID + sort.Slice(perCluster, func(i, j int) bool { + return perCluster[i].ClusterID < perCluster[j].ClusterID }) return resource } @@ -127,6 +142,9 @@ func toType(resource fleet.GitRepoResource) (string, string) { return t, resource.Namespace + "/" + resource.Name } +// addState adds per-cluster state information for nonReady and modified resources in a bundleDeployment. +// It only adds up to 10 entries to not overwhelm the status. +// It mutates resources and returns whether the reported state is incomplete and any errors encountered. func addState(bd fleet.BundleDeployment, resources map[fleet.ResourceKey][]fleet.ResourcePerClusterState) (bool, []error) { var ( incomplete bool @@ -137,7 +155,7 @@ func addState(bd fleet.BundleDeployment, resources map[fleet.ResourceKey][]fleet incomplete = true } - cluster := bd.Labels[v1alpha1.ClusterNamespaceLabel] + "/" + bd.Labels[v1alpha1.ClusterLabel] + cluster := bd.Labels[fleet.ClusterNamespaceLabel] + "/" + bd.Labels[fleet.ClusterLabel] for _, nonReady := range bd.Status.NonReadyStatus { key := fleet.ResourceKey{ Kind: nonReady.Kind, @@ -202,39 +220,39 @@ func appendState(states map[fleet.ResourceKey][]fleet.ResourcePerClusterState, k func bundleDeploymentResources(bd fleet.BundleDeployment) map[fleet.ResourceKey][]fleet.ResourcePerClusterState { bdResources := map[fleet.ResourceKey][]fleet.ResourcePerClusterState{} for _, resource := range bd.Status.Resources { - resourceKey := fleet.ResourceKey{ + key := fleet.ResourceKey{ Kind: resource.Kind, APIVersion: resource.APIVersion, Name: resource.Name, Namespace: resource.Namespace, } - bdResources[resourceKey] = []fleet.ResourcePerClusterState{} + bdResources[key] = []fleet.ResourcePerClusterState{} } return bdResources } -func countResources(status fleet.GitRepoStatus) fleet.GitRepoStatus { - status.ResourceCounts = fleet.GitRepoResourceCounts{} +func countResources(resources []fleet.GitRepoResource) fleet.GitRepoResourceCounts { + counts := fleet.GitRepoResourceCounts{} - for _, resource := range status.Resources { - status.ResourceCounts.DesiredReady++ + for _, resource := range resources { + counts.DesiredReady++ switch resource.State { case "Ready": - status.ResourceCounts.Ready++ + counts.Ready++ case "WaitApplied": - status.ResourceCounts.WaitApplied++ + counts.WaitApplied++ case "Modified": - status.ResourceCounts.Modified++ + counts.Modified++ case "Orphan": - status.ResourceCounts.Orphaned++ + counts.Orphaned++ case "Missing": - status.ResourceCounts.Missing++ + counts.Missing++ case "Unknown": - status.ResourceCounts.Unknown++ + counts.Unknown++ default: - status.ResourceCounts.NotReady++ + counts.NotReady++ } } - return status + return counts } diff --git a/internal/cmd/controller/grutil/status.go b/internal/cmd/controller/grutil/status.go index f2bfb84784..314df0d422 100644 --- a/internal/cmd/controller/grutil/status.go +++ b/internal/cmd/controller/grutil/status.go @@ -73,7 +73,7 @@ func SetStatusFromBundleDeployments(ctx context.Context, c client.Client, gitrep return err } - gitrepo.Status.Summary = fleet.BundleSummary{} + var bundleSummary fleet.BundleSummary sort.Slice(list.Items, func(i, j int) bool { return list.Items[i].UID < list.Items[j].UID @@ -87,8 +87,8 @@ func SetStatusFromBundleDeployments(ctx context.Context, c client.Client, gitrep for _, bd := range list.Items { bd := bd // fix gosec warning regarding "Implicit memory aliasing in for loop" state := summary.GetDeploymentState(&bd) - summary.IncrementState(&gitrepo.Status.Summary, bd.Name, state, summary.MessageFromDeployment(&bd), bd.Status.ModifiedStatus, bd.Status.NonReadyStatus) - gitrepo.Status.Summary.DesiredReady++ + summary.IncrementState(&bundleSummary, bd.Name, state, summary.MessageFromDeployment(&bd), bd.Status.ModifiedStatus, bd.Status.NonReadyStatus) + bundleSummary.DesiredReady++ if fleet.StateRank[state] > fleet.StateRank[maxState] { maxState = state message = summary.MessageFromDeployment(&bd) @@ -99,11 +99,13 @@ func SetStatusFromBundleDeployments(ctx context.Context, c client.Client, gitrep maxState = "" message = "" } - + gitrepo.Status.Summary = bundleSummary gitrepo.Status.Display.State = string(maxState) gitrepo.Status.Display.Message = message gitrepo.Status.Display.Error = len(message) > 0 + setResources(list, gitrepo) + return nil }