Fix port naming in codeintel-db service spec

[DaedalusG]

Fixes https://github.com/sourcegraph/sourcegraph/issues/51175.

Ultimately this change is minor, but may prevent confusion.

The port name and targetPort name are not actually used for anything besides human readability to my knowledge and there are no known issues with the current manifest. This does bring things in line with the convention in use in codeinsights-db service file though, and will eliminate possible confusion by readers

Codeinsights manifest

Checklist

• Follow the manual testing process
• Update changelog

Test plan

[efritz]

Is this used in the deployment (or any other file) by name?

[michaellzc]

the target port does not seem rigth to me:

deploy-sourcegraph-helm/charts/sourcegraph/templates/codeintel-db/codeintel-db.StatefulSet.yaml

Lines 92 to 94 in 2b4383b

	ports:
	- containerPort: 5432
	name: pgsql

shouldn't it be psql?

[DaedalusG]

Shouldn't targetPort always match the name -- just for readability? Why would this case be different than the codeinsights-db case of the service manifest?

[DaedalusG]

Any design reason we named the db container in codeintel-db pgsql that we know of?

[michaellzc]

in svc port entry

spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - protocol: TCP
      port: 80
      targetPort: 9376

it basically says, forward any incoming traffic to this svc at 80 to pod with matching selector labels at 9376

so targetPort should match the port name or number at the POD

[michaellzc]

if we really want to use codeintel-db everywhere, we will need to rename the port name in statefulset pod template s well

[michaellzc]

@efritz would you clarify how this could be confusing to users during debugging?

afaik, user shound't care about the name of port we expose.

as an admin, i usually just run

k port-forward svc/codeintel-db 5432:5432

as long as the exposed port number is correct, it shound't be a problem?

[DaedalusG]

@michaellzc this came up as a source of confusion during an upgrade call where migrator was unable to contact the codeintel-db -- it ultimately wasnt related to this naming, but we were asked to clarify the naming

[efritz]

@michaellzc This tripped up Marc debugging an issue yesterday here. We weren't sure the Helm charts were configured to point the migrator at the correct databases, and since pgsql is a pod name AND the port name for the codeintel-db pod, it looked wrong at a glance.

[efritz]

That said I don't know how to use Helm, and I think to a Helm ignorant person this change makes some sense just to disambiguate what strings are used where.

I'm up for other solutions/clarifications though.

[michaellzc]

I am still not sold.

I see Marc's comment was

Migrator commands reference frontend, while the pod name is pgsql

it looks to me the main confusion is coming from pgsql -> frontend, but obviously we can't just rename the pgsql statefulset.

Happy to just accept it as it is since there is no practical difference.

I think we have two options:

1. rename all exposed port name of pgsql, codeintel-db, codeinsights-db to postgres or postgresql, just to emphasize this is just a postgres database
2. rename the port name of each db deployment to the db name, i.e., pgsql -> pgsql, codeintel-db everywhere.

either option require renaming references to the port name in both statefulset pod template spec and service spec

[efritz]

Option No. 1 seems simple and also the least risky. Should we do the same for non-Helm deployments where the port is also named (maybe just k8s)?

[michaellzc]

Option No. 1 seems simple and also the least risky. Should we do the same for non-Helm deployments where the port is also named (maybe just k8s)?

Yeah, I think that will be great, to keep things consistent.

[marcleblanc2]

Yeah, the customer admin was using Lens to manage their deployment which showed the network port name, and as that matched the frontend db's pod name, that distracted us and wasted a bunch of our time, and prolonged their unplanned Sourcegraph prod outage, thus decreased customer and user confidence in our product.

This is the kinda tech debt we need to clean up so our outage resolution times are faster by fewer distractions / opportunities to get tripped up; see also: inconsistent naming of database targets in the multi-version upgrade docs.

We can't require this level of depth of customer admin / support / IE expertise to save us this trouble, we need to prevent this trouble earlier in the chain, which then in turn reduces the escalation engineering cost and distraction of supporting the product, so it's a win-win-win all around.

[efritz]

see also: inconsistent naming of database targets in the multi-version upgrade docs.

@marcleblanc2 covered in https://github.com/sourcegraph/sourcegraph/issues/51174, also see the board for progress over this quarter.

[marcleblanc2]

You guys are awesome, I'm a big fan, I appreciate your work a ton, and customers are going to love not tripping over this stuff.

[DaedalusG]

Okay all the ports are called postgres now @michaellzc wdyt?