Project

General

Profile

Actions

Bug #11220

closed

[SDKs] Fix misleading arv-mount/pysdk error messages by removing obsolete "fetch manifest from Keep" code

Added by Tom Clegg about 7 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
SDKs
Target version:
Story points:
-

Description

Current error message:

2017-03-01 17:27:55 arvados.arvados_fuse[10741] ERROR: Error fetching collection '{{PDH}}': Failed to retrieve collection '{{PDH}}' from either API server (<HttpError 404 when requesting https://tb05z.arvadosapi.com/arvados/v1/collections/{{PDH}}?alt=json returned "Path not found">) or Keep ({{PDH}} not found: http://keep0.tb05z.arvadosapi.com:25107/ responded with 403 HTTP/1.1 403 Forbidden

This should be reported as 404 / not found. The 403 part is a red herring.

Manifests are no longer written to Keep, and even if they were, reading without a permission token will never work on most installations, so this fallback seems pointless.

source:sdk/python/arvados/collection.py#L1362


Subtasks 1 (0 open1 closed)

Task #12438: Review 11220-manifest-fetch-errorResolvedTom Clegg10/20/2017Actions

Related issues

Related to Arvados - Bug #11190: Containers seem to run more than once, which isn't supposed to happenResolvedTom Clegg03/01/2017Actions
Actions #1

Updated by Tom Clegg about 7 years ago

  • Description updated (diff)
Actions #3

Updated by Tom Morris over 6 years ago

  • Target version set to Arvados Future Sprints
Actions #4

Updated by Tom Morris over 6 years ago

  • Target version changed from Arvados Future Sprints to 2017-10-25 Sprint
Actions #5

Updated by Tom Clegg over 6 years ago

  • Category set to SDKs
  • Assigned To set to Tom Clegg
Actions #6

Updated by Tom Clegg over 6 years ago

  • Status changed from New to In Progress
Actions #7

Updated by Tom Clegg over 6 years ago

11220-manifest-fetch-error @ 93e437b0dfd453f00df59c6a84bcc5d3ef09a9be

I removed one test that said "arv-get {block-id} -" relied on the fetch-manifest-from-Keep fallback. It looks like that's still true, and crunch-job calls arv-get that way to get manifest fragments from task outputs.

I implemented this a different way (so it also works for blocks that don't happen to be manifests) in commands/get.py, but it looks like only the manifest-from-api-server case is tested, not the data-from-keep case.

Actions #8

Updated by Tom Morris over 6 years ago

  • Target version changed from 2017-10-25 Sprint to 2017-11-08 Sprint
Actions #9

Updated by Lucas Di Pentima over 6 years ago

  • Ran tests both on Jenkins and locally, I got some services/fuse failures: https://ci.curoverse.com/job/developer-run-tests/489/
  • Collection class constructor docstring should be updated to not mention block locators as a source of manifests.
  • The above comment also for CollectionReader docstring.
  • Shouldn't Collection warn the developer with a deprecation notice when detected that manifest_locator is a block locator?
Actions #10

Updated by Tom Clegg over 6 years ago

Lucas Di Pentima wrote:

Removed one (which tested for the behavior being removed) and fixed the other.

  • Collection class constructor docstring should be updated to not mention block locators as a source of manifests.
  • The above comment also for CollectionReader docstring.

Updated.

  • Shouldn't Collection warn the developer with a deprecation notice when detected that manifest_locator is a block locator?

We could provide a more helpful error message/hint ("looks like you're trying to do ..., which is not supported any more") if the locator looks like a Keep locator (e.g., has a +A signature) -- is this what you have in mind?

11220-manifest-fetch-error @ 9dbeb1f31dafd927495ede39c7653b095495da26

Actions #11

Updated by Lucas Di Pentima over 6 years ago

Tom Clegg wrote:

We could provide a more helpful error message/hint ("looks like you're trying to do ..., which is not supported any more") if the locator looks like a Keep locator (e.g., has a +A signature) -- is this what you have in mind?

I think it would be helpful, although I'm not sure if the fact that manifests aren't saved on Keep is new enough to be worth the additional effort.

11220-manifest-fetch-error @ 9dbeb1f31dafd927495ede39c7653b095495da26

LGTM, thanks.

Actions #12

Updated by Anonymous over 6 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:e67d0f5d43c56f78694ea4a5f93acec5c93cd0fb.

Actions

Also available in: Atom PDF