Fix metadata.managedFields must be nil error in DSC #1018

VaishnaviHire · 2024-05-20T15:59:02Z

Description

Jira Issue: https://issues.redhat.com/browse/RHOAIENG-7401

This PR also refactors the manageResource function.

How Has This Been Tested?

Testing Steps:

Deploy a clusterwide resource without operator. E.g (Kuberay clusterrole)
Deploy operator
Create DSC with Kuberay set to Removed
Verify Kuberay clusterrole is not removed
Verify metadata.managedFields must be nil error is not triggered.

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

zdtsw · 2024-05-21T01:22:05Z

pkg/deploy/deploy.go

+	for _, owner := range ownerReferences {
+		if owner.Kind != "DataScienceCluster" && owner.Kind != "DataScienceInitialization" {
+			return false
+		}


if owerReerences is array, resource could have multiple ownerreference.
should it return false only when all not match DSC and DSCI?

for _, owner := range ownerReferences { if owner.Kind == "DataScienceCluster" || owner.Kind == "DataScienceInitialization" { return true } return false

zdtsw · 2024-05-21T01:39:34Z

pkg/deploy/deploy.go

+	return nil
+}
+
+func hasInvalidOwnerReferences(ownerReferences []metav1.OwnerReference) bool {


name of the func is a bit confusing. isOwnedByODHCRD() ?
so the idea is to check if resource does not have OwnerRef or it is owned by DSCI and DSC, then delete it?

zdtsw · 2024-05-21T01:42:49Z

i think there are two different cases with the metadata.managedFields error:

as stated in the jira ticket, by the way of how users creates these resoruce
for the component controllers, they could create resource with sdk but without setting ownerreference or labels
this PR should have covered both cases, but on the safe side, we need to check with component owners to ensure they have their controller updated if they want such resource to be kept or deleted.

ykaliuta

I really like the refactoring, but I would say splitting such code moves and tiny functional changes into 2 patches would make it a bit more review friendly as for me.

ykaliuta · 2024-05-21T06:59:08Z

pkg/deploy/deploy.go

-		}
+	// Return if error getting resource in cluster
+	found, err := getResource(ctx, cli, obj)
+	if err != nil && !apierrs.IsNotFound(err) {


may be

if client.IgnoreNotFound(err) != nil

makes less negations to handle. Not sure.

ykaliuta · 2024-05-21T07:14:36Z

pkg/deploy/deploy.go

@@ -488,4 +388,132 @@ func OperatorExists(cli client.Client, operatorPrefix string) (bool, error) {
 	return false, nil
 }

+func getResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured) (*unstructured.Unstructured, error) {
+	found := obj.DeepCopy()


Why it was done such a way at the first place? Get() does not need the object from manifests. But the logic was "if it is not deleted, not 'not found', but we could not fetch it, continue patching with the data we got from the manifests, not from the cluster".

Does

func getResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { found := &unstructured.Unstructured{} err := cli.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, found) if err != nil { return nil, err } return found, nil }

make it work as expected or I'm missing something?

Yep, this should work. You are rightfound will never be nil. Thanks for catching this. Will update the function.

ykaliuta · 2024-05-21T07:14:52Z

pkg/deploy/deploy.go

+}
+
+func handleDisabledComponent(ctx context.Context, cli client.Client, found *unstructured.Unstructured, componentName string, owner metav1.Object) error {
+	if found == nil {


Probably, will not work here since line 392 makes allocation.

It should since we pass it function argument

But it is never nil even if the resource does not exist

ykaliuta · 2024-05-21T07:19:46Z

pkg/deploy/deploy.go

+}
+
+func updateOwnerReferencesAndDelete(ctx context.Context, cli client.Client, found *unstructured.Unstructured, componentName string, owner metav1.Object) error {
+	existingOwnerReferences := found.GetOwnerReferences()


Ha, was it a problem in the original code? It used obj, not found.

ykaliuta · 2024-05-21T07:25:16Z

pkg/deploy/deploy.go

+
+func hasInvalidOwnerReferences(ownerReferences []metav1.OwnerReference) bool {
+	for _, owner := range ownerReferences {
+		if owner.Kind != "DataScienceCluster" && owner.Kind != "DataScienceInitialization" {


It is not "DSCInitialization", right?

ykaliuta · 2024-05-21T07:29:21Z

pkg/deploy/deploy.go

+}
+
+func createResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured, owner metav1.Object) error {
+	if obj.GetKind() != "CustomResourceDefinition" && obj.GetKind() != "OdhDashboardConfig" {


It's a change in logic with the old code, "OdhDashboardConfig" was handled here as well, right?

ykaliuta · 2024-05-21T07:33:30Z

pkg/deploy/deploy.go

+}
+
+func skipUpdateOnWhitelistedFields(obj *unstructured.Unstructured, componentName string) error {
+	if componentName == "kserve" || componentName == "model-mesh" {


A component added and label check removed comparing to the original code, right?

I think we have both components in incubation branch - https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/pkg/deploy/deploy.go#L292-L310

And the label check is not needed

VaishnaviHire · 2024-05-21T16:31:22Z

i think there are two different cases with the metadata.managedFields error:

as stated in the jira ticket, by the way of how users creates these resoruce

for the component controllers, they could create resource with sdk but without setting ownerreference or labels
this PR should have covered both cases, but on the safe side, we need to check with component owners to ensure they have their controller updated if they want such resource to be kept or deleted.

For (2), operator should not be responsible for managing and cleanup of resources created by controllers. We are not currently handling this and should not moving forward. The only resources managed by the operator should be the ones included in manifests or the resources created as part of preconditions and postconditions.

Let me know if you see a reason to manage resources that are created by a component controller

AjayJagan · 2024-05-22T05:50:05Z

changes looks good to me :)

zdtsw · 2024-05-23T01:47:56Z

i think there are two different cases with the metadata.managedFields error:

as stated in the jira ticket, by the way of how users creates these resoruce

for the component controllers, they could create resource with sdk but without setting ownerreference or labels
this PR should have covered both cases, but on the safe side, we need to check with component owners to ensure they have their controller updated if they want such resource to be kept or deleted.

For (2), operator should not be responsible for managing and cleanup of resources created by controllers. We are not currently handling this and should not moving forward. The only resources managed by the operator should be the ones included in manifests or the resources created as part of preconditions and postconditions.

Let me know if you see a reason to manage resources that are created by a component controller

i agree with you that we should not take ownership of these resource not done by our operator.
my concern, for example: end user want to use Ray but they do not use our Operator to manage/remove Ray.
they update Ray's yaml file to add "label" then do kubectl apply to create these resource => these resource are created in the cluster, with label but no ownerreference (as existingOwnerReferences == nil ?)
will our Operator remove these resource once Operator starts?

VaishnaviHire · 2024-05-23T16:43:13Z

i think there are two different cases with the metadata.managedFields error:

as stated in the jira ticket, by the way of how users creates these resoruce

for the component controllers, they could create resource with sdk but without setting ownerreference or labels
this PR should have covered both cases, but on the safe side, we need to check with component owners to ensure they have their controller updated if they want such resource to be kept or deleted.

For (2), operator should not be responsible for managing and cleanup of resources created by controllers. We are not currently handling this and should not moving forward. The only resources managed by the operator should be the ones included in manifests or the resources created as part of preconditions and postconditions.
Let me know if you see a reason to manage resources that are created by a component controller

i agree with you that we should not take ownership of these resource not done by our operator. my concern, for example: end user want to use Ray but they do not use our Operator to manage/remove Ray. they update Ray's yaml file to add "label" then do kubectl apply to create these resource => these resource are created in the cluster, with label but no ownerreference (as existingOwnerReferences == nil ?) will our Operator remove these resource once Operator starts?

Yes, operator will remove those resources. Manually adding odh labels is not a supported path for end users. So if a user add s the label to their self-managed resources, they can expect them to be managed by the operator

VaishnaviHire · 2024-05-23T22:48:05Z

/test opendatahub-operator-e2e

This commit also updates how we fetch resources from cluster

VaishnaviHire · 2024-05-28T17:58:50Z

/test opendatahub-operator-e2e

ykaliuta · 2024-05-29T07:47:36Z

pkg/deploy/deploy.go

-	if err != nil {
-		return err
+	// Create resource if it doesn't exist
+	if apierrs.IsNotFound(err) || found == nil {


This found check looks redundant. If the object lookup returns NotFound, it's created. Creation uses only manifest's object. (If it's not NotFound, line 216 returns. So, updateResource happens only when no error).

ykaliuta · 2024-05-29T07:48:33Z

pkg/deploy/deploy.go

+	// Setting gvk is required to do Get request
+	found.SetGroupVersionKind(obj.GroupVersionKind())
+	err := cli.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, found)
+	return found, err


It again ends up in found never nil.

I think we would need to return nil when we get ObjectNotFound error

Well, since we have error handling on the caller function, returning nil with any error should be ok (and actual object on non-error Get only)

ykaliuta · 2024-05-29T15:00:09Z

pkg/deploy/deploy.go

-		if err := removeResourcesFromDeployment(obj); err != nil {
-			return err
-		}
+	// Return if error getting resource in cluster


The comment is a bit misleading now since getResource does not return error if it was NotFound.

I would say the original logic flow is more clear to me. If getResource() returns (nil, error) if there was error including NotFound and returns (found, nil) if it could get the object, than original check here if IgnoreNotFound(err) != nil { return err } will catch all the errors and we can handle NotFound with found to nil check.

Then getResource() as

func getResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { found := &unstructured.Unstructured{} // Setting gvk is required to do Get request found.SetGroupVersionKind(obj.GroupVersionKind()) err := cli.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, found) if err != nil { return nil, err } return found, nil }

will not break idiomatic way of propagating error.

But I'll not insist.

Yeah I will update this. Thanks!

ykaliuta · 2024-05-29T15:04:33Z

pkg/deploy/deploy.go

+	if apierrs.IsNotFound(err) {
+		return nil, nil
+	}
+	return found, err


If it's not NotFound it will return the error and blank object. Hope, it's ok for you. It does not break the code (the check in manageResource for err will filter it).

Patching resources is not required before deletion. Updated delete function

zdtsw · 2024-05-30T09:48:44Z

/test opendatahub-operator-e2e

openshift-ci · 2024-05-30T09:49:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AjayJagan, ykaliuta, zdtsw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [AjayJagan,ykaliuta,zdtsw]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

) * Fix metadata.managedFields must be nil error in DSC * Address comments This commit also updates how we fetch resources from cluster * Fix getResource function * addressed comments * fix linters * Update resource deletion Patching resources is not required before deletion. Updated delete function * Fix linters (cherry picked from commit f4e66f0)

Fix metadata.managedFields must be nil error in DSC (#1018)

openshift-ci bot requested review from HumairAK and ykaliuta May 20, 2024 15:59

VaishnaviHire requested review from ajaypratap003 and AjayJagan and removed request for HumairAK May 20, 2024 15:59

VaishnaviHire force-pushed the rhoai_7401 branch from c245463 to e59991c Compare May 20, 2024 19:36

zdtsw reviewed May 21, 2024

View reviewed changes

ykaliuta reviewed May 21, 2024

View reviewed changes

VaishnaviHire force-pushed the rhoai_7401 branch from e59991c to 107d947 Compare May 23, 2024 19:54

VaishnaviHire added 2 commits May 28, 2024 12:47

Fix metadata.managedFields must be nil error in DSC

c8a6507

Address comments

0d133b2

This commit also updates how we fetch resources from cluster

Fix getResource function

524a80e

VaishnaviHire force-pushed the rhoai_7401 branch from 107d947 to 524a80e Compare May 28, 2024 20:56

ykaliuta reviewed May 29, 2024

View reviewed changes

VaishnaviHire added 2 commits May 29, 2024 09:11

addressed comments

4128ec2

fix linters

98d3892

ykaliuta reviewed May 29, 2024

View reviewed changes

VaishnaviHire added 2 commits May 29, 2024 16:10

Update resource deletion

3287d6d

Patching resources is not required before deletion. Updated delete function

Fix linters

3ea0a6f

VaishnaviHire added the rhoai-2.10 label May 29, 2024

zdtsw approved these changes May 30, 2024

View reviewed changes

openshift-ci bot assigned zdtsw May 30, 2024

openshift-ci bot added the lgtm label May 30, 2024

openshift-ci bot added the approved label May 30, 2024

ykaliuta approved these changes May 30, 2024

View reviewed changes

openshift-ci bot assigned ykaliuta May 30, 2024

AjayJagan approved these changes May 30, 2024

View reviewed changes

openshift-ci bot assigned AjayJagan May 30, 2024

openshift-merge-bot bot merged commit f4e66f0 into opendatahub-io:incubation May 30, 2024
8 checks passed

VaishnaviHire referenced this pull request in red-hat-data-services/rhods-operator May 31, 2024

Merge pull request #271 from VaishnaviHire/update_2.8.3

26b5c24

Fix metadata.managedFields must be nil error in DSC (#1018)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix metadata.managedFields must be nil error in DSC #1018

Fix metadata.managedFields must be nil error in DSC #1018

VaishnaviHire commented May 20, 2024 •

edited

Loading

zdtsw May 21, 2024

zdtsw May 21, 2024

zdtsw commented May 21, 2024

ykaliuta left a comment

ykaliuta May 21, 2024

ykaliuta May 21, 2024

ykaliuta May 23, 2024 •

edited

Loading

VaishnaviHire May 23, 2024

ykaliuta May 21, 2024

VaishnaviHire May 21, 2024

ykaliuta May 21, 2024 •

edited

Loading

ykaliuta May 21, 2024

ykaliuta May 21, 2024

ykaliuta May 21, 2024

ykaliuta May 21, 2024

VaishnaviHire May 21, 2024

VaishnaviHire commented May 21, 2024

AjayJagan commented May 22, 2024

zdtsw commented May 23, 2024

VaishnaviHire commented May 23, 2024

VaishnaviHire commented May 23, 2024

VaishnaviHire commented May 28, 2024

ykaliuta May 29, 2024

ykaliuta May 29, 2024

VaishnaviHire May 29, 2024

ykaliuta May 29, 2024

ykaliuta May 29, 2024

VaishnaviHire May 29, 2024

VaishnaviHire May 29, 2024

ykaliuta May 29, 2024

zdtsw commented May 30, 2024

openshift-ci bot commented May 30, 2024

Fix metadata.managedFields must be nil error in DSC #1018

Fix metadata.managedFields must be nil error in DSC #1018

Conversation

VaishnaviHire commented May 20, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdtsw commented May 21, 2024

ykaliuta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykaliuta May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykaliuta May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishnaviHire commented May 21, 2024

AjayJagan commented May 22, 2024

zdtsw commented May 23, 2024

VaishnaviHire commented May 23, 2024

VaishnaviHire commented May 23, 2024

VaishnaviHire commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdtsw commented May 30, 2024

openshift-ci bot commented May 30, 2024

VaishnaviHire commented May 20, 2024 •

edited

Loading

ykaliuta May 23, 2024 •

edited

Loading

ykaliuta May 21, 2024 •

edited

Loading