Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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: 2 additions & 0 deletions api/v2/weightsandbiases_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,8 @@ type ObjectStoreInfraStatus struct {
type ClickHouseInfraStatus struct {
WBInfraStatus `json:",inline"`
Connection ClickHouseConnection `json:"connection,omitempty"`
Replicated bool `json:"replicated,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(weightsandbiases_types\.go|.*clickhouse.*|.*ClickHouse.*|.*crd.*|.*test.*)$' | head -200

printf '%s\n' '--- relevant symbols and field references ---'
rg -n -C 5 \
  'Replicated|managedClickHouseInferStatus|ClickHouseInfraStatus|ClickHouseInfra' \
  api config charts deploy test tests 2>/dev/null | head -500 || true

printf '%s\n' '--- type outline ---'
ast-grep outline api/v2/weightsandbiases_types.go --match 'ClickHouseInfraStatus' --view expanded 2>/dev/null || true

Repository: wandb/operator

Length of output: 18184


🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- all status construction and field assignments ---'
rg -n -C 12 \
  'ClickHouseInfraStatus|Replicated\s*[:=]|managedClickHouse|InferStatus|clickhouseStatus' \
  api internal config --glob '*.go' --glob '*.yaml' | head -1000

printf '%s\n' '--- CRD schema occurrences ---'
rg -n -C 10 \
  'replicated|clusterName|clickhouseStatus|ClickHouseInfraStatus' \
  config/crd internal/crdinstaller --glob '*.yaml' --glob '*.go' | head -800

printf '%s\n' '--- likely relevant tests ---'
rg -n -C 15 \
  'Replicated|replicated|ClickHouseInfraStatus|clickhouseStatus' \
  internal/controller api --glob '*_test.go' | head -1000

printf '%s\n' '--- Make targets and working tree ---'
git status --short
rg -n -C 4 '^(lint|test|manifests|generate|sync-crd-embed):|manifests:|generate:|sync-crd-embed:' Makefile makefile **/Makefile 2>/dev/null | head -400 || true

Repository: wandb/operator

Length of output: 50371


🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- exact Replicated references ---'
rg -n -C 5 '\bReplicated\b|\breplicated\b' --glob '*.go' --glob '*.yaml' . | head -300

printf '%s\n' '--- ClickHouse reconciler symbols ---'
rg -n '^(func|type) .*ClickHouse|ClickHouse.*Infer|clickHouse.*Infer|managedClickHouse|externalClickHouse' internal/controller/reconciler internal/controller/infra --glob '*.go'

printf '%s\n' '--- ClickHouse reconciler focused source ---'
sed -n '1,260p' internal/controller/reconciler/clickhouse.go

printf '%s\n' '--- CRD clickhouseStatus schema ---'
sed -n '4410,4525p' internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml

printf '%s\n' '--- Make targets ---'
rg -n -C 6 '^(lint|test|manifests|generate|sync-crd-embed):|manifests:|generate:|sync-crd-embed:' Makefile */Makefile 2>/dev/null || true

Repository: wandb/operator

Length of output: 50370


🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- managed and external status constructors ---'
sed -n '260,325p' internal/controller/reconciler/clickhouse.go

printf '%s\n' '--- replication tests ---'
sed -n '1,155p' internal/controller/reconciler/clickhouse_replication_test.go

printf '%s\n' '--- CRD schema exact subsection ---'
sed -n '4411,4562p' config/crd/bases/apps.wandb.com_weightsandbiases.yaml

printf '%s\n' '--- generated schema consistency ---'
sha256sum \
  config/crd/bases/apps.wandb.com_weightsandbiases.yaml \
  internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml
cmp -s \
  config/crd/bases/apps.wandb.com_weightsandbiases.yaml \
  internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml \
  && echo 'schema files identical' || echo 'schema files differ'

printf '%s\n' '--- Makefile targets ---'
rg -n -C 8 '^(lint|test|manifests|generate|sync-crd-embed):|manifests:|generate:|sync-crd-embed:' Makefile */Makefile 2>/dev/null || true

printf '%s\n' '--- tool availability ---'
command -v go || true
command -v make || true

Repository: wandb/operator

Length of output: 17814


🏁 Script executed:

#!/bin/bash
set -eu

cat >/tmp/omitempty_probe.go <<'EOF'
package main

import (
	"encoding/json"
	"fmt"
)

type BoolStatus struct {
	Replicated bool `json:"replicated,omitempty"`
}

type PointerStatus struct {
	Replicated *bool `json:"replicated,omitempty"`
}

func main() {
	f := false
	for _, value := range []any{
		BoolStatus{Replicated: false},
		BoolStatus{Replicated: true},
		PointerStatus{Replicated: nil},
		PointerStatus{Replicated: &f},
	} {
		data, err := json.Marshal(value)
		if err != nil {
			panic(err)
		}
		fmt.Println(string(data))
	}
}
EOF

if command -v go >/dev/null 2>&1; then
  go run /tmp/omitempty_probe.go
else
  python3 - <<'PY'
import json
print(json.dumps({}, separators=(",", ":")))
print(json.dumps({"replicated": True}, separators=(",", ":")))
print(json.dumps({}, separators=(",", ":")))
print(json.dumps({"replicated": False}, separators=(",", ":")))
PY
fi

Repository: wandb/operator

Length of output: 199


🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- replication defaults and downstream consumers ---'
rg -n -C 8 \
  'replicated|Replicated|defaultValue|ReplicatedMergeTree|clickhouse.*status|clickhouseStatus' \
  pkg internal hack --glob '*.go' --glob '*.yaml' --glob '*.json' | head -800

Repository: wandb/operator

Length of output: 50370


Preserve false for managed single-replica ClickHouse.

bool with omitempty omits the managed false value. Use *bool; set it for managed ClickHouse and leave it nil for external ClickHouse. Regenerate deepcopy and CRD artifacts, and add JSON tests for false, true, and omitted values.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/v2/weightsandbiases_types.go` at line 837, Change Replicated from bool to
*bool so managed single-replica ClickHouse preserves false while external
ClickHouse leaves it nil; update managed ClickHouse construction to set the
pointer and regenerate deepcopy and CRD artifacts. Add JSON coverage verifying
false, true, and omitted values.

ClusterName string `json:"clusterName,omitempty"`
}

type TelemetryInfraStatus struct {
Expand Down
4 changes: 4 additions & 0 deletions config/crd/bases/apps.wandb.com_weightsandbiases.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4413,6 +4413,8 @@ spec:
clickhouseStatus:
additionalProperties:
properties:
clusterName:
type: string
conditions:
items:
properties:
Expand Down Expand Up @@ -4545,6 +4547,8 @@ spec:
type: object
ready:
type: boolean
replicated:
type: boolean
state:
type: string
required:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ func (n *NsNameBuilder) ConnectionNsName() types.NamespacedName {
func createNsNameBuilder(baseNsName types.NamespacedName) *NsNameBuilder {
return CreateNsNameBuilder(baseNsName)
}
func CHIClusterName() string { return chiClusterName }

const (
// chiClusterName is the single cluster the CHI defines.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,39 @@
package altinity

import (
"context"
"strings"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
apiv2 "github.com/wandb/operator/api/v2"
"github.com/wandb/operator/pkg/wandb/manifest"
"k8s.io/apimachinery/pkg/util/validation"
)

var _ = Describe("managed ClickHouse naming", func() {
Describe("CHIClusterName", func() {
// Applications run ReplicatedMergeTree DDL as `ON CLUSTER <name>`, so this
// must be the cluster the CHI actually declares — not ClusterName(), which
// is the truncated spec name used to build the Service name.
It("matches the cluster the CHI declares", func() {
wandb := clickHouseWandb()
spec := wandb.Spec.ClickHouse[apiv2.DefaultInstanceName].ManagedClickHouse

chi, err := ToClickHouseVendorSpec(
context.Background(), wandb, spec, clickHouseScheme(),
testObjectStorageConn(), testObjectStorageEndpoint, true, manifest.Manifest{},
)
Expect(err).NotTo(HaveOccurred())
Expect(chi.Spec.Configuration.Clusters).To(HaveLen(1))
Expect(chi.Spec.Configuration.Clusters[0].Name).To(Equal(CHIClusterName()))
})

It("is not the Service-name derivation", func() {
Expect(CHIClusterName()).NotTo(Equal(ClusterName("wandb-clickhouse-chi")))
})
})

Describe("KeeperNsName", func() {
It("pairs the Keeper with the installation via the shared base name", func() {
spec := &apiv2.ManagedClickHouseSpec{Name: "wandb-legacy-overrides-v1-chi", Namespace: "wandb"}
Expand Down
3 changes: 3 additions & 0 deletions internal/controller/reconciler/clickhouse.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,9 @@ func managedClickHouseInferStatus(
for _, e := range events {
recorder.Event(wandb, e.Type, e.Reason, e.Message)
}
managed := wandb.Spec.ClickHouse[key].ManagedClickHouse
updatedStatus.Replicated = managed.Replicas > 1
updatedStatus.ClusterName = altinity.CHIClusterName()
wandb.Status.ClickHouseStatus[key] = updatedStatus
err := updateWandbStatusIfChanged(ctx, client, wandb, statusBefore)

Expand Down
134 changes: 134 additions & 0 deletions internal/controller/reconciler/clickhouse_replication_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
package reconciler

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use a Conventional Commit PR title.

fix/Weave-Clickhouse is not a Conventional Commit title. Rename it, for example, to fix: Weave ClickHouse.

As per coding guidelines, “PR titles and squash commits must use Conventional Commits with an allowed type, and the subject must start with an uppercase letter.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/reconciler/clickhouse_replication_test.go` at line 1,
Rename the pull request title from “fix/Weave-Clickhouse” to a Conventional
Commit format using an allowed type and an uppercase subject, such as “fix:
Weave ClickHouse”.

Source: Coding guidelines


import (
"context"
"testing"

apiv2 "github.com/wandb/operator/api/v2"
"github.com/wandb/operator/internal/controller/infra/managed/clickhouse/altinity"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/tools/record"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
)
Comment on lines +1 to +13

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Use the configured Ginkgo/Gomega test suite.

This new file uses testing.T tests. Move these cases into the existing Ginkgo suite and use Gomega assertions.

As per coding guidelines, “Use Ginkgo/Gomega for tests; test suites are configured through suite_test.go files and envtest.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/reconciler/clickhouse_replication_test.go` around lines 1
- 13, Convert the tests in clickhouse_replication_test.go from testing.T
functions to the existing Ginkgo suite configured by suite_test.go, removing the
direct testing import. Register the cases with Ginkgo and replace testing.T
assertions with Gomega matchers, preserving the existing test behavior and
envtest setup.

Source: Coding guidelines


func replicationTestCR(replicas int32) *apiv2.WeightsAndBiases {
return &apiv2.WeightsAndBiases{
TypeMeta: metav1.TypeMeta{APIVersion: apiv2.GroupVersion.String(), Kind: "WeightsAndBiases"},
ObjectMeta: metav1.ObjectMeta{Name: "wandb", Namespace: "wandb"},
Spec: apiv2.WeightsAndBiasesSpec{
ClickHouse: map[string]apiv2.ClickHouseSpec{
apiv2.DefaultInstanceName: {
ManagedClickHouse: &apiv2.ManagedClickHouseSpec{
Name: "wandb-chi",
Namespace: "wandb",
Replicas: replicas,
},
},
},
},
}
}

// inferManagedClickHouseStatus runs the managed status path and returns the
// published ClickHouse status for the default instance.
func inferManagedClickHouseStatus(t *testing.T, replicas int32) apiv2.ClickHouseInfraStatus {
t.Helper()

scheme := runtime.NewScheme()
if err := apiv2.AddToScheme(scheme); err != nil {
t.Fatalf("add apiv2 to scheme: %v", err)
}

wandb := replicationTestCR(replicas)
client := fake.NewClientBuilder().
WithScheme(scheme).
WithObjects(wandb).
WithStatusSubresource(wandb).
Build()

wandb.Status.ClickHouseStatus = map[string]apiv2.ClickHouseInfraStatus{}
if _, err := managedClickHouseInferStatus(
context.Background(), client, record.NewFakeRecorder(10),
wandb, apiv2.DefaultInstanceName, nil, &apiv2.ClickHouseConnection{},
); err != nil {
t.Fatalf("managedClickHouseInferStatus: %v", err)
}
return wandb.Status.ClickHouseStatus[apiv2.DefaultInstanceName]
}

// Weave creates ReplicatedMergeTree tables only when told the cluster is
// replicated; without this the operator provisions replicas that never receive
// writes.
func TestManagedClickHouseStatusPublishesReplication(t *testing.T) {
for _, tc := range []struct {
name string
replicas int32
replicated bool
}{
{"single replica is not replicated", 1, false},
{"two replicas are replicated", 2, true},
{"three replicas are replicated", 3, true},
} {
t.Run(tc.name, func(t *testing.T) {
status := inferManagedClickHouseStatus(t, tc.replicas)
if status.Replicated != tc.replicated {
t.Fatalf("expected Replicated=%t for %d replicas, got %t",
tc.replicated, tc.replicas, status.Replicated)
}
})
}
}

// The published name is used verbatim as `ON CLUSTER <name>`, so it has to be
// the cluster the CHI declares — not the Service-name derivation.
func TestManagedClickHouseStatusPublishesCHIClusterName(t *testing.T) {
status := inferManagedClickHouseStatus(t, 2)

if status.ClusterName != altinity.CHIClusterName() {
t.Fatalf("expected ClusterName=%q, got %q", altinity.CHIClusterName(), status.ClusterName)
}
if status.ClusterName == altinity.ClusterName("wandb-chi") {
t.Fatal("published the Service-name derivation instead of the CHI cluster name")
}
}

// External ClickHouse topology is unknown to the operator: the fields must stay
// zero so the server manifest's defaultValue applies.
func TestExternalClickHouseStatusLeavesReplicationUnset(t *testing.T) {
scheme := runtime.NewScheme()
if err := apiv2.AddToScheme(scheme); err != nil {
t.Fatalf("add apiv2 to scheme: %v", err)
}

wandb := &apiv2.WeightsAndBiases{
TypeMeta: metav1.TypeMeta{APIVersion: apiv2.GroupVersion.String(), Kind: "WeightsAndBiases"},
ObjectMeta: metav1.ObjectMeta{Name: "wandb", Namespace: "wandb"},
Spec: apiv2.WeightsAndBiasesSpec{
ClickHouse: map[string]apiv2.ClickHouseSpec{
apiv2.DefaultInstanceName: {ExternalClickHouse: &apiv2.ClickHouseConnection{}},
},
},
}
client := fake.NewClientBuilder().
WithScheme(scheme).
WithObjects(wandb).
WithStatusSubresource(wandb).
Build()

wandb.Status.ClickHouseStatus = map[string]apiv2.ClickHouseInfraStatus{}
if _, err := externalClickHouseInferStatus(
context.Background(), client, wandb, apiv2.DefaultInstanceName,
nil, &apiv2.ClickHouseConnection{},
); err != nil {
t.Fatalf("externalClickHouseInferStatus: %v", err)
}

status := wandb.Status.ClickHouseStatus[apiv2.DefaultInstanceName]
if status.Replicated {
t.Error("external ClickHouse must not be reported as replicated")
}
if status.ClusterName != "" {
t.Errorf("expected an empty ClusterName for external ClickHouse, got %q", status.ClusterName)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4413,6 +4413,8 @@ spec:
clickhouseStatus:
additionalProperties:
properties:
clusterName:
type: string
conditions:
items:
properties:
Expand Down Expand Up @@ -4545,6 +4547,8 @@ spec:
type: object
ready:
type: boolean
replicated:
type: boolean
state:
type: string
required:
Expand Down
Loading