Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions tests/common/support/kueue_upgrade.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
Copyright 2026.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package support

import (
"encoding/json"
"fmt"

"github.com/onsi/gomega"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kueuev1beta2 "sigs.k8s.io/kueue/apis/kueue/v1beta2"
)

const UpgradeRHOAIVersionKey = "rhoai-version"

func AddUpgradeResourceBaseline(data map[string]string, genKey, specKey string, generation int64, spec interface{}) error {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function returns an error rather than asserting, so error attribution in test output will point to the helper rather than the caller. StoreUpgradeBaseline and VerifyUpgradeResourceSpecIntegrity correctly call test.T().Helper().

specJSON, err := json.Marshal(spec)
if err != nil {
return err
}
data[genKey] = fmt.Sprintf("%d", generation)
data[specKey] = string(specJSON)
return nil
}

func StoreUpgradeBaseline(test Test, namespace, configMapName string, data map[string]string) {
test.T().Helper()

data[UpgradeRHOAIVersionKey] = GetRHOAIVersionFromDSCI(test)
configMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: configMapName,
Namespace: namespace,
},
Data: data,
}
_ = test.Client().Core().CoreV1().ConfigMaps(namespace).Delete(test.Ctx(), configMapName, metav1.DeleteOptions{})
_, err := test.Client().Core().CoreV1().ConfigMaps(namespace).Create(test.Ctx(), configMap, metav1.CreateOptions{})
test.Expect(err).NotTo(gomega.HaveOccurred())
test.T().Logf("Stored upgrade baseline in ConfigMap %s/%s", namespace, configMapName)
}

func VerifyUpgradeResourceSpecIntegrity(test Test, resourceName string, generation int64, spec interface{},
configMap *corev1.ConfigMap, genKey, specKey string) {
test.T().Helper()

expectedGen := configMap.Data[genKey]
actualGen := fmt.Sprintf("%d", generation)
if actualGen != expectedGen {
currentSpecJSON, _ := json.Marshal(spec)
test.T().Logf("%s generation changed during upgrade (%s to %s)", resourceName, expectedGen, actualGen)
test.T().Logf("Pre-upgrade %s spec: %s", resourceName, configMap.Data[specKey])
test.T().Logf("Post-upgrade %s spec: %s", resourceName, currentSpecJSON)
}
test.Expect(actualGen).To(gomega.Equal(expectedGen),
"%s spec should be unchanged after upgrade (generation %s, expected %s)", resourceName, actualGen, expectedGen)
test.T().Logf("%s generation unchanged after upgrade: %s", resourceName, actualGen)
}
Comment on lines +60 to +75

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

VerifyUpgradeResourceSpecIntegrity never actually verifies the spec, only the generation.

currentSpecJSON (Line 67) is computed solely for the mismatch log message; the only assertion is on actualGen == expectedGen (Line 72-73). If a resource's spec silently drifts while Kueue leaves the generation unchanged (or the generation happens to coincide), this function reports success despite the spec being wrong — defeating the stated purpose ("Verify... Resource Spec Integrity") and the PR objective of validating that "resource specifications ... are preserved correctly during upgrades."

🐛 Proposed fix to compare spec content
 func VerifyUpgradeResourceSpecIntegrity(test Test, resourceName string, generation int64, spec interface{},
 	configMap *corev1.ConfigMap, genKey, specKey string) {
 	test.T().Helper()
 
 	expectedGen := configMap.Data[genKey]
 	actualGen := fmt.Sprintf("%d", generation)
+	currentSpecJSON, err := json.Marshal(spec)
+	test.Expect(err).NotTo(gomega.HaveOccurred())
 	if actualGen != expectedGen {
-		currentSpecJSON, _ := json.Marshal(spec)
 		test.T().Logf("%s generation changed during upgrade (%s to %s)", resourceName, expectedGen, actualGen)
 		test.T().Logf("Pre-upgrade %s spec: %s", resourceName, configMap.Data[specKey])
 		test.T().Logf("Post-upgrade %s spec: %s", resourceName, currentSpecJSON)
 	}
 	test.Expect(actualGen).To(gomega.Equal(expectedGen),
 		"%s spec should be unchanged after upgrade (generation %s, expected %s)", resourceName, actualGen, expectedGen)
+	test.Expect(string(currentSpecJSON)).To(gomega.MatchJSON(configMap.Data[specKey]),
+		"%s spec content should be unchanged after upgrade", resourceName)
 	test.T().Logf("%s generation unchanged after upgrade: %s", resourceName, actualGen)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func VerifyUpgradeResourceSpecIntegrity(test Test, resourceName string, generation int64, spec interface{},
configMap *corev1.ConfigMap, genKey, specKey string) {
test.T().Helper()
expectedGen := configMap.Data[genKey]
actualGen := fmt.Sprintf("%d", generation)
if actualGen != expectedGen {
currentSpecJSON, _ := json.Marshal(spec)
test.T().Logf("%s generation changed during upgrade (%s to %s)", resourceName, expectedGen, actualGen)
test.T().Logf("Pre-upgrade %s spec: %s", resourceName, configMap.Data[specKey])
test.T().Logf("Post-upgrade %s spec: %s", resourceName, currentSpecJSON)
}
test.Expect(actualGen).To(gomega.Equal(expectedGen),
"%s spec should be unchanged after upgrade (generation %s, expected %s)", resourceName, actualGen, expectedGen)
test.T().Logf("%s generation unchanged after upgrade: %s", resourceName, actualGen)
}
func VerifyUpgradeResourceSpecIntegrity(test Test, resourceName string, generation int64, spec interface{},
configMap *corev1.ConfigMap, genKey, specKey string) {
test.T().Helper()
expectedGen := configMap.Data[genKey]
actualGen := fmt.Sprintf("%d", generation)
currentSpecJSON, err := json.Marshal(spec)
test.Expect(err).NotTo(gomega.HaveOccurred())
if actualGen != expectedGen {
test.T().Logf("%s generation changed during upgrade (%s to %s)", resourceName, expectedGen, actualGen)
test.T().Logf("Pre-upgrade %s spec: %s", resourceName, configMap.Data[specKey])
test.T().Logf("Post-upgrade %s spec: %s", resourceName, currentSpecJSON)
}
test.Expect(actualGen).To(gomega.Equal(expectedGen),
"%s spec should be unchanged after upgrade (generation %s, expected %s)", resourceName, actualGen, expectedGen)
test.Expect(string(currentSpecJSON)).To(gomega.MatchJSON(configMap.Data[specKey]),
"%s spec content should be unchanged after upgrade", resourceName)
test.T().Logf("%s generation unchanged after upgrade: %s", resourceName, actualGen)
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/common/support/kueue_upgrade.go` around lines 60 - 75,
VerifyUpgradeResourceSpecIntegrity only checks generation and never asserts that
the spec content stayed the same. In VerifyUpgradeResourceSpecIntegrity, compare
the pre-upgrade spec from configMap.Data[specKey] against the post-upgrade spec
serialized from spec, and keep the generation check only as supplemental
context. Use the existing resourceName, genKey, and specKey flow to locate the
mismatch, and add a spec equality assertion so the function actually validates
resource spec integrity during upgrades.


func ClusterQueueNominalGPUQuota(clusterQueue *kueuev1beta2.ClusterQueue, gpuResourceLabel string) resource.Quantity {
for _, resourceGroup := range clusterQueue.Spec.ResourceGroups {
for _, flavor := range resourceGroup.Flavors {
for _, quota := range flavor.Resources {
if string(quota.Name) == gpuResourceLabel {
return quota.NominalQuota
}
}
}
}
return resource.Quantity{}
}
106 changes: 106 additions & 0 deletions tests/common/support/kueue_upgrade_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/*
Copyright 2026.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package support

import (
"encoding/json"
"testing"

"github.com/onsi/gomega"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
kueuev1beta2 "sigs.k8s.io/kueue/apis/kueue/v1beta2"
)
Comment on lines +19 to +28

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Imports not sorted — pipeline failing.

verify-imports reports this file fails openshift-goimports sorting.

make imports
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/common/support/kueue_upgrade_test.go` around lines 19 - 28, The import
block in kueue_upgrade_test.go is not in the order expected by
openshift-goimports, causing verify-imports to fail. Reformat the imports in the
package for the test file so they are sorted and grouped consistently with
goimports conventions, keeping the existing dependencies but adjusting their
order in the import list.

Source: Pipeline failures


func TestAddUpgradeResourceBaseline(t *testing.T) {
test := NewTest(t)

data := map[string]string{}
spec := kueuev1beta2.ClusterQueueSpec{
ResourceGroups: []kueuev1beta2.ResourceGroup{
{
CoveredResources: []corev1.ResourceName{corev1.ResourceName(NVIDIA.ResourceLabel)},
Flavors: []kueuev1beta2.FlavorQuotas{
{
Resources: []kueuev1beta2.ResourceQuota{
{
Name: corev1.ResourceName(NVIDIA.ResourceLabel),
NominalQuota: resource.MustParse("1"),
},
},
},
},
},
},
}

err := AddUpgradeResourceBaseline(data, "generation-key", "spec-key", 3, spec)
test.Expect(err).NotTo(gomega.HaveOccurred())
test.Expect(data["generation-key"]).To(gomega.Equal("3"))

var restored kueuev1beta2.ClusterQueueSpec
err = json.Unmarshal([]byte(data["spec-key"]), &restored)
test.Expect(err).NotTo(gomega.HaveOccurred())
test.Expect(restored.ResourceGroups).To(gomega.HaveLen(1))
test.Expect(restored.ResourceGroups[0].Flavors[0].Resources[0].NominalQuota).To(gomega.Equal(resource.MustParse("1")))
}

func TestClusterQueueNominalGPUQuota(t *testing.T) {
test := NewTest(t)

clusterQueue := &kueuev1beta2.ClusterQueue{
Spec: kueuev1beta2.ClusterQueueSpec{
ResourceGroups: []kueuev1beta2.ResourceGroup{
{
Flavors: []kueuev1beta2.FlavorQuotas{
{
Resources: []kueuev1beta2.ResourceQuota{
{
Name: corev1.ResourceName(NVIDIA.ResourceLabel),
NominalQuota: resource.MustParse("2"),
},
},
},
},
},
},
},
}

quota := ClusterQueueNominalGPUQuota(clusterQueue, NVIDIA.ResourceLabel)
test.Expect(quota).To(gomega.Equal(resource.MustParse("2")))
test.Expect(ClusterQueueNominalGPUQuota(clusterQueue, "missing")).To(gomega.Equal(resource.Quantity{}))
}

func TestVerifyUpgradeResourceSpecIntegrity(t *testing.T) {
test := NewTest(t)
configMap := &corev1.ConfigMap{
Data: map[string]string{
"gen-key": "5",
"spec-key": `{"nodeLabels":{"nvidia.com/gpu.present":"true"}}`,
UpgradeRHOAIVersionKey: "3.4.0",
},
}
spec := kueuev1beta2.ResourceFlavorSpec{
NodeLabels: map[string]string{
"nvidia.com/gpu.present": "true",
},
}

VerifyUpgradeResourceSpecIntegrity(test, "ResourceFlavor", 5, spec, configMap, "gen-key", "spec-key")
}
Comment on lines +90 to +106

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Test doesn't cover the actual bug in VerifyUpgradeResourceSpecIntegrity.

This only exercises the case where generation and spec both match, so it passes even though the function under test never checks spec content (see comment on kueue_upgrade.go Lines 60-75). Add a case with a mismatched spec but matching generation to catch the gap, and a case for mismatched generation to verify failure behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/common/support/kueue_upgrade_test.go` around lines 90 - 106, The
current test only covers the happy path where both generation and spec match, so
it does not expose the missing spec validation in
VerifyUpgradeResourceSpecIntegrity. Update
TestVerifyUpgradeResourceSpecIntegrity to add a mismatched ResourceFlavorSpec
with the same generation and assert it fails, and add a separate case where the
generation value differs to verify the expected failure behavior; use
VerifyUpgradeResourceSpecIntegrity, ResourceFlavorSpec, and the ConfigMap keys
already in the test to keep the coverage targeted.

Loading
Loading