From 018dc20afccc4ac151f1d84af7f5b93b60f3772d Mon Sep 17 00:00:00 2001 From: Kalaiselvim <117940852+Kalaiselvi84@users.noreply.github.com> Date: Wed, 22 Nov 2023 21:42:18 +0000 Subject: [PATCH] Move ResetMetricsOnDelete to Stable (#3518) * Move ResetMetricsOnDelete to Stable * Modify controller_test.go * changes * Delete test functions to pass when condition was false --- cloudbuild.yaml | 4 +- install/helm/agones/defaultfeaturegates.yaml | 1 - pkg/metrics/controller.go | 34 ++-- pkg/metrics/controller_test.go | 149 ------------------ pkg/util/runtime/features.go | 5 - site/content/en/docs/Guides/feature-stages.md | 2 - site/content/en/docs/Guides/metrics.md | 7 + 7 files changed, 18 insertions(+), 184 deletions(-) diff --git a/cloudbuild.yaml b/cloudbuild.yaml index e031dfeaa0..b0433c8986 100644 --- a/cloudbuild.yaml +++ b/cloudbuild.yaml @@ -296,10 +296,10 @@ steps: region=${versionsAndRegions[$version]} if [ $cloudProduct = generic ] then - featureWithGate="PlayerAllocationFilter=true&PlayerTracking=true&ResetMetricsOnDelete=false&SplitControllerAndExtensions=false&FleetAllocationOverflow=false&CountsAndLists=true&DisableResyncOnSDKServer=true&Example=true" + featureWithGate="PlayerAllocationFilter=true&PlayerTracking=true&SplitControllerAndExtensions=false&FleetAllocationOverflow=false&CountsAndLists=true&DisableResyncOnSDKServer=true&Example=true" testCluster="standard-e2e-test-cluster-${version//./-}" else - featureWithGate="PlayerAllocationFilter=true&PlayerTracking=true&ResetMetricsOnDelete=false&SplitControllerAndExtensions=true&FleetAllocationOverflow=false&CountsAndLists=true&DisableResyncOnSDKServer=true&Example=true" + featureWithGate="PlayerAllocationFilter=true&PlayerTracking=true&SplitControllerAndExtensions=true&FleetAllocationOverflow=false&CountsAndLists=true&DisableResyncOnSDKServer=true&Example=true" testCluster="gke-autopilot-e2e-test-cluster-${version//./-}" fi featureWithoutGate="" diff --git a/install/helm/agones/defaultfeaturegates.yaml b/install/helm/agones/defaultfeaturegates.yaml index 54312550f4..3b2c456a21 100644 --- a/install/helm/agones/defaultfeaturegates.yaml +++ b/install/helm/agones/defaultfeaturegates.yaml @@ -16,7 +16,6 @@ # Beta features FleetAllocationOverflow: true -ResetMetricsOnDelete: true SplitControllerAndExtensions: true # Alpha features diff --git a/pkg/metrics/controller.go b/pkg/metrics/controller.go index d79deff7d0..3ca067fc95 100644 --- a/pkg/metrics/controller.go +++ b/pkg/metrics/controller.go @@ -217,25 +217,13 @@ func (c *Controller) recordFleetAutoScalerChanges(old, next interface{}) { } func (c *Controller) recordFleetAutoScalerDeletion(obj interface{}) { - fas, ok := obj.(*autoscalingv1.FleetAutoscaler) + _, ok := obj.(*autoscalingv1.FleetAutoscaler) if !ok { return } - if runtime.FeatureEnabled(runtime.FeatureResetMetricsOnDelete) { - if err := c.resyncFleetAutoScaler(); err != nil { - c.logger.WithError(err).Warn("Could not resync Fleet Autoscaler metrics") - } - } else { - ctx, _ := tag.New(context.Background(), tag.Upsert(keyName, fas.Name), - tag.Upsert(keyFleetName, fas.Spec.FleetName), tag.Upsert(keyNamespace, fas.Namespace)) - - // recording status - stats.Record(ctx, - fasCurrentReplicasStats.M(int64(0)), - fasDesiredReplicasStats.M(int64(0)), - fasAbleToScaleStats.M(int64(0)), - fasLimitedStats.M(int64(0))) + if err := c.resyncFleetAutoScaler(); err != nil { + c.logger.WithError(err).Warn("Could not resync Fleet Autoscaler metrics") } } @@ -256,20 +244,16 @@ func (c *Controller) recordFleetChanges(obj interface{}) { } func (c *Controller) recordFleetDeletion(obj interface{}) { - f, ok := obj.(*agonesv1.Fleet) + _, ok := obj.(*agonesv1.Fleet) if !ok { return } - if runtime.FeatureEnabled(runtime.FeatureResetMetricsOnDelete) { - if err := c.resyncFleets(); err != nil { - // If for some reason resync fails, the entire metric state for fleets - // will be reset whenever the next Fleet gets deleted, in which case - // we end up back in a healthy state - so we aren't going to actively retry. - c.logger.WithError(err).Warn("Could not resync Fleet Metrics") - } - } else { - c.recordFleetReplicas(f.Name, f.Namespace, 0, 0, 0, 0, 0) + if err := c.resyncFleets(); err != nil { + // If for some reason resync fails, the entire metric state for fleets + // will be reset whenever the next Fleet gets deleted, in which case + // we end up back in a healthy state - so we aren't going to actively retry. + c.logger.WithError(err).Warn("Could not resync Fleet Metrics") } } diff --git a/pkg/metrics/controller_test.go b/pkg/metrics/controller_test.go index 4d15c52729..7493c42a11 100644 --- a/pkg/metrics/controller_test.go +++ b/pkg/metrics/controller_test.go @@ -16,7 +16,6 @@ package metrics import ( "context" - "fmt" "strings" "testing" "time" @@ -289,66 +288,9 @@ func TestControllerGameServersTotal(t *testing.T) { }) } -func TestControllerFleetReplicasCount(t *testing.T) { - runtime.FeatureTestMutex.Lock() - defer runtime.FeatureTestMutex.Unlock() - require.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=false", runtime.FeatureResetMetricsOnDelete))) - - resetMetrics() - exporter := &metricExporter{} - reader := metricexport.NewReader() - c := newFakeController() - defer c.close() - c.run(t) - - f := fleet("fleet-test", 8, 2, 5, 1, 1) - fd := fleet("fleet-deleted", 100, 100, 100, 100, 100) - c.fleetWatch.Add(f) - f = f.DeepCopy() - f.Status.ReadyReplicas = 1 - f.Spec.Replicas = 5 - c.fleetWatch.Modify(f) - c.fleetWatch.Add(fd) - c.fleetWatch.Delete(fd) - - // wait until we have a fleet deleted and it's allocation count is 0 - // since that is our last operation - require.Eventually(t, func() bool { - ex := &metricExporter{} - reader.ReadAndExport(ex) - - for _, m := range ex.metrics { - if m.Descriptor.Name == fleetReplicaCountName { - for _, d := range m.TimeSeries { - if d.LabelValues[0].Value == "fleet-deleted" && d.LabelValues[1].Value == defaultNs && d.LabelValues[2].Value == "total" { - return d.Points[0].Value == int64(0) - } - } - } - } - - return false - }, 5*time.Second, time.Second) - - reader.ReadAndExport(exporter) - assertMetricData(t, exporter, fleetReplicaCountName, []expectedMetricData{ - {labels: []string{"fleet-deleted", defaultNs, "reserved"}, val: int64(0)}, - {labels: []string{"fleet-deleted", defaultNs, "allocated"}, val: int64(0)}, - {labels: []string{"fleet-deleted", defaultNs, "desired"}, val: int64(0)}, - {labels: []string{"fleet-deleted", defaultNs, "ready"}, val: int64(0)}, - {labels: []string{"fleet-deleted", defaultNs, "total"}, val: int64(0)}, - {labels: []string{"fleet-test", defaultNs, "reserved"}, val: int64(1)}, - {labels: []string{"fleet-test", defaultNs, "allocated"}, val: int64(2)}, - {labels: []string{"fleet-test", defaultNs, "desired"}, val: int64(5)}, - {labels: []string{"fleet-test", defaultNs, "ready"}, val: int64(1)}, - {labels: []string{"fleet-test", defaultNs, "total"}, val: int64(8)}, - }) -} - func TestControllerFleetReplicasCount_ResetMetricsOnDelete(t *testing.T) { runtime.FeatureTestMutex.Lock() defer runtime.FeatureTestMutex.Unlock() - require.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true", runtime.FeatureResetMetricsOnDelete))) resetMetrics() exporter := &metricExporter{} @@ -396,100 +338,9 @@ func TestControllerFleetReplicasCount_ResetMetricsOnDelete(t *testing.T) { }) } -func TestControllerFleetAutoScalerState(t *testing.T) { - runtime.FeatureTestMutex.Lock() - defer runtime.FeatureTestMutex.Unlock() - require.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=false", runtime.FeatureResetMetricsOnDelete))) - - resetMetrics() - exporter := &metricExporter{} - reader := metricexport.NewReader() - c := newFakeController() - defer c.close() - c.run(t) - - // testing fleet name change - fasFleetNameChange := fleetAutoScaler("first-fleet", "name-switch") - c.fasWatch.Add(fasFleetNameChange) - fasFleetNameChange = fasFleetNameChange.DeepCopy() - fasFleetNameChange.Spec.Policy.Buffer.BufferSize = intstr.FromInt(10) - fasFleetNameChange.Spec.Policy.Buffer.MaxReplicas = 50 - fasFleetNameChange.Spec.Policy.Buffer.MinReplicas = 10 - fasFleetNameChange.Status.CurrentReplicas = 20 - fasFleetNameChange.Status.DesiredReplicas = 10 - fasFleetNameChange.Status.ScalingLimited = true - c.fasWatch.Modify(fasFleetNameChange) - fasFleetNameChange = fasFleetNameChange.DeepCopy() - fasFleetNameChange.Spec.FleetName = "second-fleet" - c.fasWatch.Modify(fasFleetNameChange) - // testing deletion - fasDeleted := fleetAutoScaler("deleted-fleet", "deleted") - fasDeleted.Spec.Policy.Buffer.BufferSize = intstr.FromString("50%") - fasDeleted.Spec.Policy.Buffer.MaxReplicas = 150 - fasDeleted.Spec.Policy.Buffer.MinReplicas = 15 - c.fasWatch.Add(fasDeleted) - c.fasWatch.Delete(fasDeleted) - - c.sync() - // wait until we have a fleet deleted and it's allocation count is 0 - // since that is our last operation - require.Eventually(t, func() bool { - ex := &metricExporter{} - reader.ReadAndExport(ex) - - for _, m := range ex.metrics { - if m.Descriptor.Name == fleetAutoscalersLimitedName { - for _, d := range m.TimeSeries { - if d.LabelValues[0].Value == "deleted-fleet" && d.LabelValues[1].Value == "deleted" && d.LabelValues[2].Value == defaultNs { - return d.Points[0].Value == int64(0) - } - } - } - } - - return false - }, 5*time.Second, time.Second) - - reader.ReadAndExport(exporter) - assertMetricData(t, exporter, fleetAutoscalersAbleToScaleName, []expectedMetricData{ - {labels: []string{"first-fleet", "name-switch", defaultNs}, val: int64(0)}, - {labels: []string{"second-fleet", "name-switch", defaultNs}, val: int64(1)}, - {labels: []string{"deleted-fleet", "deleted", defaultNs}, val: int64(0)}, - }) - assertMetricData(t, exporter, fleetAutoscalerBufferLimitName, []expectedMetricData{ - {labels: []string{"first-fleet", "name-switch", defaultNs, "max"}, val: int64(50)}, - {labels: []string{"first-fleet", "name-switch", defaultNs, "min"}, val: int64(10)}, - {labels: []string{"second-fleet", "name-switch", defaultNs, "max"}, val: int64(50)}, - {labels: []string{"second-fleet", "name-switch", defaultNs, "min"}, val: int64(10)}, - {labels: []string{"deleted-fleet", "deleted", defaultNs, "max"}, val: int64(150)}, - {labels: []string{"deleted-fleet", "deleted", defaultNs, "min"}, val: int64(15)}, - }) - assertMetricData(t, exporter, fleetAutoscalterBufferSizeName, []expectedMetricData{ - {labels: []string{"first-fleet", "name-switch", defaultNs, "count"}, val: int64(10)}, - {labels: []string{"second-fleet", "name-switch", defaultNs, "count"}, val: int64(10)}, - {labels: []string{"deleted-fleet", "deleted", defaultNs, "percentage"}, val: int64(50)}, - }) - assertMetricData(t, exporter, fleetAutoscalerCurrentReplicaCountName, []expectedMetricData{ - {labels: []string{"first-fleet", "name-switch", defaultNs}, val: int64(0)}, - {labels: []string{"second-fleet", "name-switch", defaultNs}, val: int64(20)}, - {labels: []string{"deleted-fleet", "deleted", defaultNs}, val: int64(0)}, - }) - assertMetricData(t, exporter, fleetAutoscalersDesiredReplicaCountName, []expectedMetricData{ - {labels: []string{"first-fleet", "name-switch", defaultNs}, val: int64(0)}, - {labels: []string{"second-fleet", "name-switch", defaultNs}, val: int64(10)}, - {labels: []string{"deleted-fleet", "deleted", defaultNs}, val: int64(0)}, - }) - assertMetricData(t, exporter, fleetAutoscalersLimitedName, []expectedMetricData{ - {labels: []string{"first-fleet", "name-switch", defaultNs}, val: int64(0)}, - {labels: []string{"second-fleet", "name-switch", defaultNs}, val: int64(1)}, - {labels: []string{"deleted-fleet", "deleted", defaultNs}, val: int64(0)}, - }) -} - func TestControllerFleetAutoScalerState_ResetMetricsOnDelete(t *testing.T) { runtime.FeatureTestMutex.Lock() defer runtime.FeatureTestMutex.Unlock() - require.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true", runtime.FeatureResetMetricsOnDelete))) resetMetrics() exporter := &metricExporter{} diff --git a/pkg/util/runtime/features.go b/pkg/util/runtime/features.go index 88571447e3..7ca446df62 100644 --- a/pkg/util/runtime/features.go +++ b/pkg/util/runtime/features.go @@ -44,10 +44,6 @@ const ( // FeaturePlayerTracking is a feature flag to enable/disable player tracking features. FeaturePlayerTracking Feature = "PlayerTracking" - // FeatureResetMetricsOnDelete is a feature flag that tells the metrics service to unregister and register - // relevant metric views to reset their state immediately when an Agones resource is deleted. - FeatureResetMetricsOnDelete Feature = "ResetMetricsOnDelete" - // FeatureFleetAllocateOverflow enables setting labels and/or annotations on Allocated GameServers // if the desired number of the underlying GameServerSet drops below the number of Allocated GameServers. FeatureFleetAllocateOverflow = "FleetAllocationOverflow" @@ -99,7 +95,6 @@ var ( featureDefaults = map[Feature]bool{ // Beta features FeatureFleetAllocateOverflow: true, - FeatureResetMetricsOnDelete: true, FeatureSplitControllerAndExtensions: true, // Alpha features diff --git a/site/content/en/docs/Guides/feature-stages.md b/site/content/en/docs/Guides/feature-stages.md index e126980068..7a787ec7c6 100644 --- a/site/content/en/docs/Guides/feature-stages.md +++ b/site/content/en/docs/Guides/feature-stages.md @@ -24,7 +24,6 @@ that can be found in the [Helm configuration]({{< ref "/docs/Installation/Instal The current set of `alpha` and `beta` feature gates: - {{% feature expiryVersion="1.37.0" %}} | Feature Name | Gate | Default | Stage | Since | |-----------------------------------------------------------------------------------------------------------------------|--------------------------------|----------|---------|--------| @@ -41,7 +40,6 @@ The current set of `alpha` and `beta` feature gates: | Feature Name | Gate | Default | Stage | Since | |-----------------------------------------------------------------------------------------------------------------------|--------------------------------|----------|---------|--------| | [Allocated GameServers are notified on relevant Fleet Updates][fleet-updates] | `FleetAllocationOverflow` | Enabled | `Beta` | 1.37.0 | -| [Reset Metric Export on Fleet / Autoscaler deletion]({{% relref "./metrics.md#dropping-metric-labels" %}}) | `ResetMetricsOnDelete` | Enabled | `Beta` | 1.32.0 | | [Split `agones-controller` ](https://github.com/googleforgames/agones/issues/2797) | `SplitControllerAndExtensions` | Enabled | `Beta` | 1.32.0 | | [GameServer player capacity filtering on GameServerAllocations](https://github.com/googleforgames/agones/issues/1239) | `PlayerAllocationFilter` | Disabled | `Alpha` | 1.14.0 | | [Player Tracking]({{< ref "/docs/Guides/player-tracking.md" >}}) | `PlayerTracking` | Disabled | `Alpha` | 1.6.0 | diff --git a/site/content/en/docs/Guides/metrics.md b/site/content/en/docs/Guides/metrics.md index 363a00e1af..8bfb09c088 100644 --- a/site/content/en/docs/Guides/metrics.md +++ b/site/content/en/docs/Guides/metrics.md @@ -77,12 +77,19 @@ Follow the [Google Cloud Monitoring installation steps](#google-cloud-monitoring ### Dropping Metric Labels +{{% feature expiryVersion="1.37.0" %}} {{% beta title="Reset Metric Export on Fleet / Autoscaler deletion" gate="ResetMetricsOnDelete" %}} When a Fleet or FleetAutoscaler is deleted from the system, Agones will automatically clear metrics that utilise their name as a label from the exported metrics, so the metrics exported do not continuously grow in size over the lifecycle of the Agones installation. +{{% /feature %}} +{{% feature publishVersion="1.37.0" %}} +When a Fleet or FleetAutoscaler is deleted from the system, Agones will automatically clear metrics that utilise +their name as a label from the exported metrics, so the metrics exported do not continuously grow in size over the +lifecycle of the Agones installation. +{{% /feature %}} ## Dashboard