feat: generate endpoints using replica numbers by abeatrix · Pull Request #45862 · sourcegraph/sourcegraph-public-snapshot

abeatrix · 2022-12-20T20:47:28Z

RE: sourcegraph/deploy-sourcegraph#4213 & https://github.com/sourcegraph/sourcegraph/issues/45861

Summary

This feature is also backward-compatible, so existing setups and instances will not be affected by this change.

This PR adds the ability to accept replicas number for Gitserver, Indexed-search, Searcher, and Symbols via existing env vars (SEARCHER_URL, SYMBOLS_URL, INDEXED_SEARCH_SERVERS, SRC_GIT_SERVERS), and then generate a new list of endpoints based on replicas number instead of listing out each replica endpoint manually.

Background

Issue: Our current default setup only works in RBAC-enabled clusters.

RBAC for frontend is used for service discovery as it is essential for frontend to arrange communications between services; however, this default setup often leads to delays and frustrating deployment experiences for those without the ability to create RBAC resources. As this is a common deployment pain point and issue that has negative impacts on both the customers and our business, our team believes this is something we should address.

Proposed Solution

A new base cluster introduces in sourcegraph/deploy-sourcegraph#4215 provide an option for users to set up a working Sourcegraph k8s instance with or without RBAC enabled for frontend.

When RBAC resources are removed from frontend, Sourcegraph will not work properly unless a list of services endpoints is included in the frontend env var list using existing env vars that are also being used in our docker-compose deployment , which is a task that relies on users inputting the correct values manually.

The functions introduce in this PR aim to minimize the time our users/customers have to spend configuring their deployment (e.g. spending time inputting the endpoint addresses for each service manually) by generating the endpoints in frontend based on the replica number provided via env vars.

IMPORTANT The changes made in this PR should NOT break any existing setup or instances. No additional changes are required in existing manifests.

Expected Behavior

The current behavior is the same for all instances (both existing instances and new instances), which means:

ALL instances that do not have the existing endpoints URLs env vars set are default to using Kubernetes service discovery
Setting env vars to empty string "" will return ""
See the Examples section listed below for the expected behaviors

Examples:

Return endpoints generated with the replica number:

        env:
        - name: SRC_GIT_SERVERS
          value: "4"
        - name: SEARCHER_URL
          value: "4"
        - name: SYMBOLS_URL
          value: "4"
        - name: INDEXED_SEARCH_SERVERS
          value: "4"

Test plan

copy and paste the context below to your sg.config.overwrite.yaml file in the root of this directory:
run sg start enterprise
You should see frontend and gitserver complaining about the new endpoints that do not exist, with the newly generated list of endpoints in the correct format for k8s deployments.

The expected new endpoints using the file shared below should look like:

4 gitserver endpoints: "gitserver-0.gitserver:3178", "gitserver-1.gitserver:3178", "gitserver-2.gitserver:3178", "gitserver-3.gitserver:3178", "gitserver-4.gitserver:3178"
3 indexed search endpoints: "indexed-search-0.indexed-search:6070", "indexed-search-1.indexed-search:6070", "indexed-search-2.indexed-search:6070"
3 symbols endpoints: "http://symbols-0.symbols:3184", "http://symbols-1.symbols:3184", "http://symbols-2.symbols:3184"
3 searcher endpoints: "http://searcher-0.searcher:3181", "http://searcher-1.searcher:3181", "http://searcher-2.searcher:3181"

sg.config.overwrite.yaml:

env:
  SRC_GIT_SERVERS: "5"
  SEARCHER_URL: "3"
  SYMBOLS_URL:"3"
  INDEXED_SEARCH_SERVERS: "3"
  DEPLOY_TYPE: kubernetes

Example output from running sg start:

[    gitserver-1] WARN server server/cleanup.go:193 current shard is not included in the list of known gitserver shards, will not delete repos {"current-hostname": "127.0.0.1:3502", "all-shards": ["gitserver-0.gitserver:3178", "gitserver-1.gitserver:3178", "gitserver-2.gitserver:3178", "gitserver-3.gitserver:3178", "gitserver-4.gitserver:3178"]}
[    gitserver-0] WARN server server/cleanup.go:193 current shard is not included in the list of known gitserver shards, will not delete repos {"current-hostname": "127.0.0.1:3501", "all-shards": ["gitserver-0.gitserver:3178", "gitserver-1.gitserver:3178", "gitserver-2.gitserver:3178", "gitserver-3.gitserver:3178", "gitserver-4.gitserver:3178"]}
[       frontend] ERROR InternalHandler httpapi/httpapi.go:281 API HTTP handler error response {"method": "POST", "request_uri": "/.internal/repos/index", "status_code": 500, "error": "hostname \"localhost:3070\" not found in Endpoints{\"indexed-search-0.indexed-search:6070\" \"indexed-search-1.indexed-search:6070\" \"indexed-search-2.indexed-search:6070\"}"}
[       frontend] ERROR internal.http trace/httptrace.go:250 unexpected status code 500 {"route_name": "internal.repos.index", "method": "POST", "url": "/.internal/repos/index", "code": 500, "duration": "2.073375ms", "shouldTrace": false, "error": "hostname \"localhost:3070\" not found in Endpoints{\"indexed-search-0.indexed-search:6070\" \"indexed-search-1.indexed-search:6070\" \"indexed-search-2.indexed-search:6070\"}"}

sourcegraph-bot · 2022-12-20T20:49:10Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 11ba68c...02dd250.

Notify	File(s)
@camdencheek	internal/search/backend/indexers_test.go
@keegancsmith	cmd/symbols/build-ctags.sh cmd/symbols/internal/api/handler_test.go internal/search/backend/indexers_test.go internal/symbols/client.go internal/symbols/client_test.go
@sourcegraph/delivery	doc/admin/deploy/docker-compose/configuration.md

Co-authored-by: Beyang Liu <beyang@sourcegraph.com>

davejrt

Overall looks good. It be worth looking into the possibility of using existing envvars that are present in kuberetes or compose to determine the deployment type, rather than having to explicitly define it. For example checking for the presence of one of these could safely determine that it's k8s or not.

/ $ env | grep KUBE
KUBERNETES_SERVICE_PORT=443
KUBERNETES_PORT=tcp://10.253.0.1:443
KUBERNETES_PORT_443_TCP_ADDR=10.253.0.1
KUBERNETES_PORT_443_TCP_PORT=443
KUBERNETES_PORT_443_TCP_PROTO=tcp
KUBERNETES_PORT_443_TCP=tcp://10.253.0.1:443
KUBERNETES_SERVICE_PORT_HTTPS=443
KUBERNETES_SERVICE_HOST=10.253.0.1

abeatrix · 2023-01-18T13:01:45Z

Thanks for clarifying @rafax ! I’ve talked to Michael last week regarding this change and he helped me understand why service discovery is important in some use case (e.g standard k8s practice and cli automation etc).

The idea to remove rbac resources from frontend by default is driven by the incidents where customers not having the ability to create them, led to long turnaround time.

We have built a kustomize component that would update the env vars in frontend automatically when deploying using kustomize; however, if service discovery is preferred, the site admins / cluster admins do have the option to add service discovery to frontend anytime (using kustomize).

This allows site admins to move forward with configuring their instance after initial deployment while cluster admins help with deploying RBAC resources collectively--do you think this helps addressing your team's use case here?

rafax · 2023-01-19T09:13:08Z

We have built a kustomize component that would update the env vars in frontend automatically when deploying using kustomize; however, if service discovery is preferred, the site admins / cluster admins do have the option to add service discovery to frontend anytime (using kustomize).

If we have an option to support service discovery (and therefore, can scale deployments without reconfiguring clients / redeploying kustomize) we're good on Cloud side.

keegancsmith · 2023-01-24T17:38:05Z

@abeatrix I'll take a look tomorrow, but I noticed that there are a lot of conflicts. You probably wanna merge in origin/main or rebase onto it.

abeatrix · 2023-01-24T19:22:00Z

adding @slimsag as a reviewer to make sure the new changes will also work with App as expected

emidoots

LGTM aside from my comments

keegancsmith

Nice code!

One blocking comment inline, so requesting changes.

keegancsmith · 2023-01-25T06:56:47Z

+	// Detect 'go test' and setup default addresses in that case.
+	p, err := os.Executable()
+	if err == nil && strings.HasSuffix(p, ".test") {
+		return "gitserver:3178", nil
+	}


I am assuming this messes with your tests? You can't actually validate the default, but this fine we can read code instead :)

keegancsmith · 2023-01-25T07:16:48Z

+			return conns.Symbols
+		})
+	})
+	return symbolsURLs.String()


Blocker: This change is not safe. It will use a static configuration that won't change over the lifetime of the process. Additionally I worry about potential deadlocks if the conf package is not ready given this is called inside of LoadConfig.

This required a bit more context to fix, so I went ahead and pushed a fix for this :)

keegancsmith · 2023-01-25T07:34:28Z

+			return conns.Symbols
+		})
+	})
+	return symbolsURLs.String()


This required a bit more context to fix, so I went ahead and pushed a fix for this :)

Co-authored-by: Keegan Carruthers-Smith <keegan.csmith@gmail.com>

Generate endpoints using replica numbers

cbfd2db

cla-bot Bot added the cla-signed label Dec 20, 2022

abeatrix requested review from a team and beyang December 20, 2022 20:47

abeatrix added deployment team/delivery Delivery team labels Dec 20, 2022

abeatrix added 3 commits December 20, 2022 14:48

Get symbols urls from envs in frontend

e9ffd3d

Clean up

62e5fa9

Move symbol endpoints compute steps back to symbols

5f4f2f8

beyang reviewed Dec 21, 2022

View reviewed changes

Comment thread CHANGELOG.md Outdated

beyang reviewed Dec 21, 2022

View reviewed changes

Comment thread cmd/frontend/internal/cli/config.go Outdated

beyang suggested changes Dec 21, 2022

View reviewed changes

Comment thread internal/conf/conftypes/conftypes.go Outdated

Comment thread cmd/symbols/build-ctags.sh

Comment thread internal/symbols/client.go Outdated

Comment thread internal/symbols/client.go Outdated

abeatrix and others added 10 commits December 21, 2022 11:20

Update internal/conf/conftypes/conftypes.go

352a4c4

Co-authored-by: Beyang Liu <beyang@sourcegraph.com>

Apply suggestions from code review

16251f0

Co-authored-by: Beyang Liu <beyang@sourcegraph.com>

Refactor to Docker-Compose endpoints only

cff28ac

Allow statefulset endpoint for indexer

c067334

Generate k8s endpoints

c27e0d6

TODO: add tests

6a88aad

Add INDEXED_SEARCH_BASENAME

2752c58

Merge branch 'main' into bee/env

003a7f0

Fix symbols url endpoint

0b8b9f1

Merge branch 'main' into bee/env

28b934c

abeatrix mentioned this pull request Jan 3, 2023

feat: non-privileged base cluster with new kustomize sourcegraph/deploy-sourcegraph#4215

Closed

6 tasks

abeatrix added 2 commits January 3, 2023 11:32

Add tests for replica endpoints

c7122fe

Update docs

0fb8bc6

abeatrix requested review from beyang and davejrt January 3, 2023 20:13

davejrt reviewed Jan 4, 2023

View reviewed changes

Comment thread cmd/frontend/internal/cli/config.go Outdated

davejrt reviewed Jan 4, 2023

View reviewed changes

Add error handling

76022a9

abeatrix requested a review from keegancsmith January 24, 2023 16:43

Merge branch 'main' into bee/env

58a243a

abeatrix requested a review from emidoots January 24, 2023 18:46

Update docker-compose docs

99af7c3