From 4c593a7f4ac23b7532fe3a9fb97dddacbc2b7f4c Mon Sep 17 00:00:00 2001 From: Wen Zhou Date: Fri, 10 Jan 2025 11:07:48 +0100 Subject: [PATCH 1/4] feat: add support for kueue to work with VAP on OCP 4.16+ - only add VAP specific manifests into list if latest OCP version in history is over 16 in min version update: explicit check 4.17.0 not just minor version - now each component can get ocp version without calculate - add test in kueue - make VAPB object user configable (as no ownerreference set) - user cannot update VAP object ( it get reconciled back to default value) Signed-off-by: Wen Zhou --- ...atahub-operator.clusterserviceversion.yaml | 2 + config/rbac/role.yaml | 2 + controllers/components/kueue/kueue.go | 2 + .../components/kueue/kueue_controller.go | 15 +++++ .../kueue/kueue_controller_actions.go | 19 ++++++- controllers/components/kueue/kueue_support.go | 15 +++++ .../datasciencecluster/kubebuilder_rbac.go | 4 +- pkg/cluster/cluster_config.go | 57 +++++++++++++++++-- pkg/cluster/const.go | 3 + pkg/cluster/gvk/gvk.go | 12 ++++ tests/e2e/kueue_test.go | 36 ++++++++++++ 11 files changed, 161 insertions(+), 6 deletions(-) diff --git a/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml b/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml index e249fbe3ad5..fa61b6093e7 100644 --- a/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml +++ b/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml @@ -293,6 +293,8 @@ spec: - admissionregistration.k8s.io resources: - mutatingwebhookconfigurations + - validatingadmissionpolicies + - validatingadmissionpolicybindings - validatingwebhookconfigurations verbs: - create diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index de7385f6a9d..06ab6910f9e 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -28,6 +28,8 @@ rules: - admissionregistration.k8s.io resources: - mutatingwebhookconfigurations + - validatingadmissionpolicies + - validatingadmissionpolicybindings - validatingwebhookconfigurations verbs: - create diff --git a/controllers/components/kueue/kueue.go b/controllers/components/kueue/kueue.go index 23287e9898f..2c2f2bdafb3 100644 --- a/controllers/components/kueue/kueue.go +++ b/controllers/components/kueue/kueue.go @@ -23,6 +23,8 @@ import ( type componentHandler struct{} +var enableVAP bool + func init() { //nolint:gochecknoinits cr.Add(&componentHandler{}) } diff --git a/controllers/components/kueue/kueue_controller.go b/controllers/components/kueue/kueue_controller.go index f8cbe32a105..58fde2339ed 100644 --- a/controllers/components/kueue/kueue_controller.go +++ b/controllers/components/kueue/kueue_controller.go @@ -19,6 +19,7 @@ package kueue import ( "context" + "github.com/blang/semver/v4" promv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" @@ -29,6 +30,8 @@ import ( ctrl "sigs.k8s.io/controller-runtime" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/gc" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render/kustomize" @@ -41,6 +44,7 @@ import ( ) func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { + enableVAP = cluster.GetClusterInfo().Version.GTE(semver.MustParse("4.17.0")) _, err := reconciler.ReconcilerFor(mgr, &componentApi.Kueue{}). // customized Owns() for Component with new predicates Owns(&corev1.ConfigMap{}). @@ -56,6 +60,16 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. Owns(&promv1.PrometheusRule{}). Owns(&admissionregistrationv1.MutatingWebhookConfiguration{}). Owns(&admissionregistrationv1.ValidatingWebhookConfiguration{}). + // We need dynamically "watch" VAP, because we want it to be configable by user and it can be left behind when kueue is removed. + OwnsGVK( + gvk.ValidatingAdmissionPolicy, + reconciler.Dynamic(vapPredicate), + ). + // We need dynamically "own" VAPB, because we want it has owner so when kueue is removed it gets cleaned. + WatchesGVK( + gvk.ValidatingAdmissionPolicyBinding, + reconciler.Dynamic(vapPredicate), + ). Owns(&appsv1.Deployment{}, reconciler.WithPredicates(resources.NewDeploymentPredicate())). Watches( &extv1.CustomResourceDefinition{}, @@ -72,6 +86,7 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. kustomize.WithLabel(labels.ODH.Component(LegacyComponentName), labels.True), kustomize.WithLabel(labels.K8SCommon.PartOf, LegacyComponentName), )). + WithAction(customizeResources). WithAction(deploy.NewAction( deploy.WithCache(), )). diff --git a/controllers/components/kueue/kueue_controller_actions.go b/controllers/components/kueue/kueue_controller_actions.go index 35ed0e1562b..4e0c506d303 100644 --- a/controllers/components/kueue/kueue_controller_actions.go +++ b/controllers/components/kueue/kueue_controller_actions.go @@ -5,13 +5,19 @@ import ( "fmt" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" odhtypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) func initialize(_ context.Context, rr *odhtypes.ReconciliationRequest) error { rr.Manifests = append(rr.Manifests, manifestsPath()) - + // Add specific manifests if OCP is greater or equal 4.17. + if enableVAP { + rr.Manifests = append(rr.Manifests, extramanifestsPath()) + } return nil } @@ -42,3 +48,14 @@ func devFlags(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { return nil } + +func customizeResources(_ context.Context, rr *odhtypes.ReconciliationRequest) error { + for i := range rr.Resources { + if rr.Resources[i].GroupVersionKind() == gvk.ValidatingAdmissionPolicyBinding { + // admin can update this resource + resources.SetAnnotation(&rr.Resources[i], annotations.ManagedByODHOperator, "false") + break // fast exist function + } + } + return nil +} diff --git a/controllers/components/kueue/kueue_support.go b/controllers/components/kueue/kueue_support.go index 40169e4bd25..ca5980a72f4 100644 --- a/controllers/components/kueue/kueue_support.go +++ b/controllers/components/kueue/kueue_support.go @@ -1,6 +1,8 @@ package kueue import ( + "context" + conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" @@ -33,3 +35,16 @@ func manifestsPath() odhtypes.ManifestInfo { SourcePath: "rhoai", } } + +func extramanifestsPath() odhtypes.ManifestInfo { + return odhtypes.ManifestInfo{ + Path: odhdeploy.DefaultManifestPath, + ContextDir: ComponentName, + SourcePath: "rhoai/ocp-4.17-addons", + } +} + +// return true if OCP is greater or equal 4.17. +func vapPredicate(context.Context, *odhtypes.ReconciliationRequest) bool { + return enableVAP +} diff --git a/controllers/datasciencecluster/kubebuilder_rbac.go b/controllers/datasciencecluster/kubebuilder_rbac.go index dcf03dbf194..86f4956a1d2 100644 --- a/controllers/datasciencecluster/kubebuilder_rbac.go +++ b/controllers/datasciencecluster/kubebuilder_rbac.go @@ -156,6 +156,8 @@ package datasciencecluster // +kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=kueues/finalizers,verbs=update // +kubebuilder:rbac:groups="monitoring.coreos.com",resources=prometheusrules,verbs=get;create;patch;delete;deletecollection;list;watch // +kubebuilder:rbac:groups="monitoring.coreos.com",resources=podmonitors,verbs=get;create;delete;update;watch;list;patch +// +kubebuilder:rbac:groups="admissionregistration.k8s.io",resources=validatingadmissionpolicybindings,verbs=get;create;delete;update;watch;list;patch +// +kubebuilder:rbac:groups="admissionregistration.k8s.io",resources=validatingadmissionpolicies,verbs=get;create;delete;update;watch;list;patch // CFO //+kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=codeflares,verbs=get;list;watch;create;update;patch;delete @@ -191,7 +193,7 @@ package datasciencecluster // +kubebuilder:rbac:groups="operator.knative.dev",resources=knativeservings,verbs=* // +kubebuilder:rbac:groups="config.openshift.io",resources=ingresses,verbs=get -// TODO: WB +// WB // +kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=workbenches,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=workbenches/status,verbs=get;update;patch // +kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=workbenches/finalizers,verbs=update diff --git a/pkg/cluster/cluster_config.go b/pkg/cluster/cluster_config.go index 08a1c445bfe..2afbbdf1a63 100644 --- a/pkg/cluster/cluster_config.go +++ b/pkg/cluster/cluster_config.go @@ -32,9 +32,15 @@ type Release struct { Version version.OperatorVersion `json:"version,omitempty"` } +type ClusterInfo struct { + Type string `json:"type,omitempty"` // openshift , TODO: can be other value if we later support other type + Version version.OperatorVersion `json:"version,omitempty"` +} + var clusterConfig struct { - Namespace string - Release Release + Namespace string + Release Release + ClusterInfo ClusterInfo } // Init initializes cluster configuration variables on startup @@ -54,6 +60,11 @@ func Init(ctx context.Context, cli client.Client) error { return err } + clusterConfig.ClusterInfo, err = getClusterInfo(ctx, cli) + if err != nil { + return err + } + printClusterConfig(log) return nil @@ -61,8 +72,9 @@ func Init(ctx context.Context, cli client.Client) error { func printClusterConfig(log logr.Logger) { log.Info("Cluster config", - "Namespace", clusterConfig.Namespace, - "Release", clusterConfig.Release) + "Operator Namespace", clusterConfig.Namespace, + "Release", clusterConfig.Release, + "Cluster", clusterConfig.ClusterInfo) } func GetOperatorNamespace() (string, error) { @@ -76,6 +88,10 @@ func GetRelease() Release { return clusterConfig.Release } +func GetClusterInfo() ClusterInfo { + return clusterConfig.ClusterInfo +} + func GetDomain(ctx context.Context, c client.Client) (string, error) { ingress := &unstructured.Unstructured{} ingress.SetGroupVersionKind(gvk.OpenshiftIngress) @@ -95,6 +111,21 @@ func GetDomain(ctx context.Context, c client.Client) (string, error) { return domain, err } +// This is an openshift speicifc implementation. +func getOCPVersion(ctx context.Context, c client.Client) (version.OperatorVersion, error) { + clusterVersion := &configv1.ClusterVersion{} + if err := c.Get(ctx, client.ObjectKey{ + Name: OpenShiftVersionObj, + }, clusterVersion); err != nil { + return version.OperatorVersion{}, errors.New("unable to get OCP version") + } + v, err := semver.ParseTolerant(clusterVersion.Status.History[0].Version) + if err != nil { + return version.OperatorVersion{}, errors.New("unable to parse OCP version") + } + return version.OperatorVersion{Version: v}, nil +} + func getOperatorNamespace() (string, error) { operatorNS, exist := os.LookupEnv("OPERATOR_NAMESPACE") if exist && operatorNS != "" { @@ -199,6 +230,7 @@ func getRelease(ctx context.Context, cli client.Client) (Release, error) { Version: semver.Version{}, }, } + // Set platform platform, err := getPlatform(ctx, cli) if err != nil { @@ -230,6 +262,23 @@ func getRelease(ctx context.Context, cli client.Client) (Release, error) { return initRelease, nil } +func getClusterInfo(ctx context.Context, cli client.Client) (ClusterInfo, error) { + c := ClusterInfo{ + Version: version.OperatorVersion{ + Version: semver.Version{}, + }, + Type: "OpenShift", + } + // Set OCP + ocpVersion, err := getOCPVersion(ctx, cli) + if err != nil { + return c, err + } + c.Version = ocpVersion + + return c, nil +} + // IsDefaultAuthMethod returns true if the default authentication method is IntegratedOAuth or empty. // This will give indication that Operator should create userGroups or not in the cluster. func IsDefaultAuthMethod(ctx context.Context, cli client.Client) (bool, error) { diff --git a/pkg/cluster/const.go b/pkg/cluster/const.go index 6a6562bff07..af8242ded2d 100644 --- a/pkg/cluster/const.go +++ b/pkg/cluster/const.go @@ -15,4 +15,7 @@ const ( // Default cluster-scope Authentication CR name. ClusterAuthenticationObj = "cluster" + + // Default OpenShift version CR name. + OpenShiftVersionObj = "version" ) diff --git a/pkg/cluster/gvk/gvk.go b/pkg/cluster/gvk/gvk.go index 06a7f199699..0c3fd589682 100644 --- a/pkg/cluster/gvk/gvk.go +++ b/pkg/cluster/gvk/gvk.go @@ -219,4 +219,16 @@ var ( Version: "v1alpha1", Kind: "Auth", } + + ValidatingAdmissionPolicy = schema.GroupVersionKind{ + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "ValidatingAdmissionPolicy", + } + + ValidatingAdmissionPolicyBinding = schema.GroupVersionKind{ + Group: "admissionregistration.k8s.io", + Version: "v1", + Kind: "ValidatingAdmissionPolicyBinding", + } ) diff --git a/tests/e2e/kueue_test.go b/tests/e2e/kueue_test.go index 4f3299f184c..2b7a19328cc 100644 --- a/tests/e2e/kueue_test.go +++ b/tests/e2e/kueue_test.go @@ -3,9 +3,19 @@ package e2e_test import ( "testing" + "github.com/blang/semver/v4" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/matchers/jq" + + . "github.com/onsi/gomega" ) func kueueTestSuite(t *testing.T) { @@ -20,6 +30,7 @@ func kueueTestSuite(t *testing.T) { t.Run("Validate component enabled", componentCtx.ValidateComponentEnabled) t.Run("Validate operands have OwnerReferences", componentCtx.ValidateOperandsOwnerReferences) + t.Run("Validate Kueue Dynamically create VAP", componentCtx.validateKueueVAPReady) t.Run("Validate update operand resources", componentCtx.ValidateUpdateDeploymentsResources) t.Run("Validate component disabled", componentCtx.ValidateComponentDisabled) } @@ -27,3 +38,28 @@ func kueueTestSuite(t *testing.T) { type KueueTestCtx struct { *ComponentTestCtx } + +func (tc *KueueTestCtx) validateKueueVAPReady(t *testing.T) { + g := tc.NewWithT(t) + if cluster.GetClusterInfo().Version.GTE(semver.MustParse("4.17.0")) { + g.Get(gvk.ValidatingAdmissionPolicy, types.NamespacedName{Name: "kueue-validating-admission-policy"}).Eventually().Should( + jq.Match(`.metadata.ownerReferences == "%s"`, componentApi.KueueInstanceName), + ) + vapb, err := g.Get(gvk.ValidatingAdmissionPolicyBinding, types.NamespacedName{Name: "kueue-validating-admission-policy-binding"}).Get() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(vapb.GetOwnerReferences()).Should(BeEmpty()) + return + } + scheme := runtime.NewScheme() + vap := &unstructured.Unstructured{} + vap.SetKind(gvk.ValidatingAdmissionPolicy.Kind) + err := resources.EnsureGroupVersionKind(scheme, vap) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to get GVK")) + + vapb := &unstructured.Unstructured{} + vapb.SetKind(gvk.ValidatingAdmissionPolicyBinding.Kind) + err = resources.EnsureGroupVersionKind(scheme, vapb) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to get GVK")) +} From 1ed97831c04b0504bf1f02643d09c727e1a97bd4 Mon Sep 17 00:00:00 2001 From: Wen Zhou Date: Tue, 14 Jan 2025 14:13:59 +0100 Subject: [PATCH 2/4] update Signed-off-by: Wen Zhou --- controllers/components/kueue/kueue.go | 2 -- .../components/kueue/kueue_controller.go | 24 +++++++------------ .../kueue/kueue_controller_actions.go | 8 ++++--- controllers/components/kueue/kueue_support.go | 7 ------ 4 files changed, 14 insertions(+), 27 deletions(-) diff --git a/controllers/components/kueue/kueue.go b/controllers/components/kueue/kueue.go index 2c2f2bdafb3..23287e9898f 100644 --- a/controllers/components/kueue/kueue.go +++ b/controllers/components/kueue/kueue.go @@ -23,8 +23,6 @@ import ( type componentHandler struct{} -var enableVAP bool - func init() { //nolint:gochecknoinits cr.Add(&componentHandler{}) } diff --git a/controllers/components/kueue/kueue_controller.go b/controllers/components/kueue/kueue_controller.go index 58fde2339ed..37da9104d0f 100644 --- a/controllers/components/kueue/kueue_controller.go +++ b/controllers/components/kueue/kueue_controller.go @@ -44,8 +44,7 @@ import ( ) func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { - enableVAP = cluster.GetClusterInfo().Version.GTE(semver.MustParse("4.17.0")) - _, err := reconciler.ReconcilerFor(mgr, &componentApi.Kueue{}). + b := reconciler.ReconcilerFor(mgr, &componentApi.Kueue{}). // customized Owns() for Component with new predicates Owns(&corev1.ConfigMap{}). Owns(&corev1.Secret{}). @@ -60,16 +59,6 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. Owns(&promv1.PrometheusRule{}). Owns(&admissionregistrationv1.MutatingWebhookConfiguration{}). Owns(&admissionregistrationv1.ValidatingWebhookConfiguration{}). - // We need dynamically "watch" VAP, because we want it to be configable by user and it can be left behind when kueue is removed. - OwnsGVK( - gvk.ValidatingAdmissionPolicy, - reconciler.Dynamic(vapPredicate), - ). - // We need dynamically "own" VAPB, because we want it has owner so when kueue is removed it gets cleaned. - WatchesGVK( - gvk.ValidatingAdmissionPolicyBinding, - reconciler.Dynamic(vapPredicate), - ). Owns(&appsv1.Deployment{}, reconciler.WithPredicates(resources.NewDeploymentPredicate())). Watches( &extv1.CustomResourceDefinition{}, @@ -92,10 +81,15 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. )). WithAction(updatestatus.NewAction()). // must be the final action - WithAction(gc.NewAction()). - Build(ctx) + WithAction(gc.NewAction()) + + if cluster.GetClusterInfo().Version.GTE(semver.MustParse("4.17.0")) { + b = b.OwnsGVK(gvk.ValidatingAdmissionPolicy) // "own" VAP, because we want it has owner so when kueue is removed it gets cleaned. + b = b.WatchesGVK(gvk.ValidatingAdmissionPolicyBinding) // "watch" VAPB, because we want it to be configable by user and it can be left behind when kueue is remov + b = b.WithAction(extraInitialize) + } - if err != nil { + if _, err := b.Build(ctx); err != nil { return err // no need customize error, it is done in the caller main } diff --git a/controllers/components/kueue/kueue_controller_actions.go b/controllers/components/kueue/kueue_controller_actions.go index 4e0c506d303..d8324f5ed6c 100644 --- a/controllers/components/kueue/kueue_controller_actions.go +++ b/controllers/components/kueue/kueue_controller_actions.go @@ -14,10 +14,12 @@ import ( func initialize(_ context.Context, rr *odhtypes.ReconciliationRequest) error { rr.Manifests = append(rr.Manifests, manifestsPath()) + return nil +} + +func extraInitialize(_ context.Context, rr *odhtypes.ReconciliationRequest) error { // Add specific manifests if OCP is greater or equal 4.17. - if enableVAP { - rr.Manifests = append(rr.Manifests, extramanifestsPath()) - } + rr.Manifests = append(rr.Manifests, extramanifestsPath()) return nil } diff --git a/controllers/components/kueue/kueue_support.go b/controllers/components/kueue/kueue_support.go index ca5980a72f4..4632b0ca8ad 100644 --- a/controllers/components/kueue/kueue_support.go +++ b/controllers/components/kueue/kueue_support.go @@ -1,8 +1,6 @@ package kueue import ( - "context" - conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" @@ -43,8 +41,3 @@ func extramanifestsPath() odhtypes.ManifestInfo { SourcePath: "rhoai/ocp-4.17-addons", } } - -// return true if OCP is greater or equal 4.17. -func vapPredicate(context.Context, *odhtypes.ReconciliationRequest) bool { - return enableVAP -} From 36afb1e131372a61974ab02d1fa9a8f195bf2fa4 Mon Sep 17 00:00:00 2001 From: Wen Zhou Date: Wed, 15 Jan 2025 13:02:31 +0100 Subject: [PATCH 3/4] update: - fix order for kueue, sinde the deploy need to be called after we get all manifests - testcase to explictly check error matching no CRD of VAP/VAPB found for < 4.17 - cannot use cluster.GetClusterInfo since that is init/set in the main, tc wont have that value Signed-off-by: Wen Zhou --- .../components/kueue/kueue_controller.go | 18 ++++----- tests/e2e/components_test.go | 13 +++++++ tests/e2e/kueue_test.go | 37 ++++++++----------- 3 files changed, 37 insertions(+), 31 deletions(-) diff --git a/controllers/components/kueue/kueue_controller.go b/controllers/components/kueue/kueue_controller.go index 37da9104d0f..332ee3a68af 100644 --- a/controllers/components/kueue/kueue_controller.go +++ b/controllers/components/kueue/kueue_controller.go @@ -44,9 +44,15 @@ import ( ) func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { - b := reconciler.ReconcilerFor(mgr, &componentApi.Kueue{}). - // customized Owns() for Component with new predicates - Owns(&corev1.ConfigMap{}). + b := reconciler.ReconcilerFor(mgr, &componentApi.Kueue{}) + + if cluster.GetClusterInfo().Version.GTE(semver.MustParse("4.17.0")) { + b = b.OwnsGVK(gvk.ValidatingAdmissionPolicy). // "own" VAP, because we want it has owner so when kueue is removed it gets cleaned. + WatchesGVK(gvk.ValidatingAdmissionPolicyBinding). // "watch" VAPB, because we want it to be configable by user and it can be left behind when kueue is remov + WithAction(extraInitialize) + } + // customized Owns() for Component with new predicates + b.Owns(&corev1.ConfigMap{}). Owns(&corev1.Secret{}). Owns(&rbacv1.ClusterRoleBinding{}). Owns(&rbacv1.ClusterRole{}). @@ -83,12 +89,6 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. // must be the final action WithAction(gc.NewAction()) - if cluster.GetClusterInfo().Version.GTE(semver.MustParse("4.17.0")) { - b = b.OwnsGVK(gvk.ValidatingAdmissionPolicy) // "own" VAP, because we want it has owner so when kueue is removed it gets cleaned. - b = b.WatchesGVK(gvk.ValidatingAdmissionPolicyBinding) // "watch" VAPB, because we want it to be configable by user and it can be left behind when kueue is remov - b = b.WithAction(extraInitialize) - } - if _, err := b.Build(ctx); err != nil { return err // no need customize error, it is done in the caller main } diff --git a/tests/e2e/components_test.go b/tests/e2e/components_test.go index 9ceac4c4f6f..c7f37290d1a 100644 --- a/tests/e2e/components_test.go +++ b/tests/e2e/components_test.go @@ -6,6 +6,8 @@ import ( "testing" "time" + "github.com/blang/semver/v4" + configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -15,6 +17,7 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" @@ -278,3 +281,13 @@ func (c *ComponentTestCtx) GetDSCI() (*dsciv1.DSCInitialization, error) { return &obj, nil } + +func (c *ComponentTestCtx) GetClusterVersion() (semver.Version, error) { + clusterVersion := &configv1.ClusterVersion{} + if err := c.Client().Get(c.Context(), client.ObjectKey{ + Name: cluster.OpenShiftVersionObj, + }, clusterVersion); err != nil { + return semver.Version{}, err + } + return semver.ParseTolerant(clusterVersion.Status.History[0].Version) +} diff --git a/tests/e2e/kueue_test.go b/tests/e2e/kueue_test.go index 2b7a19328cc..96273b2264d 100644 --- a/tests/e2e/kueue_test.go +++ b/tests/e2e/kueue_test.go @@ -2,17 +2,15 @@ package e2e_test import ( "testing" + "time" "github.com/blang/semver/v4" "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/types" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/matchers/jq" . "github.com/onsi/gomega" @@ -41,25 +39,20 @@ type KueueTestCtx struct { func (tc *KueueTestCtx) validateKueueVAPReady(t *testing.T) { g := tc.NewWithT(t) - if cluster.GetClusterInfo().Version.GTE(semver.MustParse("4.17.0")) { - g.Get(gvk.ValidatingAdmissionPolicy, types.NamespacedName{Name: "kueue-validating-admission-policy"}).Eventually().Should( - jq.Match(`.metadata.ownerReferences == "%s"`, componentApi.KueueInstanceName), + v, err := tc.GetClusterVersion() + g.Expect(err).ToNot(HaveOccurred()) + if v.GTE(semver.MustParse("4.17.0")) { + g.Get(gvk.ValidatingAdmissionPolicy, types.NamespacedName{Name: "kueue-validating-admission-policy"}).Eventually().WithTimeout(1 * time.Second). + Should( + jq.Match(`.metadata.ownerReferences[0].name == "%s"`, componentApi.KueueInstanceName), + ) + g.Get(gvk.ValidatingAdmissionPolicyBinding, types.NamespacedName{Name: "kueue-validating-admission-policy-binding"}).Eventually().WithTimeout(1 * time.Second).Should( + jq.Match(`.metadata.ownerReferences | length == 0`), ) - vapb, err := g.Get(gvk.ValidatingAdmissionPolicyBinding, types.NamespacedName{Name: "kueue-validating-admission-policy-binding"}).Get() - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(vapb.GetOwnerReferences()).Should(BeEmpty()) return } - scheme := runtime.NewScheme() - vap := &unstructured.Unstructured{} - vap.SetKind(gvk.ValidatingAdmissionPolicy.Kind) - err := resources.EnsureGroupVersionKind(scheme, vap) - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring("failed to get GVK")) - - vapb := &unstructured.Unstructured{} - vapb.SetKind(gvk.ValidatingAdmissionPolicyBinding.Kind) - err = resources.EnsureGroupVersionKind(scheme, vapb) - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring("failed to get GVK")) + _, err = g.Get(gvk.ValidatingAdmissionPolicy, types.NamespacedName{Name: "kueue-validating-admission-policy"}).Get() + g.Expect(err).Should(MatchError(&meta.NoKindMatchError{})) + _, err = g.Get(gvk.ValidatingAdmissionPolicyBinding, types.NamespacedName{Name: "kueue-validating-admission-policy-binding"}).Get() + g.Expect(err).Should(MatchError(&meta.NoKindMatchError{})) } From c8372c618b444059b2da5f24ebbd533fe23c5617 Mon Sep 17 00:00:00 2001 From: Wen Zhou Date: Wed, 15 Jan 2025 15:05:40 +0100 Subject: [PATCH 4/4] test: move check VAP/VAPB after test of "kueue CR ready" Signed-off-by: Wen Zhou --- tests/e2e/kueue_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/kueue_test.go b/tests/e2e/kueue_test.go index 96273b2264d..66fe05e7b9a 100644 --- a/tests/e2e/kueue_test.go +++ b/tests/e2e/kueue_test.go @@ -28,8 +28,8 @@ func kueueTestSuite(t *testing.T) { t.Run("Validate component enabled", componentCtx.ValidateComponentEnabled) t.Run("Validate operands have OwnerReferences", componentCtx.ValidateOperandsOwnerReferences) - t.Run("Validate Kueue Dynamically create VAP", componentCtx.validateKueueVAPReady) t.Run("Validate update operand resources", componentCtx.ValidateUpdateDeploymentsResources) + t.Run("Validate Kueue Dynamically create VAP and VAPB", componentCtx.validateKueueVAPReady) t.Run("Validate component disabled", componentCtx.ValidateComponentDisabled) }