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

thamos images errors #1184

Closed
goern opened this issue Oct 10, 2022 · 12 comments · Fixed by thoth-station/user-api#1840
Closed

thamos images errors #1184

goern opened this issue Oct 10, 2022 · 12 comments · Fixed by thoth-station/user-api#1840
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/user-experience Issues or PRs related to the User Experience of our Services, Tools, and Libraries. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@goern
Copy link
Member

goern commented Oct 10, 2022

thamos images throws an error:

/kind bug

@goern goern added the kind/bug Categorizes issue or PR as related to a bug. label Oct 10, 2022
@codificat
Copy link
Member

thamos advise

is it thamos advise or thamos images (mentioned in the title)?

An example command invocation (and config?) that reproduces the problem would help

throws an error:

which error?

Can we have some more context?

@codificat
Copy link
Member

/triage needs-information

@sesheta sesheta added the triage/needs-information Indicates an issue needs more information in order to work on it. label Oct 10, 2022
@goern
Copy link
Member Author

goern commented Oct 10, 2022

it is images:

2022-10-10 13:57:00,994 [1289268] ERROR    thamos: (404)
Reason: NOT FOUND
HTTP response headers: HTTPHeaderDict({'server': 'gunicorn', 'date': 'Mon, 10 Oct 2022 11:57:00 GMT', 'content-type': 'application/json', 'content-length': '314', 'x-thoth-version': '0.35.8', 'x-user-api-service-version': '0.35.8+messaging.0.16.2.storages.0.73.3.common.0.36.4.python.0.16.10', 'x-thoth-search-ui-url': 'https://thoth-station.ninja/search/', 'access-control-allow-origin': '*', 'set-cookie': '99770cb82864be05282857f803e02327=66c8c60f3ea5d397db93d125852ede8c; path=/; HttpOnly; Secure; SameSite=None', 'cache-control': 'private'})
HTTP response body: b'{\n  "error": "No Image found",\n  "parameters": {\n    "cuda_version": null,\n    "image_name": null,\n    "library_name": null,\n    "os_name": null,\n    "os_version": null,\n    "package_name": null,\n    "page": 3,\n    "per_page": 25,\n    "python_version": null,\n    "rpm_package_name": null,\n    "symbol": null\n  }\n}\n'

@codificat
Copy link
Member

The bug is thamos not handling pagination and 404/NotFound gracefully enough.

Note the request is for page 3 when there is no page 3:

$ http --headers 'https://khemenu.thoth-station.ninja/api/v1/container-images?page=0&per_page=25' | grep count
page_count: 2
entries_count: 41

/transfer thamos

The user API started returning a 404 in this situation starting in v0.35.6 after this change: thoth-station/user-api@ab32cad#diff-0c1d9c73b678717b9143917cfc52d1fccbfcdf488adb2375865c8adca9a27a6eR376-R379

/remove-triage needs-information
/triage accepted
/priority critical-urgent
/sig user-experience

@sesheta sesheta added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Oct 11, 2022
@sesheta sesheta transferred this issue from thoth-station/integration-tests Oct 11, 2022
@codificat
Copy link
Member

/sig user-experience

@codificat codificat moved this to 🆕 New in Planning Board Oct 11, 2022
@codificat codificat added the sig/user-experience Issues or PRs related to the User Experience of our Services, Tools, and Libraries. label Oct 11, 2022
@codificat
Copy link
Member

The user API started returning a 404 in this situation starting in v0.35.6

By the way, I am not sure if a 404 is appropriate here: maybe the API should just return a 200/OK with a total of 0 results, no?

Still, it sounds like thamos should handle pagination better by looking at the page count in the response headers

@codificat
Copy link
Member

Related: #1109

@codificat codificat moved this from 🆕 New to 🔖 Next in Planning Board Oct 17, 2022
@codificat codificat moved this from 🔖 Next to 📋 Backlog in Planning Board Oct 17, 2022
@codificat codificat moved this from 📋 Backlog to 🆕 New in Planning Board Oct 17, 2022
@goern
Copy link
Member Author

goern commented Oct 18, 2022

delivering a 404 sounds reasonable when no images are available, or when requesting page 5 with per_page 10 and a total of 8 images available. 404 is the server signaling, that the requrested data could not be found, giving 200 with an empty list if a a incorrect behavior, isnt it?

@goern
Copy link
Member Author

goern commented Oct 18, 2022

I tried to create an integration test covering this, see thoth-station/integration-tests#351

@codificat
Copy link
Member

delivering a 404 sounds reasonable when no images are available

Maybe. But:

  • a 404 would also be returned if you try to access a non-existing API endpoint. Same code for different meanings.
  • returning a 200 with an empty result sounds also reasonable to me: a way of saying "I found 0 occurrences of this", which is not necessarily an error.

or when requesting page 5 with per_page 10 and a total of 8 images available. 404 is the server signaling, that the requrested data could not be found,

That might better be answered by a 400 (bad request) instead? Or even a 200 with empty results (meaning "this is what I found: nothing").

Note that the API response includes a total item count and a page count in the response headers; these complement an empty 200 response nicely I think.

giving 200 with an empty list if a a incorrect behavior, isnt it?

IMO I think it is a better behavior when appropriate... but I can buy the 404 too, no too-hard feelings.

@Gkrumbach07 Gkrumbach07 moved this from 🆕 New to 📋 Backlog in Planning Board Nov 1, 2022
@Gkrumbach07 Gkrumbach07 moved this from 📋 Backlog to 🔖 Next in Planning Board Nov 7, 2022
@Gkrumbach07
Copy link
Member

Acceptance Criteria

  • Fix pagination for thamos images
  • correct error codes so they properly reflect if there is an issue

@Gkrumbach07 Gkrumbach07 self-assigned this Nov 7, 2022
@Gkrumbach07
Copy link
Member

This is not a thamos issue but a user-api issue. The correct response code is 200 for an empty list. 404 would indicate that the collection of images does not exist.

@Gkrumbach07 Gkrumbach07 moved this from 🔖 Next to 🏗 In progress in Planning Board Jan 25, 2023
@Gkrumbach07 Gkrumbach07 moved this from 🏗 In progress to 👀 In review in Planning Board Jan 25, 2023
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Planning Board Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/user-experience Issues or PRs related to the User Experience of our Services, Tools, and Libraries. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants