Project

General

Profile

Actions

Bug #16328

closed

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

Added by Ward Vandewege over 4 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
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 1 (0 open1 closed)

Task #16342: Review 16328-keep-proxy-uses-config.yaml-to-find-keepstoresResolvedWard Vandewege04/21/2020Actions

Related issues

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

Updated by Ward Vandewege over 4 years ago

  • Status changed from New to In Progress
Actions #2

Updated by Ward Vandewege over 4 years ago

  • Description updated (diff)
Actions #3

Updated by Ward Vandewege over 4 years ago

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

Updated by Ward Vandewege over 4 years ago

Ready for review @commit:de1f02c44bf8da29b1c5194efc29daf781b750bc

Actions #5

Updated by Tom Clegg over 4 years 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.

Actions #6

Updated by Ward Vandewege over 4 years 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

Actions #7

Updated by Ward Vandewege over 4 years ago

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

Actions #8

Updated by Ward Vandewege over 4 years ago

  • Release set to 25
  • Status changed from In Progress to Resolved
Actions #9

Updated by Ward Vandewege over 4 years ago

  • Release changed from 25 to 33
Actions #10

Updated by Ward Vandewege over 4 years ago

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

Also available in: Atom PDF