APPROVED: Proposal: Services (containers / image names) should be renamed to be consistent and legible to new admins

slimsag commented 4 years ago

@sourcegraph/core-services @sourcegraph/code-intel @sourcegraph/distribution to those interested, please review by April 20th, 2020.

@sourcegraph/code-intel @efritz you may wish to review earlier.

Today

Right now, we have these container / image names in all deployments:

frontend
gitserver
github-proxy
precise-code-intel-api-server
precise-code-intel-bundle-manager
precise-code-intel-worker
query-runner
replacer
repo-updater
searcher
symbols
syntect-server
zoekt-indexserver (hidden under "indexed-search/" in Kubernetes, but nowhere else)
zoekt-webserver (hidden under "indexed-search/" in Kubernetes, but nowhere else)
pgsql
redis-store
redis-cache
prometheus
grafana
jaeger

Problem 1: Consistency

I see many consistency problems here:

1) When is "server" or "manager" used? Consider:

zoekt-indexserver
-precise-code-intel-bundle-manager
+precise-code-intel-bundle-server

2) When is "server" NOT used? Everything we have is a "server", so why is it in some things but not others? Consider:

-frontend
+frontend-server

-symbols
+symbols-server

-gitserver
+git

3) When are dashes used, or not used? Consider:

-gitserver
+git-server
zoekt-indexserver
+zoekt-index-server
-repo-updater
+repoupdater

Problem 2: Legibility

Our services don't describe what they actually do to new site admins who may be trying to debug the system or determine what part of it they should look at:

query-runner: sounds very important, it must run search queries
searcher sounds extremely important, must be responsible for all of search
zoekt-* no clue, doesn't sound important.
syntect-server no clue, doesn't sound important.
precise-code-intel-api-server sounds like a silly API server, probably not the main thing doing work.
symbols: must be doing all of the symbol search and indexing of code.

Proposal

Use the following service naming guidelines, and rename all existing services (primarily the docker containers / image names) accordingly: https://gist.github.com/slimsag/da2744d200bc498b7a24b2d57bfd4744

This renaming could be done primarily in v3.16, but also on a case-by-case basis before then if desirable for other reasons.

slimsag commented 4 years ago

@sourcegraph/code-intel I know you have an ongoing discussion particularly around precise-code-intel naming; so I am keen to get your thoughts here on that aspect. I have not yet read through all of your RFC (but plan to) and in particular it is worth considering naming here of just the docker images / container names in the more global scope of our entire app's services.

slimsag commented 4 years ago

@keegancsmith I intend to version zoekt- alongside Sourcegraph ASAP before the 3.15 release. Currently, `zoekt-have semver-incompatible tags published (likesourcegraph/zoekt-webserver:2019.08.16-75b5f54which are "greater than literally every Sourcegraph version") so renaming the image is needed in any case. Can you review that single aspect here ASAP? (i.e. if you are opposed toindexed-searchinstead ofzoekt-web-server`)

unknwon commented 4 years ago

Cross-post here: https://gist.github.com/slimsag/da2744d200bc498b7a24b2d57bfd4744#gistcomment-3252387

To catch this train, it's a good time to rename repo-updater to codehost-syncer or code-host-syncer, since it is no longer just syncing repos anymore (now changesets and permissions). The rename was proposed originally in background permissions syncing RFC but wasn't a good time do that immediately.

slimsag commented 4 years ago

@efritz said:

Why do you retain git-server and precise-code-intel-server in your proposal? Also, precise-code-intel-indexer is by definition incorrect: it's not indexing anything (it receives output from precise-code-intel indexers such as lsif-go, lsif-tsc). precise-code-intel-converter would be more precise in this case.

slimsag commented 4 years ago

@unknwon Great point, I will incorporate that.

slimsag commented 4 years ago

Why do you retain git-server and precise-code-intel-server in your proposal?

To quote the guidelines:

A "server". suffix may ONLY be included if a more suitable noun cannot be found.

Example: "git-server" has no other suitable noun.

Example: "indexed-search-server" has a more suitable noun "searcher", so use "indexed-searcher" instead.

In other words, I was not able to come up with a more suitable noun for either of these. I'd be very open to suggestions on either while following the other guidelines. Any thoughts? And does this answer your question?

Also, precise-code-intel-indexer is by definition incorrect: it's not indexing anything (it receives output from precise-code-intel indexers such as lsif-go, lsif-tsc). precise-code-intel-converter would be more precise in this case.

Got it, in that case precise-code-intel-converter sounds more correct. WIll update.

slimsag commented 4 years ago

Updated to reflect both points, PTAL @unknwon @efritz and "LGTM" if you approve otherwise

creachadair commented 4 years ago

On a whole this proposal seems good to me. I have only a few comments, none of which is an objection.

Minor nit: "verb" should be replaced with "noun", to describe the affix (there's just one case of it so far, but it's already spreading in the comment thread above).
The principle that a service with a well-known name and a corresponding UI can use its project name should be made explicit. Also that these cases may omit the -affix convention.
Please consider copying the principles to the issue rather than linking to the Gist. It's tedious to have to switch back and forth.

unknwon commented 4 years ago

@slimsag Any reason we use -search not -searcher? e.g. indexed-search vs indexed-searcher. Other than that, LGTM.

efritz commented 4 years ago

LGTM!

slimsag commented 4 years ago

Any reason we use -search not -searcher? e.g. indexed-search vs indexed-searcher.

Good observation, changed.

slimsag commented 4 years ago

Minor nit: "verb" should be replaced with "noun", to describe the affix (there's just one case of it so far, but it's already spreading in the comment thread above).

Thanks, fixed, my bad.

The principle that a service with a well-known name and a corresponding UI can use its project name should be made explicit. Also that these cases may omit the -affix convention.

Great point, done.

Please consider copying the principles to the issue rather than linking to the Gist. It's tedious to have to switch back and forth.

Originally, I was going to do this, but the display on GitHub issues is so narrow the table was extremely hard to read, so I used a Gist which at least has slightly wider display width. I will send a PR to move this to our handbook soon.

keegancsmith commented 4 years ago

General comment: How will we do this without introducing unnecessary burden on admins? eg merge conflicts, "--prune", custom alerts, our alerts using service name, auditing other places. This burden is the reason these sort of name changes haven't happened before.

@keegancsmith I intend to version zoekt-* alongside Sourcegraph ASAP before the 3.15 release.

What is your plan for this?

Can you review that single aspect here ASAP? (i.e. if you are opposed to indexed-search instead of zoekt-web-server)

LGTM.

unindexed-searcher

In other places we have referred to this as just in time search, but I think this is the clearer name.

unindexed-symbol-searcher

Related to the above, this does actually create an index but does it just in time. Naming is hard. I think this name is good though. FYI we have considered folding this service (and other unindexed searches) all into unindexed-searcher.

slimsag commented 4 years ago

General comment: How will we do this without introducing unnecessary burden on admins? eg merge conflicts, "--prune", custom alerts, our alerts using service name, auditing other places. This burden is the reason these sort of name changes haven't happened before.

Yes, this is a great question and one @beyang was also worried about / curious about. The simple answer is: it is going to be painful, but I think the pain our existing users will experience is outweighed by the pain new users will experience.

The general idea right now is to make this "one painful update" (e.g. instead of "several painful updates" over the next six months). Also worth noting:

merge conflicts, "--prune"

There are broader discussions ongoing about merge conflicts in k8s deployments and --prune (very undecided currently) https://docs.google.com/document/d/1tsksAlqe77NmdPLw7oyy2u0-1rYDQeVpPeDo1T0Xt50/edit

custom alerts

This is one of the many reasons admins should be using our alerting rules / dashboards, which are rapidly improving.

our alerts using service name, auditing other places.

Doing this at the start of the iteration will help weed out such issues.

What is your plan for this?

Let's discuss that in #9251

FYI we have considered folding this service (and other unindexed searches) all into unindexed-searcher.

That would be great! The less services needed the more win, in my view. There is also discussion that we can remove replacer and query-runner, and sounds like we can remove github-proxy too https://github.com/sourcegraph/sourcegraph/issues/9663

nicksnyder commented 4 years ago

@slimsag Are you (or someone on distribution) going to own doing the actual renames of all the services?

slimsag commented 4 years ago

Yes

nicksnyder commented 4 years ago

I removed the other team labels because it doesn't seem like this work needs to show up in their tracking issues (since distribution owns the actual work).

slimsag commented 4 years ago

Update: to make this as painless as possible, I have decided to block the execution of this until the following have happened first:

[ ] github-proxy has been removed: https://github.com/sourcegraph/sourcegraph/issues/9663
[ ] replacer has been removed (me and Rijnard agreed this can be done as it is not in use right now, just requires someone to do it)
[ ] query-runner is moved into the frontend (there is an issue somewhere which describes why having this as a separate service was a major mistake on my part, but the TL;DR is it is HEAVILY dependent on the frontend's APIs. -- just requires someone to do it)
[ ] Most importantly, https://github.com/sourcegraph/sourcegraph/issues/10100 has been completed, since it will substantially reduce the amount of merge conflicts Kubernetes users would have when performing this upgrade
[ ] The dust has settled on https://github.com/sourcegraph/sourcegraph/issues/10144

github-actions[bot] commented 2 years ago

Heads up @davejrt @ggilmore @dan-mckean @caugustus-sourcegraph @stephanx - the "team/delivery" label was applied to this issue.

sourcegraph / sourcegraph-public-snapshot