Repository: argoproj/argo-rollouts

Issue:
Job's label value exceeds max value of 63 characters.
## Summary 
Assume we have created an analysis template as below:
```
apiVersion: argoproj.io/v1alpha1
kind: AnalysisTemplate
metadata:
  name: service-endpoint-reachable
spec:
  args:
  - name: service-fqdn
  - name: status-code
  metrics:
  - name: service-endpoint-reachable
    provider:
      job:
        spec:
          backoffLimit: 1
          template:
            spec:
              restartPolicy: Never
              containers:
                - name: curl-service-endpoint
                  image: curlimages/curl:7.73.0
                  command:
                    - /bin/sh
                    - -c
                    - |
                      curl -I {{args.service-fqdn}}
                      status_code=$(curl --write-out "%{http_code}\n" --silent --output /dev/null "{{args.service-fqdn}}");
                      [ $status_code -eq {{args.status-code}} ]
```
argo rollout will failed to create the wanted job due to the limit of max label value length(63) in kubernetes.
```
 message: 'Job.batch "990a01d4-ff15-402b-8516-0914c420e2b2.service-endpoint-reachable.5"
        is invalid: spec.template.labels: Invalid value: "990a01d4-ff15-402b-8516-0914c420e2b2.service-endpoint-reachable.5":
        must be no more than 63 characters'
```
I guess this is a known issue [1], but it's very likely to happen, due to the generation style.
[1]: https://github.com/argoproj/argo-rollouts/blob/a98a2742e522549a660e1de20b7b7c08dcc6884a/metricproviders/job/job.go#L57

## Diagnostics

What version of Argo Rollouts are you running?

```
# Paste the logs from the rollout controller

# Logs for the entire controller:
kubectl logs -n argo-rollouts deployment/argo-rollouts

# Logs for a specific rollout:
kubectl logs -n argo-rollouts deployment/argo-rollouts | grep rollout=<ROLLOUTNAME>
```

---
<!-- Issue Author: Don't delete this message to encourage other users to support your issue! -->
**Message from the maintainers**:

Impacted by this bug? Give it a 👍. We prioritize the issues with the most 👍.

Discussion:
@davidxia: I'm running into this issue as well. As a user of argo-rollouts, I'd like my attempts to create [Cluster]AnalysisTemplate with metric names that are too long to fail at creation time instead of failing later at AnalysisRun run-time. Perhaps the code @TommyLike referenced above can check the Job name length and hash or truncate the provided name in some way?

https://github.com/argoproj/argo-rollouts/blob/96ba0302f4d904e9aba36c3341025f4f51eaed81/metricproviders/job/job.go#L57-L63@jstewart612: As a workaround for this issue, I have truncated all my job names to be 20 characters or less, because that's how many characters it was over in my case.  I don't know if the length is variable in what is generated here or not by Argo Rollouts, but I haven't had the issue since.  I would assume it's only variable if the number of jobs in the analysistemplate is double-digits or greater, as I would expect the UUID to be of fixed length that seems to be prepended to this label.@github-actions: This issue is stale because it has been open 60 days with no activity.@SleepyBrett: It is unacceptable for this issue to be stale. It persists still. If you are using labels to find the child job you should break this up and use multiple labels.@kostis-codefresh: What would be the recommended solution here? Simply truncating the name would seem to be the quickest workaround, but then we run the danger of having different jobs getting truncated to the same name. Or this is a not a possible/interesting use case?@Qasim-Aziz: facing same issue @4wdonny: also impacted by this

Solution patch:
diff --git a/.github/workflows/image-reuse.yaml b/.github/workflows/image-reuse.yaml
index 57678f2e..da8618a0 100644
--- a/.github/workflows/image-reuse.yaml
+++ b/.github/workflows/image-reuse.yaml
@@ -79,7 +79,7 @@ jobs:
           cosign-release: 'v2.2.0'
 
       - uses: docker/setup-qemu-action@29109295f81e9208d7d86ff1c6c12d2833863392 # v3.6.0
-      - uses: docker/setup-buildx-action@b5ca514318bd6ebac0fb2aedd5d36ec1b5c232a2 # v3.10.0
+      - uses: docker/setup-buildx-action@e468171a9de216ec08956ac3ada2f0791b6bd435 # v3.11.1
 
       - name: Setup tags for container image as a CSV type
         run: |
diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml
index f6fdfed6..da5b4275 100644
--- a/.github/workflows/release.yaml
+++ b/.github/workflows/release.yaml
@@ -98,7 +98,7 @@ jobs:
         uses: docker/setup-qemu-action@29109295f81e9208d7d86ff1c6c12d2833863392 # v3.6.0
 
       - name: Set up Docker Buildx
-        uses: docker/setup-buildx-action@b5ca514318bd6ebac0fb2aedd5d36ec1b5c232a2 # v3.10.0
+        uses: docker/setup-buildx-action@e468171a9de216ec08956ac3ada2f0791b6bd435 # v3.11.1
 
       - name: Generate release artifacts
         run: |
diff --git a/docs/concepts.md b/docs/concepts.md
index 6f8312cd..433cbd53 100644
--- a/docs/concepts.md
+++ b/docs/concepts.md
@@ -75,4 +75,4 @@ Here is a summary table for the possible approaches.
 | Advanced routing scenarios |         No               |       No                   |         Yes                    |
 |      Failure Blast Radius | Massive impact            |  Low impact                       | Low impact              |
 
-Note that that traffic manager can be any compatible Service Mesh or Ingress Controller or Gateway API implementation (via a plugin).
+Note that the traffic manager can be any compatible Service Mesh or Ingress Controller or Gateway API implementation (via a plugin).
diff --git a/experiments/experiment.go b/experiments/experiment.go
index c85084e4..69dda224 100644
--- a/experiments/experiment.go
+++ b/experiments/experiment.go
@@ -137,7 +137,14 @@ func (ec *experimentContext) reconcileTemplate(template v1alpha1.TemplateSpec) {
 			// Create service for template if service field is set
 			if desiredReplicaCount != 0 {
 				ec.createTemplateService(&template, templateStatus, rs)
+			} else {
+				if rs.Status.AvailableReplicas == 0 {
+					// Check if service should be deleted when ReplicaSet has scaled down to 0 available replicas
+					svc := ec.templateServices[template.Name]
+					ec.deleteTemplateService(svc, templateStatus, template.Name)
+				}
 			}
+
 		} else {
 			// If service field nil but service exists, then delete it
 			// Code should not enter this path
@@ -157,6 +164,7 @@ func (ec *experimentContext) reconcileTemplate(template v1alpha1.TemplateSpec) {
 			ec.scaleTemplateRS(rs, template, templateStatus, desiredReplicaCount, experimentReplicas)
 			templateStatus.LastTransitionTime = &now
 		}
+
 	}
 
 	if rs == nil {
@@ -272,11 +280,6 @@ func (ec *experimentContext) scaleTemplateRS(rs *appsv1.ReplicaSet, template v1a
 	if err != nil {
 		templateStatus.Status = v1alpha1.TemplateStatusError
 		templateStatus.Message = fmt.Sprintf("Unable to scale ReplicaSet for template '%s' to desired replica count '%v': %v", templateStatus.Name, desiredReplicaCount, err)
-	} else {
-		if desiredReplicaCount == 0 && template.Service != nil {
-			svc := ec.templateServices[template.Name]
-			ec.deleteTemplateService(svc, templateStatus, template.Name)
-		}
 	}
 }
 
diff --git a/go.mod b/go.mod
index 18d39688..ac56388a 100644
--- a/go.mod
+++ b/go.mod
@@ -8,7 +8,7 @@ require (
 	github.com/argoproj/pkg v0.13.6
 	github.com/aws/aws-sdk-go-v2 v1.36.3
 	github.com/aws/aws-sdk-go-v2/config v1.29.14
-	github.com/aws/aws-sdk-go-v2/service/cloudwatch v1.44.1
+	github.com/aws/aws-sdk-go-v2/service/cloudwatch v1.45.1
 	github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2 v1.31.3
 	github.com/aws/smithy-go v1.22.3
 	github.com/blang/semver v3.5.1+incompatible
diff --git a/go.sum b/go.sum
index 3017914c..b8bb03c2 100644
--- a/go.sum
+++ b/go.sum
@@ -110,8 +110,8 @@ github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.34 h1:SZwFm17ZUNNg5Np0io
 github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.34/go.mod h1:dFZsC0BLo346mvKQLWmoJxT+Sjp+qcVR1tRVHQGOH9Q=
 github.com/aws/aws-sdk-go-v2/internal/ini v1.8.3 h1:bIqFDwgGXXN1Kpp99pDOdKMTTb5d2KyU5X/BZxjOkRo=
 github.com/aws/aws-sdk-go-v2/internal/ini v1.8.3/go.mod h1:H5O/EsxDWyU+LP/V8i5sm8cxoZgc2fdNR9bxlOFrQTo=
-github.com/aws/aws-sdk-go-v2/service/cloudwatch v1.44.1 h1:ac0UBlcUK+tFcFiAuNbtKqUEtM+iyQgmffEhUACGwD0=
-github.com/aws/aws-sdk-go-v2/service/cloudwatch v1.44.1/go.mod h1:HJlcOk+S/wjJuR/8jPa8GhnEKdKqqiQ5wjsE1PjuO1o=
+github.com/aws/aws-sdk-go-v2/service/cloudwatch v1.45.1 h1:AZhtDqdDVCSBc+52OobKirno9PMePDKOwOW++gu3+fE=
+github.com/aws/aws-sdk-go-v2/service/cloudwatch v1.45.1/go.mod h1:HJlcOk+S/wjJuR/8jPa8GhnEKdKqqiQ5wjsE1PjuO1o=
 github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2 v1.31.3 h1:Avh8YS+sgb2OKRht0wdNwY8tqtsCzVrmc8dG8Wfy9LI=
 github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2 v1.31.3/go.mod h1:HbtHaw/hnNPaiqcyYnheILVyn81wOZiX9n2gYF5tPmM=
 github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.12.3 h1:eAh2A4b5IzM/lum78bZ590jy36+d/aFLgKF/4Vd1xPE=
diff --git a/metricproviders/job/job.go b/metricproviders/job/job.go
index 5a85c5c7..b7ae8685 100644
--- a/metricproviders/job/job.go
+++ b/metricproviders/job/job.go
@@ -17,6 +17,7 @@ import (
 
 	"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
 	analysisutil "github.com/argoproj/argo-rollouts/utils/analysis"
+	"github.com/argoproj/argo-rollouts/utils/defaults"
 	metricutil "github.com/argoproj/argo-rollouts/utils/metric"
 	timeutil "github.com/argoproj/argo-rollouts/utils/time"
 )
@@ -73,8 +74,19 @@ func (p *JobProvider) GetMetadata(metric v1alpha1.Metric) map[string]string {
 // fit into a 63 character label, since the k8s job controller incorporates the job name into the
 // pod spec labels.
 func newJobName(run *v1alpha1.AnalysisRun, metric v1alpha1.Metric) string {
+
 	jobID := getJobIDSuffix(run, metric.Name)
-	return fmt.Sprintf("%s.%s.%d", run.UID, metric.Name, jobID)
+	jobName := fmt.Sprintf("%s.%s.%d", run.UID, metric.Name, jobID)
+
+	// Kubernetes can accept this job name without any issues
+	if len(jobName) <= defaults.Kubernetes_DNS_Limit {
+		return jobName
+	}
+
+	//We are over 63 characters so Kubernetes will reject this job name. We need to truncate it to 63 characters.
+	charactersOverLimit := len(jobName) - defaults.Kubernetes_DNS_Limit
+	truncateTo := len(metric.Name) - charactersOverLimit
+	return fmt.Sprintf("%s.%s.%d", run.UID, metric.Name[:truncateTo], jobID)
 }
 
 // getJobIDSuffix returns a numeric id which will be used as part of the job name. This is equal
diff --git a/utils/defaults/defaults.go b/utils/defaults/defaults.go
index f1c38dce..28431a44 100644
--- a/utils/defaults/defaults.go
+++ b/utils/defaults/defaults.go
@@ -50,6 +50,8 @@ const (
 	DefaultRolloutPluginFolder = "plugin-bin"
 	// DefaultDescribeTagsLimit is the default number resources (ARNs) in a single call
 	DefaultDescribeTagsLimit int = 20
+	// Kubernetes_DNS_Limit is the maximum length of a DNS name in Kubernetes. Currently used for Analysis Job names
+	Kubernetes_DNS_Limit int = 63
 )
 
 const (



Test patch:
diff --git a/experiments/experiment_test.go b/experiments/experiment_test.go
index 813a9847..9e5d6dab 100644
--- a/experiments/experiment_test.go
+++ b/experiments/experiment_test.go
@@ -175,12 +175,10 @@ func TestScaleDownRSAfterFinish(t *testing.T) {
 	inThePast := timeutil.Now().Add(-10 * time.Second).UTC().Format(time.RFC3339)
 	rs1.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey] = inThePast
 	rs2.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey] = inThePast
-
 	f := newFixture(t, e, rs1, rs2, s1)
 	defer f.Close()
 
 	updateRs1Index := f.expectUpdateReplicaSetAction(rs1)
-	f.expectDeleteServiceAction(s1)
 	updateRs2Index := f.expectUpdateReplicaSetAction(rs2)
 	expPatchIndex := f.expectPatchExperimentAction(e)
 
@@ -195,6 +193,60 @@ func TestScaleDownRSAfterFinish(t *testing.T) {
 
 	expPatchObj := f.getPatchedExperimentAsObj(expPatchIndex)
 	assert.Equal(t, v1alpha1.AnalysisPhaseSuccessful, expPatchObj.Status.Phase)
+
+	rs1.Status.AvailableReplicas = 0
+	rs2.Status.AvailableReplicas = 0
+
+	f = newFixture(t, e, rs1, rs2, s1)
+	defer f.Close()
+	f.expectDeleteServiceAction(s1)
+}
+
+// TestScaleDownRSAWhenSvcNotDeleted verifies that ScaleDownDelaySeconds annotation is added to ReplicaSet that is to be scaled down and service is not deleted because available replicas are not 0
+func TestScaleDownRSWhenSvcNotDeleted(t *testing.T) {
+	tmpl := generateTemplates("template1", "template2")
+	tmpl[0].Service = &v1alpha1.TemplateService{}
+
+	exp := newExperiment("test-exp", tmpl, "")
+	replicaSet1 := templateToRS(exp, tmpl[0], 1)
+	replicaSet2 := templateToRS(exp, tmpl[1], 1)
+	svc := templateToService(exp, tmpl[0], *replicaSet1)
+
+	exp.Status.AvailableAt = now()
+	exp.Status.Phase = v1alpha1.AnalysisPhaseRunning
+	exp.Status.TemplateStatuses = []v1alpha1.TemplateStatus{
+		generateTemplatesStatus("template1", 1, 1, v1alpha1.TemplateStatusSuccessful, now()),
+		generateTemplatesStatus("template2", 1, 1, v1alpha1.TemplateStatusSuccessful, now()),
+	}
+	exp.Spec.Terminate = true
+	exp.Status.TemplateStatuses[0].ServiceName = svc.Name
+	condition := conditions.NewExperimentConditions(v1alpha1.ExperimentProgressing, corev1.ConditionTrue, conditions.NewRSAvailableReason, "Experiment \"test-exp\" is running.")
+	exp.Status.Conditions = append(exp.Status.Conditions, *condition)
+
+	pastTime := timeutil.Now().Add(-10 * time.Second).UTC().Format(time.RFC3339)
+	replicaSet1.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey] = pastTime
+	replicaSet2.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey] = pastTime
+	replicaSet1.Status.AvailableReplicas = 1
+	replicaSet2.Status.AvailableReplicas = 1
+	fixture := newFixture(t, exp, replicaSet1, replicaSet2, svc)
+	defer fixture.Close()
+
+	rs1UpdateIdx := fixture.expectUpdateReplicaSetAction(replicaSet1)
+	rs2UpdateIdx := fixture.expectUpdateReplicaSetAction(replicaSet2)
+	expPatchIdx := fixture.expectPatchExperimentAction(exp)
+
+	fixture.run(getKey(exp, t))
+
+	updatedRS1 := fixture.getUpdatedReplicaSet(rs1UpdateIdx)
+	assert.NotNil(t, updatedRS1)
+	assert.Equal(t, int32(0), *updatedRS1.Spec.Replicas)
+
+	updatedRS2 := fixture.getUpdatedReplicaSet(rs2UpdateIdx)
+	assert.NotNil(t, updatedRS2)
+	assert.Equal(t, int32(0), *updatedRS2.Spec.Replicas)
+
+	patchedExp := fixture.getPatchedExperimentAsObj(expPatchIdx)
+	assert.Equal(t, v1alpha1.AnalysisPhaseSuccessful, patchedExp.Status.Phase)
 }
 
 func TestSetAvailableAt(t *testing.T) {
diff --git a/metricproviders/job/job_test.go b/metricproviders/job/job_test.go
index eb7a6a16..67e34593 100644
--- a/metricproviders/job/job_test.go
+++ b/metricproviders/job/job_test.go
@@ -21,6 +21,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 
 	"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
+	"github.com/argoproj/argo-rollouts/utils/defaults"
 )
 
 var noResyncPeriodFunc = func() time.Duration { return 0 }
@@ -324,3 +325,75 @@ func TestGarbageCollect(t *testing.T) {
 		}
 	}
 }
+
+func TestJobNameWithin63characters(t *testing.T) {
+	ctx := context.Background()
+	p := newTestJobProvider()
+	run := newRunWithJobMetric()
+
+	// Set the UID to a realistic value
+	run.UID = types.UID("34e4d823-4ba9-4afe-8775-6384561d7ef3")
+
+	// Create a metric with a short name that is within 63 characters (Kubernetes JOB DNS restriction)
+	shortMetricName := "short-job-name"
+	run.Spec.Metrics[0].Name = shortMetricName
+	metric := run.Spec.Metrics[0]
+
+	// First measurement
+	measurement := p.Run(run, metric)
+	expectedName := fmt.Sprintf("%s.%s.1", run.UID, metric.Name)
+	assert.Equal(t, expectedName, measurement.Metadata[JobNameKey], "Job name should not be truncated")
+
+	// Ensure the job was created with the correct name
+	jobs, err := p.kubeclientset.BatchV1().Jobs(run.Namespace).List(ctx, metav1.ListOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, expectedName, jobs.Items[0].Name, "Job name should match the expected name")
+
+	// Verify that the job name is less than or equal to 63 characters
+	assert.LessOrEqual(t, len(jobs.Items[0].Name), defaults.Kubernetes_DNS_Limit, "Job name should be less than or equal to 63 characters")
+}
+
+func TestJobNameWithTruncation(t *testing.T) {
+	ctx := context.Background()
+	p := newTestJobProvider()
+	run := newRunWithJobMetric()
+
+	// Set the UID to a realistic value
+	run.UID = types.UID("5d610152-d78d-4af9-aa6a-81c227ea422c")
+
+	// Create a metric where the name itself is within limits, but it is larger than 63 characters if the UUID is included
+	longMetricName := "service-endpoint-reachable"
+	run.Spec.Metrics[0].Name = longMetricName
+	metric := run.Spec.Metrics[0]
+
+	// First measurement
+	measurement := p.Run(run, metric)
+	assert.Len(t, measurement.Metadata[JobNameKey], defaults.Kubernetes_DNS_Limit, "Job name should be truncated to 63 characters")
+
+	// Ensure that job is submitted
+	_, err := p.kubeclientset.BatchV1().Jobs(run.Namespace).List(ctx, metav1.ListOptions{})
+	assert.NoError(t, err)
+}
+
+func TestLongJobName(t *testing.T) {
+	ctx := context.Background()
+	p := newTestJobProvider()
+	run := newRunWithJobMetric()
+
+	// Set the UID to a realistic value
+	run.UID = types.UID("a3f5c9b2-1d4e-4b8a-9f3e-2c7d8e6a4b1c")
+
+	// Create a metric with a long name (70 characters) that is obviously longer than 63 characters
+	longMetricName := "this-is-a-very-long-metric-name-that-is-exactly-seventy-characters-long"
+	run.Spec.Metrics[0].Name = longMetricName
+	metric := run.Spec.Metrics[0]
+
+	// First measurement
+	measurement := p.Run(run, metric)
+	assert.Len(t, measurement.Metadata[JobNameKey], defaults.Kubernetes_DNS_Limit, "Job name should be truncated to 63 characters")
+
+	// Ensure that job is submitted
+	_, err := p.kubeclientset.BatchV1().Jobs(run.Namespace).List(ctx, metav1.ListOptions{})
+	assert.NoError(t, err)
+
+}
diff --git a/test/e2e/experiment_test.go b/test/e2e/experiment_test.go
index da96b9c0..dbe8c2ba 100644
--- a/test/e2e/experiment_test.go
+++ b/test/e2e/experiment_test.go
@@ -103,6 +103,9 @@ func (s *ExperimentSuite) TestExperimentWithServiceAndScaleDownDelay() {
 		WaitForExperimentCondition("experiment-with-service", func(ex *rov1.Experiment) bool {
 			return s.GetReplicaSetFromExperiment(ex, "test").Status.Replicas == 0
 		}, "number-of-rs-pods-meet", fixtures.E2EWaitTimeout).
+		WaitForExperimentCondition("experiment-with-service", func(ex *rov1.Experiment) bool {
+			return s.GetReplicaSetFromExperiment(ex, "test").Status.AvailableReplicas == 0
+		}, "number-of-rs-pods-meet", fixtures.E2EWaitTimeout).
 		Then().
 		ExpectExperimentTemplateReplicaSetNumReplicas("experiment-with-service", "test", 0).
 		ExpectExperimentServiceCount("experiment-with-service", 0)
@@ -124,6 +127,9 @@ func (s *ExperimentSuite) TestExperimentWithServiceNameAndScaleDownDelay() {
 		WaitForExperimentCondition("experiment-with-service-name", func(ex *rov1.Experiment) bool {
 			return s.GetReplicaSetFromExperiment(ex, "test").Status.Replicas == 0
 		}, "number-of-rs-pods-meet", fixtures.E2EWaitTimeout).
+		WaitForExperimentCondition("experiment-with-service-name", func(ex *rov1.Experiment) bool {
+			return s.GetReplicaSetFromExperiment(ex, "test").Status.AvailableReplicas == 0
+		}, "number-of-rs-pods-meet", fixtures.E2EWaitTimeout).
 		Then().
 		ExpectExperimentTemplateReplicaSetNumReplicas("experiment-with-service-name", "test", 0).
 		ExpectExperimentServiceCount("experiment-with-service-name", 0)
@@ -145,6 +151,9 @@ func (s *ExperimentSuite) TestExperimentWithMultiportServiceAndScaleDownDelay()
 		WaitForExperimentCondition("experiment-with-multiport-service", func(ex *rov1.Experiment) bool {
 			return s.GetReplicaSetFromExperiment(ex, "test").Status.Replicas == 0
 		}, "number-of-rs-pods-meet", fixtures.E2EWaitTimeout).
+		WaitForExperimentCondition("experiment-with-multiport-service", func(ex *rov1.Experiment) bool {
+			return s.GetReplicaSetFromExperiment(ex, "test").Status.AvailableReplicas == 0
+		}, "number-of-rs-pods-meet", fixtures.E2EWaitTimeout).
 		Then().
 		ExpectExperimentTemplateReplicaSetNumReplicas("experiment-with-multiport-service", "test", 0).
 		ExpectExperimentServiceCount("experiment-with-multiport-service", 0)