Remove --http-port and --port from the cockroach operator by prafull01 · Pull Request #1058 · cockroachdb/cockroach-operator

prafull01 · 2024-10-21T14:56:05Z

udnay · 2024-10-21T19:46:20Z

install/crds.yaml

@@ -1084,12 +1084,12 @@ spec:
                    type: boolean
                type: object
              grpcPort:


Is this still just a port? Do we need to update the CRD here for something like grpcAddr and httpAddr

If we change the fields from port to grpcAddr and http-port to httpAddr, we need to have some kind of migration to actually migrate the old CRs to these new values. This is why to keep it simple I have just changed the description and not fields.

udnay · 2024-10-21T19:47:35Z

pkg/resource/testdata/TestStatefulSetBuilder/automount_sa.golden

        - 'exec /cockroach/cockroach.sh start --advertise-host=$(POD_NAME).test-cluster.test-ns
-          --certs-dir=/cockroach/cockroach-certs/ --http-port=8080 --sql-addr=:26257
+          --certs-dir=/cockroach/cockroach-certs/ --http-addr=:8080 --sql-addr=:26257
          --listen-addr=:26258 --log="{sinks: {stderr: {channels: [OPS, HEALTH], redact:


Wait, did we already make the change to listen-addr in the operator? What value is it pulling from?

Yes we already have change of listen-addr which is equal to :grpcPort, I did the same thing from http-port to http-addr=:httpPort
You can check existing values coming from here

udnay · 2024-10-21T19:48:02Z

config/crd/bases/crdb.cockroachlabs.com_crdbclusters.yaml

                type: object
              grpcPort:
-                description: '(Optional) The database port (`--port` CLI parameter
+                description: '(Optional) The database port (`--listen-addr` CLI parameter


Are these changes backwards compatible?

Yes. I have only changed the description and not the field so it is backward compatible.

udnay · 2024-10-21T19:49:47Z

pkg/scale/drainer.go

 // after it's confirmed that there are 0 replicas on the node.
 func (d *CockroachNodeDrainer) markNodeAsDecommissioned(ctx context.Context, id uint, gRPCPort int32) error {
-	cmd := []string{"./cockroach", "node", "decommission", fmt.Sprintf("%d", id), fmt.Sprintf("--port=%d", gRPCPort)}
+	cmd := []string{"./cockroach", "node", "decommission", fmt.Sprintf("%d", id), fmt.Sprintf("--host=:%d", gRPCPort)}


What does --host=:<grpcPort> default to? Is it localhost?

Description specifies:

--host <addr/host>[:<port>] CockroachDB node to connect to. This can be specified either as an address/hostname, or together with a port number as in -s myhost:26257. If the port number is left unspecified, it defaults to 26257. An IPv6 address can also be specified with the notation [...], for example [::1]:26257 or [fe80::f6f2:::]:26257. Environment variable: COCKROACH_HOST (default :26257)

It is similar to listenAddr or httpAddr where we can only specify port with :port

udnay

Please make sure that the tests all pass on the branch, other than that LGTM

udnay · 2024-10-25T13:33:39Z

apis/v1alpha1/webhook.go


-	if r.Spec.GRPCPort == nil {
-		r.Spec.GRPCPort = &DefaultGRPCPort
+	if r.Spec.GRPCPort == nil && r.Spec.ListenAddr == nil {


How does this work, I'm not super familiar with it. Do we need to persist the spec somewhere? I know we pull it often from the k8s api, do we need to update it if we change some of the values here?

This Default() func always gets called in mutating configuration so whenever a create or update request comes on this func gets called and the changes we mutate on crdbCluster goes to apiserver and it gets persisted to etcd.

Now when in controller you get the crdbcluster object from apiserver it returns the object with all the mutation we did in Default() func.

So if I do kubectl get crdbcluster foo -o yaml I'll see listenAddr set now?

Yes. If you create a crdbcluster CR with these changes you will see the listenAddr, httpAddr and sqlAddr set.

If you have old CR, then you don't see it (so no automatic rolling restart). However as soon as you update that CR with new image, your rolling restart will happen and now you see addresses instead of ports.

udnay · 2024-10-25T13:36:48Z

pkg/resource/cluster.go

@@ -48,7 +49,6 @@ const (

 func NewCluster(original *api.CrdbCluster) Cluster {


This is what I'm curious about, does the original crdb cluster get the defaults or will we always need to do the migration. I guess we'll always do the migration so that there is no conflict between the persisted object and whatever the customer is using to update it.

This will always gets the object which is processed by default (see my prev comment). Whenever we get the object in controller we get the object which has already mutated using Default func. so we don't have to migrate it again. This is the reason I have removed the call to Default func again. This could cause migration on non migrated object.

If that call is present, after upgrade the migration doesn't happen on the actual CR, but in our logic that func does the migration which is two different states for the same object. We should always use the state which is persisted in etcd.

prafull01 · 2024-10-25T14:01:21Z

Please make sure that the tests all pass on the branch, other than that LGTM

For tests to pass, I am starting a webhook server with controller manager so that this Default() func gets called and does the basic mutation on crdbcluster object. It will need some changes on test side so I will ask for a review again.

…ts to addresses

prafull01 force-pushed the remove-http-port branch from 4733aa4 to 3b43236 Compare October 21, 2024 18:34

prafull01 requested a review from udnay October 21, 2024 18:55

prafull01 force-pushed the remove-http-port branch from 3b43236 to a3078de Compare October 21, 2024 19:20

udnay reviewed Oct 21, 2024

View reviewed changes

prafull01 force-pushed the remove-http-port branch 2 times, most recently from 8c47b28 to f1bf6cf Compare October 25, 2024 12:28

udnay approved these changes Oct 25, 2024

View reviewed changes

prafull01 force-pushed the remove-http-port branch from f1bf6cf to 40b1bc9 Compare October 25, 2024 14:00

cockroachdb deleted a comment from prafull11 Oct 25, 2024

prafull01 force-pushed the remove-http-port branch from 40b1bc9 to bdcd189 Compare October 25, 2024 14:13

prafull01 added 2 commits November 25, 2024 16:23

Remove --http-port and --port from the cockroach operator

65ad33e

Add httpAddr, sqlAddr, listenAddr in CrdbCluster API and migrated por…

ada69ac

…ts to addresses

prafull01 force-pushed the remove-http-port branch from bdcd189 to ada69ac Compare November 25, 2024 10:54

		@@ -48,7 +49,6 @@ const (

		func NewCluster(original *api.CrdbCluster) Cluster {

Conversation

prafull01 commented Oct 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prafull01 Oct 22, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

udnay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prafull01 commented Oct 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

prafull01 Oct 22, 2024 •

edited

Loading