diff --git a/pkg/apis/clickhouse-keeper.altinity.com/v1/type_cluster.go b/pkg/apis/clickhouse-keeper.altinity.com/v1/type_cluster.go index 301287826..e73e0acf0 100644 --- a/pkg/apis/clickhouse-keeper.altinity.com/v1/type_cluster.go +++ b/pkg/apis/clickhouse-keeper.altinity.com/v1/type_cluster.go @@ -163,7 +163,7 @@ func (cluster *Cluster) isShardExplicitlySpecified() bool { // isReplicaExplicitlySpecified checks whether replica is explicitly specified func (cluster *Cluster) isReplicaExplicitlySpecified() bool { - return cluster.Layout.ReplicasExplicitlySpecified && !cluster.isShardExplicitlySpecified() + return cluster.Layout.ReplicasExplicitlySpecified } // IsShardSpecified checks whether shard is explicitly specified @@ -172,6 +172,12 @@ func (cluster *Cluster) isShardToBeUsedToInheritSettingsFrom() bool { return true } + // When both shards and replicas are explicitly specified, prefer replicas + // since replica-level configuration is more specific than shard-level + if cluster.isShardExplicitlySpecified() && cluster.isReplicaExplicitlySpecified() { + return false + } + return cluster.isShardExplicitlySpecified() } diff --git a/pkg/apis/clickhouse-keeper.altinity.com/v1/type_cluster_test.go b/pkg/apis/clickhouse-keeper.altinity.com/v1/type_cluster_test.go new file mode 100644 index 000000000..440fdb496 --- /dev/null +++ b/pkg/apis/clickhouse-keeper.altinity.com/v1/type_cluster_test.go @@ -0,0 +1,133 @@ +package v1 + +import ( + "testing" + + "github.com/stretchr/testify/require" + + apiChi "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" +) + +// Test_KeeperClusterSettingsSource_PreferReplicaOverShard tests that when both +// shard and replica settings are explicitly specified, replica settings +// take precedence as they are more fine-grained control points. +func Test_KeeperClusterSettingsSource_PreferReplicaOverShard(t *testing.T) { + testCases := []struct { + name string + shardsExplicit bool + replicasExplicit bool + expectShardAsSource bool + description string + }{ + { + name: "neither_shards_nor_replicas_explicit", + shardsExplicit: false, + replicasExplicit: false, + expectShardAsSource: true, + description: "When neither shards nor replicas are explicitly specified, use shard as settings source", + }, + { + name: "only_shards_explicit", + shardsExplicit: true, + replicasExplicit: false, + expectShardAsSource: true, + description: "When only shards are explicitly specified, use shard as settings source", + }, + { + name: "only_replicas_explicit", + shardsExplicit: false, + replicasExplicit: true, + expectShardAsSource: false, + description: "When only replicas are explicitly specified, use replica as settings source", + }, + { + name: "both_shards_and_replicas_explicit", + shardsExplicit: true, + replicasExplicit: true, + expectShardAsSource: false, + description: "When both shards and replicas are explicitly specified, prefer replica settings as they are more fine-grained", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(tt *testing.T) { + cluster := &Cluster{ + Layout: &ChkClusterLayout{ + ShardsExplicitlySpecified: tc.shardsExplicit, + ReplicasExplicitlySpecified: tc.replicasExplicit, + }, + } + + // Test isShardToBeUsedToInheritSettingsFrom + actualShardSource := cluster.isShardToBeUsedToInheritSettingsFrom() + require.Equal(tt, tc.expectShardAsSource, actualShardSource, + "%s: isShardToBeUsedToInheritSettingsFrom() returned %v, expected %v", + tc.description, actualShardSource, tc.expectShardAsSource) + + // Test SelectSettingsSourceFrom + shard := &apiChi.ChiShard{Name: "test-shard"} + replica := &apiChi.ChiReplica{Name: "test-replica"} + + src := cluster.SelectSettingsSourceFrom(shard, replica) + if tc.expectShardAsSource { + require.Equal(tt, shard, src, + "%s: SelectSettingsSourceFrom() should return shard", tc.description) + } else { + require.Equal(tt, replica, src, + "%s: SelectSettingsSourceFrom() should return replica", tc.description) + } + }) + } +} + +// Test_KeeperClusterReplicaExplicitlySpecified tests that isReplicaExplicitlySpecified +// returns true whenever replicas are explicitly specified, regardless of shard specification +func Test_KeeperClusterReplicaExplicitlySpecified(t *testing.T) { + testCases := []struct { + name string + shardsExplicit bool + replicasExplicit bool + expected bool + }{ + { + name: "replicas_not_explicit", + shardsExplicit: false, + replicasExplicit: false, + expected: false, + }, + { + name: "replicas_explicit_shards_not", + shardsExplicit: false, + replicasExplicit: true, + expected: true, + }, + { + name: "replicas_explicit_shards_explicit", + shardsExplicit: true, + replicasExplicit: true, + expected: true, + }, + { + name: "replicas_not_explicit_shards_explicit", + shardsExplicit: true, + replicasExplicit: false, + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(tt *testing.T) { + cluster := &Cluster{ + Layout: &ChkClusterLayout{ + ShardsExplicitlySpecified: tc.shardsExplicit, + ReplicasExplicitlySpecified: tc.replicasExplicit, + }, + } + + actual := cluster.isReplicaExplicitlySpecified() + require.Equal(tt, tc.expected, actual, + "isReplicaExplicitlySpecified() returned %v, expected %v", + actual, tc.expected) + }) + } +} diff --git a/pkg/apis/clickhouse.altinity.com/v1/type_cluster.go b/pkg/apis/clickhouse.altinity.com/v1/type_cluster.go index cebf8a6ec..3f267b65a 100644 --- a/pkg/apis/clickhouse.altinity.com/v1/type_cluster.go +++ b/pkg/apis/clickhouse.altinity.com/v1/type_cluster.go @@ -172,7 +172,7 @@ func (cluster *Cluster) isShardExplicitlySpecified() bool { // isReplicaExplicitlySpecified checks whether replica is explicitly specified func (cluster *Cluster) isReplicaExplicitlySpecified() bool { - return cluster.Layout.ReplicasExplicitlySpecified && !cluster.isShardExplicitlySpecified() + return cluster.Layout.ReplicasExplicitlySpecified } // IsShardSpecified checks whether shard is explicitly specified @@ -181,6 +181,12 @@ func (cluster *Cluster) isShardToBeUsedToInheritSettingsFrom() bool { return true } + // When both shards and replicas are explicitly specified, prefer replicas + // since replica-level configuration is more specific than shard-level + if cluster.isShardExplicitlySpecified() && cluster.isReplicaExplicitlySpecified() { + return false + } + return cluster.isShardExplicitlySpecified() } diff --git a/pkg/apis/clickhouse.altinity.com/v1/type_cluster_test.go b/pkg/apis/clickhouse.altinity.com/v1/type_cluster_test.go new file mode 100644 index 000000000..65fdd17a2 --- /dev/null +++ b/pkg/apis/clickhouse.altinity.com/v1/type_cluster_test.go @@ -0,0 +1,131 @@ +package v1 + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// Test_ClusterSettingsSource_PreferReplicaOverShard tests that when both +// shard and replica settings are explicitly specified, replica settings +// take precedence as they are more fine-grained control points. +func Test_ClusterSettingsSource_PreferReplicaOverShard(t *testing.T) { + testCases := []struct { + name string + shardsExplicit bool + replicasExplicit bool + expectShardAsSource bool + description string + }{ + { + name: "neither_shards_nor_replicas_explicit", + shardsExplicit: false, + replicasExplicit: false, + expectShardAsSource: true, + description: "When neither shards nor replicas are explicitly specified, use shard as settings source", + }, + { + name: "only_shards_explicit", + shardsExplicit: true, + replicasExplicit: false, + expectShardAsSource: true, + description: "When only shards are explicitly specified, use shard as settings source", + }, + { + name: "only_replicas_explicit", + shardsExplicit: false, + replicasExplicit: true, + expectShardAsSource: false, + description: "When only replicas are explicitly specified, use replica as settings source", + }, + { + name: "both_shards_and_replicas_explicit", + shardsExplicit: true, + replicasExplicit: true, + expectShardAsSource: false, + description: "When both shards and replicas are explicitly specified, prefer replica settings as they are more fine-grained", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(tt *testing.T) { + cluster := &Cluster{ + Layout: &ChiClusterLayout{ + ShardsExplicitlySpecified: tc.shardsExplicit, + ReplicasExplicitlySpecified: tc.replicasExplicit, + }, + } + + // Test isShardToBeUsedToInheritSettingsFrom + actualShardSource := cluster.isShardToBeUsedToInheritSettingsFrom() + require.Equal(tt, tc.expectShardAsSource, actualShardSource, + "%s: isShardToBeUsedToInheritSettingsFrom() returned %v, expected %v", + tc.description, actualShardSource, tc.expectShardAsSource) + + // Test SelectSettingsSourceFrom + shard := &ChiShard{Name: "test-shard"} + replica := &ChiReplica{Name: "test-replica"} + + src := cluster.SelectSettingsSourceFrom(shard, replica) + if tc.expectShardAsSource { + require.Equal(tt, shard, src, + "%s: SelectSettingsSourceFrom() should return shard", tc.description) + } else { + require.Equal(tt, replica, src, + "%s: SelectSettingsSourceFrom() should return replica", tc.description) + } + }) + } +} + +// Test_ClusterReplicaExplicitlySpecified tests that isReplicaExplicitlySpecified +// returns true whenever replicas are explicitly specified, regardless of shard specification +func Test_ClusterReplicaExplicitlySpecified(t *testing.T) { + testCases := []struct { + name string + shardsExplicit bool + replicasExplicit bool + expected bool + }{ + { + name: "replicas_not_explicit", + shardsExplicit: false, + replicasExplicit: false, + expected: false, + }, + { + name: "replicas_explicit_shards_not", + shardsExplicit: false, + replicasExplicit: true, + expected: true, + }, + { + name: "replicas_explicit_shards_explicit", + shardsExplicit: true, + replicasExplicit: true, + expected: true, + }, + { + name: "replicas_not_explicit_shards_explicit", + shardsExplicit: true, + replicasExplicit: false, + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(tt *testing.T) { + cluster := &Cluster{ + Layout: &ChiClusterLayout{ + ShardsExplicitlySpecified: tc.shardsExplicit, + ReplicasExplicitlySpecified: tc.replicasExplicit, + }, + } + + actual := cluster.isReplicaExplicitlySpecified() + require.Equal(tt, tc.expected, actual, + "isReplicaExplicitlySpecified() returned %v, expected %v", + actual, tc.expected) + }) + } +} diff --git a/pkg/apis/clickhouse.altinity.com/v1/type_status_test.go b/pkg/apis/clickhouse.altinity.com/v1/type_status_test.go index 1f1c3cc09..becfef083 100644 --- a/pkg/apis/clickhouse.altinity.com/v1/type_status_test.go +++ b/pkg/apis/clickhouse.altinity.com/v1/type_status_test.go @@ -5,6 +5,8 @@ import ( "github.com/stretchr/testify/require" "sync" "testing" + + "github.com/altinity/clickhouse-operator/pkg/apis/common/types" ) var normalizedChiA = &ClickHouseInstallation{} @@ -158,12 +160,14 @@ func Test_ChiStatus_BasicOperations_SingleStatus_ConcurrencyTest(t *testing.T) { name: "CopyFrom", goRoutineA: func(s *Status) { s.PushAction("always-present-action") // CopyFrom preserves existing actions (does not clobber) - s.CopyFrom(copyTestStatusFrom, CopyStatusOptions{ - Actions: true, - Errors: true, - MainFields: true, - WholeStatus: true, - InheritableFields: true, + s.CopyFrom(copyTestStatusFrom, types.CopyStatusOptions{ + CopyStatusFieldGroup: types.CopyStatusFieldGroup{ + FieldGroupActions: true, + FieldGroupErrors: true, + FieldGroupMain: true, + FieldGroupWholeStatus: true, + FieldGroupInheritable: true, + }, }) }, goRoutineB: func(s *Status) { @@ -171,7 +175,6 @@ func Test_ChiStatus_BasicOperations_SingleStatus_ConcurrencyTest(t *testing.T) { }, postConditionsVerification: func(tt *testing.T, s *Status) { if len(s.GetActions()) == len(copyTestStatusFrom.GetActions())+2 { - require.Equal(tt, copyTestStatusFrom.GetActions(), s.GetActions()) require.Contains(tt, s.GetActions(), "always-present-action") require.Contains(tt, s.GetActions(), "additional-action") for _, action := range copyTestStatusFrom.GetActions() { @@ -179,7 +182,7 @@ func Test_ChiStatus_BasicOperations_SingleStatus_ConcurrencyTest(t *testing.T) { } } else { require.Equal(tt, len(copyTestStatusFrom.GetActions())+1, len(s.GetActions())) - require.Contains(tt, s.GetActions(), "additional-action") + require.Contains(tt, s.GetActions(), "always-present-action") for _, action := range copyTestStatusFrom.GetActions() { require.Contains(tt, s.GetActions(), action) }