Skip to content

Commit

Permalink
feat: add support for kueue to work with VAP on OCP 4.16+
Browse files Browse the repository at this point in the history
- 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 <wenzhou@redhat.com>
  • Loading branch information
zdtsw committed Jan 14, 2025
1 parent 8865ea6 commit 7f51b9e
Show file tree
Hide file tree
Showing 11 changed files with 161 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,8 @@ spec:
- admissionregistration.k8s.io
resources:
- mutatingwebhookconfigurations
- validatingadmissionpolicies
- validatingadmissionpolicybindings
- validatingwebhookconfigurations
verbs:
- create
Expand Down
2 changes: 2 additions & 0 deletions config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ rules:
- admissionregistration.k8s.io
resources:
- mutatingwebhookconfigurations
- validatingadmissionpolicies
- validatingadmissionpolicybindings
- validatingwebhookconfigurations
verbs:
- create
Expand Down
2 changes: 2 additions & 0 deletions controllers/components/kueue/kueue.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (

type componentHandler struct{}

var enableVAP bool

func init() { //nolint:gochecknoinits
cr.Add(&componentHandler{})
}
Expand Down
15 changes: 15 additions & 0 deletions controllers/components/kueue/kueue_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand All @@ -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{}).
Expand All @@ -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{},
Expand All @@ -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(),
)).
Expand Down
19 changes: 18 additions & 1 deletion controllers/components/kueue/kueue_controller_actions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
15 changes: 15 additions & 0 deletions controllers/components/kueue/kueue_support.go
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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
}
4 changes: 3 additions & 1 deletion controllers/datasciencecluster/kubebuilder_rbac.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
57 changes: 53 additions & 4 deletions pkg/cluster/cluster_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -54,15 +60,21 @@ 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
}

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) {
Expand All @@ -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)
Expand All @@ -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 != "" {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 3 additions & 0 deletions pkg/cluster/const.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,7 @@ const (

// Default cluster-scope Authentication CR name.
ClusterAuthenticationObj = "cluster"

// Default OpenShift version CR name.
OpenShiftVersionObj = "version"
)
12 changes: 12 additions & 0 deletions pkg/cluster/gvk/gvk.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
}
)
36 changes: 36 additions & 0 deletions tests/e2e/kueue_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -20,10 +30,36 @@ 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)
}

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"))
}

0 comments on commit 7f51b9e

Please sign in to comment.