Bug #16328

[keep-proxy] should use config.yaml to identify upstream keepstores

Added by Ward Vandewege 3 months ago. Updated 2 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
04/21/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

keep-proxy uses the old api server discovery mechanism to find its upstream keepstores. This means that the geo-ip configuration of the api server must be set up properly, or keep-proxy will start talking to itself.

Now that we have config.yaml, keep-proxy should just read the list of keepstore URIs from there instead.


Subtasks

Task #16342: Review 16328-keep-proxy-uses-config.yaml-to-find-keepstoresResolvedWard Vandewege


Related issues

Related to Arvados - Bug #16392: [keep] trailing slash in InternalURLs entry for keepstore causes problems with keepproxyResolved05/13/2020

Associated revisions

Revision 6b6008fc (diff)
Added by Ward Vandewege 3 months ago

If config.yml is available, use the keepstores defined there instead of
the legacy autodiscover mechanism via the API server.

refs #16328

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision f6753e7a (diff)
Added by Ward Vandewege 3 months ago

If config.yml is available, use the keepstores defined there instead of
the legacy autodiscover mechanism via the API server.

refs #16328

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision 230e87b9 (diff)
Added by Ward Vandewege 3 months ago

If config.yml is available, use the keepstores defined there instead of
the legacy autodiscover mechanism via the API server.

refs #16328

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision 66c1d31e (diff)
Added by Ward Vandewege 3 months ago

If config.yml is available, use the keepstores defined there instead of
the legacy autodiscover mechanism via the API server.

refs #16328

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision 0887e534 (diff)
Added by Ward Vandewege 3 months ago

If config.yml is available, use the keepstores defined there instead of
the legacy autodiscover mechanism via the API server.

refs #16328

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision de1f02c4 (diff)
Added by Ward Vandewege 2 months ago

If config.yml is available, use the keepstores defined there instead of
the legacy autodiscover mechanism via the API server.

refs #16328

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision f2e3fb0f
Added by Ward Vandewege 2 months ago

Merge branch '16328-keep-proxy-uses-config.yaml-to-find-keepstores'

closes #16328

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

History

#1 Updated by Ward Vandewege 3 months ago

  • Status changed from New to In Progress

#2 Updated by Ward Vandewege 3 months ago

  • Description updated (diff)

#3 Updated by Ward Vandewege 2 months ago

  • Target version changed from To Be Groomed to 2020-04-22

#4 Updated by Ward Vandewege 2 months ago

Ready for review @commit:de1f02c44bf8da29b1c5194efc29daf781b750bc

#5 Updated by Tom Clegg 2 months ago

Don't do the "options ...bool" thing in the test suite. Just add a loadKeepstoresFromConfig bool arg, and add ,true or ,false to the existing runProxy() uses.

It looks like the additional ServerRequiredConfigYmlSuite ended up being unnecessary, and the new TestGetIndex could be (*ServerRequiredSuite)TestGetIndexUsingConfigYAML()... or is there a subtle reason why the separate suite is needed? (Need to restart servers in between? Need more than 2 keepstores?)

You can avoid the extra "associated revisions" in redmine (which don't go away when you amend/rebase) by tagging commits "16328: blah..." and leaving out the "refs #16328" bit until the merge-to-master commit.

Nit: Even though identifiers in _test.go don't get exported anyway, I'd use private lowercase getIndexWorker instead of GetIndexWorker.

The actual code change LGTM.

#6 Updated by Ward Vandewege 2 months ago

Tom Clegg wrote:

Don't do the "options ...bool" thing in the test suite. Just add a loadKeepstoresFromConfig bool arg, and add ,true or ,false to the existing runProxy() uses.

OK. I've simplified this further and removed the additional argument to runProxy. I've replaced it with a variable that is set in SetUpSuite for ServerRequiredConfigYmlSuite.

It looks like the additional ServerRequiredConfigYmlSuite ended up being unnecessary, and the new TestGetIndex could be (*ServerRequiredSuite)TestGetIndexUsingConfigYAML()... or is there a subtle reason why the separate suite is needed? (Need to restart servers in between? Need more than 2 keepstores?)

Yeah, the config.yml defines 4 keepstores, so I need them all to be up (or the index test will fail). I've put a comment to that effect in the SetUpSuite for ServerRequiredConfigYmlSuite.

You can avoid the extra "associated revisions" in redmine (which don't go away when you amend/rebase) by tagging commits "16328: blah..." and leaving out the "refs #16328" bit until the merge-to-master commit.

Ah, yes. Will do going forward.

Nit: Even though identifiers in _test.go don't get exported anyway, I'd use private lowercase getIndexWorker instead of GetIndexWorker.

Sure, changed.

The actual code change LGTM.

Thanks, updated changes at a77a856600ef5d22208eff74d909ff6d1dc4664d

#7 Updated by Ward Vandewege 2 months ago

OK, I reverted the simplification after talking to Tom and added the paramter to runProxy, and merged. Thanks!

#8 Updated by Ward Vandewege 2 months ago

  • Release set to 25
  • Status changed from In Progress to Resolved

#9 Updated by Ward Vandewege 2 months ago

  • Release changed from 25 to 33

#10 Updated by Ward Vandewege 2 months ago

  • Related to Bug #16392: [keep] trailing slash in InternalURLs entry for keepstore causes problems with keepproxy added

Also available in: Atom PDF