Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+297 −78
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff902fd
Remove --http-port and --port from the cockroach operator
prafull01 Oct 21, 2024
bdcd189
Add httpAddr, sqlAddr, listenAddr in CrdbCluster API and migrated por…
prafull01 Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ test/pkg:
# takes a bit of time.
.PHONY: test/verify
test/verify:
bazel test //hack/...
bazel test --test_timeout=1200 //hack/...

.PHONY: test/lint
test/lint:
Expand Down
20 changes: 18 additions & 2 deletions apis/v1alpha1/cluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,34 @@ type CrdbClusterSpec struct {
// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Cockroach Database Image"
// +optional
Image *PodImage `json:"image,omitempty"`
// (Optional) The database port (`--port` CLI parameter when starting the service)
// (Optional) The database port (`--listen-addr` CLI parameter when starting the service)
// Default: 26258
// +optional
// Deprecated: Use ListenAddr instead of GRPCPort
GRPCPort *int32 `json:"grpcPort,omitempty"`
// (Optional) The web UI port (`--http-port` CLI parameter when starting the service)
// (Optional) The database port (`--listen-addr` CLI parameter when starting the service)
// Default: ":26258"
// +optional
ListenAddr *string `json:"listenAddr,omitempty"`
// (Optional) The web UI port (`--http-addr` CLI parameter when starting the service)
// Default: 8080
// +optional
// Deprecated: Use HTTPAddr instead of HTTPPort
HTTPPort *int32 `json:"httpPort,omitempty"`
// (Optional) The IP address/hostname and port on which to listen for DB Console HTTP requests.
// (`--http-addr` CLI parameter when starting the service)
// Default: ":8080"
// +optional
HTTPAddr *string `json:"httpAddr,omitempty"`
// (Optional) The SQL Port number
// Default: 26257
// +optional
// Deprecated: Use SQLAddr instead of SQLPort
SQLPort *int32 `json:"sqlPort,omitempty"`
// (Optional) The IP address/hostname and port on which to listen for SQL connections from clients.
// Default: ":26257"
// +optional
SQLAddr *string `json:"sqlAddr,omitempty"`
// (Optional) TLSEnabled determines if TLS is enabled for your CockroachDB Cluster
// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="TLS Enabled",xDescriptors="urn:alm:descriptor:com.tectonic.ui:booleanSwitch"
// +optional
Expand Down
36 changes: 24 additions & 12 deletions apis/v1alpha1/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ import (
)

var (
// DefaultGRPCPort is the default port used for GRPC communication
DefaultGRPCPort int32 = 26258
// DefaultSQLPort is the default port used for SQL connections
DefaultSQLPort int32 = 26257
// DefaultHTTPPort is the default port for the Web UI
DefaultHTTPPort int32 = 8080
// DefaultGRPCAddr is the default grpc address used for GRPC communication
DefaultGRPCAddr string = ":26258"
// DefaultSQLAddr is the default sql address used for SQL connections
DefaultSQLAddr string = ":26257"
// DefaultHTTPAddr is the default http address for the Web UI
DefaultHTTPAddr string = ":8080"
// DefaultMaxUnavailable is the default max unavailable nodes during a rollout
DefaultMaxUnavailable int32 = 1
)
Expand All @@ -59,16 +59,28 @@ func (r *CrdbCluster) SetupWebhookWithManager(mgr ctrl.Manager) error {
func (r *CrdbCluster) Default() {
webhookLog.Info("default", "name", r.Name)

if r.Spec.GRPCPort == nil {
r.Spec.GRPCPort = &DefaultGRPCPort
if r.Spec.GRPCPort == nil && r.Spec.ListenAddr == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

r.Spec.ListenAddr = &DefaultGRPCAddr
} else if r.Spec.GRPCPort != nil && r.Spec.ListenAddr == nil {
listenAddr := fmt.Sprintf(":%d", *r.Spec.GRPCPort)
r.Spec.ListenAddr = &listenAddr
r.Spec.GRPCPort = nil
}

if r.Spec.SQLPort == nil {
r.Spec.SQLPort = &DefaultSQLPort
if r.Spec.SQLPort == nil && r.Spec.SQLAddr == nil {
r.Spec.SQLAddr = &DefaultSQLAddr
} else if r.Spec.SQLPort != nil && r.Spec.SQLAddr == nil {
sqlAddr := fmt.Sprintf(":%d", *r.Spec.SQLPort)
r.Spec.SQLAddr = &sqlAddr
r.Spec.SQLPort = nil
}

if r.Spec.HTTPPort == nil {
r.Spec.HTTPPort = &DefaultHTTPPort
if r.Spec.HTTPPort == nil && r.Spec.HTTPAddr == nil {
r.Spec.HTTPAddr = &DefaultHTTPAddr
} else if r.Spec.HTTPPort != nil && r.Spec.HTTPAddr == nil {
httpAddr := fmt.Sprintf(":%d", *r.Spec.HTTPPort)
r.Spec.HTTPAddr = &httpAddr
r.Spec.HTTPPort = nil
}

if r.Spec.MaxUnavailable == nil && r.Spec.MinAvailable == nil {
Expand Down
6 changes: 3 additions & 3 deletions apis/v1alpha1/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ func TestCrdbClusterDefault(t *testing.T) {
maxUnavailable := int32(1)
policy := v1.PullIfNotPresent
expected := CrdbClusterSpec{
GRPCPort: &DefaultGRPCPort,
HTTPPort: &DefaultHTTPPort,
SQLPort: &DefaultSQLPort,
ListenAddr: &DefaultGRPCAddr,
SQLAddr: &DefaultSQLAddr,
HTTPAddr: &DefaultHTTPAddr,
MaxUnavailable: &maxUnavailable,
Image: &PodImage{PullPolicyName: &policy},
}
Expand Down
15 changes: 15 additions & 0 deletions apis/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 21 additions & 5 deletions config/crd/bases/crdb.cockroachlabs.com_crdbclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1086,13 +1086,20 @@ spec:
type: boolean
type: object
grpcPort:
description: '(Optional) The database port (`--port` CLI parameter
when starting the service) Default: 26258'
description: '(Optional) The database port (`--listen-addr` CLI parameter
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these changes backwards compatible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

when starting the service) Default: 26258 Deprecated: Use ListenAddr
instead of GRPCPort'
format: int32
type: integer
httpAddr:
description: '(Optional) The IP address/hostname and port on which
to listen for DB Console HTTP requests. (`--http-addr` CLI parameter
when starting the service) Default: ":8080"'
type: string
httpPort:
description: '(Optional) The web UI port (`--http-port` CLI parameter
when starting the service) Default: 8080'
description: '(Optional) The web UI port (`--http-addr` CLI parameter
when starting the service) Default: 8080 Deprecated: Use HTTPAddr
instead of HTTPPort'
format: int32
type: integer
image:
Expand Down Expand Up @@ -1214,6 +1221,10 @@ spec:
- host
type: object
type: object
listenAddr:
description: '(Optional) The database port (`--listen-addr` CLI parameter
when starting the service) Default: ":26258"'
type: string
logConfigMap:
description: '(Optional) LogConfigMap define the config map which
contains log configuration used to send the logs through the proper
Expand Down Expand Up @@ -1389,8 +1400,13 @@ spec:
to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
type: object
sqlAddr:
description: '(Optional) The IP address/hostname and port on which
to listen for SQL connections from clients. Default: ":26257"'
type: string
sqlPort:
description: '(Optional) The SQL Port number Default: 26257'
description: '(Optional) The SQL Port number Default: 26257 Deprecated:
Use SQLAddr instead of SQLPort'
format: int32
type: integer
tlsEnabled:
Expand Down
16 changes: 16 additions & 0 deletions e2e/create/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ func TestCreateInsecureCluster(t *testing.T) {
WithImage(e2e.MajorVersion).WithClusterLogging("logging-configmap").
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

steps := testutil.Steps{
{
Name: "creates 3-node insecure cluster",
Expand Down Expand Up @@ -105,6 +108,9 @@ func TestCreatesSecureCluster(t *testing.T) {
WithImage(e2e.MajorVersion).
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

steps := testutil.Steps{
{
Name: "creates 3-node secure cluster",
Expand Down Expand Up @@ -160,6 +166,10 @@ func TestCreateSecureClusterWithInvalidVersion(t *testing.T) {
builder := testutil.NewBuilder("crdb").WithNodeCount(3).WithTLS().
WithImage(testcase.imageVersion).
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

if testcase.cockroachVersion != "" {
os.Setenv(relatedImageEnvName, e2e.NonExistentVersion)
builder = builder.WithCockroachDBVersion(testcase.cockroachVersion)
Expand Down Expand Up @@ -221,6 +231,9 @@ func TestCreateSecureClusterWithNonCRDBImage(t *testing.T) {
WithImage(testcase.imageVersion).
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

if testcase.cockroachVersion != "" {
os.Setenv(relatedImageEnvName, e2e.InvalidImage)
builder = builder.WithCockroachDBVersion(testcase.cockroachVersion)
Expand Down Expand Up @@ -268,6 +281,9 @@ func TestCreateSecureClusterWithCRDBVersionSet(t *testing.T) {
WithPVDataStore("1Gi").
WithCockroachDBVersion(crdbVersion).WithImageObject(nil)

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

require.NoError(subT, sb.Create(builder.Cr()))
testutil.RequireClusterToBeReadyEventuallyTimeout(subT, sb, builder,
e2e.CreateClusterTimeout)
Expand Down
6 changes: 6 additions & 0 deletions e2e/decommission/decommission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ func TestDecommissionFunctionalityWithPrune(t *testing.T) {
WithImage(e2e.MajorVersion).
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

testutil.Steps{
{
Name: "creates a 4-node secure cluster and tests db",
Expand Down Expand Up @@ -126,6 +129,9 @@ func TestDecommissionFunctionality(t *testing.T) {
WithImage(e2e.MajorVersion).
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

testutil.Steps{
{
Name: "creates a 4-node secure cluster and tests db",
Expand Down
4 changes: 4 additions & 0 deletions e2e/pvcresize/pvcresize_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ func TestPVCResize(t *testing.T) {
builder := testutil.NewBuilder("crdb").WithNodeCount(3).WithTLS().
WithImage(e2e.MajorVersion).
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

steps := testutil.Steps{
{
Name: "creates a 3-node secure cluster db",
Expand Down
15 changes: 15 additions & 0 deletions e2e/upgrades/upgrades_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,9 @@ func TestUpgradesMajorVersion20_1To20_2(t *testing.T) {
WithImage("cockroachdb/cockroach:v20.1.16").
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

steps := testutil.Steps{
{
Name: "creates a 3-node secure cluster",
Expand Down Expand Up @@ -211,6 +214,9 @@ func TestUpgradesMinorVersionThenRollback(t *testing.T) {
WithImage(e2e.MinorVersion1).
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

steps := testutil.Steps{
{
Name: "creates a 3-node secure cluster",
Expand Down Expand Up @@ -276,6 +282,9 @@ func TestUpgradeWithInvalidVersion(t *testing.T) {
WithImage(e2e.MinorVersion1).
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

steps := testutil.Steps{
{
Name: "creates a 3-node secure cluster",
Expand Down Expand Up @@ -326,6 +335,9 @@ func TestUpgradeWithInvalidImage(t *testing.T) {
WithImage(e2e.MinorVersion1).
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

steps := testutil.Steps{
{
Name: "creates a 3-node secure cluster",
Expand Down Expand Up @@ -376,6 +388,9 @@ func TestUpgradeWithMajorVersionExcludingMajorFeature(t *testing.T) {
WithImage(e2e.SkipFeatureVersion).
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

steps := testutil.Steps{
{
Name: "creates a 1-node secure cluster",
Expand Down
8 changes: 8 additions & 0 deletions e2e/upgradessha256/upgradessha256_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ func TestUpgradesMinorVersion(t *testing.T) {
WithCockroachDBVersion("v20.2.8").
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()
steps := testutil.Steps{
{
Name: "creates a 3-nodes secure cluster",
Expand Down Expand Up @@ -116,6 +118,9 @@ func TestUpgradesMajorVersion20to21(t *testing.T) {
WithCockroachDBVersion("v20.2.10").
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

steps := testutil.Steps{
{
Name: "creates a 3-nodes secure cluster",
Expand Down Expand Up @@ -170,6 +175,9 @@ func TestUpgradesMajorVersion20_1To20_2(t *testing.T) {
WithCockroachDBVersion("v20.1.16").
WithPVDataStore("1Gi")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

steps := testutil.Steps{
{
Name: "creates a 3-node secure cluster",
Expand Down
6 changes: 6 additions & 0 deletions e2e/versionchecker/versionchecker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ func TestVersionCheckerJobPodPending(t *testing.T) {
corev1.ResourceMemory: apiresource.MustParse("1000T"),
},
})

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()
steps := testutil.Steps{
{
Name: "start an unschedulable job",
Expand Down Expand Up @@ -97,6 +100,9 @@ func TestLoggingAPIValidCheck(t *testing.T) {
WithPVDataStore("32Mi").
WithClusterLogging("logging-configmap")

// This defaulting is done by webhook mutation config, but in tests we are doing it manually.
builder.Cr().Default()

steps := testutil.Steps{
{
Name: "validate the logging API input",
Expand Down
4 changes: 2 additions & 2 deletions hack/build/repos.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5336,8 +5336,8 @@ def _go_dependencies():
build_file_generation = "on",
build_file_proto_mode = "disable",
importpath = "gopkg.in/yaml.v3",
sum = "h1:h8qDotaEPuJATrMmW04NCwg7v22aHH28wwpauUhK9Oo=",
version = "v3.0.0-20210107192922-496545a6307b",
sum = "h1:hjy8E9ON/egN1tAYqKb61G10WtihqetD4sz2H+8nIeA=",
version = "v3.0.0",
)
go_repository(
name = "io_etcd_go_bbolt",
Expand Down
Loading
Loading