Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

feat(appliance): deploy searcher#63191

Merged
craigfurman merged 2 commits intomainfrom
craig/rel-82-service-definition-searcher
Jun 11, 2024
Merged

feat(appliance): deploy searcher#63191
craigfurman merged 2 commits intomainfrom
craig/rel-82-service-definition-searcher

Conversation

@craigfurman
Copy link
Contributor

@craigfurman craigfurman commented Jun 10, 2024

Fix reference to incorrect service in appliance config

feat(appliance): deploy searcher

Draft until base branch merged but can be reviewed independently.

Closes https://linear.app/sourcegraph/issue/REL-82/service-definition-searcher

Test plan

Golden tests included.

Changelog

  • Appliance (as yet unreleased) can deploy Searcher.

@cla-bot cla-bot bot added the cla-signed label Jun 10, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! This one is my bad from a previous PR. Shows how error-prone it can be doing all the grunt work of service configs... the golden tests didn't catch this (and are unchanged here), for a complicated and fragile reason: NewService() takes the optional standard config element and checks for a prometheus port, optionally adding the relevant scrape directive annotations.

Symbols and RepoUpdater happen to have the same prometheus port. If an admin had overridden this in symbols' config, they would have exposed a bug, since the repo-updater config is being checked here. But, overriding this in config wouldn't make any sense, since the service unconditionally listens on some port to expose prometheus metrics - that aspect can't be overridden. Even though there is probably little chance of this bug leaking out, it is something to keep in mind...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotated output of:

❯ go run ./internal/appliance/dev/compare-helm \
  -deploy-sourcegraph-helm-path ../../deploy-sourcegraph-helm \
  -component searcher \
  -golden-file internal/appliance/reconciler/testdata/golden-fixtures/searcher/default.yaml \
  -helm-template-extra-args '--set searcher.serviceAccount.create=true'

One thing I've been forgetting to note lately in these compare-helm diffs: the
commit that is checked out in my deploy-sourcegraph-helm directory! I'm using
v5.3.9104, the same as the requested version-under-test.

In
https://linear.app/sourcegraph/issue/REL-122/run-compare-helm-against-all-services-and-squash-any-problematic
we can bump that version to latest and double-check our configs against helm.

I can't see much interesting here, but as usual please take a look.

2c2
< # helm: ServiceAccount/searcher
---
> # golden: ServiceAccount/searcher
5a6,8
>   annotations:
>     appliance.sourcegraph.com/configHash: 2fec30f60a396508a28da4a0d7c03ecbe530b8370ddb0ae38b355e52bd844446
>   creationTimestamp: "2024-04-19T00:00:00Z"
7,8d9
<     app.kubernetes.io/component: searcher
<     category: rbac
10a12,21
>   namespace: NORMALIZED_FOR_TESTING
>   ownerReferences:
>     - apiVersion: v1
>       blockOwnerDeletion: true
>       controller: true
>       kind: ConfigMap
>       name: sg
>       uid: NORMALIZED_FOR_TESTING
>   resourceVersion: NORMALIZED_FOR_TESTING
>   uid: NORMALIZED_FOR_TESTING
12c23
< # helm: Service/searcher
---
> # golden: Service/searcher
16a28
>     appliance.sourcegraph.com/configHash: 2fec30f60a396508a28da4a0d7c03ecbe530b8370ddb0ae38b355e52bd844446
18a31
>   creationTimestamp: "2024-04-19T00:00:00Z"
23a37,46
>   namespace: NORMALIZED_FOR_TESTING
>   ownerReferences:
>     - apiVersion: v1
>       blockOwnerDeletion: true
>       controller: true
>       kind: ConfigMap
>       name: sg
>       uid: NORMALIZED_FOR_TESTING
>   resourceVersion: NORMALIZED_FOR_TESTING
>   uid: NORMALIZED_FOR_TESTING
25c48,54
<   clusterIP: None
---
>   clusterIP: NORMALIZED_FOR_TESTING
>   clusterIPs:
>     - NORMALIZED_FOR_TESTING
>   internalTrafficPolicy: Cluster
>   ipFamilies:
>     - IPv4
>   ipFamilyPolicy: SingleStack
28a58
>       protocol: TCP
31a62
>       protocol: TCP
35,36c66,69
<     app.kubernetes.io/instance: release-name
<     app.kubernetes.io/name: sourcegraph
---
>   sessionAffinity: None
>   type: ClusterIP
> status:
>   loadBalancer: {}
38c71
< # helm: StatefulSet/searcher
---
> # golden: StatefulSet/searcher
43c76,78
<     description: Backend for text search operations.
---
>     appliance.sourcegraph.com/configHash: 2fec30f60a396508a28da4a0d7c03ecbe530b8370ddb0ae38b355e52bd844446
>   creationTimestamp: "2024-04-19T00:00:00Z"
>   generation: 1
46,47d80
<     app.kubernetes.io/instance: release-name
<     app.kubernetes.io/managed-by: Helm
51d83
<     helm.sh/chart: sourcegraph-5.3.9104
52a85,94
>   namespace: NORMALIZED_FOR_TESTING
>   ownerReferences:
>     - apiVersion: v1
>       blockOwnerDeletion: true
>       controller: true
>       kind: ConfigMap
>       name: sg
>       uid: NORMALIZED_FOR_TESTING
>   resourceVersion: NORMALIZED_FOR_TESTING
>   uid: NORMALIZED_FOR_TESTING
53a96,100
>   minReadySeconds: 10
>   persistentVolumeClaimRetentionPolicy:
>     whenDeleted: Retain
>     whenScaled: Retain
>   podManagementPolicy: OrderedReady
59,60d105
<       app.kubernetes.io/instance: release-name
<       app.kubernetes.io/name: sourcegraph
65d109
<         checksum/redis: 63b58e05a2640417d599c4aee6d866cb9063e3a9aa452dc08dbfff836b7781b7
66a111
>       creationTimestamp: null
69,70d113
<         app.kubernetes.io/instance: release-name
<         app.kubernetes.io/name: sourcegraph
71a115
>       name: searcher
73d116
<       affinity: null
87c130
<               value: "23400"
---
>               value: "23961"

1024 MB to a GiB vs 1000 to a GB difference, probably. This code backing this
parses the storage size and sets the value of this var to 90% of it.

90a134
>                   apiVersion: v1
96a141
>                   apiVersion: v1
100c145
<           image: index.docker.io/sourcegraph/searcher:5.3.9104@sha256:abf63df1d39a5dce8d506551b798050b7ad008faefd5f319128bd0355736a79a
---
>           image: index.docker.io/sourcegraph/searcher:5.3.9104
105a151
>               protocol: TCP
107a154
>               protocol: TCP
114a162
>             successThreshold: 1
127a176
>           terminationMessagePath: /dev/termination-log
134c183,185
<       nodeSelector: null
---
>       dnsPolicy: ClusterFirst
>       restartPolicy: Always
>       schedulerName: default-scheduler
137a189
>         runAsGroup: 101
138a191
>       serviceAccount: searcher
140c193
<       tolerations: null
---
>       terminationGracePeriodSeconds: 30
149c202,207
<     - metadata:
---
>     - apiVersion: v1
>       kind: PersistentVolumeClaim
>       metadata:
>         creationTimestamp: null
>         labels:
>           deploy: sourcegraph
150a209
>         namespace: NORMALIZED_FOR_TESTING
157c216,291
<         storageClassName: sourcegraph
---
>         volumeMode: Filesystem
>       status:
>         phase: Pending
> status:
>   availableReplicas: 0
>   replicas: 0
> ---
> # golden: ConfigMap/sg
> apiVersion: v1
> data:
>   spec: |
>     spec:
>       requestedVersion: "5.3.9104"
> 
>       blobstore:
>         disabled: true
> 
>       codeInsights:
>         disabled: true
> 
>       codeIntel:
>         disabled: true
> 
>       frontend:
>         disabled: true
> 
>       gitServer:
>         disabled: true
> 
>       indexedSearch:
>         disabled: true
> 
>       indexedSearchIndexer:
>         disabled: true
> 
>       pgsql:
>         disabled: true
> 
>       postgresExporter:
>         disabled: true
> 
>       preciseCodeIntel:
>         disabled: true
> 
>       redisCache:
>         disabled: true
> 
>       redisStore:
>         disabled: true
> 
>       repoUpdater:
>         disabled: true
> 
>       searcher: {}
> 
>       symbols:
>         disabled: true
> 
>       syntectServer:
>         disabled: true
> 
>       worker:
>         disabled: true
> 
>       prometheus:
>         disabled: true
> kind: ConfigMap
> metadata:
>   annotations:
>     appliance.sourcegraph.com/currentVersion: 5.3.9104
>     appliance.sourcegraph.com/managed: "true"
>   creationTimestamp: "2024-04-19T00:00:00Z"
>   name: sg
>   namespace: NORMALIZED_FOR_TESTING
>   resourceVersion: NORMALIZED_FOR_TESTING
>   uid: NORMALIZED_FOR_TESTING

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I've been forgetting to note lately in these compare-helm diffs: the
commit that is checked out in my deploy-sourcegraph-helm directory! I'm using
v5.3.9104, the same as the requested version-under-test.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❯ diff internal/appliance/reconciler/testdata/golden-fixtures/searcher/default.yaml internal/appliance/reconciler/testdata/golden-fixtures/searcher/with-storage.yaml
6c6
<         appliance.sourcegraph.com/configHash: 2fec30f60a396508a28da4a0d7c03ecbe530b8370ddb0ae38b355e52bd844446
---
>         appliance.sourcegraph.com/configHash: 25b03d6fa226ee5e00548da8fffd9b6a70d3f14b8b1ea254b174105c425c2cdd
60c60
<                   value: "23961"
---
>                   value: "921600"
145c145
<                 storage: 26Gi
---
>                 storage: 1000Gi
197c197,199
<           searcher: {}
---
>           searcher:
>             persistentVolumeConfig:
>               storageSize: "1000Gi"
224c226
<         appliance.sourcegraph.com/configHash: 2fec30f60a396508a28da4a0d7c03ecbe530b8370ddb0ae38b355e52bd844446
---
>         appliance.sourcegraph.com/configHash: 25b03d6fa226ee5e00548da8fffd9b6a70d3f14b8b1ea254b174105c425c2cdd
243c245
<         appliance.sourcegraph.com/configHash: 2fec30f60a396508a28da4a0d7c03ecbe530b8370ddb0ae38b355e52bd844446
---
>         appliance.sourcegraph.com/configHash: 25b03d6fa226ee5e00548da8fffd9b6a70d3f14b8b1ea254b174105c425c2cdd

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❯ diff internal/appliance/reconciler/testdata/golden-fixtures/searcher/default.yaml internal/appliance/reconciler/testdata/golden-fixtures/searcher/with-replicas.yaml
6c6
<         appliance.sourcegraph.com/configHash: 2fec30f60a396508a28da4a0d7c03ecbe530b8370ddb0ae38b355e52bd844446
---
>         appliance.sourcegraph.com/configHash: c878c6daa974c7ef411448ebabf07a65e9eb202152898e1f786066a15de2209a
31c31
<       replicas: 1
---
>       replicas: 8
197c197,198
<           searcher: {}
---
>           searcher:
>             replicas: 8
224c225
<         appliance.sourcegraph.com/configHash: 2fec30f60a396508a28da4a0d7c03ecbe530b8370ddb0ae38b355e52bd844446
---
>         appliance.sourcegraph.com/configHash: c878c6daa974c7ef411448ebabf07a65e9eb202152898e1f786066a15de2209a
243c244
<         appliance.sourcegraph.com/configHash: 2fec30f60a396508a28da4a0d7c03ecbe530b8370ddb0ae38b355e52bd844446
---
>         appliance.sourcegraph.com/configHash: c878c6daa974c7ef411448ebabf07a65e9eb202152898e1f786066a15de2209a

@craigfurman craigfurman requested a review from jdpleiness June 10, 2024 15:23
Base automatically changed from craig/rel-125-remove-references-to-embeddings to main June 11, 2024 08:15
@craigfurman craigfurman force-pushed the craig/rel-82-service-definition-searcher branch from 725b233 to f5f977a Compare June 11, 2024 08:17
@craigfurman craigfurman marked this pull request as ready for review June 11, 2024 08:18
@craigfurman craigfurman enabled auto-merge (squash) June 11, 2024 08:18
@craigfurman craigfurman merged commit c5598e0 into main Jun 11, 2024
@craigfurman craigfurman deleted the craig/rel-82-service-definition-searcher branch June 11, 2024 08:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants