Skip to content

MGMT-22939: Fix cache key to get mustgather image #9940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Mar 25, 2026
Merged

MGMT-22939: Fix cache key to get mustgather image #9940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,9 +557,13 @@ run-db-container:
run-unit-test:
SKIP_UT_DB=1 $(MAKE) _unit_test TIMEOUT=30m TEST="$(or $(TEST),$(shell go list ./... | grep -v subsystem))"

run-unit-test-api:
Comment thread
pastequo marked this conversation as resolved.
cd api/ && go test ./...

ci-unit-test:
./hack/start_db.sh
$(MAKE) run-unit-test
$(MAKE) run-unit-test-api

kill-db-container:
$(CONTAINER_COMMAND) kill postgres
Expand Down
5 changes: 1 addition & 4 deletions api/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ require (
github.com/thoas/go-funk v0.9.2
k8s.io/api v0.29.5
k8s.io/apimachinery v0.29.5
k8s.io/client-go v0.29.2
sigs.k8s.io/controller-runtime v0.13.1
sigs.k8s.io/yaml v1.4.0
)
Expand All @@ -28,7 +27,6 @@ require (
github.com/evanphx/json-patch/v5 v5.8.0 // indirect
github.com/fsnotify/fsnotify v1.7.0 // indirect
github.com/go-logr/logr v1.4.3 // indirect
github.com/go-logr/zapr v1.3.0 // indirect
github.com/go-openapi/analysis v0.21.4 // indirect
github.com/go-openapi/errors v0.20.4 // indirect
github.com/go-openapi/jsonpointer v0.20.0 // indirect
Expand Down Expand Up @@ -70,8 +68,6 @@ require (
github.com/prometheus/procfs v0.12.0 // indirect
github.com/spf13/pflag v1.0.6-0.20210604193023-d5e0c0615ace // indirect
go.mongodb.org/mongo-driver v1.12.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
go.uber.org/zap v1.26.0 // indirect
golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e // indirect
golang.org/x/net v0.23.0 // indirect
golang.org/x/oauth2 v0.12.0 // indirect
Expand All @@ -88,6 +84,7 @@ require (
gopkg.in/yaml.v3 v3.0.1 // indirect
gorm.io/gorm v1.24.5 // indirect
k8s.io/apiextensions-apiserver v0.29.2 // indirect
k8s.io/client-go v0.29.2 // indirect
k8s.io/component-base v0.29.2 // indirect
k8s.io/klog/v2 v2.110.1 // indirect
k8s.io/kube-openapi v0.0.0-20231010175941-2dd684a91f00 // indirect
Expand Down
14 changes: 8 additions & 6 deletions api/v1beta1/agent_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package v1beta1

import (
"context"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/openshift/assisted-service/models"
Expand Down Expand Up @@ -48,7 +50,7 @@ var _ = Describe("ValidateUpdate", func() {
Name: "new-cluster-deployment",
Namespace: namespace,
}
warns, err := newAgent.ValidateUpdate(oldAgent)
warns, err := newAgent.ValidateUpdate(context.TODO(), oldAgent, newAgent)
Expect(warns).To(BeNil())
Expect(err).NotTo(BeNil())
})
Expand All @@ -62,7 +64,7 @@ var _ = Describe("ValidateUpdate", func() {
Name: "new-cluster-deployment",
Namespace: namespace,
}
warns, err := newAgent.ValidateUpdate(oldAgent)
warns, err := newAgent.ValidateUpdate(context.TODO(), oldAgent, newAgent)
Expect(warns).To(BeNil())
Expect(err).To(BeNil())
})
Expand All @@ -77,14 +79,14 @@ var _ = Describe("ValidateUpdate", func() {
Name: "new-cluster-deployment",
Namespace: namespace,
}
warns, err := newAgent.ValidateUpdate(oldAgent)
warns, err := newAgent.ValidateUpdate(context.TODO(), oldAgent, newAgent)
Expect(warns).To(BeNil())
Expect(err).To(BeNil())
})

It("succeeds if Agent's ClusterReference is nil and remains the same", func() {
newAgent := oldAgent.DeepCopy()
warns, err := newAgent.ValidateUpdate(oldAgent)
warns, err := newAgent.ValidateUpdate(context.TODO(), oldAgent, newAgent)
Expect(warns).To(BeNil())
Expect(err).To(BeNil())
})
Expand All @@ -95,7 +97,7 @@ var _ = Describe("ValidateUpdate", func() {
Namespace: namespace,
}
newAgent := oldAgent.DeepCopy()
warns, err := newAgent.ValidateUpdate(oldAgent)
warns, err := newAgent.ValidateUpdate(context.TODO(), oldAgent, newAgent)
Expect(warns).To(BeNil())
Expect(err).To(BeNil())
})
Expand All @@ -106,7 +108,7 @@ var _ = Describe("ValidateUpdate", func() {
}
oldAgent.Status = AgentStatus{DebugInfo: DebugInfo{State: models.HostStatusInstalling}}
newAgent := oldAgent.DeepCopy()
warns, err := newAgent.ValidateUpdate(oldAgent)
warns, err := newAgent.ValidateUpdate(context.TODO(), oldAgent, newAgent)
Expect(warns).To(BeNil())
Expect(err).To(BeNil())
})
Expand Down
30 changes: 11 additions & 19 deletions api/v1beta1/agentclassification_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package v1beta1

import (
"context"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -51,55 +53,55 @@ var _ = Describe("ValidateCreate", func() {
agentClassification.Spec.LabelKey = invalidKey
agentClassification.Spec.LabelValue = validValue
agentClassification.Spec.Query = validQuery
warn, err := agentClassification.ValidateCreate()
warn, err := agentClassification.ValidateCreate(context.TODO(), agentClassification)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
It("fails if label key has a prefix", func() {
agentClassification.Spec.LabelKey = keyWithPrefix
agentClassification.Spec.LabelValue = validValue
agentClassification.Spec.Query = validQuery
warn, err := agentClassification.ValidateCreate()
warn, err := agentClassification.ValidateCreate(context.TODO(), agentClassification)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
It("fails if label value is invalid", func() {
agentClassification.Spec.LabelKey = validKey
agentClassification.Spec.LabelValue = invalidValue
agentClassification.Spec.Query = validQuery
warn, err := agentClassification.ValidateCreate()
warn, err := agentClassification.ValidateCreate(context.TODO(), agentClassification)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
It("fails if label value starts with QUERYERROR", func() {
agentClassification.Spec.LabelKey = validKey
agentClassification.Spec.LabelValue = "QUERYERROR-foo"
agentClassification.Spec.Query = validQuery
warn, err := agentClassification.ValidateCreate()
warn, err := agentClassification.ValidateCreate(context.TODO(), agentClassification)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
It("fails if label query is invalid", func() {
agentClassification.Spec.LabelKey = validKey
agentClassification.Spec.LabelValue = validValue
agentClassification.Spec.Query = invalidQuery
warn, err := agentClassification.ValidateCreate()
warn, err := agentClassification.ValidateCreate(context.TODO(), agentClassification)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
It("fails if everything is invalid", func() {
agentClassification.Spec.LabelKey = invalidKey
agentClassification.Spec.LabelValue = invalidValue
agentClassification.Spec.Query = invalidQuery
warn, err := agentClassification.ValidateCreate()
warn, err := agentClassification.ValidateCreate(context.TODO(), agentClassification)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
It("succeeds if everything is valid", func() {
agentClassification.Spec.LabelKey = validKey
agentClassification.Spec.LabelValue = validValue
agentClassification.Spec.Query = validQuery
warn, err := agentClassification.ValidateCreate()
warn, err := agentClassification.ValidateCreate(context.TODO(), agentClassification)
Expect(warn).To(BeNil())
Expect(err).To(BeNil())
})
Expand Down Expand Up @@ -127,7 +129,7 @@ var _ = Describe("ValidateUpdate", func() {
oldAgentClassification.Spec.Query = validQuery
newAgentClassification := oldAgentClassification.DeepCopy()
newAgentClassification.Spec.LabelKey = "newKey"
warn, err := newAgentClassification.ValidateUpdate(oldAgentClassification)
warn, err := newAgentClassification.ValidateUpdate(context.TODO(), oldAgentClassification, newAgentClassification)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
Expand All @@ -137,17 +139,7 @@ var _ = Describe("ValidateUpdate", func() {
oldAgentClassification.Spec.Query = validQuery
newAgentClassification := oldAgentClassification.DeepCopy()
newAgentClassification.Spec.LabelValue = "newvalue"
warn, err := newAgentClassification.ValidateUpdate(oldAgentClassification)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
It("fails if label query is changed", func() {
oldAgentClassification.Spec.LabelKey = validKey
oldAgentClassification.Spec.LabelValue = validValue
oldAgentClassification.Spec.Query = ".cpu.count == 2"
newAgentClassification := oldAgentClassification.DeepCopy()
newAgentClassification.Spec.Query = validQuery
warn, err := newAgentClassification.ValidateUpdate(oldAgentClassification)
warn, err := newAgentClassification.ValidateUpdate(context.TODO(), oldAgentClassification, newAgentClassification)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
Expand Down
8 changes: 6 additions & 2 deletions api/v1beta1/agentserviceconfig_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ type MustGatherImage struct {
// OpenshiftVersion is the Major.Minor version of OpenShift that this image
// is to be associated with.
OpenshiftVersion string `json:"openshiftVersion"`
// CPUArchitecture is the CPU architecture of the image (x86_64/arm64/multi/etc).
Comment thread
pastequo marked this conversation as resolved.
// +kubebuilder:validation:Enum=x86_64;aarch64;arm64;ppc64le;s390x;multi
// +optional
Copy link
Copy Markdown
Contributor Author

@pastequo pastequo Feb 27, 2026

Choose a reason for hiding this comment

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

I didn't make it required to be backward compatible, even if I'm not convinced it makes a lot of sense

Copy link
Copy Markdown
Contributor

@CrystalChun CrystalChun Mar 19, 2026

Choose a reason for hiding this comment

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

Oh actually I just realized how come we didn't see many issues before about arch mismatches?

Like was the architecture of a must-gather image important?
Did the must-gather image never really run so we never got complaints? Did people just generally use x86?

Copy link
Copy Markdown
Contributor Author

@pastequo pastequo Mar 20, 2026

Choose a reason for hiding this comment

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

As far as the cache was related, we were populating the cache from those CR (without a cpu arch) and reading the cache with the cpu arch. So AFAIU the cache was never used

CPUArchitecture string `json:"cpuArchitecture"`
// Name specifies the name of the component (e.g. operator)
// that the image is used to collect information about.
Name string `json:"name"`
Expand Down Expand Up @@ -255,8 +259,8 @@ const (

// AgentServiceConfigStatus defines the observed state of AgentServiceConfig
type AgentServiceConfigStatus struct {
Conditions []conditionsv1.Condition `json:"conditions,omitempty"`
ImmutableAnnotations map[string]string `json:"immutableAnnotations,omitempty"`
Conditions []conditionsv1.Condition `json:"conditions,omitempty"`
ImmutableAnnotations map[string]string `json:"immutableAnnotations,omitempty"`
}

// +kubebuilder:object:root=true
Expand Down
24 changes: 13 additions & 11 deletions api/v1beta1/infraenv_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package v1beta1

import (
"context"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -42,7 +44,7 @@ var _ = Describe("ValidateCreate", func() {
Namespace: namespace,
}
infraEnv.Spec.OSImageVersion = "4.14"
warn, err := infraEnv.ValidateCreate()
warn, err := infraEnv.ValidateCreate(context.TODO(), infraEnv)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
Expand All @@ -53,20 +55,20 @@ var _ = Describe("ValidateCreate", func() {
Namespace: namespace,
}

warn, err := infraEnv.ValidateCreate()
warn, err := infraEnv.ValidateCreate(context.TODO(), infraEnv)
Expect(warn).To(BeNil())
Expect(err).To(BeNil())
})

It("succeeds if only OsImageVersion is set", func() {
infraEnv.Spec.OSImageVersion = "4.14"
warn, err := infraEnv.ValidateCreate()
warn, err := infraEnv.ValidateCreate(context.TODO(), infraEnv)
Expect(warn).To(BeNil())
Expect(err).To(BeNil())
})

It("succeeds if neither ClusterRef or OsImageVersion is set", func() {
warn, err := infraEnv.ValidateCreate()
warn, err := infraEnv.ValidateCreate(context.TODO(), infraEnv)
Expect(warn).To(BeNil())
Expect(err).To(BeNil())
})
Expand All @@ -92,7 +94,7 @@ var _ = Describe("ValidateUpdate", func() {
Name: "cluster-deployment",
Namespace: namespace,
}
warn, err := newInfraEnv.ValidateUpdate(oldInfraEnv)
warn, err := newInfraEnv.ValidateUpdate(context.TODO(), oldInfraEnv, newInfraEnv)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
Expand All @@ -103,7 +105,7 @@ var _ = Describe("ValidateUpdate", func() {
}
newInfraEnv := oldInfraEnv.DeepCopy()
newInfraEnv.Spec.ClusterRef = nil
warn, err := newInfraEnv.ValidateUpdate(oldInfraEnv)
warn, err := newInfraEnv.ValidateUpdate(context.TODO(), oldInfraEnv, newInfraEnv)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
Expand All @@ -117,7 +119,7 @@ var _ = Describe("ValidateUpdate", func() {
Name: "new-cluster-deployment",
Namespace: namespace,
}
warn, err := newInfraEnv.ValidateUpdate(oldInfraEnv)
warn, err := newInfraEnv.ValidateUpdate(context.TODO(), oldInfraEnv, newInfraEnv)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
Expand All @@ -130,7 +132,7 @@ var _ = Describe("ValidateUpdate", func() {
oldInfraEnv.Spec.ClusterRef = cdRef
newInfraEnv := oldInfraEnv.DeepCopy()
cdRef.Name = "new-cluster-deployment"
warn, err := newInfraEnv.ValidateUpdate(oldInfraEnv)
warn, err := newInfraEnv.ValidateUpdate(context.TODO(), oldInfraEnv, newInfraEnv)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
Expand All @@ -143,7 +145,7 @@ var _ = Describe("ValidateUpdate", func() {
oldInfraEnv.Spec.ClusterRef = cdRef
newInfraEnv := oldInfraEnv.DeepCopy()
cdRef.Namespace = "new-cluster-deployment-namespace"
warn, err := newInfraEnv.ValidateUpdate(oldInfraEnv)
warn, err := newInfraEnv.ValidateUpdate(context.TODO(), oldInfraEnv, newInfraEnv)
Expect(warn).To(BeNil())
Expect(err).NotTo(BeNil())
})
Expand All @@ -156,15 +158,15 @@ var _ = Describe("ValidateUpdate", func() {
oldInfraEnv.Spec.ClusterRef = cdRef
newInfraEnv := oldInfraEnv.DeepCopy()
newInfraEnv.Spec.SSHAuthorizedKey = "different-ssh-key"
warn, err := newInfraEnv.ValidateUpdate(oldInfraEnv)
warn, err := newInfraEnv.ValidateUpdate(context.TODO(), oldInfraEnv, newInfraEnv)
Expect(warn).To(BeNil())
Expect(err).To(BeNil())
})

It("succeeds if ClusterRef was not set and is not changed", func() {
newInfraEnv := oldInfraEnv.DeepCopy()
newInfraEnv.Spec.SSHAuthorizedKey = "different-ssh-key"
warn, err := newInfraEnv.ValidateUpdate(oldInfraEnv)
warn, err := newInfraEnv.ValidateUpdate(context.TODO(), oldInfraEnv, newInfraEnv)
Expect(warn).To(BeNil())
Expect(err).To(BeNil())
})
Expand Down
Loading