Repository: kubernetes-sigs/kueue

Issue:
[Deflake] Stop checking  TAS NodeToReplaceAnnotation directly in test code
/kind flake

**What happened**:

The tests are flaky and time sensitive. 

The issue with the asserts on NodeToReplaceAnnotation  is that when the node is replaced then the annotation is removed. 

**What you expected to happen**:

No failures when 1s sleep is injected . Ideally we don't have any integration and e2e tests which check NodeToReplaceAnnotation directly. Instead, we should observe the behavior that the node is changed.

**How to reproduce it (as minimally and precisely as possible)**:

To show that just inject time.Sleep(1s) from [here](https://github.com/kubernetes-sigs/kueue/blob/main/test/integration/singlecluster/tas/tas_test.go#L945) 

```
ginkgo.By("deleting the node", func() {
	nodeToUpdate := &corev1.Node{}
	gomega.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, nodeToUpdate)).Should(gomega.Succeed())
	gomega.Expect(k8sClient.Delete(ctx, nodeToUpdate)).Should(gomega.Succeed())
})
  
time.Sleep(time.Second)

ginkgo.By("verify the workload has the NodeToReplaceAnnotation", func() {
	gomega.Eventually(func(g gomega.Gomega) {
		g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wl1)).To(gomega.Succeed())
		g.Expect(wl1.Annotations).Should(gomega.HaveKeyWithValue(kueuealpha.NodeToReplaceAnnotation, nodeName))
	}, util.LongTimeout, util.Interval).Should(gomega.Succeed())
})
```

**Anything else we need to know?**:

I think the asserts where added as a temporary measures before setting the annotation was done as the first PR, and only then we added the node replacing. However, it is now important to cleanup the technical debt.

Discussion:
@mimowo: cc @PBundyra @pajakd @mbobrovskyi @mszadkow @mimowo: The additional flakiness in the test makes it really hard to debug https://github.com/kubernetes-sigs/kueue/issues/5511, similarly as https://github.com/kubernetes-sigs/kueue/issues/5559.
@pajakd: /assign@pajakd: Not sure how to deal with e2e tests. The sole purpose of the tests "Should update nodesToReplace at the workload when a node fails" and "Should update nodesToReplace at the workload when a node is deleted" is to validate that the annotation behaves correctly upon node failure. We do have integration tests that cover this so perhaps the e2e tests are redundant. Instead, we should perhaps have e2e tests that checks the full functionality, wdyt?

@mimowo: Yes, so currently the integration tests provide more coverage than e2e. 

I think ultimately we will want to have an e2e test for the feature which is true e2e, without checking the presence of the annotation as interim step.

I'm ok to drop the existing tests even for a time being until we have a proper test, given that in the current form the e2e cause non-obvious flakes in other tests, see: https://github.com/kubernetes-sigs/kueue/issues/5555#issuecomment-2954781126. 

Solution patch:
diff --git a/Makefile b/Makefile
index b752d7e6d..0d8ca82b4 100644
--- a/Makefile
+++ b/Makefile
@@ -221,7 +221,7 @@ run: manifests generate fmt vet ## Run a controller from your host.
 .PHONY: image-local-build
 image-local-build:
 	BUILDER=$(shell $(DOCKER_BUILDX_CMD) create --use)
-	$(MAKE) image-build PUSH=$(PUSH)
+	$(MAKE) image-build PUSH=$(PUSH) IMAGE_BUILD_EXTRA_OPTS=$(IMAGE_BUILD_EXTRA_OPTS)
 	$(DOCKER_BUILDX_CMD) rm $$BUILDER
 
 # Build the multiplatform container image locally and push to repo.
@@ -241,6 +241,7 @@ image-build:
 		--build-arg GIT_TAG=$(GIT_TAG) \
 		--build-arg GIT_COMMIT=$(GIT_COMMIT) \
 		$(PUSH) \
+		$(IMAGE_BUILD_EXTRA_OPTS) \
 		./
 
 .PHONY: image-push
@@ -374,6 +375,7 @@ importer-image-build:
 		--build-arg BUILDER_IMAGE=$(BUILDER_IMAGE) \
 		--build-arg CGO_ENABLED=$(CGO_ENABLED) \
 		$(PUSH) \
+		$(IMAGE_BUILD_EXTRA_OPTS) \
 		-f ./cmd/importer/Dockerfile ./
 
 .PHONY: importer-image-push
@@ -398,12 +400,14 @@ kueueviz-image-build:
 		--build-arg BUILDER_IMAGE=$(BUILDER_IMAGE) \
 		--build-arg CGO_ENABLED=$(CGO_ENABLED) \
 		$(PUSH) \
+		$(IMAGE_BUILD_EXTRA_OPTS) \
 		-f ./cmd/kueueviz/backend/Dockerfile ./cmd/kueueviz/backend
 	$(IMAGE_BUILD_CMD) \
 		-t $(IMAGE_TAG_KUEUEVIZ_FRONTEND) \
 		-t $(IMAGE_REPO_KUEUEVIZ_FRONTEND):$(RELEASE_BRANCH) \
 		--platform=$(VIZ_PLATFORMS) \
 		$(PUSH) \
+		$(IMAGE_BUILD_EXTRA_OPTS) \
 		-f ./cmd/kueueviz/frontend/Dockerfile ./cmd/kueueviz/frontend
 
 .PHONY: kueueviz-image-push
@@ -440,6 +444,7 @@ ray-project-mini-image-build:
 		--platform=$(PLATFORMS) \
 		--build-arg RAY_VERSION=$(RAY_VERSION) \
 		$(PUSH) \
+		$(IMAGE_BUILD_EXTRA_OPTS) \
 		-f ./hack/internal/test-images/ray/Dockerfile ./ \
 
 # The step is required for local e2e test run
diff --git a/go.mod b/go.mod
index 0291e8ca5..f4b0e8f7a 100644
--- a/go.mod
+++ b/go.mod
@@ -20,7 +20,7 @@ require (
 	github.com/spf13/cobra v1.9.1
 	github.com/spf13/pflag v1.0.6
 	go.uber.org/zap v1.27.0
-	golang.org/x/sync v0.14.0
+	golang.org/x/sync v0.15.0
 	k8s.io/api v0.33.1
 	k8s.io/apimachinery v0.33.1
 	k8s.io/apiserver v0.33.1
diff --git a/go.sum b/go.sum
index 1b1139fd5..2b6511898 100644
--- a/go.sum
+++ b/go.sum
@@ -290,8 +290,8 @@ golang.org/x/oauth2 v0.28.0/go.mod h1:onh5ek6nERTohokkhCD/y2cV4Do3fxFHFuAejCkRWT
 golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
-golang.org/x/sync v0.14.0 h1:woo0S4Yywslg6hp4eUFjTVOyKt0RookbpAHG4c1HmhQ=
-golang.org/x/sync v0.14.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA=
+golang.org/x/sync v0.15.0 h1:KWH3jNZsfyT6xfAfKiz6MRNmd46ByHDYaZ7KSkCtdW8=
+golang.org/x/sync v0.15.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
diff --git a/site/content/en/docs/tasks/manage/monitor_pending_workloads/pending_workloads_on_demand.md b/site/content/en/docs/tasks/manage/monitor_pending_workloads/pending_workloads_on_demand.md
index 434b05f9f..8fedf1e07 100644
--- a/site/content/en/docs/tasks/manage/monitor_pending_workloads/pending_workloads_on_demand.md
+++ b/site/content/en/docs/tasks/manage/monitor_pending_workloads/pending_workloads_on_demand.md
@@ -60,7 +60,7 @@ You then need to create `ClusterRole` and `ClusterRoleBinding` for that same (de
 using a command:
 
 ```shell
-kubectl apply -f https://kueue.sigs.k8s.io/examples/admin/cluster-role-and-binding.yaml
+kubectl apply -f https://kueue.sigs.k8s.io/examples/visibility/cluster-role-and-binding.yaml
 ```
 
 ## Monitor pending workloads on demand
diff --git a/vendor/golang.org/x/sync/errgroup/errgroup.go b/vendor/golang.org/x/sync/errgroup/errgroup.go
index cfafed5b5..cb6bb9ad3 100644
--- a/vendor/golang.org/x/sync/errgroup/errgroup.go
+++ b/vendor/golang.org/x/sync/errgroup/errgroup.go
@@ -76,10 +76,8 @@ func (g *Group) Wait() error {
 }
 
 // Go calls the given function in a new goroutine.
-// The first call to Go must happen before a Wait.
-// It blocks until the new goroutine can be added without the number of
-// active goroutines in the group exceeding the configured limit.
 //
+// The first call to Go must happen before a Wait.
 // It blocks until the new goroutine can be added without the number of
 // goroutines in the group exceeding the configured limit.
 //
@@ -185,8 +183,9 @@ type PanicError struct {
 }
 
 func (p PanicError) Error() string {
-	// A Go Error method conventionally does not include a stack dump, so omit it
-	// here. (Callers who care can extract it from the Stack field.)
+	if len(p.Stack) > 0 {
+		return fmt.Sprintf("recovered from errgroup.Group: %v\n%s", p.Recovered, p.Stack)
+	}
 	return fmt.Sprintf("recovered from errgroup.Group: %v", p.Recovered)
 }
 
diff --git a/vendor/modules.txt b/vendor/modules.txt
index c99e35d07..d3cfa4529 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -479,7 +479,7 @@ golang.org/x/net/websocket
 ## explicit; go 1.23.0
 golang.org/x/oauth2
 golang.org/x/oauth2/internal
-# golang.org/x/sync v0.14.0
+# golang.org/x/sync v0.15.0
 ## explicit; go 1.23.0
 golang.org/x/sync/errgroup
 golang.org/x/sync/singleflight



Test patch:
diff --git a/test/integration/singlecluster/tas/tas_test.go b/test/integration/singlecluster/tas/tas_test.go
index 55e2332a5..f1548be80 100644
--- a/test/integration/singlecluster/tas/tas_test.go
+++ b/test/integration/singlecluster/tas/tas_test.go
@@ -942,13 +942,6 @@ var _ = ginkgo.Describe("Topology Aware Scheduling", ginkgo.Ordered, func() {
 					gomega.Expect(k8sClient.Delete(ctx, nodeToUpdate)).Should(gomega.Succeed())
 				})
 
-				ginkgo.By("verify the workload has the NodeToReplaceAnnotation", func() {
-					gomega.Eventually(func(g gomega.Gomega) {
-						g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wl1)).To(gomega.Succeed())
-						g.Expect(wl1.Annotations).Should(gomega.HaveKeyWithValue(kueuealpha.NodeToReplaceAnnotation, nodeName))
-					}, util.LongTimeout, util.Interval).Should(gomega.Succeed())
-				})
-
 				ginkgo.By("verify the workload has corrected TopologyAssignment and no NodeToReplaceAnnotation", func() {
 					gomega.Eventually(func(g gomega.Gomega) {
 						g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wl1)).To(gomega.Succeed())
@@ -992,22 +985,6 @@ var _ = ginkgo.Describe("Topology Aware Scheduling", ginkgo.Ordered, func() {
 					))
 				})
 
-				ginkgo.By("making the node NotReady", func() {
-					nodeToUpdate := &corev1.Node{}
-					gomega.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, nodeToUpdate)).Should(gomega.Succeed())
-					util.SetNodeCondition(ctx, k8sClient, nodeToUpdate, &corev1.NodeCondition{
-						Type:   corev1.NodeReady,
-						Status: corev1.ConditionFalse,
-					})
-				})
-
-				ginkgo.By("verify the workload does not have the NodeToReplaceAnnotation", func() {
-					gomega.Eventually(func(g gomega.Gomega) {
-						g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wl1)).To(gomega.Succeed())
-						g.Expect(wl1.Annotations).ShouldNot(gomega.HaveKey(kueuealpha.NodeToReplaceAnnotation))
-					}, util.LongTimeout, util.Interval).Should(gomega.Succeed())
-				})
-
 				ginkgo.By("making the node NotReady 30s in the past", func() {
 					nodeToUpdate := &corev1.Node{}
 					gomega.Expect(k8sClient.Get(ctx, apitypes.NamespacedName{Name: nodeName}, nodeToUpdate)).Should(gomega.Succeed())
@@ -1019,13 +996,6 @@ var _ = ginkgo.Describe("Topology Aware Scheduling", ginkgo.Ordered, func() {
 					})
 				})
 
-				ginkgo.By("verify the workload has the NodeToReplaceAnnotation", func() {
-					gomega.Eventually(func(g gomega.Gomega) {
-						g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wl1)).To(gomega.Succeed())
-						g.Expect(wl1.Annotations).Should(gomega.HaveKeyWithValue(kueuealpha.NodeToReplaceAnnotation, nodeName))
-					}, util.LongTimeout, util.Interval).Should(gomega.Succeed())
-				})
-
 				ginkgo.By("verify the workload has corrected TopologyAssignment and no NodeToReplaceAnnotation", func() {
 					gomega.Eventually(func(g gomega.Gomega) {
 						g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wl1)).To(gomega.Succeed())
@@ -1104,13 +1074,6 @@ var _ = ginkgo.Describe("Topology Aware Scheduling", ginkgo.Ordered, func() {
 					})
 				})
 
-				ginkgo.By("verify the workload has the NodeToReplaceAnnotation", func() {
-					gomega.Eventually(func(g gomega.Gomega) {
-						g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wl1)).To(gomega.Succeed())
-						g.Expect(wl1.Annotations).Should(gomega.HaveKeyWithValue(kueuealpha.NodeToReplaceAnnotation, nodeName))
-					}, util.LongTimeout, util.Interval).Should(gomega.Succeed())
-				})
-
 				ginkgo.By("verify the workload has the same TopologyAssignment as there is no free capacity for replacement", func() {
 					gomega.Consistently(func(g gomega.Gomega) {
 						g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wl1)).To(gomega.Succeed())
@@ -1180,13 +1143,6 @@ var _ = ginkgo.Describe("Topology Aware Scheduling", ginkgo.Ordered, func() {
 					util.ExpectObjectToBeDeleted(ctx, k8sClient, nodeToDelete, false)
 				})
 
-				ginkgo.By("verify the workload has the NodeToReplaceAnnotation", func() {
-					gomega.Eventually(func(g gomega.Gomega) {
-						g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wl1)).To(gomega.Succeed())
-						g.Expect(wl1.Annotations).Should(gomega.HaveKeyWithValue(kueuealpha.NodeToReplaceAnnotation, node1Name))
-					}, util.LongTimeout, util.Interval).Should(gomega.Succeed())
-				})
-
 				ginkgo.By("deleting the second assigned node: "+node2Name, func() {
 					nodeToDelete := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: node2Name}}
 					gomega.Expect(k8sClient.Delete(ctx, nodeToDelete)).Should(gomega.Succeed())
@@ -1243,13 +1199,6 @@ var _ = ginkgo.Describe("Topology Aware Scheduling", ginkgo.Ordered, func() {
 					})
 				})
 
-				ginkgo.By("verify the workload has the NodeToReplaceAnnotation", func() {
-					gomega.Eventually(func(g gomega.Gomega) {
-						g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wl1)).To(gomega.Succeed())
-						g.Expect(wl1.Annotations).Should(gomega.HaveKeyWithValue(kueuealpha.NodeToReplaceAnnotation, node1Name))
-					}, util.LongTimeout, util.Interval).Should(gomega.Succeed())
-				})
-
 				ginkgo.By("updating nodeReady condition of the second node", func() {
 					nodeToUpdate := &corev1.Node{}
 					gomega.Expect(k8sClient.Get(ctx, apitypes.NamespacedName{Name: node2Name}, nodeToUpdate)).Should(gomega.Succeed())