add sync-cluster-config command #231

chrisseto · 2024-09-13T19:07:56Z

This commit adds the sync-cluster-config command which is designed to replace the bash scripts in the helm chart's post-__ hooks.

Rather than re-inventing the wheel and loading up the admin API configuration, we re-use RPK's config loading as the redpanda.yaml and environment variables are known to be valid.

RafalKorepta · 2024-09-14T22:30:07Z

src/go/k8s/cmd/syncclusterconfig/sync.go

+
+			// NB: It's unclear why this endpoint is being hit. It's preserved
+			// as a historical artifact.
+			if err := client.RestartService(ctx, "schema-registry"); err != nil {


I don't remember why this restart is needed. The PR which introduced this change have 0 context. I can only guess now that cluster which is upgraded to from no SASL to cluster with SASL enabled might have problem with Schema Registry. It would be good to create follow up to create test case for such upgrade and remove this schema registry restart.

redpanda-data/helm-charts#846

https://redpandadata.atlassian.net/wiki/spaces/CORE/pages/326500353/Schema+Registry+Restart

redpanda-data/redpanda#9631

I'll file an issue to circle back on this one and leave it in place for now unless you have any major objections?

No objection I tried to provide context to the best of my ability.

RafalKorepta · 2024-09-16T19:56:49Z

https://buildkite.com/redpanda/redpanda-operator/builds/2603#_

Test containers was waiting for pulling Redpanda container and apparently it failed :(


2024/09/16 17:55:12 ⏳ Waiting for container id 94e70c6744c0 image: docker.redpanda.com/redpandadata/redpanda:v24.2.4. Waiting for: &{timeout:<nil> deadline:<nil> Strategies:[0xc0002468d0 0xc000246930 0xc000246990]}
--
  | 2024/09/16 17:56:12 container logs (wait until ready: get state: Get "http://%2Fvar%2Frun%2Fdocker.sock/v1.44/containers/94e70c6744c034b922f43c35d1b0a9a5ae54b0cc91b0cb7d6422d6f2b3b01c19/json": context deadline exceeded):
  |  
  | sync_test.go:47:
  | Error Trace:	/work/src/go/k8s/cmd/syncclusterconfig/sync_test.go:47
  | Error:      	Received unexpected error:
  | start container: started hook: wait until ready: get state: Get "http://%2Fvar%2Frun%2Fdocker.sock/v1.44/containers/94e70c6744c034b922f43c35d1b0a9a5ae54b0cc91b0cb7d6422d6f2b3b01c19/json": context deadline exceeded
  | Test:       	TestSync
  | FAIL: TestSync (72.72s)

RafalKorepta · 2024-09-16T20:01:24Z

src/go/k8s/cmd/syncclusterconfig/sync.go

+		},
+	}
+
+	cmd.Flags().StringVar(&bootstrapYAMLPath, "bootstrap-yaml", "", "")


Not a blocker.

As far as I know the default place where Redpanda is looking for .bootstrap.yaml file is /etc/redpanda/ path.

RafalKorepta · 2024-09-16T20:06:33Z

src/go/k8s/go.mod

+	github.com/testcontainers/testcontainers-go v0.33.0
+	github.com/testcontainers/testcontainers-go/modules/redpanda v0.33.0


Something broke with go dependency update. #231 (comment)

Could you downgrade some of the dependency to see if that helps?

RafalKorepta · 2024-09-16T20:22:32Z

I wonder if CI have some problems and my hack would work?

redpanda-operator/src/go/k8s/internal/controller/redpanda/topic_controller_test.go

Lines 63 to 66 in 7ac4262

    
           if os.Getenv("CI") == "true" { 
        
           	err := os.Setenv("TESTCONTAINERS_RYUK_DISABLED", "true") 
        
           	require.NoError(t, err) 
        
           }

RafalKorepta

LGTM

I'm not sure why CI is failing. Maybe new version of testcontainer introduced a bug.

andrewstucki

I generally like the movement to go code, though it makes me wonder why this hook exists in the first place.

Additionally, thoughts about the library upgrades.

andrewstucki · 2024-09-16T21:28:35Z

src/go/k8s/go.mod

+	k8s.io/api v0.30.3
+	k8s.io/apiextensions-apiserver v0.30.1
+	k8s.io/apimachinery v0.30.3
+	k8s.io/client-go v0.30.3
 	k8s.io/component-helpers v0.29.5
 	k8s.io/kubectl v0.29.5
 	k8s.io/utils v0.0.0-20240711033017-18e509b52bc8
 	pgregory.net/rapid v1.1.0
-	sigs.k8s.io/controller-runtime v0.17.2
+	sigs.k8s.io/controller-runtime v0.18.5


Not entirely sure on all of these changes, but I know that a bunch of Kubernetes stuff along with controller-runtime are highly sensitive to minor version changes in the underlying libraries. Just looking above it looks like we also have kubectl and component-helpers at v0.29.5... I'm not sure if that'll cause odd problems or not.

I think that the helm-charts repo also has a controller-runtime dependency? Might version mismatches there cause problems here?

I've yet to experience any issues from version mismatches that don't appear at either compile time or very early on in startup / init() code. Have you seen runtime issues before?

I think that the helm-charts repo also has a controller-runtime dependency? Might version mismatches there cause problems here?

It does but it uses very little of the API(s) exposed. I'm fairly certain that it's just the client.Client and some configuration helpers. I don't imagine any issues occurring?

Synced all the k8s.io/* deps with the exception of k8s.io/utils which doesn't really have any published versions.

andrewstucki · 2024-09-16T21:32:46Z

src/go/k8s/cmd/syncclusterconfig/sync.go

+				logger.Info(fmt.Sprintf("$%s not set. Skipping license setting...", licenseEnvvar))
+			} else {
+				logger.Info(fmt.Sprintf("$%s set. Setting license...", licenseEnvvar))
+				if err := client.SetLicense(cmd.Context(), strings.NewReader(license)); err != nil {


Random question, but do we know why we're setting the license like this? Is it because wherever the license is being dumped doesn't get re-read by the cluster nodes? Would it make sense to put something like this in the config watching sidecar instead?

AFAIK the license has to be set like this. I don't believe there's anywhere it actually gets read from.

andrewstucki · 2024-09-16T21:33:54Z

src/go/k8s/cmd/syncclusterconfig/sync.go

+			// NB: It's unclear why this endpoint is being hit. It's preserved
+			// as a historical artifact.
+			// See also: https://github.com/redpanda-data/redpanda-operator/issues/232
+			if err := client.RestartService(ctx, "schema-registry"); err != nil {


Same thought with this -- if it's something that just fires after a configuration component changes and is meant to avoid an issue where we aren't re-reading configuration off disk, can't we just add this to the config watcher?

It's suspected that this is due to a cluster level configuration change which isn't something that's read from disk. I guess technically we could watch the bootstrap file but that's only read once at cluster initialization time.

This commit adds the `sync-cluster-config` command which is designed to replace the bash scripts in the helm chart's post-__ hooks. Rather than re-inventing the wheel and loading up the admin API configuration, we re-use RPK's config loading as the redpanda.yaml and environment variables are known to be valid.

chrisseto force-pushed the chris/p/sync-cluster-config branch from d256041 to 4dff2eb Compare September 13, 2024 20:10

Base automatically changed from chris/p/add-envsubst to main September 13, 2024 20:23

RafalKorepta reviewed Sep 14, 2024

View reviewed changes

chrisseto marked this pull request as ready for review September 16, 2024 14:14

chrisseto mentioned this pull request Sep 16, 2024

re-visit the need to restart the schema registry service after performing updates #232

Open

chrisseto force-pushed the chris/p/sync-cluster-config branch from 4dff2eb to f3424df Compare September 16, 2024 15:56

RafalKorepta reviewed Sep 16, 2024

View reviewed changes

RafalKorepta approved these changes Sep 16, 2024

View reviewed changes

chrisseto force-pushed the chris/p/sync-cluster-config branch from f3424df to 0d4c9a0 Compare September 16, 2024 21:02

chrisseto requested a review from andrewstucki as a code owner September 16, 2024 21:02

chrisseto force-pushed the chris/p/sync-cluster-config branch 2 times, most recently from 388e2b4 to ca8f15e Compare September 16, 2024 21:20

chrisseto mentioned this pull request Sep 16, 2024

TODO #223

Closed

andrewstucki reviewed Sep 16, 2024

View reviewed changes

chrisseto force-pushed the chris/p/sync-cluster-config branch from ca8f15e to b91bacd Compare September 17, 2024 19:20

chrisseto force-pushed the chris/p/sync-cluster-config branch from b91bacd to 187a795 Compare September 17, 2024 19:21

chrisseto enabled auto-merge (rebase) September 17, 2024 19:22

chrisseto merged commit 967e6b3 into main Sep 17, 2024
4 of 5 checks passed

RafalKorepta deleted the chris/p/sync-cluster-config branch December 2, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add sync-cluster-config command #231

add sync-cluster-config command #231

chrisseto commented Sep 13, 2024

RafalKorepta Sep 14, 2024

chrisseto Sep 16, 2024

RafalKorepta Sep 16, 2024

RafalKorepta commented Sep 16, 2024

RafalKorepta Sep 16, 2024 •

edited

Loading

RafalKorepta Sep 16, 2024

RafalKorepta commented Sep 16, 2024

RafalKorepta left a comment

andrewstucki left a comment

andrewstucki Sep 16, 2024

chrisseto Sep 17, 2024

chrisseto Sep 17, 2024

andrewstucki Sep 16, 2024

chrisseto Sep 17, 2024

andrewstucki Sep 16, 2024

chrisseto Sep 17, 2024

		github.com/testcontainers/testcontainers-go v0.33.0
		github.com/testcontainers/testcontainers-go/modules/redpanda v0.33.0

add sync-cluster-config command #231

add sync-cluster-config command #231

Conversation

chrisseto commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafalKorepta commented Sep 16, 2024

RafalKorepta Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafalKorepta commented Sep 16, 2024

RafalKorepta left a comment

Choose a reason for hiding this comment

andrewstucki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafalKorepta Sep 16, 2024 •

edited

Loading