feat(appliance): Adopt Frontend Service + Ingress by Chickensoupwithrice · Pull Request #63893 · sourcegraph/sourcegraph-public-snapshot

Chickensoupwithrice · 2024-07-17T22:38:50Z

Resolving section 1 REL-213: Setting controllerOwner references to k8s objects that will exist before appliance is installed.
Part 2 (serializing helm values into an annotation and using that as input) will happen in a subsequent PR

We need to adopt Frontend's Service and Ingress for Appliance to manage.

~~This is still a work in progress PR.~~

Currently handles and adopts frontend's Service (golden test included)
Yet to adopt frontend's Service.

Test plan

Golden test included

Changelog

feat(appliance): Appliance adopts Frontend's Ingress + Service

craigfurman · 2024-07-18T09:10:21Z

+      resourceVersion: NORMALIZED_FOR_TESTING
+      uid: NORMALIZED_FOR_TESTING
+  - apiVersion: v1
+    kind: Service


indeed, here's the service we provisioned, collected by the golden test framework. It has no ControllerReference, so we haven't adopted it.

There are no other resources apart from our configmap, which was puzzling to me at first, but read on

Running go test -v ./internal/appliance/reconciler -run TestApplianceTestSuite/TestAdoptFrontend, we see the following reconciler logs:

core.go:57: INFO TestApplianceTestSuite logr@v1.4.2/logr.go:280 reconciling sourcegraph appliance {"controller": "configmap", "controllerGroup": "", "controllerKind": "ConfigMap", "ConfigMap": "test-appliance-dc9fea/sg", "namespace": "test-appliance-dc9fea", "name": "sg", "reconcileID": "e1657450-ee2c-4b5d-8fa3-2223fa4af7f0"} core.go:57: INFO TestApplianceTestSuite logr@v1.4.2/logr.go:280 didn't find existing object, creating it {"controller": "configmap", "controllerGroup": "", "controllerKind": "ConfigMap", "ConfigMap": "test-appliance-dc9fea/sg", "namespace": "test-appliance-dc9fea", "name": "sg", "reconcileID": "e1657450-ee2c-4b5d-8fa3-2223fa4af7f0", "kind": "/, Kind=", "namespace": "test-appliance-dc9fea", "name": "sourcegraph-frontend"} I0718 10:03:50.810672 49837 controller.go:624] quota admission added evaluator for: deployments.apps core.go:57: INFO TestApplianceTestSuite logr@v1.4.2/logr.go:280 refusing to update non-owned resource {"controller": "configmap", "controllerGroup": "", "controllerKind": "ConfigMap", "ConfigMap": "test-appliance-dc9fea/sg", "namespace": "test-appliance-dc9fea", "name": "sg", "reconcileID": "e1657450-ee2c-4b5d-8fa3-2223fa4af7f0", "kind": "/, Kind=", "namespace": "test-appliance-dc9fea", "name": "sourcegraph-frontend"} core.go:57: INFO TestApplianceTestSuite logr@v1.4.2/logr.go:280 didn't find existing object, creating it {"controller": "configmap", "controllerGroup": "", "controllerKind": "ConfigMap", "ConfigMap": "test-appliance-dc9fea/sg", "namespace": "test-appliance-dc9fea", "name": "sg", "reconcileID": "e1657450-ee2c-4b5d-8fa3-2223fa4af7f0", "kind": "/, Kind=", "namespace": "test-appliance-dc9fea", "name": "sourcegraph-frontend-internal"} I0718 10:03:50.817441 49837 alloc.go:330] "allocated clusterIPs" service="test-appliance-dc9fea/sourcegraph-frontend-internal" clusterIPs={"IPv4":"10.0.0.89"} core.go:57: INFO TestApplianceTestSuite logr@v1.4.2/logr.go:280 didn't find existing object, creating it {"controller": "configmap", "controllerGroup": "", "controllerKind": "ConfigMap", "ConfigMap": "test-appliance-dc9fea/sg", "namespace": "test-appliance-dc9fea", "name": "sg", "reconcileID": "e1657450-ee2c-4b5d-8fa3-2223fa4af7f0", "kind": "/, Kind=", "namespace": "test-appliance-dc9fea", "name": "sourcegraph-frontend"} I0718 10:03:50.819103 49837 controller.go:624] quota admission added evaluator for: serviceaccounts core.go:57: INFO TestApplianceTestSuite logr@v1.4.2/logr.go:280 didn't find existing object, creating it {"controller": "configmap", "controllerGroup": "", "controllerKind": "ConfigMap", "ConfigMap": "test-appliance-dc9fea/sg", "namespace": "test-appliance-dc9fea", "name": "sg", "reconcileID": "e1657450-ee2c-4b5d-8fa3-2223fa4af7f0", "kind": "/, Kind=", "namespace": "test-appliance-dc9fea", "name": "sourcegraph-frontend"} core.go:57: INFO TestApplianceTestSuite logr@v1.4.2/logr.go:280 didn't find existing object, creating it {"controller": "configmap", "controllerGroup": "", "controllerKind": "ConfigMap", "ConfigMap": "test-appliance-dc9fea/sg", "namespace": "test-appliance-dc9fea", "name": "sg", "reconcileID": "e1657450-ee2c-4b5d-8fa3-2223fa4af7f0", "kind": "/, Kind=", "namespace": "test-appliance-dc9fea", "name": "sourcegraph-frontend"} core.go:57: DEBUG TestApplianceTestSuite logr@v1.4.2/logr.go:280 Reconcile successful {"controller": "configmap", "controllerGroup": "", "controllerKind": "ConfigMap", "ConfigMap": "test-appli

Things that stand out:

we do see our "refusing to update non-owned resource", as we expect. That's no surprise, this PR doesn't add any logic for adoption of specific GVKNs, yet

we can't see what resources each line refers to ("kind": "/, Kind=", - wat). I'll start investigating that separately, will probably open an issue.

We don't see the frontend Deployment and other resources in this golden fixture 🤔. We see a bunch of messages "didn't find existing object, creating it", and the refusal to update non-owned resources is non-fatal. This appears to be because your test has a subtle race condition in it. See my note on the makeGoldenAssertions() call for details. When I fix that, I see the other resources, and diff internal/appliance/reconciler/testdata/golden-fixtures/frontend/default.yaml internal/appliance/reconciler/testdata/golden-fixtures/frontend/adopt-service.yaml looks as we expect (ownerReference stuff missing)

https://linear.app/sourcegraph/issue/REL-275/reconciler-logs-dont-contain-resource-metadata

Chickensoupwithrice · 2024-07-18T21:54:47Z

+		if existingRes.GetObjectKind().GroupVersionKind().Kind == "Service" {
+			logger.Info(fmt.Sprintf("Taking control of previously unowned object: %s", existingRes.GetName()))
+			if err := ctrl.SetControllerReference(owner, existingRes, r.Scheme); err != nil {
+				return errors.Newf("setting controller reference for service", err)
+			}
+		} else {
+			logger.Info("refusing to update non-owned resource")
+			return nil


I'm a little wary that we could inadvertently cobble other services here. Given the namespace check above, it should be enough?

I think, if we relax this check for whole kinds, we may as well drop it altogether. I've outlined an alternative plan in another comment, LMK what you think 🙏

Chickensoupwithrice · 2024-07-18T23:14:48Z

+  - apiVersion: networking.k8s.io/v1
+    kind: Ingress
+    metadata:
+      annotations:
+        appliance.sourcegraph.com/configHash: d3eb623947fedba566dfc56adc4733ff5ef1b2887a5cb63d75dbc1df452b0b5c
+      creationTimestamp: "2024-04-19T00:00:00Z"
+      generation: 2
+      labels:
+        deploy: sourcegraph
+      name: sourcegraph-frontend
+      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
+    spec:
+      rules:
+        - http:
+            paths:
+              - backend:
+                  service:
+                    name: sourcegraph-frontend
+                    port:
+                      number: 30080
+                path: /
+                pathType: Prefix
+    status:
+      loadBalancer: {}


We successfully clobbered the ingress as well, but didn't keep any of the previous definitions (intended behaviour for now)

craigfurman · 2024-07-19T09:31:53Z

+		if existingRes.GetObjectKind().GroupVersionKind().Kind == "Service" {
+			logger.Info(fmt.Sprintf("Taking control of previously unowned object: %s", existingRes.GetName()))
+			if err := ctrl.SetControllerReference(owner, existingRes, r.Scheme); err != nil {
+				return errors.Newf("setting controller reference for service", err)
+			}
+		} else {
+			logger.Info("refusing to update non-owned resource")
+			return nil


I think, if we relax this check for whole kinds, we may as well drop it altogether. I've outlined an alternative plan in another comment, LMK what you think 🙏

craigfurman · 2024-07-19T09:38:52Z

+          protocol: TCP
+          targetPort: http
+      selector:
+        app: sourcegraph-frontend


I know we are still figuring out where/when the appliance will flip the service selector between itself/frontend when frontend is unhealthy. One potential issue with allowing the service definition to do it, is that on first installation, the user might be looking at a progress bar, which will stop working as soon as installation starts, because we point the ingress-facing service away from the appliance.

I think we can leave this as-is, and I've left a comment in an issue to figure this out: https://linear.app/sourcegraph/issue/REL-78/when-sourcegraph-frontend-is-down-a-user-trying-to-access-sourcegraph#comment-d3b6cd0d

cc @jdpleiness

craigfurman

🚀

nice!

Co-authored-by: Craig Furman <craig.furman@sourcegraph.com>

…s with-ingress)

Resolving section 1 [REL-213](https://linear.app/sourcegraph/issue/REL-218/ingressservice-consistency-for-helm-deployment): Setting controllerOwner references to k8s objects that will exist before appliance is installed. Part 2 (serializing helm values into an annotation and using that as input) will happen in a subsequent PR We need to adopt Frontend's `Service` and `Ingress` for Appliance to manage. ~~This is still a work in progress PR.~~ Currently handles and adopts frontend's Service (golden test included) Yet to adopt frontend's Service. ## Test plan Golden test included  ## Changelog  - feat(appliance): Appliance adopts Frontend's `Ingress` + `Service` --------- Co-authored-by: Craig Furman <craig.furman@sourcegraph.com>

feat(appliance): Adopt Frontend Service + Ingress

f757588

cla-bot Bot added the cla-signed label Jul 17, 2024

Chickensoupwithrice requested review from a team and craigfurman July 18, 2024 01:37

fix(appliance): run bazel configure

824ea71

craigfurman self-assigned this Jul 18, 2024

craigfurman reviewed Jul 18, 2024

View reviewed changes

Chickensoupwithrice commented Jul 18, 2024

View reviewed changes

Comment thread internal/appliance/reconciler/testdata/golden-fixtures/frontend/adopt-service.yaml Outdated

Chickensoupwithrice added 5 commits July 18, 2024 12:13

fix(appliance): resolve race condition in frontend_test

8516758

fix(appliance): update golden fixture with no race condition

c51ea5c

fix(appliance): appliance takes ownership of frontend service

264a4fb

misc(appliance): remove comments from frontend_test.go

1e3fcf6

fix(appliance): removed unsupported fields from frontend/adopt-service

703d1b0

Chickensoupwithrice commented Jul 18, 2024

View reviewed changes

Chickensoupwithrice added 2 commits July 18, 2024 14:57

fix(appliance): broken fixture for frontend/apdopt-service

553e391

feat(appliance): takes ownership of frontend's ingress

d0935a1

Chickensoupwithrice commented Jul 18, 2024

View reviewed changes

Chickensoupwithrice marked this pull request as ready for review July 18, 2024 23:17

Chickensoupwithrice requested a review from craigfurman July 18, 2024 23:17

Chickensoupwithrice and others added 5 commits July 18, 2024 18:02

fix(appliance): run bazel configure

cebd5ca

Merge branch 'main' into al/REL-218/adopt-frontend

f804ef7

revert changes to kubernetes.go

bd176ac

fix: can't do logic on ownerreferences for non-namespaced resources

a53e46b

Add facility for service definitions to mark objects for adoption

2de832f

craigfurman reviewed Jul 19, 2024

View reviewed changes

craigfurman approved these changes Jul 22, 2024

View reviewed changes

Comment thread internal/appliance/reconciler/frontend_test.go Outdated

Comment thread internal/appliance/reconciler/frontend_test.go Outdated

Chickensoupwithrice and others added 3 commits July 22, 2024 10:46

Update internal/appliance/reconciler/frontend_test.go

e7d9016

Co-authored-by: Craig Furman <craig.furman@sourcegraph.com>

Update internal/appliance/reconciler/frontend_test.go

2acd125

Co-authored-by: Craig Furman <craig.furman@sourcegraph.com>

Merge branch 'main' into al/REL-218/adopt-frontend

81576fe

Chickensoupwithrice enabled auto-merge (squash) July 22, 2024 18:05

Chickensoupwithrice added 5 commits July 22, 2024 11:22

fix(appliance): fix missing } in test

2cdc20c

fix(appliance): use fixture for apdoption test

24e195c

misc(appliance): remove adopt-service yaml test case (it's the same a…

cbe73f8

…s with-ingress)

fix(appliance): update redis golden files

9fe4965

fix(appliance): regenerate appliance frontend tests

4331ca6

Chickensoupwithrice merged commit a61f353 into main Jul 22, 2024

Chickensoupwithrice deleted the al/REL-218/adopt-frontend branch July 22, 2024 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appliance): Adopt Frontend Service + Ingress#63893

feat(appliance): Adopt Frontend Service + Ingress#63893
Chickensoupwithrice merged 22 commits into
mainfrom
al/REL-218/adopt-frontend

Chickensoupwithrice commented Jul 17, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

craigfurman Jul 18, 2024

Uh oh!

craigfurman Jul 18, 2024

Uh oh!

Uh oh!

Uh oh!

Chickensoupwithrice Jul 18, 2024

Uh oh!

craigfurman Jul 19, 2024

Uh oh!

Chickensoupwithrice Jul 18, 2024 •

edited

Loading

Uh oh!

Uh oh!

craigfurman Jul 19, 2024

Uh oh!

craigfurman Jul 19, 2024

Uh oh!

craigfurman left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Chickensoupwithrice commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Changelog

Uh oh!

Uh oh!

Uh oh!

craigfurman Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

craigfurman Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Chickensoupwithrice Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

craigfurman Jul 19, 2024

Choose a reason for hiding this comment

Uh oh!

Chickensoupwithrice Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

craigfurman Jul 19, 2024

Choose a reason for hiding this comment

Uh oh!

craigfurman Jul 19, 2024

Choose a reason for hiding this comment

Uh oh!

craigfurman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Chickensoupwithrice commented Jul 17, 2024 •

edited

Loading

Chickensoupwithrice Jul 18, 2024 •

edited

Loading