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

🐛 check the underlying storage for existing cluster catalog content #290

Merged

Conversation

itroyano
Copy link
Contributor

Fixes #288

@itroyano itroyano requested a review from a team as a code owner June 25, 2024 15:40
@itroyano itroyano marked this pull request as draft June 25, 2024 15:40
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2024
@itroyano itroyano force-pushed the storage-content-check branch 3 times, most recently from 896eed7 to 0f6d857 Compare June 25, 2024 17:59
@@ -60,6 +61,13 @@ func (s LocalDir) StorageServerHandler() http.Handler {
return mux
}

func (s LocalDir) ContentExists(catalog string) bool {
if _, err := os.Stat(filepath.Join(s.RootDir, catalog)); errors.Is(err, os.ErrNotExist) {
Copy link
Member

Choose a reason for hiding this comment

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

If there is some error other than os.ErrNotExist returned from Stat, I think we need to return it to the caller, which would likely want to attempt to delete whatever is there and unpack again.

Also, I think we should be explicitly checking for an all.json entry and that it is a file.

Copy link
Contributor Author

@itroyano itroyano Jun 27, 2024

Choose a reason for hiding this comment

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

Re: return to the caller I'm not sure if we should because it might lead to some ugly code like this inside the needsUnpacking(catalog) func -

exists, err := r.Storage.ContentExists(catalog.Name)
if err != nil {
	// content is corrupted. delete and unpack again
	r.Storage.Delete(catalog.Name)
	return true
}

IMO the Unpacker itself should be clearing out the path in case it's not empty, and it does - with Cleanup().
The needsUnpacking(catalog) func should only indicate if the action should take place, not do any actions on its own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add the all.json file check shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL

Copy link

codecov bot commented Jun 26, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 33.53%. Comparing base (7615cf4) to head (70d91c0).
Report is 2 commits behind head on main.

Files Patch % Lines
pkg/controllers/core/clustercatalog_controller.go 71.42% 1 Missing and 1 partial ⚠️
pkg/storage/localdir.go 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #290      +/-   ##
==========================================
+ Coverage   33.12%   33.53%   +0.40%     
==========================================
  Files          15       15              
  Lines         646      656      +10     
==========================================
+ Hits          214      220       +6     
- Misses        411      413       +2     
- Partials       21       23       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joelanford
Copy link
Member

@everettraven, an aside: I see a failure in go-apidiff. I wouldn't expect this code to be in our public API. In preparation for a v1.0, should we take a moment to make sure that the only code in our public API are the catalogd types?

@everettraven
Copy link
Collaborator

@everettraven, an aside: I see a failure in go-apidiff. I wouldn't expect this code to be in our public API. In preparation for a v1.0, should we take a moment to make sure that the only code in our public API are the catalogd types?

@joelanford i think that is reasonable. I imagine that should fall under the v1.0 epic to evaluate catalogd's public API surface?

@itroyano itroyano marked this pull request as ready for review June 27, 2024 15:16
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 27, 2024
@@ -60,6 +60,18 @@ func (s LocalDir) StorageServerHandler() http.Handler {
return mux
}

func (s LocalDir) ContentExists(catalog string) bool {
file, err := os.Stat(s.ContentURL(catalog))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does s.ContentURL actually work as expected here? IIUC that would return something like http://<url>/<catalog.Name>/all.json which I would expect to be an invalid filepath due to it having the http:// or https:// scheme present.

I think we may need to instead do something along the lines of filepath.Join(s.RootDir, catalog, "all.json")

Related, could we add some unit tests for this function to ensure it is functioning as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I got confused because saw a place where the extension is being stripped after calling.

Signed-off-by: Igor Troyanovsky <itroyano@redhat.com>
@everettraven everettraven added this pull request to the merge queue Jul 1, 2024
Merged via the queue into operator-framework:main with commit 91e899f Jul 1, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: re-spun catalogd pods return 404
3 participants