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

fix: when delete resource if no CRD in the cluster #1044

Merged

Conversation

zdtsw
Copy link
Member

@zdtsw zdtsw commented Jun 7, 2024

  • for Removed, resource wont get removed since CRD not exist, resource wont exists as well.
  • thanks @ykaliuta pointed to the error for this case

Description

ref: https://issues.redhat.com/browse/RHOAIENG-8297

How Has This Been Tested?

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 zdtsw requested review from VaishnaviHire and ykaliuta and removed request for mattmahoneyrh June 7, 2024 12:46
pkg/deploy/deploy.go Outdated Show resolved Hide resolved
@@ -359,6 +358,9 @@ func getResource(ctx context.Context, cli client.Client, obj *unstructured.Unstr
found.SetGroupVersionKind(obj.GroupVersionKind())
err := cli.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, found)
if err != nil {
if strings.Contains(err.Error(), "no matches for kind") {
return nil, nil // ignore mising CRD error
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be ok to return new NotFound? Argumentation is the same as af749ec

Copy link
Member Author

@zdtsw zdtsw Jun 7, 2024

Choose a reason for hiding this comment

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

should we just handle the notfound directly in the getResource()?
and we
e.g

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 {
		if errors.Is(err, &meta.NoKindMatchError{}) {
			return nil, nil // ignore mising CRD error
		}
		if client.IgnoreNotFound(err) == nil {
			return nil, nil // not found resource
		}
		return nil, err
	}
	return found, nil
}

for the caller:

	// Return if error getting resource in cluster
	found, err := getResource(ctx, cli, obj); 
	if err != nil {
		return err
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

This will work, but I do not like the idea to return nil object with nil error. It's prone to mistakes. But I do not have a strong opinion. Let's ask one more point of view? :) @bartoszmajsak

Copy link
Contributor

Choose a reason for hiding this comment

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

@zdtsw @ykaliuta In my opinion, returning nil, nil can cause more harm than good. The response should either be the requested item or an error. This way, you only need to check for nil in the case of an error, allowing you to handle exceptions appropriately. Otherwise, we risk cluttering the codebase with nil checks or encountering nil pointer errors at runtime, as the caller may assume the returned instance is valid if the error is nil. Therefore, I believe it is preferable to return apierrs.NewNotFound as @ykaliuta suggests.

Copy link
Member Author

Choose a reason for hiding this comment

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

follow up in #1045 to address comments.

Copy link

openshift-ci bot commented Jun 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ykaliuta

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:

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

@openshift-ci openshift-ci bot added the approved label Jun 7, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit c040f77 into opendatahub-io:incubation Jun 7, 2024
8 checks passed
VaishnaviHire pushed a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Jun 10, 2024
VaishnaviHire pushed a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Jun 10, 2024
- since we already have these resource for create, list, delete etc, no
harm to add get on them

Signed-off-by: Wen Zhou <[email protected]>
(cherry picked from commit bd9b83c)

fix: when delete resource if no CRD in the cluster (opendatahub-io#1044)

Signed-off-by: Wen Zhou <[email protected]>
(cherry picked from commit c040f77)

deploy: refactor getResource to return NotFound for both cases (opendatahub-io#1046)

Avoid returning (nil, nil), convert error if CRD not present to
NotFound and handle them in the caller code.

Avoid checking for found == nil but handle only error code.

This is more idiomatic and less error prone.

Signed-off-by: Yauheni Kaliuta <[email protected]>
(cherry picked from commit 9651b76)

Handle Kserve exception

Fix csv
VaishnaviHire pushed a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Jun 10, 2024
- since we already have these resource for create, list, delete etc, no
harm to add get on them

Signed-off-by: Wen Zhou <[email protected]>
(cherry picked from commit bd9b83c)

fix: when delete resource if no CRD in the cluster (opendatahub-io#1044)

Signed-off-by: Wen Zhou <[email protected]>
(cherry picked from commit c040f77)

deploy: refactor getResource to return NotFound for both cases (opendatahub-io#1046)

Avoid returning (nil, nil), convert error if CRD not present to
NotFound and handle them in the caller code.

Avoid checking for found == nil but handle only error code.

This is more idiomatic and less error prone.

Signed-off-by: Yauheni Kaliuta <[email protected]>
(cherry picked from commit 9651b76)

Handle Kserve exception

Fix csv

(cherry picked from commit e5f65bd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants