Skip to content
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.

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

feat: add predicates on deployment #1172

Merged
merged 4 commits into from
Aug 27, 2024
Merged
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
28 changes: 25 additions & 3 deletions controllers/datasciencecluster/datasciencecluster_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import (
"github.com/opendatahub-io/opendatahub-operator/v2/components/datasciencepipelines"
"github.com/opendatahub-io/opendatahub-operator/v2/controllers/status"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster"
annotations "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/upgrade"
)
Expand Down Expand Up @@ -366,7 +367,25 @@ var configMapPredicates = predicate.Funcs{
return false
}
// Do not reconcile on kserver's inferenceservice-config CM updates, for rawdeployment
if e.ObjectNew.GetName() == "inferenceservice-config" && (e.ObjectNew.GetNamespace() == "redhat-ods-applications" || e.ObjectNew.GetNamespace() == "opendatahub") {
namespace := e.ObjectNew.GetNamespace()
if e.ObjectNew.GetName() == "inferenceservice-config" && (namespace == "redhat-ods-applications" || namespace == "opendatahub") { //nolint:goconst
return false
}
return true
},
}

// reduce unnecessary reconcile triggered by odh component's deployment change due to ManagedByODHOperator annotation.
var componentDeploymentPredicates = predicate.Funcs{
UpdateFunc: func(e event.UpdateEvent) bool {
namespace := e.ObjectNew.GetNamespace()
if namespace == "opendatahub" || namespace == "redhat-ods-applications" {
oldManaged, oldExists := e.ObjectOld.GetAnnotations()[annotations.ManagedByODHOperator]
newManaged := e.ObjectNew.GetAnnotations()[annotations.ManagedByODHOperator]
// only reoncile if annotation from "not exist" to "set to true", or from "non-true" value to "true"
if newManaged == "true" && (!oldExists || oldManaged != "true") {
return true
}
return false
}
return true
Expand All @@ -376,7 +395,8 @@ var configMapPredicates = predicate.Funcs{
// a workaround for 2.5 due to odh-model-controller serivceaccount keeps updates with label.
var saPredicates = predicate.Funcs{
UpdateFunc: func(e event.UpdateEvent) bool {
if e.ObjectNew.GetName() == "odh-model-controller" && (e.ObjectNew.GetNamespace() == "redhat-ods-applications" || e.ObjectNew.GetNamespace() == "opendatahub") {
namespace := e.ObjectNew.GetNamespace()
if e.ObjectNew.GetName() == "odh-model-controller" && (namespace == "redhat-ods-applications" || namespace == "opendatahub") {
return false
}
return true
Expand Down Expand Up @@ -457,7 +477,9 @@ func (r *DataScienceClusterReconciler) SetupWithManager(ctx context.Context, mgr
Owns(
&rbacv1.ClusterRoleBinding{},
builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, modelMeshRBPredicates))).
Owns(&appsv1.Deployment{}).
Owns(
&appsv1.Deployment{},
builder.WithPredicates(componentDeploymentPredicates)).
Owns(&corev1.PersistentVolumeClaim{}).
Owns(
&corev1.Service{},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,6 @@ var CMContentChangedPredicate = predicate.Funcs{

var DSCDeletionPredicate = predicate.Funcs{
DeleteFunc: func(e event.DeleteEvent) bool {

return true
},
}
Expand Down
47 changes: 25 additions & 22 deletions pkg/deploy/deploy.go
Copy link
Contributor

Choose a reason for hiding this comment

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

What the point of this file changes? For me they make it less readable. And at the first glance I do not see they are related to the rest so we can discuss them in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, I still do not support that part of 0261670 ("chore: refactor on some logic for component manifests resources (#1121)")

Copy link
Member Author

Choose a reason for hiding this comment

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

i did some rework on the deploy part

Original file line number Diff line number Diff line change
Expand Up @@ -204,30 +204,31 @@ func manageResource(ctx context.Context, cli client.Client, res *resource.Resour

found, err := getResource(ctx, cli, res)

if err != nil {
if !k8serr.IsNotFound(err) {
return err
}
// Create resource if it doesn't exist and component enabled
if err == nil {
// when resource is found
if enabled {
return createResource(ctx, cli, res, owner)
// Exception to not update kserve with managed annotation
// do not reconcile kserve resource with annotation "opendatahub.io/managed: false"
// TODO: remove this exception when we define managed annotation across odh
if found.GetAnnotations()[annotations.ManagedByODHOperator] == "false" && componentName == "kserve" {
return nil
}
return updateResource(ctx, cli, res, found, owner)
}
// Skip if resource doesn't exist and component is disabled
return nil
// Delete resource if it exists or do nothing if not found
return handleDisabledComponent(ctx, cli, found, componentName)
}

// when resource is found
if !k8serr.IsNotFound(err) {
return err
}

// Create resource when component enabled
if enabled {
// Exception to not update kserve with managed annotation
// do not reconcile kserve resource with annotation "opendatahub.io/managed: false"
// TODO: remove this exception when we define managed annotation across odh
if found.GetAnnotations()[annotations.ManagedByODHOperator] == "false" && componentName == "kserve" {
return nil
}
return updateResource(ctx, cli, res, found, owner)
return createResource(ctx, cli, res, owner)
}
// Delete resource if it exists and component is disabled
return handleDisabledComponent(ctx, cli, found, componentName)
// Skip if resource doesn't exist and component is disabled
return nil
}

func getResource(ctx context.Context, cli client.Client, obj *resource.Resource) (*unstructured.Unstructured, error) {
Expand Down Expand Up @@ -287,15 +288,15 @@ func createResource(ctx context.Context, cli client.Client, res *resource.Resour
return cli.Create(ctx, obj)
}

// Exception to skip ODHDashboardConfig CR reconcile.
func updateResource(ctx context.Context, cli client.Client, res *resource.Resource, found *unstructured.Unstructured, owner metav1.Object) error {
// Skip ODHDashboardConfig Update
if found.GetKind() == "OdhDashboardConfig" {
return nil
}

// only reconcile whiltelistedFields if the existing resource has annoation set to "true"
// all other cases, whiltelistedfields will be skipped by ODH operator
if managed, exists := found.GetAnnotations()[annotations.ManagedByODHOperator]; !exists || managed != "true" {
// Operator reconcile allowedListfield only when resource is managed by operator(annotation is true)
// all other cases: no annotation at all, required annotation not present, of annotation is non-true value, skip reconcile
if managed := found.GetAnnotations()[annotations.ManagedByODHOperator]; managed != "true" {
if err := skipUpdateOnAllowlistedFields(res); err != nil {
return err
}
Expand Down Expand Up @@ -336,11 +337,13 @@ func updateLabels(found, obj *unstructured.Unstructured) {
obj.SetLabels(foundLabels)
}

// preformPatch works for update cases.
func performPatch(ctx context.Context, cli client.Client, obj, found *unstructured.Unstructured, owner metav1.Object) error {
data, err := json.Marshal(obj)
if err != nil {
return err
}
// force owner to be default-dsc/default-dsci
return cli.Patch(ctx, found, client.RawPatch(types.ApplyPatchType, data), client.ForceOwnership, client.FieldOwner(owner.GetName()))
}

Expand Down
Loading