diff --git a/pkg/controller/clusterpool/clusterpool_controller.go b/pkg/controller/clusterpool/clusterpool_controller.go index 5cd81ca8035..29e55d877e5 100644 --- a/pkg/controller/clusterpool/clusterpool_controller.go +++ b/pkg/controller/clusterpool/clusterpool_controller.go @@ -285,7 +285,9 @@ func (r *ReconcileClusterPool) Reconcile(ctx context.Context, request reconcile. return reconcile.Result{}, nil } - cds, err := getAllClusterDeploymentsForPool(r.Client, clp, logger) + poolVersion := calculatePoolVersion(clp) + + cds, err := getAllClusterDeploymentsForPool(r.Client, clp, poolVersion, logger) if err != nil { return reconcile.Result{}, err } @@ -355,8 +357,6 @@ func (r *ReconcileClusterPool) Reconcile(ctx context.Context, request reconcile. } availableCurrent -= toDel - poolVersion := calculatePoolVersion(clp) - switch drift := reserveSize - int(clp.Spec.Size); { // activity quota exceeded, so no action case availableCurrent <= 0: @@ -383,6 +383,16 @@ func (r *ReconcileClusterPool) Reconcile(ctx context.Context, request reconcile. log.WithError(err).Error("error adding clusters") return reconcile.Result{}, err } + // Special case for stale CDs: allow deleting one if all CDs are installed. + case drift == 0 && len(cds.Installing()) == 0 && len(cds.Stale()) > 0: + toDelete := cds.Stale()[0] + logger := logger.WithField("cluster", toDelete.Name) + logger.Info("deleting cluster deployment") + if err := cds.Delete(r.Client, toDelete.Name); err != nil { + logger.WithError(err).Error("error deleting cluster deployment") + return reconcile.Result{}, err + } + metricStaleClusterDeploymentsDeleted.WithLabelValues(clp.Namespace, clp.Name).Inc() } // One more (possible) status update: wait until the end to detect whether all unassigned CDs @@ -704,7 +714,8 @@ func (r *ReconcileClusterPool) reconcileDeletedPool(pool *hivev1.ClusterPool, lo if !controllerutils.HasFinalizer(pool, finalizer) { return nil } - cds, err := getAllClusterDeploymentsForPool(r.Client, pool, logger) + // Don't care about the poolVersion here since we're deleting everything. + cds, err := getAllClusterDeploymentsForPool(r.Client, pool, "", logger) if err != nil { return err } diff --git a/pkg/controller/clusterpool/clusterpool_controller_test.go b/pkg/controller/clusterpool/clusterpool_controller_test.go index 79eeef1acc0..6554d2ae1f9 100644 --- a/pkg/controller/clusterpool/clusterpool_controller_test.go +++ b/pkg/controller/clusterpool/clusterpool_controller_test.go @@ -156,6 +156,110 @@ func TestReconcileClusterPool(t *testing.T) { }, expectPoolVersionChanged: true, }, + { + // This also proves we only delete one stale cluster at a time + name: "delete oldest stale cluster first", + existing: []runtime.Object{ + initializedPoolBuilder.Build(testcp.WithSize(2)), + unclaimedCDBuilder("c1").Build( + testcd.WithPoolVersion("abc123"), + testcd.Installed(), + testcd.Generic(generic.WithCreationTimestamp(nowish)), + ), + unclaimedCDBuilder("c2").Build( + testcd.WithPoolVersion("def345"), + testcd.Installed(), + testcd.Generic(generic.WithCreationTimestamp(nowish.Add(-time.Hour))), + ), + }, + expectedTotalClusters: 1, + // Note: these observed counts are calculated before we start adding/deleting + // clusters, so they include the stale one we deleted. + expectedObservedSize: 2, + expectedObservedReady: 2, + expectedCDCurrentStatus: corev1.ConditionFalse, + expectedDeletedClusters: []string{"c2"}, + }, + { + name: "delete stale unknown-version cluster first", + existing: []runtime.Object{ + initializedPoolBuilder.Build(testcp.WithSize(2)), + // Two CDs: One with an "unknown" poolVersion, the other older. Proving we favor the unknown for deletion. + unclaimedCDBuilder("c1").Build( + testcd.WithPoolVersion(""), + testcd.Installed(), + testcd.Generic(generic.WithCreationTimestamp(nowish)), + ), + unclaimedCDBuilder("c2").Build( + testcd.WithPoolVersion("bogus, but set"), + testcd.Installed(), + testcd.Generic(generic.WithCreationTimestamp(nowish.Add(-time.Hour))), + ), + }, + expectedTotalClusters: 1, + expectedObservedSize: 2, + expectedObservedReady: 2, + expectedCDCurrentStatus: corev1.ConditionFalse, + expectedDeletedClusters: []string{"c1"}, + }, + { + name: "stale clusters not deleted if any CD still installing", + existing: []runtime.Object{ + initializedPoolBuilder.Build(testcp.WithSize(2)), + unclaimedCDBuilder("c1").Build(), + unclaimedCDBuilder("c2").Build( + testcd.WithPoolVersion("stale"), + testcd.Installed(), + testcd.Generic(generic.WithCreationTimestamp(nowish.Add(-time.Hour))), + ), + }, + expectedTotalClusters: 2, + expectedObservedSize: 2, + expectedObservedReady: 1, + expectedCDCurrentStatus: corev1.ConditionFalse, + }, + { + name: "stale clusters not deleted if under capacity", + existing: []runtime.Object{ + initializedPoolBuilder.Build(testcp.WithSize(3)), + unclaimedCDBuilder("c1").Build( + testcd.Installed(), + ), + unclaimedCDBuilder("c2").Build( + testcd.WithPoolVersion("stale"), + testcd.Installed(), + testcd.Generic(generic.WithCreationTimestamp(nowish.Add(-time.Hour))), + ), + }, + expectedTotalClusters: 3, + expectedObservedSize: 2, + expectedObservedReady: 2, + expectedCDCurrentStatus: corev1.ConditionFalse, + }, + { + name: "stale clusters not deleted if over capacity", + existing: []runtime.Object{ + initializedPoolBuilder.Build(testcp.WithSize(1)), + unclaimedCDBuilder("c1").Build( + testcd.Installed(), + ), + unclaimedCDBuilder("c2").Build( + testcd.WithPoolVersion("stale"), + testcd.Installed(), + testcd.Generic(generic.WithCreationTimestamp(nowish.Add(-time.Hour))), + ), + }, + // This deletion happens because we're over capacity, not because we have staleness. + // This is proven below. + expectedTotalClusters: 1, + expectedObservedSize: 2, + expectedObservedReady: 2, + expectedCDCurrentStatus: corev1.ConditionFalse, + // So this is kind of interesting. We delete c1 even though c2 is a) older, b) stale. + // This is because we prioritize keeping installed clusters, as they can satisfy claims + // more quickly. Possible subject of a future optimization. + expectedDeletedClusters: []string{"c1"}, + }, { name: "create all clusters", existing: []runtime.Object{ diff --git a/pkg/controller/clusterpool/collections.go b/pkg/controller/clusterpool/collections.go index ac6e5ecd48e..7e029671dd0 100644 --- a/pkg/controller/clusterpool/collections.go +++ b/pkg/controller/clusterpool/collections.go @@ -198,6 +198,10 @@ type cdCollection struct { deleting []*hivev1.ClusterDeployment // Clusters with the ClusterClaimRemoveClusterAnnotation. Mutually exclusive with deleting. markedForDeletion []*hivev1.ClusterDeployment + // Clusters with a missing or empty pool version annotation + unknownPoolVersion []*hivev1.ClusterDeployment + // Clusters whose pool version annotation doesn't match the pool's + mismatchedPoolVersion []*hivev1.ClusterDeployment // All CDs in this pool byCDName map[string]*hivev1.ClusterDeployment // This contains only claimed CDs @@ -206,7 +210,7 @@ type cdCollection struct { // getAllClusterDeploymentsForPool is the constructor for a cdCollection // comprising all the ClusterDeployments created for the specified ClusterPool. -func getAllClusterDeploymentsForPool(c client.Client, pool *hivev1.ClusterPool, logger log.FieldLogger) (*cdCollection, error) { +func getAllClusterDeploymentsForPool(c client.Client, pool *hivev1.ClusterPool, poolVersion string, logger log.FieldLogger) (*cdCollection, error) { cdList := &hivev1.ClusterDeploymentList{} if err := c.List(context.Background(), cdList, client.MatchingFields{cdClusterPoolIndex: poolKey(pool.GetNamespace(), pool.GetName())}); err != nil { @@ -214,11 +218,13 @@ func getAllClusterDeploymentsForPool(c client.Client, pool *hivev1.ClusterPool, return nil, err } cdCol := cdCollection{ - assignable: make([]*hivev1.ClusterDeployment, 0), - installing: make([]*hivev1.ClusterDeployment, 0), - deleting: make([]*hivev1.ClusterDeployment, 0), - byCDName: make(map[string]*hivev1.ClusterDeployment), - byClaimName: make(map[string]*hivev1.ClusterDeployment), + assignable: make([]*hivev1.ClusterDeployment, 0), + installing: make([]*hivev1.ClusterDeployment, 0), + deleting: make([]*hivev1.ClusterDeployment, 0), + unknownPoolVersion: make([]*hivev1.ClusterDeployment, 0), + mismatchedPoolVersion: make([]*hivev1.ClusterDeployment, 0), + byCDName: make(map[string]*hivev1.ClusterDeployment), + byClaimName: make(map[string]*hivev1.ClusterDeployment), } for i, cd := range cdList.Items { poolRef := cd.Spec.ClusterPoolRef @@ -245,6 +251,16 @@ func getAllClusterDeploymentsForPool(c client.Client, pool *hivev1.ClusterPool, } else { cdCol.installing = append(cdCol.installing, ref) } + // Count stale CDs (poolVersion either unknown or mismatched) + if cdPoolVersion, ok := cd.Annotations[constants.ClusterDeploymentPoolSpecHashAnnotation]; !ok || cdPoolVersion == "" { + // Annotation is either missing or empty. This could be due to upgrade (this CD was + // created before this code was installed) or manual intervention (outside agent mucked + // with the annotation). Either way we don't know whether the CD matches or not. + cdCol.unknownPoolVersion = append(cdCol.unknownPoolVersion, ref) + } else if cdPoolVersion != poolVersion { + cdCol.mismatchedPoolVersion = append(cdCol.mismatchedPoolVersion, ref) + } + } // Register all claimed CDs, even if they're deleting/marked if claimName != "" { @@ -270,6 +286,19 @@ func getAllClusterDeploymentsForPool(c client.Client, pool *hivev1.ClusterPool, return cdCol.installing[i].CreationTimestamp.After(cdCol.installing[j].CreationTimestamp.Time) }, ) + // Sort stale CDs by age so we delete the oldest first + sort.Slice( + cdCol.unknownPoolVersion, + func(i, j int) bool { + return cdCol.unknownPoolVersion[i].CreationTimestamp.Before(&cdCol.unknownPoolVersion[j].CreationTimestamp) + }, + ) + sort.Slice( + cdCol.mismatchedPoolVersion, + func(i, j int) bool { + return cdCol.mismatchedPoolVersion[i].CreationTimestamp.Before(&cdCol.mismatchedPoolVersion[j].CreationTimestamp) + }, + ) logger.WithFields(log.Fields{ "assignable": len(cdCol.assignable), @@ -322,6 +351,24 @@ func (cds *cdCollection) Installing() []*hivev1.ClusterDeployment { return cds.installing } +// UnknownPoolVersion returns the list of ClusterDeployments whose pool version annotation is +// missing or empty. +func (cds *cdCollection) UnknownPoolVersion() []*hivev1.ClusterDeployment { + return cds.unknownPoolVersion +} + +// MismatchedPoolVersion returns the list of ClusterDeployments whose pool version annotation +// doesn't match the version of the pool. +func (cds *cdCollection) MismatchedPoolVersion() []*hivev1.ClusterDeployment { + return cds.mismatchedPoolVersion +} + +// Stale returns the list of ClusterDeployments whose pool version annotation doesn't match the +// version of the pool. Put "unknown" first becuase they're annoying. +func (cds *cdCollection) Stale() []*hivev1.ClusterDeployment { + return append(cds.unknownPoolVersion, cds.mismatchedPoolVersion...) +} + // Assign assigns the specified ClusterDeployment to the specified claim, updating its spec on the // server. Errors from the update are bubbled up. Returns an error if the CD is already assigned // (to *any* claim). The CD must be from the Assignable() list; otherwise it is an error. @@ -409,6 +456,8 @@ func (cds *cdCollection) Delete(c client.Client, cdName string) error { removeCDsFromSlice(&cds.assignable, cdName) removeCDsFromSlice(&cds.installing, cdName) removeCDsFromSlice(&cds.assignable, cdName) + removeCDsFromSlice(&cds.unknownPoolVersion, cdName) + removeCDsFromSlice(&cds.mismatchedPoolVersion, cdName) removeCDsFromSlice(&cds.markedForDeletion, cdName) return nil } @@ -416,36 +465,26 @@ func (cds *cdCollection) Delete(c client.Client, cdName string) error { // setCDsCurrentCondition idempotently sets the ClusterDeploymentsCurrent condition on the // ClusterPool according to whether all unassigned CDs have the same PoolVersion as the pool. func setCDsCurrentCondition(c client.Client, cds *cdCollection, clp *hivev1.ClusterPool, poolVersion string) error { - // CDs with mismatched poolVersion - mismatchedCDs := make([]string, 0) - // CDs with no poolVersion - unknownCDs := make([]string, 0) - - for _, cd := range append(cds.Assignable(), cds.Installing()...) { - if cdPoolVersion, ok := cd.Annotations[constants.ClusterDeploymentPoolSpecHashAnnotation]; !ok || cdPoolVersion == "" { - // Annotation is either missing or empty. This could be due to upgrade (this CD was - // created before this code was installed) or manual intervention (outside agent mucked - // with the annotation). Either way we don't know whether the CD matches or not. - unknownCDs = append(unknownCDs, cd.Name) - } else if cdPoolVersion != poolVersion { - mismatchedCDs = append(mismatchedCDs, cd.Name) - } - } - var status corev1.ConditionStatus var reason, message string - if len(mismatchedCDs) != 0 { + names := func(cdList []*hivev1.ClusterDeployment) string { + names := make([]string, len(cdList)) + for i, cd := range cdList { + names[i] = cd.Name + } + sort.Strings(names) + return strings.Join(names, ", ") + } + if len(cds.MismatchedPoolVersion()) != 0 { // We can assert staleness if there are any mismatches status = corev1.ConditionFalse reason = "SomeClusterDeploymentsStale" - sort.Strings(mismatchedCDs) - message = fmt.Sprintf("Some unassigned ClusterDeployments do not match the pool configuration: %s", strings.Join(mismatchedCDs, ", ")) - } else if len(unknownCDs) != 0 { + message = fmt.Sprintf("Some unassigned ClusterDeployments do not match the pool configuration: %s", names(cds.MismatchedPoolVersion())) + } else if len(cds.UnknownPoolVersion()) != 0 { // There are no mismatches, but some unknowns. Note that this is a different "unknown" from "we haven't looked yet". status = corev1.ConditionUnknown reason = "SomeClusterDeploymentsUnknown" - sort.Strings(unknownCDs) - message = fmt.Sprintf("Some unassigned ClusterDeployments are missing their pool spec hash annotation: %s", strings.Join(unknownCDs, ", ")) + message = fmt.Sprintf("Some unassigned ClusterDeployments are missing their pool spec hash annotation: %s", names(cds.UnknownPoolVersion())) } else { // All match (or there are no CDs, which is also fine) status = corev1.ConditionTrue diff --git a/pkg/controller/clusterpool/metrics.go b/pkg/controller/clusterpool/metrics.go new file mode 100644 index 00000000000..06bdcb93f5f --- /dev/null +++ b/pkg/controller/clusterpool/metrics.go @@ -0,0 +1,22 @@ +package clusterpool + +import ( + "github.com/prometheus/client_golang/prometheus" + + "sigs.k8s.io/controller-runtime/pkg/metrics" +) + +var ( + // metricStaleClusterDeploymentsDeleted tracks the total number of CDs we delete because they + // became "stale". That is, the ClusterPool was modified in a substantive way such that these + // CDs no longer match its spec. Note that this only counts stale CDs we've *deleted* -- there + // may be other stale CDs we haven't gotten around to deleting yet. + metricStaleClusterDeploymentsDeleted = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "hive_clusterpool_stale_clusterdeployments_deleted", + Help: "The number of ClusterDeployments deleted because they no longer match the spec of their ClusterPool.", + }, []string{"clusterpool_namespace", "clusterpool_name"}) +) + +func init() { + metrics.Registry.MustRegister() +} diff --git a/pkg/controller/metrics/metrics.go b/pkg/controller/metrics/metrics.go index 9ad8660b582..4d2221eeb70 100644 --- a/pkg/controller/metrics/metrics.go +++ b/pkg/controller/metrics/metrics.go @@ -385,7 +385,7 @@ func processJobs(jobs []batchv1.Job) (runningTotal, succeededTotal, failedTotal } // clusterAccumulator is an object used to process cluster deployments and sort them so we can -// increment the appropriate metrics counter based on it's type, installed state, length of time +// increment the appropriate metrics counter based on its type, installed state, length of time // it has been uninstalled, and the conditions it has. type clusterAccumulator struct { // ageFilter can optionally be specified to skip processing clusters older than this duration. If this is not desired,