Skip to content

Commit

Permalink
Remove DSC ownership from deployed components resources (opendatahub-…
Browse files Browse the repository at this point in the history
…io#1437)

* Remove DSC ownership from deployed components resources

Component owner resources must be reowned from DSC to the component
specific CR

* Fix findings

* unit tests

* Fix linter

* Fix e2e
  • Loading branch information
lburgazzoli authored Dec 10, 2024
1 parent f9a2b9b commit 6d836db
Show file tree
Hide file tree
Showing 12 changed files with 461 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
appsv1 "k8s.io/api/apps/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

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"
Expand Down Expand Up @@ -66,7 +65,6 @@ func TestDeleteResourcesAction(t *testing.T) {
Client: cl,
Instance: nil,
DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}},
DSC: &dscv1.DataScienceCluster{},
Release: cluster.Release{Name: cluster.OpenDataHub},
})

Expand Down
71 changes: 27 additions & 44 deletions pkg/controller/actions/deploy/action_deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,38 @@ func (a *Action) run(ctx context.Context, rr *odhTypes.ReconciliationRequest) er

for i := range rr.Resources {
res := rr.Resources[i]
current := resources.GvkToUnstructured(res.GroupVersionKind())

lookupErr := rr.Client.Get(ctx, client.ObjectKeyFromObject(&res), current)
switch {
case k8serr.IsNotFound(lookupErr):
// set it to nil fto pass it down to other methods and signal
// that there's no previous known state of the resource
current = nil
case lookupErr != nil:
return fmt.Errorf("failed to lookup object %s/%s: %w", res.GetNamespace(), res.GetName(), lookupErr)
default:
// Remove the DSC and DSCI owner reference if set, This is required during the
// transition from the old to the new operator.
if err := removeOwnerReferences(ctx, rr.Client, current, isLegacyOwnerRef); err != nil {
return err
}

// the user has explicitly marked the current object as not owned by the operator, so
// skip any further processing
if resources.GetAnnotation(current, annotations.ManagedByODHOperator) == "false" {
continue
}
}

var ok bool
var err error

switch rr.Resources[i].GroupVersionKind() {
case gvk.CustomResourceDefinition:
ok, err = a.deployCRD(ctx, rr, res)
ok, err = a.deployCRD(ctx, rr, res, current)
default:
ok, err = a.deploy(ctx, rr, res)
ok, err = a.deploy(ctx, rr, res, current)
}

if err != nil {
Expand All @@ -148,18 +171,8 @@ func (a *Action) deployCRD(
ctx context.Context,
rr *odhTypes.ReconciliationRequest,
obj unstructured.Unstructured,
current *unstructured.Unstructured,
) (bool, error) {
current, lookupErr := a.lookup(ctx, rr.Client, obj)
if lookupErr != nil {
return false, fmt.Errorf("failed to lookup object %s/%s: %w", obj.GetNamespace(), obj.GetName(), lookupErr)
}

// the user has explicitly marked the current object as not owned by the operator, so
// skip any further processing
if current != nil && resources.GetAnnotation(current, annotations.ManagedByODHOperator) == "false" {
return false, nil
}

resources.SetLabels(&obj, a.labels)
resources.SetAnnotations(&obj, a.annotations)
resources.SetLabel(&obj, labels.PlatformPartOf, labels.Platform)
Expand Down Expand Up @@ -215,18 +228,8 @@ func (a *Action) deploy(
ctx context.Context,
rr *odhTypes.ReconciliationRequest,
obj unstructured.Unstructured,
current *unstructured.Unstructured,
) (bool, error) {
current, lookupErr := a.lookup(ctx, rr.Client, obj)
if lookupErr != nil {
return false, fmt.Errorf("failed to lookup object %s/%s: %w", obj.GetNamespace(), obj.GetName(), lookupErr)
}

// the user has explicitly marked the current object as not owned by the operator, so
// skip any further processing
if current != nil && resources.GetAnnotation(current, annotations.ManagedByODHOperator) == "false" {
return false, nil
}

fo := a.fieldOwner
if fo == "" {
kind, err := resources.KindForObject(rr.Client.Scheme(), rr.Instance)
Expand Down Expand Up @@ -316,26 +319,6 @@ func (a *Action) deploy(
return true, nil
}

func (a *Action) lookup(
ctx context.Context,
c *odhClient.Client,
obj unstructured.Unstructured,
) (*unstructured.Unstructured, error) {
found := unstructured.Unstructured{}
found.SetGroupVersionKind(obj.GroupVersionKind())

// TODO: use PartialObjectMetadata for resources where it make sense
err := c.Get(ctx, client.ObjectKeyFromObject(&obj), &found)
switch {
case err == nil:
return &found, nil
case k8serr.IsNotFound(err):
return nil, nil
default:
return nil, err
}
}

func (a *Action) create(
ctx context.Context,
c *odhClient.Client,
Expand Down
7 changes: 2 additions & 5 deletions pkg/controller/actions/deploy/action_deploy_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"sigs.k8s.io/controller-runtime/pkg/envtest"

componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1"
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"
Expand Down Expand Up @@ -87,7 +86,7 @@ func TestDeployWithCacheAction(t *testing.T) {
true)
})

t.Run("NonExistingResource(", func(t *testing.T) {
t.Run("NonExistingResource", func(t *testing.T) {
testResourceNotReDeployed(
t,
cli,
Expand All @@ -104,7 +103,7 @@ func TestDeployWithCacheAction(t *testing.T) {
false)
})

t.Run("CacheTTL(", func(t *testing.T) {
t.Run("CacheTTL", func(t *testing.T) {
testCacheTTL(
t,
cli,
Expand Down Expand Up @@ -148,7 +147,6 @@ func testResourceNotReDeployed(t *testing.T, cli *client.Client, obj ctrlCli.Obj
DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{
ApplicationsNamespace: in.GetNamespace()},
},
DSC: &dscv1.DataScienceCluster{},
Instance: &componentApi.Dashboard{
ObjectMeta: metav1.ObjectMeta{
Generation: 1,
Expand Down Expand Up @@ -222,7 +220,6 @@ func testCacheTTL(t *testing.T, cli *client.Client, obj ctrlCli.Object) {
DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{
ApplicationsNamespace: in.GetNamespace()},
},
DSC: &dscv1.DataScienceCluster{},
Instance: &componentApi.Dashboard{
ObjectMeta: metav1.ObjectMeta{
Generation: 1,
Expand Down
73 changes: 73 additions & 0 deletions pkg/controller/actions/deploy/action_deploy_support.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package deploy

import (
"context"
"fmt"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk"
)

func isLegacyOwnerRef(or metav1.OwnerReference) bool {
switch {
case or.APIVersion == gvk.DataScienceCluster.GroupVersion().String() && or.Kind == gvk.DataScienceCluster.Kind:
return true
case or.APIVersion == gvk.DSCInitialization.GroupVersion().String() && or.Kind == gvk.DSCInitialization.Kind:
return true
default:
return false
}
}

// removeOwnerReferences removes all owner references from a Kubernetes object that match the provided predicate.
//
// This function iterates through the OwnerReferences of the given object, filters out those that satisfy
// the predicate, and updates the object in the cluster using the provided client.
//
// Parameters:
// - ctx: The context for the request, which can carry deadlines, cancellation signals, and other request-scoped values.
// - cli: A controller-runtime client used to update the Kubernetes object.
// - obj: The Kubernetes object whose OwnerReferences are to be filtered. It must implement client.Object.
// - predicate: A function that takes an OwnerReference and returns true if the reference should be removed.
//
// Returns:
// - An error if the update operation fails, otherwise nil.
func removeOwnerReferences(
ctx context.Context,
cli client.Client,
obj client.Object,
predicate func(reference metav1.OwnerReference) bool,
) error {
oldRefs := obj.GetOwnerReferences()
if len(oldRefs) == 0 {
return nil
}

newRefs := oldRefs[:0]
for _, ref := range oldRefs {
if !predicate(ref) {
newRefs = append(newRefs, ref)
}
}

if len(newRefs) == len(oldRefs) {
return nil
}

obj.SetOwnerReferences(newRefs)

// Update the object in the cluster
if err := cli.Update(ctx, obj); err != nil {
return fmt.Errorf(
"failed to remove owner references from object %s/%s with gvk %s: %w",
obj.GetNamespace(),
obj.GetName(),
obj.GetObjectKind().GroupVersionKind(),
err,
)
}

return nil
}
Loading

0 comments on commit 6d836db

Please sign in to comment.