From eb1e041d3b41bb9d1d3cc767777bffe5397d23d7 Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Thu, 29 Feb 2024 12:44:56 +0100 Subject: [PATCH] Removes cluster node status reporting from agent Removes cluster node status reporting from agent and adds a basic unit test for the `clusterstatus Ticker` Refers to: https://github.com/rancher/fleet/issues/1973 Signed-off-by: Xavi Garcia --- charts/fleet-crd/templates/crds.yaml | 42 ---------- .../cmd/agent/clusterstatus/suite_test.go | 22 ++++++ internal/cmd/agent/clusterstatus/ticker.go | 65 +--------------- .../cmd/agent/clusterstatus/ticker_test.go | 77 +++++++++++++++++++ .../reconciler/cluster_controller.go | 14 ---- .../fleet.cattle.io/v1alpha1/cluster_types.go | 22 ------ .../v1alpha1/zz_generated.deepcopy.go | 10 --- 7 files changed, 101 insertions(+), 151 deletions(-) create mode 100644 internal/cmd/agent/clusterstatus/suite_test.go create mode 100644 internal/cmd/agent/clusterstatus/ticker_test.go diff --git a/charts/fleet-crd/templates/crds.yaml b/charts/fleet-crd/templates/crds.yaml index 6cff07c37b..11448dba22 100644 --- a/charts/fleet-crd/templates/crds.yaml +++ b/charts/fleet-crd/templates/crds.yaml @@ -3626,12 +3626,6 @@ spec: - jsonPath: .status.display.readyBundles name: Bundles-Ready type: string - - jsonPath: .status.display.readyNodes - name: Nodes-Ready - type: string - - jsonPath: .status.display.sampleNode - name: Sample-Node - type: string - jsonPath: .status.agent.lastSeen name: Last-Seen type: string @@ -5109,29 +5103,6 @@ spec: e.g. "cattle-fleet-system". nullable: true type: string - nonReadyNodeNames: - description: 'NonReadyNode contains the names of non-ready nodes. - The list is - - limited to at most 3 names.' - items: - type: string - type: array - nonReadyNodes: - description: NonReadyNodes is the number of nodes that are not - ready. - type: integer - readyNodeNames: - description: 'ReadyNodes contains the names of ready nodes. - The list is limited to - - at most 3 names.' - items: - type: string - type: array - readyNodes: - description: ReadyNodes is the number of nodes that are ready. - type: integer type: object agentAffinityHash: description: 'AgentAffinityHash is a hash of the agent''s affinity @@ -5260,19 +5231,6 @@ spec: to be ready.' type: string - readyNodes: - description: 'ReadyNodes is a string in the form "%d/%d", that - describes the - - number of nodes that are ready vs. the number of expected - nodes.' - type: string - sampleNode: - description: 'SampleNode is the name of one of the nodes that - are ready. If no - - node is ready, it''s the name of a node that is not ready.' - type: string state: description: State of the cluster, either one of the bundle states, or "WaitCheckIn". diff --git a/internal/cmd/agent/clusterstatus/suite_test.go b/internal/cmd/agent/clusterstatus/suite_test.go new file mode 100644 index 0000000000..10a9beb629 --- /dev/null +++ b/internal/cmd/agent/clusterstatus/suite_test.go @@ -0,0 +1,22 @@ +package clusterstatus_test + +import ( + "testing" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +const ( + timeout = 30 * time.Second +) + +func TestFleet(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "ClusterStatus Suite") +} + +var _ = BeforeSuite(func() { + SetDefaultEventuallyTimeout(timeout) +}) diff --git a/internal/cmd/agent/clusterstatus/ticker.go b/internal/cmd/agent/clusterstatus/ticker.go index d93e6dc6f5..e2e4c0f64f 100644 --- a/internal/cmd/agent/clusterstatus/ticker.go +++ b/internal/cmd/agent/clusterstatus/ticker.go @@ -4,7 +4,6 @@ package clusterstatus import ( "context" "encoding/json" - "sort" "time" "github.com/sirupsen/logrus" @@ -16,7 +15,6 @@ import ( corecontrollers "github.com/rancher/wrangler/v2/pkg/generated/controllers/core/v1" "github.com/rancher/wrangler/v2/pkg/ticker" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -72,30 +70,11 @@ func Ticker(ctx context.Context, // Update the cluster.fleet.cattle.io status in the upstream cluster with the current node status func (h *handler) Update() error { - nodes, err := h.nodes.List(metav1.ListOptions{}) - if err != nil { - return err - } - - ready, nonReady := sortReadyUnready(nodes.Items) - agentStatus := fleet.AgentStatus{ - LastSeen: metav1.Now(), - Namespace: h.agentNamespace, - NonReadyNodes: len(nonReady), - ReadyNodes: len(ready), + LastSeen: metav1.Now(), + Namespace: h.agentNamespace, } - if len(ready) > 3 { - ready = ready[:3] - } - if len(nonReady) > 3 { - nonReady = nonReady[:3] - } - - agentStatus.ReadyNodeNames = ready - agentStatus.NonReadyNodeNames = nonReady - if equality.Semantic.DeepEqual(h.reported, agentStatus) { return nil } @@ -117,43 +96,3 @@ func (h *handler) Update() error { h.reported = agentStatus return nil } - -func sortReadyUnready(nodes []corev1.Node) (ready []string, nonReady []string) { - var ( - masterNodeNames []string - nonReadyMasterNodeNames []string - readyNodes []string - nonReadyNodes []string - ) - - for _, node := range nodes { - ready := false - for _, cond := range node.Status.Conditions { - if cond.Type == corev1.NodeReady && cond.Status == corev1.ConditionTrue { - ready = true - break - } - } - - if node.Annotations["node-role.kubernetes.io/master"] == "true" { - if ready { - masterNodeNames = append(masterNodeNames, node.Name) - } else { - nonReadyMasterNodeNames = append(nonReadyMasterNodeNames, node.Name) - } - } else { - if ready { - readyNodes = append(readyNodes, node.Name) - } else { - nonReadyNodes = append(nonReadyNodes, node.Name) - } - } - } - - sort.Strings(masterNodeNames) - sort.Strings(nonReadyMasterNodeNames) - sort.Strings(readyNodes) - sort.Strings(nonReadyNodes) - - return append(masterNodeNames, readyNodes...), append(nonReadyMasterNodeNames, nonReadyNodes...) -} diff --git a/internal/cmd/agent/clusterstatus/ticker_test.go b/internal/cmd/agent/clusterstatus/ticker_test.go new file mode 100644 index 0000000000..04bc6c6aba --- /dev/null +++ b/internal/cmd/agent/clusterstatus/ticker_test.go @@ -0,0 +1,77 @@ +package clusterstatus + +import ( + "context" + "encoding/json" + "time" + + "github.com/golang/mock/gomock" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + "github.com/rancher/wrangler/v2/pkg/generic/fake" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +var _ = Describe("ClusterStatus Ticker", func() { + var ( + clusterClient *fake.MockClientInterface[*fleet.Cluster, *fleet.ClusterList] + nodeClient *fake.MockNonNamespacedClientInterface[*v1.Node, *v1.NodeList] + ctx context.Context + cancel context.CancelFunc + agentNamespace string + clusterName string + clusterNamespace string + baseTime metav1.Time + lastTime metav1.Time + agentStatusNamespace string + ) + + BeforeEach(func() { + ctrl := gomock.NewController(GinkgoT()) + clusterClient = fake.NewMockClientInterface[*fleet.Cluster, *fleet.ClusterList](ctrl) + nodeClient = fake.NewMockNonNamespacedClientInterface[*v1.Node, *v1.NodeList](ctrl) + agentNamespace = "cattle-fleet-system" + clusterName = "cluster-name" + clusterNamespace = "cluster-namespace" + baseTime = metav1.Now() + ctx, cancel = context.WithCancel(context.TODO()) + clusterClient.EXPECT().Patch(clusterNamespace, clusterName, types.MergePatchType, gomock.Any(), "status"). + DoAndReturn(func(namespace, name string, pt types.PatchType, data []byte, subresources ...string) (result fleet.Cluster, err error) { + cluster := &fleet.Cluster{} + err = json.Unmarshal(data, cluster) + if err != nil { + return fleet.Cluster{}, err + } + // only storing the lastseen time value here. + // We're not checking for values here because calling Expect, + // for example, makes the mock call panic when it doesn't succeed + lastTime = cluster.Status.Agent.LastSeen + agentStatusNamespace = cluster.Status.Agent.Namespace + return *cluster, nil + }).AnyTimes() + Ticker(ctx, agentNamespace, clusterNamespace, clusterName, time.Second*1, nodeClient, clusterClient) + }) + + It("Increases the timestamp used to call Patch", func() { + By("Comparing every 2 seconds for a 6 seconds period") + Consistently(func() bool { + // return true when we're calling before Path was even called + if lastTime.IsZero() { + return true + } + // check that the timestamp increases and the namespace is the expected one + result := baseTime.Before(&lastTime) && agentStatusNamespace == agentNamespace + baseTime = lastTime + return result + }, 6*time.Second, 2*time.Second).Should(BeTrue()) + // ensure that lastTime was set (which means Patch was successfully called) + Expect(lastTime).ShouldNot(BeZero()) + }) + + AfterEach(func() { + cancel() + }) +}) diff --git a/internal/cmd/controller/reconciler/cluster_controller.go b/internal/cmd/controller/reconciler/cluster_controller.go index fc1f9b9ea1..febdc444bd 100644 --- a/internal/cmd/controller/reconciler/cluster_controller.go +++ b/internal/cmd/controller/reconciler/cluster_controller.go @@ -137,10 +137,6 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct cluster.Status.Display.ReadyBundles = fmt.Sprintf("%d/%d", cluster.Status.Summary.Ready, cluster.Status.Summary.DesiredReady) - cluster.Status.Display.ReadyNodes = fmt.Sprintf("%d/%d", - cluster.Status.Agent.ReadyNodes, - cluster.Status.Agent.NonReadyNodes+cluster.Status.Agent.ReadyNodes) - cluster.Status.Display.SampleNode = sampleNode(cluster.Status) var state fleet.BundleState for _, nonReady := range cluster.Status.Summary.NonReadyResources { @@ -190,13 +186,3 @@ func (r *ClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { // cluster.Status.Namespace to create the namespace. Complete(r) } - -func sampleNode(status fleet.ClusterStatus) string { - if len(status.Agent.ReadyNodeNames) > 0 { - return status.Agent.ReadyNodeNames[0] - } - if len(status.Agent.NonReadyNodeNames) > 0 { - return status.Agent.NonReadyNodeNames[0] - } - return "" -} diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/cluster_types.go b/pkg/apis/fleet.cattle.io/v1alpha1/cluster_types.go index 14e2fe5021..d3f753bbf7 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/cluster_types.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/cluster_types.go @@ -46,8 +46,6 @@ var ( // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:printcolumn:name="Bundles-Ready",type=string,JSONPath=`.status.display.readyBundles` -// +kubebuilder:printcolumn:name="Nodes-Ready",type=string,JSONPath=`.status.display.readyNodes` -// +kubebuilder:printcolumn:name="Sample-Node",type=string,JSONPath=`.status.display.sampleNode` // +kubebuilder:printcolumn:name="Last-Seen",type=string,JSONPath=`.status.agent.lastSeen` // +kubebuilder:printcolumn:name="Status",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].message` @@ -200,12 +198,6 @@ type ClusterDisplay struct { // number of bundles that are ready vs. the number of bundles desired // to be ready. ReadyBundles string `json:"readyBundles,omitempty"` - // ReadyNodes is a string in the form "%d/%d", that describes the - // number of nodes that are ready vs. the number of expected nodes. - ReadyNodes string `json:"readyNodes,omitempty"` - // SampleNode is the name of one of the nodes that are ready. If no - // node is ready, it's the name of a node that is not ready. - SampleNode string `json:"sampleNode,omitempty"` // State of the cluster, either one of the bundle states, or "WaitCheckIn". State string `json:"state,omitempty"` } @@ -220,18 +212,4 @@ type AgentStatus struct { // +nullable // +optional Namespace string `json:"namespace"` - // NonReadyNodes is the number of nodes that are not ready. - // +optional - NonReadyNodes int `json:"nonReadyNodes"` - // ReadyNodes is the number of nodes that are ready. - // +optional - ReadyNodes int `json:"readyNodes"` - // NonReadyNode contains the names of non-ready nodes. The list is - // limited to at most 3 names. - // +optional - NonReadyNodeNames []string `json:"nonReadyNodeNames"` - // ReadyNodes contains the names of ready nodes. The list is limited to - // at most 3 names. - // +optional - ReadyNodeNames []string `json:"readyNodeNames"` } diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go index ca4129d38a..d8be5ab8a0 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go @@ -32,16 +32,6 @@ import ( func (in *AgentStatus) DeepCopyInto(out *AgentStatus) { *out = *in in.LastSeen.DeepCopyInto(&out.LastSeen) - if in.NonReadyNodeNames != nil { - in, out := &in.NonReadyNodeNames, &out.NonReadyNodeNames - *out = make([]string, len(*in)) - copy(*out, *in) - } - if in.ReadyNodeNames != nil { - in, out := &in.ReadyNodeNames, &out.ReadyNodeNames - *out = make([]string, len(*in)) - copy(*out, *in) - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AgentStatus.