-
Notifications
You must be signed in to change notification settings - Fork 31
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
Improve CatalogSource controller error handling #34
Improve CatalogSource controller error handling #34
Conversation
Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com>
861d1e8
to
f2fc05e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on @rashmigottipati ! I know it has likely been hectic with rebases and scope change due to other PRs so I appreciate you taking the time to get this PR up!
Here is my first round of review:
// If BundleMetadata CR already exists, continue | ||
if errors.IsNotFound(err) { | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am reading this right, I think this should be:
// If BundleMetadata CR already exists, continue | |
if errors.IsNotFound(err) { | |
return nil | |
} | |
// If BundleMetadata CR already exists, continue | |
if !errors.IsAlreadyExists(err) { | |
return err | |
} |
// If BundleMetadata CR already exists, continue | ||
if errors.IsNotFound(err) { | ||
return nil | ||
} | ||
return fmt.Errorf("creating bundlemetadata %q: %w", bundleMeta.Name, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something that was discussed in https://github.com/operator-framework/catalogd/pull/30/files#r1164309453 is instead of returning an error when first encountered, we should aggregate the errors and return all the errors that occurred. This would mean the first occurrence of an error doesn't completely stop the unpacking process.
That being said, I'm not sure how our plans to switch to a different mechanism would impact this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't think I'd worry too much about this kind of thing right now unless it impacts
operator-framework/operator-controller#161
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would impact operator-framework/operator-controller#161 any more than the current process does. If information fails to get unpacked we set the ready condition to false regardless to signal that you shouldn't attempt to get information from that catalog.
// If Create fails due to Package CR already being present, continue | ||
if errors.IsNotFound(err) { | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto to wrong error check, I think it should be:
// If Create fails due to Package CR already being present, continue | |
if errors.IsNotFound(err) { | |
return nil | |
} | |
// If Create fails due to Package CR already being present, continue | |
if !errors.IsAlreadyExists(err) { | |
return err | |
} |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Just had an offline chat with @everettraven and the changes in this PR are being covered as a part of the larger PR that adds extensible unpacking interface. So closing this PR in favor of the other PR. |
IF the unpack Job can not be created, update the CatalogSource resource status indicating the unpack failure
IF the unpack Job’s Pod’s logs can not be read, update the CatalogSource resource status indicating the unpack failure
IF the error is because the Pod no longer exists, requeue to attempt unpacking process again as the Job could have been cleaned up by another process
IF a Package CR can not be created; and IF is the error is caused by the Package already existing - continue
IF a BundleMetadata CR can not be created; and IF the error is caused by the BundleMetadata already existing - continue