diff --git a/deploy/crds/atlassian.com_nodegroups_crd.yaml b/deploy/crds/atlassian.com_nodegroups_crd.yaml index b537504..ac7055a 100644 --- a/deploy/crds/atlassian.com_nodegroups_crd.yaml +++ b/deploy/crds/atlassian.com_nodegroups_crd.yaml @@ -149,6 +149,11 @@ spec: - waitPeriod type: object type: array + maxFailedCycleNodeRequests: + description: MaxFailedCycleNodeRequests defines the maximum number + of allowed failed CNRs for a nodegroup before the observer stops + generating them. + type: integer nodeGroupName: description: NodeGroupName is the name of the node group in the cloud provider that corresponds to this NodeGroup resource. diff --git a/pkg/apis/atlassian/v1/common.go b/pkg/apis/atlassian/v1/common.go index 6570b22..a1101f4 100644 --- a/pkg/apis/atlassian/v1/common.go +++ b/pkg/apis/atlassian/v1/common.go @@ -31,3 +31,23 @@ func buildNodeGroupNames(nodeGroupsList []string, nodeGroupName string) []string return nodeGroups } + +// sameNodeGroups compares two lists of nodegroup names and check they are the +// same. Ordering does not affect equality. +func sameNodeGroups(groupA, groupB []string) bool { + if len(groupA) != len(groupB) { + return false + } + + groupMap := make(map[string]struct{}) + for _, group := range groupA { + groupMap[group] = struct{}{} + } + + for _, group := range groupB { + if _, ok := groupMap[group]; !ok { + return false + } + } + return true +} diff --git a/pkg/apis/atlassian/v1/common_test.go b/pkg/apis/atlassian/v1/common_test.go index 1d60d6a..fac9f16 100644 --- a/pkg/apis/atlassian/v1/common_test.go +++ b/pkg/apis/atlassian/v1/common_test.go @@ -87,3 +87,44 @@ func TestBuildNodeGroupNames(t *testing.T) { }) } } + +func Test_sameNodeGroups(t *testing.T) { + tests := []struct { + name string + groupA []string + groupB []string + expect bool + }{ + { + "pass case with same order", + []string{"ingress-us-west-2a", "ingress-us-west-2b", "ingress-us-west-2c"}, + []string{"ingress-us-west-2a", "ingress-us-west-2b", "ingress-us-west-2c"}, + true, + }, + { + "pass case with different order", + []string{"ingress-us-west-2a", "ingress-us-west-2b", "ingress-us-west-2c"}, + []string{"ingress-us-west-2b", "ingress-us-west-2c", "ingress-us-west-2a"}, + true, + }, + { + "failure case with different length", + []string{"ingress-us-west-2a", "ingress-us-west-2b", "ingress-us-west-2c"}, + []string{"ingress-us-west-2b", "ingress-us-west-2c"}, + false, + }, + { + "failure case with different items", + []string{"ingress-us-west-2a", "ingress-us-west-2b", "ingress-us-west-2c"}, + []string{"ingress-us-west-2b", "ingress-us-west-2c", "system"}, + false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := sameNodeGroups(tt.groupA, tt.groupB) + assert.Equal(t, tt.expect, got) + }) + } +} diff --git a/pkg/apis/atlassian/v1/cyclenoderequest.go b/pkg/apis/atlassian/v1/cyclenoderequest.go index 72da8ab..e645868 100644 --- a/pkg/apis/atlassian/v1/cyclenoderequest.go +++ b/pkg/apis/atlassian/v1/cyclenoderequest.go @@ -15,3 +15,24 @@ func (in *CycleNodeRequest) NodeLabelSelector() (labels.Selector, error) { func (in *CycleNodeRequest) GetNodeGroupNames() []string { return buildNodeGroupNames(in.Spec.NodeGroupsList, in.Spec.NodeGroupName) } + +// IsPartOfNodeGroup returns whether the CycleNodeRequest is part of the +// provided NodeGroup by comparing the list of named cloud provider nodegroups +// defined in each one. Ordering does not affect equality. +func (in *CycleNodeRequest) IsFromNodeGroup(nodegroup NodeGroup) bool { + return sameNodeGroups( + buildNodeGroupNames(in.Spec.NodeGroupsList, in.Spec.NodeGroupName), + nodegroup.GetNodeGroupNames(), + ) +} + +// IsFromSameNodeGroup returns whether the CycleNodeRequest is part of the +// same Nodegroup provided as the provided CycleNodeRequest by comparing the +// list of named cloud provider nodegroups defined in each one. Ordering does +// not affect equality. +func (in *CycleNodeRequest) IsFromSameNodeGroup(cnr CycleNodeRequest) bool { + return sameNodeGroups( + buildNodeGroupNames(in.Spec.NodeGroupsList, in.Spec.NodeGroupName), + cnr.GetNodeGroupNames(), + ) +} diff --git a/pkg/apis/atlassian/v1/nodegroup_types.go b/pkg/apis/atlassian/v1/nodegroup_types.go index 9d0e0e0..c5e2c5e 100644 --- a/pkg/apis/atlassian/v1/nodegroup_types.go +++ b/pkg/apis/atlassian/v1/nodegroup_types.go @@ -19,6 +19,10 @@ type NodeGroupSpec struct { // CycleSettings stores the settings to use for cycling the nodes. CycleSettings CycleSettings `json:"cycleSettings"` + // MaxFailedCycleNodeRequests defines the maximum number of allowed failed CNRs for a nodegroup before the observer + // stops generating them. + MaxFailedCycleNodeRequests uint `json:"maxFailedCycleNodeRequests,omitempty"` + // ValidationOptions stores the settings to use for validating state of nodegroups // in kube and the cloud provider for cycling the nodes. ValidationOptions ValidationOptions `json:"validationOptions,omitempty"` diff --git a/pkg/controller/cyclenoderequest/transitioner/test_helpers.go b/pkg/controller/cyclenoderequest/transitioner/test_helpers.go index 7e238d0..b658c7e 100644 --- a/pkg/controller/cyclenoderequest/transitioner/test_helpers.go +++ b/pkg/controller/cyclenoderequest/transitioner/test_helpers.go @@ -6,6 +6,8 @@ import ( v1 "github.com/atlassian-labs/cyclops/pkg/apis/atlassian/v1" "github.com/atlassian-labs/cyclops/pkg/controller" "github.com/atlassian-labs/cyclops/pkg/mock" + + "sigs.k8s.io/controller-runtime/pkg/client" ) type Option func(t *Transitioner) @@ -22,6 +24,18 @@ func WithKubeNodes(nodes []*mock.Node) Option { } } +func WithExtraKubeObject(extraKubeObject client.Object) Option { + return func(t *Transitioner) { + t.extraKubeObjects = append(t.extraKubeObjects, extraKubeObject) + } +} + +func WithTransitionerOptions(options Options) Option { + return func(t *Transitioner) { + t.transitionerOptions = options + } +} + // ************************************************************************** // type Transitioner struct { @@ -30,6 +44,10 @@ type Transitioner struct { CloudProviderInstances []*mock.Node KubeNodes []*mock.Node + + extraKubeObjects []client.Object + + transitionerOptions Options } func NewFakeTransitioner(cnr *v1.CycleNodeRequest, opts ...Option) *Transitioner { @@ -38,13 +56,17 @@ func NewFakeTransitioner(cnr *v1.CycleNodeRequest, opts ...Option) *Transitioner // override these as needed CloudProviderInstances: make([]*mock.Node, 0), KubeNodes: make([]*mock.Node, 0), + extraKubeObjects: []client.Object{cnr}, + transitionerOptions: Options{}, } for _, opt := range opts { opt(t) } - t.Client = mock.NewClient(t.KubeNodes, t.CloudProviderInstances, cnr) + t.Client = mock.NewClient( + t.KubeNodes, t.CloudProviderInstances, t.extraKubeObjects..., + ) rm := &controller.ResourceManager{ Client: t.K8sClient, @@ -54,7 +76,7 @@ func NewFakeTransitioner(cnr *v1.CycleNodeRequest, opts ...Option) *Transitioner } t.CycleNodeRequestTransitioner = NewCycleNodeRequestTransitioner( - cnr, rm, Options{}, + cnr, rm, t.transitionerOptions, ) return t diff --git a/pkg/controller/cyclenoderequest/transitioner/transitions.go b/pkg/controller/cyclenoderequest/transitioner/transitions.go index 310c7e6..420b572 100644 --- a/pkg/controller/cyclenoderequest/transitioner/transitions.go +++ b/pkg/controller/cyclenoderequest/transitioner/transitions.go @@ -582,10 +582,18 @@ func (t *CycleNodeRequestTransitioner) transitionSuccessful() (reconcile.Result, if err != nil { return t.transitionToHealing(err) } + if shouldRequeue { return reconcile.Result{Requeue: true, RequeueAfter: transitionDuration}, nil } + // Delete failed sibling CNRs regardless of whether the CNR for the + // transitioner should be deleted. If failed CNRs pile up that will prevent + // Cyclops observer from auto-generating new CNRs for a nodegroup. + if err := t.deleteFailedSiblingCNRs(); err != nil { + return t.transitionToHealing(err) + } + // If deleting CycleNodeRequests is not enabled, stop here if !t.options.DeleteCNR { return reconcile.Result{}, nil @@ -595,10 +603,11 @@ func (t *CycleNodeRequestTransitioner) transitionSuccessful() (reconcile.Result, // than the time configured to keep them for. if t.cycleNodeRequest.CreationTimestamp.Add(t.options.DeleteCNRExpiry).Before(time.Now()) { t.rm.Logger.Info("Deleting CycleNodeRequest") - err := t.rm.Client.Delete(context.TODO(), t.cycleNodeRequest) - if err != nil { + + if err := t.rm.Client.Delete(context.TODO(), t.cycleNodeRequest); err != nil { t.rm.Logger.Error(err, "Unable to delete expired CycleNodeRequest") } + return reconcile.Result{}, nil } diff --git a/pkg/controller/cyclenoderequest/transitioner/transitions_pending_test.go b/pkg/controller/cyclenoderequest/transitioner/transitions_pending_test.go index f44231f..5fc0bca 100644 --- a/pkg/controller/cyclenoderequest/transitioner/transitions_pending_test.go +++ b/pkg/controller/cyclenoderequest/transitioner/transitions_pending_test.go @@ -546,6 +546,74 @@ func TestPendingTimeoutReached(t *testing.T) { assert.Equal(t, v1.CycleNodeRequestHealing, cnr.Status.Phase) } +// Test to ensure that Cyclops will not proceed if there is node detached from +// the nodegroup on the cloud provider. It should try to wait for the issue to +// resolve and transition to Initialised when it does before reaching the +// timeout period. +func TestPendingReattachedCloudProviderNode(t *testing.T) { + nodegroup, err := mock.NewNodegroup("ng-1", 2) + if err != nil { + assert.NoError(t, err) + } + + // "detach" one instance from the asg + nodegroup[0].Nodegroup = "" + + cnr := &v1.CycleNodeRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cnr-1", + Namespace: "kube-system", + }, + Spec: v1.CycleNodeRequestSpec{ + NodeGroupsList: []string{"ng-1"}, + CycleSettings: v1.CycleSettings{ + Concurrency: 1, + Method: v1.CycleNodeRequestMethodDrain, + }, + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "customer": "kitt", + }, + }, + }, + Status: v1.CycleNodeRequestStatus{ + Phase: v1.CycleNodeRequestPending, + }, + } + + fakeTransitioner := NewFakeTransitioner(cnr, + WithKubeNodes(nodegroup), + WithCloudProviderInstances(nodegroup), + ) + + // Should requeue while it tries to wait + _, err = fakeTransitioner.Run() + assert.NoError(t, err) + assert.Equal(t, v1.CycleNodeRequestPending, cnr.Status.Phase) + + // Simulate waiting for 1s less than the wait limit + cnr.Status.EquilibriumWaitStarted = &metav1.Time{ + Time: time.Now().Add(-nodeEquilibriumWaitLimit + time.Second), + } + + _, err = fakeTransitioner.Autoscaling.AttachInstances(&autoscaling.AttachInstancesInput{ + AutoScalingGroupName: aws.String("ng-1"), + InstanceIds: aws.StringSlice([]string{nodegroup[0].InstanceID}), + }) + + assert.NoError(t, err) + + // "re-attach" the instance to the asg + fakeTransitioner.CloudProviderInstances[0].Nodegroup = "ng-1" + + // The CNR should transition to the Initialised phase because the state of + // the nodes is now correct and this happened within the timeout period. + _, err = fakeTransitioner.Run() + assert.NoError(t, err) + assert.Equal(t, v1.CycleNodeRequestInitialised, cnr.Status.Phase) + assert.Len(t, cnr.Status.NodesToTerminate, 2) +} + // Test to ensure that Cyclops will not proceed if there is node detached from // the nodegroup on the cloud provider. It should wait and especially should not // succeed if the instance is re-attached by the final requeuing of the Pending @@ -596,6 +664,16 @@ func TestPendingReattachedCloudProviderNodeTooLate(t *testing.T) { Time: time.Now().Add(-nodeEquilibriumWaitLimit - time.Second), } + _, err = fakeTransitioner.Autoscaling.AttachInstances(&autoscaling.AttachInstancesInput{ + AutoScalingGroupName: aws.String("ng-1"), + InstanceIds: aws.StringSlice([]string{nodegroup[0].InstanceID}), + }) + + assert.NoError(t, err) + + // "re-attach" the instance to the asg + fakeTransitioner.CloudProviderInstances[0].Nodegroup = "ng-1" + // This time should transition to the healing phase even though the state // is correct because the timeout check happens first _, err = fakeTransitioner.Run() diff --git a/pkg/controller/cyclenoderequest/transitioner/transitions_successful_test.go b/pkg/controller/cyclenoderequest/transitioner/transitions_successful_test.go new file mode 100644 index 0000000..96a1467 --- /dev/null +++ b/pkg/controller/cyclenoderequest/transitioner/transitions_successful_test.go @@ -0,0 +1,323 @@ +package transitioner + +import ( + "context" + "testing" + "time" + + v1 "github.com/atlassian-labs/cyclops/pkg/apis/atlassian/v1" + "github.com/stretchr/testify/assert" + + "sigs.k8s.io/controller-runtime/pkg/client" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +// Test that if no delete options are given to the transitioner, then the CNR +// will not be deleted. +func TestSuccessfulNoDelete(t *testing.T) { + cnr := &v1.CycleNodeRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cnr-1", + Namespace: "kube-system", + CreationTimestamp: metav1.Now(), + }, + Status: v1.CycleNodeRequestStatus{ + Phase: v1.CycleNodeRequestSuccessful, + Message: "", + }, + } + + fakeTransitioner := NewFakeTransitioner(cnr) + + var list v1.CycleNodeRequestList + + assert.NoError(t, + fakeTransitioner.Client.K8sClient.List(context.TODO(), &list, &client.ListOptions{}), + ) + + assert.Len(t, list.Items, 1) + + // Execute the Successful phase + _, err := fakeTransitioner.Run() + assert.NoError(t, err) + + assert.NoError(t, + fakeTransitioner.Client.K8sClient.List(context.TODO(), &list, &client.ListOptions{}), + ) + + assert.Len(t, list.Items, 1) +} + +// Test that if the phase executes before the CNR deletion expiry time then the +// CNR won't be deleted. +func TestSuccessfulAfterDeleteTime(t *testing.T) { + cnr := &v1.CycleNodeRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cnr-1", + Namespace: "kube-system", + CreationTimestamp: metav1.Now(), + }, + Status: v1.CycleNodeRequestStatus{ + Phase: v1.CycleNodeRequestSuccessful, + Message: "", + }, + } + + fakeTransitioner := NewFakeTransitioner(cnr, + WithTransitionerOptions(Options{ + DeleteCNR: true, + DeleteCNRExpiry: 0 * time.Second, + }), + ) + + var list v1.CycleNodeRequestList + + assert.NoError(t, + fakeTransitioner.Client.K8sClient.List(context.TODO(), &list, &client.ListOptions{}), + ) + + assert.Len(t, list.Items, 1) + + // Execute the Successful phase + _, err := fakeTransitioner.Run() + assert.NoError(t, err) + + assert.NoError(t, + fakeTransitioner.Client.K8sClient.List(context.TODO(), &list, &client.ListOptions{}), + ) + + assert.Len(t, list.Items, 0) +} + +// Test that if the phase executes after the CNR deletion expiry time then the +// CNR will be deleted. +func TestSuccessfulBeforeDeleteTime(t *testing.T) { + cnr := &v1.CycleNodeRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cnr-1", + Namespace: "kube-system", + CreationTimestamp: metav1.Now(), + }, + Status: v1.CycleNodeRequestStatus{ + Phase: v1.CycleNodeRequestSuccessful, + Message: "", + }, + } + + fakeTransitioner := NewFakeTransitioner(cnr, + WithTransitionerOptions(Options{ + DeleteCNR: true, + DeleteCNRExpiry: 5 * time.Second, + }), + ) + + var list v1.CycleNodeRequestList + + assert.NoError(t, + fakeTransitioner.Client.K8sClient.List(context.TODO(), &list, &client.ListOptions{}), + ) + + assert.Len(t, list.Items, 1) + + // Execute the Successful phase + _, err := fakeTransitioner.Run() + assert.NoError(t, err) + + assert.NoError(t, + fakeTransitioner.Client.K8sClient.List(context.TODO(), &list, &client.ListOptions{}), + ) + + assert.Len(t, list.Items, 1) +} + +// Test that the Successful phase will delete sibling CNRs for the same +// nodegroup that are in the failed phase created at the same time or before the +// Successful CNR execution. No other CNRs should be deleted. +func TestSuccessfulDeleteFailedSiblingCNRs(t *testing.T) { + // CNR used for the execution + // Should NOT be deleted + cnr1 := &v1.CycleNodeRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cnr-1", + Namespace: "kube-system", + CreationTimestamp: metav1.Now(), + }, + Spec: v1.CycleNodeRequestSpec{ + NodeGroupName: "ng-1", + }, + Status: v1.CycleNodeRequestStatus{ + Phase: v1.CycleNodeRequestSuccessful, + Message: "", + }, + } + + // Failed CNR for the same nodegroup created at the same time as cnr1 + // Should be deleted + cnr2 := &v1.CycleNodeRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cnr-2", + Namespace: "kube-system", + CreationTimestamp: cnr1.CreationTimestamp, + }, + Spec: v1.CycleNodeRequestSpec{ + NodeGroupName: "ng-1", + }, + Status: v1.CycleNodeRequestStatus{ + Phase: v1.CycleNodeRequestFailed, + Message: "", + }, + } + + // Failed CNR for the same nodegroup created before cnr1 + // Should NOT be deleted + cnr3 := &v1.CycleNodeRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cnr-3", + Namespace: "kube-system", + CreationTimestamp: metav1.NewTime(cnr1.CreationTimestamp.Add(-time.Second)), + }, + Spec: v1.CycleNodeRequestSpec{ + NodeGroupName: "ng-1", + }, + Status: v1.CycleNodeRequestStatus{ + Phase: v1.CycleNodeRequestFailed, + Message: "", + }, + } + + // Failed CNR for the same nodegroup created after cnr1 + // Should NOT be deleted + cnr4 := &v1.CycleNodeRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cnr-4", + Namespace: "kube-system", + CreationTimestamp: metav1.NewTime(cnr1.CreationTimestamp.Add(time.Second)), + }, + Spec: v1.CycleNodeRequestSpec{ + NodeGroupName: "ng-1", + }, + Status: v1.CycleNodeRequestStatus{ + Phase: v1.CycleNodeRequestFailed, + Message: "", + }, + } + + // Pending CNR for the same nodegroup + // Should NOT be deleted + cnr5 := &v1.CycleNodeRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cnr-5", + Namespace: "kube-system", + CreationTimestamp: metav1.NewTime(cnr1.CreationTimestamp.Add(-time.Second)), + }, + Spec: v1.CycleNodeRequestSpec{ + NodeGroupName: "ng-1", + }, + Status: v1.CycleNodeRequestStatus{ + Phase: v1.CycleNodeRequestPending, + Message: "", + }, + } + + // Failed CNR for a different nodegroup + // Should NOT be deleted + cnr6 := &v1.CycleNodeRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cnr-6", + Namespace: "kube-system", + CreationTimestamp: metav1.NewTime(cnr1.CreationTimestamp.Add(-time.Second)), + }, + Spec: v1.CycleNodeRequestSpec{ + NodeGroupName: "ng-2", + }, + Status: v1.CycleNodeRequestStatus{ + Phase: v1.CycleNodeRequestFailed, + Message: "", + }, + } + + // Failed CNR for the same nodegroup in a different namespace + // Should NOT be deleted + cnr7 := &v1.CycleNodeRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cnr-7", + Namespace: "default", + CreationTimestamp: metav1.NewTime(cnr1.CreationTimestamp.Add(-time.Second)), + }, + Spec: v1.CycleNodeRequestSpec{ + NodeGroupName: "ng-1", + }, + Status: v1.CycleNodeRequestStatus{ + Phase: v1.CycleNodeRequestFailed, + Message: "", + }, + } + + fakeTransitioner := NewFakeTransitioner(cnr1, + WithExtraKubeObject(cnr2), + WithExtraKubeObject(cnr3), + WithExtraKubeObject(cnr4), + WithExtraKubeObject(cnr5), + WithExtraKubeObject(cnr6), + WithExtraKubeObject(cnr7), + ) + + var list v1.CycleNodeRequestList + + assert.NoError(t, + fakeTransitioner.Client.K8sClient.List(context.TODO(), &list, &client.ListOptions{}), + ) + + assert.Len(t, list.Items, 7) + + // Execute the Successful phase + _, err := fakeTransitioner.Run() + assert.NoError(t, err) + + assert.NoError(t, + fakeTransitioner.Client.K8sClient.List(context.TODO(), &list, &client.ListOptions{}), + ) + + assert.Len(t, list.Items, 5) + + var cnr v1.CycleNodeRequest + + assert.NoError(t, fakeTransitioner.Client.K8sClient.Get(context.TODO(), types.NamespacedName{ + Name: cnr1.Name, + Namespace: cnr1.Namespace, + }, &cnr)) + + // CNR 2 should be deleted + assert.Error(t, fakeTransitioner.Client.K8sClient.Get(context.TODO(), types.NamespacedName{ + Name: cnr2.Name, + Namespace: cnr2.Namespace, + }, &cnr)) + + // CNR 3 should be deleted + assert.Error(t, fakeTransitioner.Client.K8sClient.Get(context.TODO(), types.NamespacedName{ + Name: cnr3.Name, + Namespace: cnr3.Namespace, + }, &cnr)) + + assert.NoError(t, fakeTransitioner.Client.K8sClient.Get(context.TODO(), types.NamespacedName{ + Name: cnr4.Name, + Namespace: cnr4.Namespace, + }, &cnr)) + + assert.NoError(t, fakeTransitioner.Client.K8sClient.Get(context.TODO(), types.NamespacedName{ + Name: cnr5.Name, + Namespace: cnr5.Namespace, + }, &cnr)) + + assert.NoError(t, fakeTransitioner.Client.K8sClient.Get(context.TODO(), types.NamespacedName{ + Name: cnr6.Name, + Namespace: cnr6.Namespace, + }, &cnr)) + + assert.NoError(t, fakeTransitioner.Client.K8sClient.Get(context.TODO(), types.NamespacedName{ + Name: cnr7.Name, + Namespace: cnr7.Namespace, + }, &cnr)) +} diff --git a/pkg/controller/cyclenoderequest/transitioner/util.go b/pkg/controller/cyclenoderequest/transitioner/util.go index 36074fa..b1e1a2c 100644 --- a/pkg/controller/cyclenoderequest/transitioner/util.go +++ b/pkg/controller/cyclenoderequest/transitioner/util.go @@ -536,3 +536,45 @@ func (t *CycleNodeRequestTransitioner) validateInstanceState(validNodeGroupInsta return false, nil } + +// deleteFailedSiblingCNRs finds the CNRs generated for the same nodegroup as +// the one in the transitioner. It filters for deleted CNRs in the same +// namespace and deletes them. +func (t *CycleNodeRequestTransitioner) deleteFailedSiblingCNRs() error { + ctx := context.TODO() + + var list v1.CycleNodeRequestList + + err := t.rm.Client.List(ctx, &list, &client.ListOptions{ + Namespace: t.cycleNodeRequest.Namespace, + }) + + if err != nil { + return err + } + + for _, cnr := range list.Items { + // Filter out CNRs generated for another Nodegroup + if !t.cycleNodeRequest.IsFromSameNodeGroup(cnr) { + continue + } + + // Filter out CNRs not in the Failed phase + if cnr.Status.Phase != v1.CycleNodeRequestFailed { + continue + } + + // The Failed CNR should have been created prior to or at the same time + // as the CNR now in Successful otherwise new CNRs could fail and be + // cleaned up. + if t.cycleNodeRequest.CreationTimestamp.Before(&cnr.CreationTimestamp) { + continue + } + + if err := t.rm.Client.Delete(ctx, &cnr); err != nil { + return err + } + } + + return nil +} diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index 1e70699..c775070 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -5,11 +5,11 @@ import ( "fmt" "time" + v1 "github.com/atlassian-labs/cyclops/pkg/apis/atlassian/v1" "github.com/go-logr/logr" "github.com/prometheus/client_golang/prometheus" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/metrics" - v1 "github.com/atlassian-labs/cyclops/pkg/apis/atlassian/v1" ) const namespace = "cyclops" @@ -25,8 +25,8 @@ var ( // CycleNodeRequestsByPhase is the number of CycleNodeRequests in the cluster by phase CycleNodeRequestsByPhase = prometheus.NewDesc( fmt.Sprintf("%v_cycle_node_requests_by_phase", namespace), - "Number of CycleNodeRequests in the cluster by phase", - []string{"phase"}, + "Number of CycleNodeRequests in the cluster by phase for each nodegroup", + []string{"phase", "nodegroup"}, nil, ) // CycleNodeStatuses is the number of CycleNodeStatuses in the cluster @@ -51,6 +51,7 @@ func Register(client client.Client, logger logr.Logger, namespace string) { client: client, logger: logger, namespace: namespace, + nodeGroupList: &v1.NodeGroupList{}, cycleNodeRequestList: &v1.CycleNodeRequestList{}, cycleNodeStatusList: &v1.CycleNodeStatusList{}, } @@ -68,6 +69,7 @@ type cyclopsCollector struct { client client.Client logger logr.Logger namespace string + nodeGroupList *v1.NodeGroupList cycleNodeRequestList *v1.CycleNodeRequestList cycleNodeStatusList *v1.CycleNodeStatusList } @@ -77,11 +79,20 @@ func (c cyclopsCollector) fetch() { listOptions := client.ListOptions{ Namespace: c.namespace, } - err := c.client.List(context.TODO(), c.cycleNodeRequestList, &listOptions) + + // NodeGroup is a cluster scoped resource + err := c.client.List(context.TODO(), c.nodeGroupList, &client.ListOptions{}) + if err != nil { + c.logger.Error(err, "unable to list NodeGroups for metrics") + return + } + + err = c.client.List(context.TODO(), c.cycleNodeRequestList, &listOptions) if err != nil { c.logger.Error(err, "unable to list CycleNodeRequests for metrics") return } + err = c.client.List(context.TODO(), c.cycleNodeStatusList, &listOptions) if err != nil { c.logger.Error(err, "unable to list CycleNodeStatuses for metrics") @@ -97,18 +108,44 @@ func (c cyclopsCollector) Describe(ch chan<- *prometheus.Desc) { } func (c cyclopsCollector) Collect(ch chan<- prometheus.Metric) { - requestPhaseCounts := make(map[string]int) + // map counting CNRs in each phase for each nodegroup + requestPhaseCounts := make(map[string]map[string]int) + requestPhaseCounts[""] = make(map[string]int) + + for _, nodegroup := range c.nodeGroupList.Items { + requestPhaseCounts[nodegroup.Name] = make(map[string]int) + } + for _, cycleNodeRequest := range c.cycleNodeRequestList.Items { - requestPhaseCounts[string(cycleNodeRequest.Status.Phase)]++ + partOfANodegroup := false + + for _, nodegroup := range c.nodeGroupList.Items { + if cycleNodeRequest.IsFromNodeGroup(nodegroup) { + requestPhaseCounts[nodegroup.Name][string(cycleNodeRequest.Status.Phase)]++ + partOfANodegroup = true + break + } + } + + // Account manually created CNRs which do not share the same defined + // node group names as any NodeGroups. + if !partOfANodegroup { + requestPhaseCounts[""][string(cycleNodeRequest.Status.Phase)]++ + } } - for phase, count := range requestPhaseCounts { - ch <- prometheus.MustNewConstMetric( - CycleNodeRequestsByPhase, - prometheus.GaugeValue, - float64(count), - phase, - ) + + for nodegroupName, cycleNodeRequestsByPhase := range requestPhaseCounts { + for phase, count := range cycleNodeRequestsByPhase { + ch <- prometheus.MustNewConstMetric( + CycleNodeRequestsByPhase, + prometheus.GaugeValue, + float64(count), + phase, + nodegroupName, + ) + } } + ch <- prometheus.MustNewConstMetric( CycleNodeRequests, prometheus.GaugeValue, @@ -116,9 +153,11 @@ func (c cyclopsCollector) Collect(ch chan<- prometheus.Metric) { ) statusPhaseCounts := make(map[string]int) + for _, cycleNodeStatus := range c.cycleNodeStatusList.Items { statusPhaseCounts[string(cycleNodeStatus.Status.Phase)]++ } + for phase, count := range statusPhaseCounts { ch <- prometheus.MustNewConstMetric( CycleNodeStatusesByPhase, @@ -127,6 +166,7 @@ func (c cyclopsCollector) Collect(ch chan<- prometheus.Metric) { phase, ) } + ch <- prometheus.MustNewConstMetric( CycleNodeStatuses, prometheus.GaugeValue, diff --git a/pkg/observer/controller.go b/pkg/observer/controller.go index 4ce76ee..cf7f30a 100644 --- a/pkg/observer/controller.go +++ b/pkg/observer/controller.go @@ -230,21 +230,47 @@ func (c *controller) inProgressCNRs() v1.CycleNodeRequestList { // dropInProgressNodeGroups matches nodeGroups to CNRs and filters out any that match func (c *controller) dropInProgressNodeGroups(nodeGroups v1.NodeGroupList, cnrs v1.CycleNodeRequestList) v1.NodeGroupList { - // filter out nodegroups that aren't current in progress with a cnr + // Filter out nodegroups that aren't currently in progress with a cnr. Count + // failed CNRs only if they don't outnumber the max threshold defined for + // the nodegroup var restingNodeGroups v1.NodeGroupList + for i, nodeGroup := range nodeGroups.Items { - var found bool + var dropNodeGroup bool + var failedCNRsFound uint + for _, cnr := range cnrs.Items { - if sameNodeGroups(cnr.GetNodeGroupNames(), nodeGroup.GetNodeGroupNames()) { - found = true - break + // CNR doesn't match nodegroup, skip it + if !cnr.IsFromNodeGroup(nodeGroup) { + continue + } + + // Count the Failed CNRs separately, they need to be counted before + // they can be considered to drop the nodegroup + if cnr.Status.Phase == v1.CycleNodeRequestFailed { + failedCNRsFound++ + } else { + dropNodeGroup = true + } + + // If the number of Failed CNRs exceeds the threshold in the + // nodegroup then drop it + if failedCNRsFound > nodeGroup.Spec.MaxFailedCycleNodeRequests { + dropNodeGroup = true } } - if found { - klog.Warningf("nodegroup %q has an in progress CNR.. skipping this nodegroup", nodeGroup.Name) + + if dropNodeGroup { + if failedCNRsFound > nodeGroup.Spec.MaxFailedCycleNodeRequests { + klog.Warningf("nodegroup %q has too many failed CNRs.. skipping this nodegroup", nodeGroup.Name) + } else { + klog.Warningf("nodegroup %q has an in progress CNR.. skipping this nodegroup", nodeGroup.Name) + } + c.NodeGroupsLocked.WithLabelValues(nodeGroup.Name).Inc() continue } + restingNodeGroups.Items = append(restingNodeGroups.Items, nodeGroups.Items[i]) } @@ -439,20 +465,3 @@ func (c *controller) RunForever() { } } } - -func sameNodeGroups(groupA, groupB []string) bool { - if len(groupA) != len(groupB) { - return false - } - - groupMap := make(map[string]struct{}) - for _, group := range groupA { - groupMap[group] = struct{}{} - } - for _, group := range groupB { - if _, ok := groupMap[group]; !ok { - return false - } - } - return true -} diff --git a/pkg/observer/controller_test.go b/pkg/observer/controller_test.go index 3157ea0..df102ff 100644 --- a/pkg/observer/controller_test.go +++ b/pkg/observer/controller_test.go @@ -3,9 +3,10 @@ package observer import ( "context" "fmt" - "sigs.k8s.io/controller-runtime/pkg/client" "testing" + "sigs.k8s.io/controller-runtime/pkg/client" + atlassianv1 "github.com/atlassian-labs/cyclops/pkg/apis/atlassian/v1" "github.com/atlassian-labs/cyclops/pkg/test" "github.com/stretchr/testify/assert" @@ -190,7 +191,7 @@ func Test_inProgressCNRs(t *testing.T) { } } -func Test_dropInProgressNodeGroups(t *testing.T) { +func TestScenarios_dropInProgressNodeGroups(t *testing.T) { scenario := test.BuildTestScenario(test.ScenarioOpts{ Keys: []string{"a", "b", "c"}, @@ -279,43 +280,106 @@ func Test_dropInProgressNodeGroups(t *testing.T) { } } -func Test_sameNodeGroups(t *testing.T) { +func Test_dropInProgressNodeGroups(t *testing.T) { + nodegroup := atlassianv1.NodeGroup{ + ObjectMeta: v1.ObjectMeta{ + Name: "ng-1", + Namespace: "kube-system", + }, + Spec: atlassianv1.NodeGroupSpec{ + NodeGroupName: "ng-1", + }, + } + + cnr1 := atlassianv1.CycleNodeRequest{ + ObjectMeta: v1.ObjectMeta{ + Name: "cnr-1", + Namespace: "kube-system", + }, + Spec: atlassianv1.CycleNodeRequestSpec{ + NodeGroupName: "ng-1", + }, + Status: atlassianv1.CycleNodeRequestStatus{ + Phase: atlassianv1.CycleNodeRequestPending, + }, + } + + cnr2 := atlassianv1.CycleNodeRequest{ + ObjectMeta: v1.ObjectMeta{ + Name: "cnr-2", + Namespace: "kube-system", + }, + Spec: atlassianv1.CycleNodeRequestSpec{ + NodeGroupName: "ng-1", + }, + Status: atlassianv1.CycleNodeRequestStatus{ + Phase: atlassianv1.CycleNodeRequestFailed, + }, + } + tests := []struct { - name string - groupA []string - groupB []string - expect bool + name string + threshold int + CNRs []atlassianv1.CycleNodeRequest + dropNodegroup bool }{ { - "pass case with same order", - []string{"ingress-us-west-2a", "ingress-us-west-2b", "ingress-us-west-2c"}, - []string{"ingress-us-west-2a", "ingress-us-west-2b", "ingress-us-west-2c"}, - true, + "test no CNRs for nodegroups", + 0, + []atlassianv1.CycleNodeRequest{}, + false, }, { - "pass case with different order", - []string{"ingress-us-west-2a", "ingress-us-west-2b", "ingress-us-west-2c"}, - []string{"ingress-us-west-2b", "ingress-us-west-2c", "ingress-us-west-2a"}, + "test Pending CNR for nodegroup", + 0, + []atlassianv1.CycleNodeRequest{cnr1}, true, }, { - "failure case with different length", - []string{"ingress-us-west-2a", "ingress-us-west-2b", "ingress-us-west-2c"}, - []string{"ingress-us-west-2b", "ingress-us-west-2c"}, + "test less failed CNRs than threshold", + 2, + []atlassianv1.CycleNodeRequest{cnr2}, false, }, { - "failure case with different items", - []string{"ingress-us-west-2a", "ingress-us-west-2b", "ingress-us-west-2c"}, - []string{"ingress-us-west-2b", "ingress-us-west-2c", "system"}, + "test same number of failed CNRs as threshold", + 1, + []atlassianv1.CycleNodeRequest{cnr2}, false, }, + { + "test more failed CNRs than threshold", + 0, + []atlassianv1.CycleNodeRequest{cnr2}, + true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := sameNodeGroups(tt.groupA, tt.groupB) - assert.Equal(t, tt.expect, got) + c := controller{ + client: nil, + Options: Options{}, + metrics: newMetrics(), + } + + nodegroup.Spec.MaxFailedCycleNodeRequests = uint(tt.threshold) + + nodegroupList := atlassianv1.NodeGroupList{ + Items: []atlassianv1.NodeGroup{nodegroup}, + } + + cnrList := atlassianv1.CycleNodeRequestList{ + Items: tt.CNRs, + } + + got := c.dropInProgressNodeGroups(nodegroupList, cnrList) + + if tt.dropNodegroup { + assert.Len(t, got.Items, 0) + } else { + assert.Len(t, got.Items, 1) + } }) } }