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

chore: support resolving vars from multiple components consistently #8816

Merged
merged 2 commits into from
Jan 21, 2025
Merged

chore: support resolving vars from multiple components consistently #8816

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
f1017e3
chore: support resolving vars from multiple components consistently
leon-inf Jan 16, 2025
b1632cf
comments
leon-inf Jan 17, 2025
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
6 changes: 6 additions & 0 deletions apis/apps/v1/componentdefinition_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,12 @@ type ClusterObjectReference struct {

// MultipleClusterObjectOption defines the options for handling multiple cluster objects matched.
type MultipleClusterObjectOption struct {
// RequireAllComponentObjects controls whether all component objects must exist before resolving.
// If set to true, resolving will only proceed if all component objects are present.
//
// +optional
RequireAllComponentObjects *bool `json:"requireAllComponentObjects,omitempty"`

// Define the strategy for handling multiple cluster objects.
//
// +kubebuilder:validation:Required
Expand Down
5 changes: 5 additions & 0 deletions apis/apps/v1/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.

30 changes: 30 additions & 0 deletions config/crd/bases/apps.kubeblocks.io_componentdefinitions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16986,6 +16986,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down Expand Up @@ -17133,6 +17138,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down Expand Up @@ -17238,6 +17248,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down Expand Up @@ -17341,6 +17356,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down Expand Up @@ -17454,6 +17474,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down Expand Up @@ -17554,6 +17579,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down
30 changes: 30 additions & 0 deletions config/crd/bases/apps.kubeblocks.io_sidecardefinitions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1683,6 +1683,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down Expand Up @@ -1830,6 +1835,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down Expand Up @@ -1935,6 +1945,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down Expand Up @@ -2038,6 +2053,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down Expand Up @@ -2151,6 +2171,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down Expand Up @@ -2251,6 +2276,11 @@ spec:
components.
type: string
type: object
requireAllComponentObjects:
description: |-
RequireAllComponentObjects controls whether all component objects must exist before resolving.
If set to true, resolving will only proceed if all component objects are present.
type: boolean
strategy:
description: Define the strategy for handling multiple
cluster objects.
Expand Down
60 changes: 56 additions & 4 deletions controllers/apps/shardingdefinition_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,21 +190,73 @@ func (r *ShardingDefinitionReconciler) validateShardsLimit(ctx context.Context,

func (r *ShardingDefinitionReconciler) validateProvisionNUpdateStrategy(ctx context.Context, cli client.Client,
shardingDef *appsv1.ShardingDefinition) error {
var (
provision = shardingDef.Spec.ProvisionStrategy
update = shardingDef.Spec.UpdateStrategy
)

supported := func(strategy *appsv1.UpdateStrategy) bool {
if strategy == nil {
return true
}
return *strategy == appsv1.SerialStrategy || *strategy == appsv1.ParallelStrategy
}
if !supported(shardingDef.Spec.ProvisionStrategy) {
return fmt.Errorf("unsupported provision strategy: %s", *shardingDef.Spec.ProvisionStrategy)
if !supported(provision) {
return fmt.Errorf("unsupported provision strategy: %s", *provision)
}
if !supported(update) {
return fmt.Errorf("unsupported update strategy: %s", *update)
}
if !supported(shardingDef.Spec.UpdateStrategy) {
return fmt.Errorf("unsupported update strategy: %s", *shardingDef.Spec.UpdateStrategy)

if provision != nil && *provision == appsv1.SerialStrategy && r.requireParallelProvision() {
return fmt.Errorf("serial provision strategy is conflicted with vars that requires parallel provision when mutiple objects matched")
}
return nil
}

// requireParallelProvision checks whether the provision strategy must be parallel.
//
// If any Vars in the ShardingDefinition have requireAllComponentObjects set to true,
// all sharding components must exist before Vars resolving can proceed. This requirement
// conflicts with a serial provision strategy, where components are created one at a time,
// potentially leading to a logical deadlock.
func (r *ShardingDefinitionReconciler) requireParallelProvision() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some comments to explain why parallel provisioning is needed when some of the RequireAllComponentObjects fields are set to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

requireAll := func(opt *appsv1.MultipleClusterObjectOption) bool {
return opt != nil && opt.RequireAllComponentObjects != nil && *opt.RequireAllComponentObjects
}
require := func(v appsv1.EnvVar) bool {
if v.ValueFrom != nil {
if v.ValueFrom.HostNetworkVarRef != nil {
return requireAll(v.ValueFrom.HostNetworkVarRef.MultipleClusterObjectOption)
}
if v.ValueFrom.ServiceVarRef != nil {
return requireAll(v.ValueFrom.ServiceVarRef.MultipleClusterObjectOption)
}
if v.ValueFrom.CredentialVarRef != nil {
return requireAll(v.ValueFrom.CredentialVarRef.MultipleClusterObjectOption)
}
if v.ValueFrom.TLSVarRef != nil {
return requireAll(v.ValueFrom.TLSVarRef.MultipleClusterObjectOption)
}
if v.ValueFrom.ServiceRefVarRef != nil {
return requireAll(v.ValueFrom.ServiceRefVarRef.MultipleClusterObjectOption)
}
if v.ValueFrom.ComponentVarRef != nil {
return requireAll(v.ValueFrom.ComponentVarRef.MultipleClusterObjectOption)
}
}
return false
}
for _, compDef := range r.compDefs {
for _, v := range compDef.Spec.Vars {
if require(v) {
return true
}
}
}
return false
}

func (r *ShardingDefinitionReconciler) validateLifecycleActions(ctx context.Context, cli client.Client,
shardingDef *appsv1.ShardingDefinition) error {
return nil
Expand Down
80 changes: 77 additions & 3 deletions controllers/apps/shardingdefinition_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

"k8s.io/utils/pointer"
"k8s.io/utils/ptr"
"sigs.k8s.io/controller-runtime/pkg/client"

appsv1 "github.com/apecloud/kubeblocks/apis/apps/v1"
Expand Down Expand Up @@ -131,13 +131,87 @@ var _ = Describe("ShardingDefinition Controller", func() {

checkObjectStatus(shardingDefObj, appsv1.UnavailablePhase)
})

It("provision strategy & vars - serial", func() {
By("create a ShardingDefinition obj")
shardingDefObj := testapps.NewShardingDefinitionFactory(shardingDefName, compDefObj.GetName()).
SetProvisionStrategy(appsv1.SerialStrategy).
SetUpdateStrategy(appsv1.SerialStrategy).
Create(&testCtx).GetObject()

checkObjectStatus(shardingDefObj, appsv1.AvailablePhase)
})

It("provision strategy & vars - parallel & all", func() {
By("add a var in cmpd")
Expect(testapps.GetAndChangeObj(&testCtx, client.ObjectKeyFromObject(compDefObj), func(compDef *appsv1.ComponentDefinition) {
compDef.Spec.Vars = []appsv1.EnvVar{
{
Name: "test",
ValueFrom: &appsv1.VarSource{
ComponentVarRef: &appsv1.ComponentVarSelector{
ClusterObjectReference: appsv1.ClusterObjectReference{
MultipleClusterObjectOption: &appsv1.MultipleClusterObjectOption{
RequireAllComponentObjects: ptr.To(true),
Strategy: appsv1.MultipleClusterObjectStrategyCombined,
},
},
ComponentVars: appsv1.ComponentVars{
PodFQDNs: &appsv1.VarRequired,
},
},
},
},
}
})()).Should(Succeed())

By("create a ShardingDefinition obj")
shardingDefObj := testapps.NewShardingDefinitionFactory(shardingDefName, compDefObj.GetName()).
SetProvisionStrategy(appsv1.ParallelStrategy).
SetUpdateStrategy(appsv1.ParallelStrategy).
Create(&testCtx).GetObject()

checkObjectStatus(shardingDefObj, appsv1.AvailablePhase)
})

It("provision strategy & vars - serial & all", func() {
By("add a var in cmpd")
Expect(testapps.GetAndChangeObj(&testCtx, client.ObjectKeyFromObject(compDefObj), func(compDef *appsv1.ComponentDefinition) {
compDef.Spec.Vars = []appsv1.EnvVar{
{
Name: "test",
ValueFrom: &appsv1.VarSource{
ComponentVarRef: &appsv1.ComponentVarSelector{
ClusterObjectReference: appsv1.ClusterObjectReference{
MultipleClusterObjectOption: &appsv1.MultipleClusterObjectOption{
RequireAllComponentObjects: ptr.To(true),
Strategy: appsv1.MultipleClusterObjectStrategyCombined,
},
},
ComponentVars: appsv1.ComponentVars{
PodFQDNs: &appsv1.VarRequired,
},
},
},
},
}
})()).Should(Succeed())

By("create a ShardingDefinition obj")
shardingDefObj := testapps.NewShardingDefinitionFactory(shardingDefName, compDefObj.GetName()).
SetProvisionStrategy(appsv1.SerialStrategy).
SetUpdateStrategy(appsv1.SerialStrategy).
Create(&testCtx).GetObject()

checkObjectStatus(shardingDefObj, appsv1.UnavailablePhase)
})
})

Context("system accounts", func() {
It("ok", func() {
By("create a ShardingDefinition obj")
shardingDefObj := testapps.NewShardingDefinitionFactory(shardingDefName, compDefObj.GetName()).
AddSystemAccount(appsv1.ShardingSystemAccount{Name: adminAccount, Shared: pointer.Bool(true)}).
AddSystemAccount(appsv1.ShardingSystemAccount{Name: adminAccount, Shared: ptr.To(true)}).
Create(&testCtx).GetObject()

checkObjectStatus(shardingDefObj, appsv1.AvailablePhase)
Expand All @@ -147,7 +221,7 @@ var _ = Describe("ShardingDefinition Controller", func() {
By("create a ShardingDefinition obj")
shardingDefObj := testapps.NewShardingDefinitionFactory(shardingDefName, compDefObj.GetName()).
AddSystemAccount(appsv1.ShardingSystemAccount{Name: adminAccount}).
AddSystemAccount(appsv1.ShardingSystemAccount{Name: adminAccount, Shared: pointer.Bool(true)}).
AddSystemAccount(appsv1.ShardingSystemAccount{Name: adminAccount, Shared: ptr.To(true)}).
Create(&testCtx).GetObject()

checkObjectStatus(shardingDefObj, appsv1.UnavailablePhase)
Expand Down
Loading
Loading