Bug #11220

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

Added by Tom Clegg 9 months ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
SDKs
Target version:
Start date:
10/20/2017
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
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

Task #12438: Review 11220-manifest-fetch-errorResolvedTom Clegg


Related issues

Related to Arvados - Bug #11190: Containers seem to run more than once, which isn't supposed to happenResolved2017-03-01

Associated revisions

Revision e67d0f5d
Added by Tom Clegg about 1 month ago

Merge branch '11220-manifest-fetch-error'

closes #11220

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision 7c02cd4b (diff)
Added by Tom Clegg about 1 month ago

11220: Fix test setup.

refs #11220

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision 3ff7fc1d (diff)
Added by Tom Clegg about 1 month ago

Add missing module name.

refs #11220
fixes #12590

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Tom Clegg 9 months ago

  • Description updated (diff)

#3 Updated by Tom Morris 4 months ago

  • Target version set to Arvados Future Sprints

#4 Updated by Tom Morris 2 months ago

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

#5 Updated by Tom Clegg 2 months ago

  • Category set to SDKs
  • Assigned To set to Tom Clegg

#6 Updated by Tom Clegg about 2 months ago

  • Status changed from New to In Progress

#7 Updated by Tom Clegg about 2 months 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.

#8 Updated by Tom Morris about 2 months ago

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

#9 Updated by Lucas Di Pentima about 1 month 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?

#10 Updated by Tom Clegg about 1 month 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

#11 Updated by Lucas Di Pentima about 1 month 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.

#12 Updated by Anonymous about 1 month ago

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

Applied in changeset arvados|commit:e67d0f5d43c56f78694ea4a5f93acec5c93cd0fb.

Also available in: Atom PDF