Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
18 changes: 15 additions & 3 deletions pkg/controller/lifecycle/kbagent.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ type lifecycleAction interface {
parameters(ctx context.Context, cli client.Reader) (map[string]string, error)
}

// authoritativeActionParameters marks action parameters that are defined by
// the runtime contract and therefore cannot be overridden by template vars.
type authoritativeActionParameters interface {
authoritativeActionParameters()
}

type kbagent struct {
namespace string
clusterName string
Expand Down Expand Up @@ -302,11 +308,17 @@ func (a *kbagent) parameters(ctx context.Context, cli client.Reader, lfa lifecyc
return nil, err
}

for k, v := range sys {
// template vars take precedence
if _, ok := m[k]; !ok {
if _, authoritative := lfa.(authoritativeActionParameters); authoritative {
Comment thread
weicao marked this conversation as resolved.
for k, v := range sys {
m[k] = v

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P1] This authoritative assignment can still be undone by addShellSafeParameterAliases below. That helper overwrites an existing alias when its value is empty. For example, a legal template variable kb_switchover_candidate_name=pod-x is converted to KB_SWITCHOVER_CANDIDATE_NAME; when automatic candidate selection intentionally produced the authoritative empty value, the alias pass replaces it with pod-x and changes the request into an explicit-candidate switchover. Empty candidate and role values must remain authoritative through the complete parameter composition pipeline.

}
} else {
for k, v := range sys {
// template vars take precedence for existing lifecycle actions.
if _, ok := m[k]; !ok {
m[k] = v
}
}
}
addShellSafeParameterAliases(m)
return m, nil
Expand Down
74 changes: 73 additions & 1 deletion pkg/controller/lifecycle/kbagent_parameters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.

package lifecycle

import "testing"
import (
"context"
"testing"
)

func TestAddShellSafeParameterAliasesPreservesRawParameter(t *testing.T) {
parameters := map[string]string{
Expand Down Expand Up @@ -75,3 +78,72 @@ func TestShellSafeParameterAlias(t *testing.T) {
}
}
}

func TestSwitchoverRuntimeParametersOverrideTemplateVars(t *testing.T) {
action := &kbagent{
templateVars: map[string]string{
switchoverCurrentName: "spoofed-current",
switchoverCurrentFQDN: "spoofed-current-fqdn",
switchoverCandidateName: "spoofed-candidate",
switchoverCandidateFQDN: "spoofed-candidate-fqdn",
switchoverRole: "spoofed-role",
"USER_DEFINED": "preserved",
},
}
runtime := &switchover{
namespace: "default",
clusterName: "demo",
compName: "redis",
role: "primary",
currentPod: "demo-redis-0",
candidatePod: "demo-redis-1",
}

parameters, err := action.parameters(context.Background(), nil, runtime)
if err != nil {
t.Fatalf("parameters returned error: %v", err)
}
want := map[string]string{
switchoverCurrentName: "demo-redis-0",
switchoverCurrentFQDN: "demo-redis-0.demo-redis-headless.default.svc.cluster.local",
switchoverCandidateName: "demo-redis-1",
switchoverCandidateFQDN: "demo-redis-1.demo-redis-headless.default.svc.cluster.local",
switchoverRole: "primary",
"USER_DEFINED": "preserved",
}
for key, expected := range want {
if got := parameters[key]; got != expected {
t.Fatalf("parameter %s=%q, want %q", key, got, expected)
}
}
}

func TestSwitchoverEmptyCandidateOverridesTemplateVars(t *testing.T) {
action := &kbagent{
templateVars: map[string]string{
switchoverCandidateName: "spoofed-candidate",
switchoverCandidateFQDN: "spoofed-candidate-fqdn",
},
}
runtime := &switchover{
namespace: "default",
clusterName: "demo",
compName: "redis",
role: "primary",
currentPod: "demo-redis-0",
}

parameters, err := action.parameters(context.Background(), nil, runtime)
if err != nil {
t.Fatalf("parameters returned error: %v", err)
}
for _, key := range []string{switchoverCandidateName, switchoverCandidateFQDN} {
value, ok := parameters[key]
if !ok {
t.Fatalf("authoritative empty parameter %s is missing", key)
}
if value != "" {
t.Fatalf("parameter %s=%q, want authoritative empty value", key, value)
}
}
}
7 changes: 6 additions & 1 deletion pkg/controller/lifecycle/lfa_member.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,14 @@ func (a *switchover) name() string {
return "switchover"
}

func (a *switchover) authoritativeActionParameters() {}

func (a *switchover) parameters(ctx context.Context, cli client.Reader) (map[string]string, error) {
// refer to ComponentLifecycleActions.Switchover's documentation for explanation of each variable.
m := make(map[string]string)
m := map[string]string{
switchoverCandidateName: "",
switchoverCandidateFQDN: "",
}
compName := constant.GenerateClusterComponentName(a.clusterName, a.compName)
if len(a.candidatePod) > 0 {
m[switchoverCandidateName] = a.candidatePod
Expand Down
46 changes: 14 additions & 32 deletions pkg/operations/ops_runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"

appsv1 "github.com/apecloud/kubeblocks/apis/apps/v1"
opsv1alpha1 "github.com/apecloud/kubeblocks/apis/operations/v1alpha1"
workloads "github.com/apecloud/kubeblocks/apis/workloads/v1"
"github.com/apecloud/kubeblocks/pkg/constant"
"github.com/apecloud/kubeblocks/pkg/controller/component"
Expand Down Expand Up @@ -191,69 +190,52 @@ func (r *opsRuntime) GenerateTemplateInstanceNames(clusterName, compName, templa
return instanceset.GenerateInstanceNamesFromTemplate(workloadName, templateName, replicas, offlineInstances, ordinalList)
}

func (r *opsRuntime) Switchover(ctx context.Context, namespace, clusterName, compName, instanceName, candidateName string) error {
synthesizedComp, err := r.buildSynthesizedCompByCompName(ctx, r.cli, namespace, clusterName, compName)
if err != nil {
return err
}
switchover := &opsv1alpha1.Switchover{
ComponentName: compName,
InstanceName: instanceName,
CandidateName: candidateName,
}
return r.doSwitchover(ctx, r.cli, synthesizedComp, switchover)
}

func (r *opsRuntime) buildSynthesizedCompByCompName(ctx context.Context, cli client.Client, namespace, clusterName, compName string) (*component.SynthesizedComponent, error) {
compObj, compDefObj, err := component.GetCompNCompDefByName(ctx, cli, namespace, constant.GenerateClusterComponentName(clusterName, compName))
if err != nil {
return nil, err
}
return component.BuildSynthesizedComponent(ctx, cli, compDefObj, compObj)
func (r *opsRuntime) Switchover(ctx context.Context, actionCtx SwitchoverActionContext, instanceName, candidateName string) error {
return r.doSwitchover(ctx, r.cli, actionCtx, instanceName, candidateName)
}

// We consider a switchover action succeeds if the action returns without error.
// We don't need to know if a switchover is actually executed.
func (r *opsRuntime) doSwitchover(ctx context.Context, cli client.Reader, synthesizedComp *component.SynthesizedComponent,
switchover *opsv1alpha1.Switchover) error {
pods, err := component.ListOwnedPods(r.dataContext(), cli, synthesizedComp.Namespace, synthesizedComp.ClusterName, synthesizedComp.Name, r.dataListOpts...)
func (r *opsRuntime) doSwitchover(ctx context.Context, cli client.Reader, actionCtx SwitchoverActionContext,
Comment thread
weicao marked this conversation as resolved.
instanceName, candidateName string) error {
pods, err := component.ListOwnedPods(r.dataContext(), cli, actionCtx.Namespace, actionCtx.ClusterName, actionCtx.ComponentName, r.dataListOpts...)
if err != nil {
return err
}

pod := &corev1.Pod{}
for _, p := range pods {
if p.Name == switchover.InstanceName {
if p.Name == instanceName {
pod = p
break
}
}
if pod.Name == "" {
return intctrlutil.NewFatalError(fmt.Sprintf(`instance "%s" not found`, switchover.InstanceName))
return intctrlutil.NewFatalError(fmt.Sprintf(`instance "%s" not found`, instanceName))
}
if switchover.CandidateName != "" {
candidate, err := r.GetInstance(synthesizedComp.Namespace, synthesizedComp.ClusterName, synthesizedComp.Name, switchover.CandidateName)
if candidateName != "" {
candidate, err := r.GetInstance(actionCtx.Namespace, actionCtx.ClusterName, actionCtx.ComponentName, candidateName)
if err != nil {
if apierrors.IsNotFound(err) {
return intctrlutil.NewFatalError(fmt.Sprintf(`candidate instance "%s" not found`, switchover.CandidateName))
return intctrlutil.NewFatalError(fmt.Sprintf(`candidate instance "%s" not found`, candidateName))
}
return err
}
if !candidate.HasPod() {
return intctrlutil.NewFatalError(fmt.Sprintf(`candidate instance "%s" not found`, switchover.CandidateName))
return intctrlutil.NewFatalError(fmt.Sprintf(`candidate instance "%s" not found`, candidateName))
}
}

lfa, err := lifecycle.New(synthesizedComp.Namespace, synthesizedComp.ClusterName, synthesizedComp.Name,
synthesizedComp.LifecycleActions.ComponentLifecycleActions, synthesizedComp.TemplateVars, pod, pods)
lfa, err := lifecycle.New(actionCtx.Namespace, actionCtx.ClusterName, actionCtx.ComponentName,
actionCtx.LifecycleActions, actionCtx.TemplateVars, pod, pods)
if err != nil {
return err
}

// NOTE: switchover is a blocking action currently. May change to non-blocking for better performance.
// Lifecycle preconditions still use the lifecycle reader contract as-is. If a multi-cluster
// action needs data-plane runtime readiness checks, model that explicitly in the lifecycle API.
return lfa.Switchover(ctx, cli, nil, switchover.CandidateName)
return lfa.Switchover(ctx, cli, nil, candidateName)
}

func (r *opsRuntime) buildInstances(namespace, clusterName, compName string, pods []*corev1.Pod) ([]Instance, error) {
Expand Down
9 changes: 8 additions & 1 deletion pkg/operations/switchover.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,14 @@ func handleSwitchover(reqCtx intctrlutil.RequestCtx, cli client.Client, opsRes *

switch progressDetail.Status {
case opsv1alpha1.PendingProgressStatus:
if err = runtime.Switchover(reqCtx.Ctx, synthesizedComp.Namespace, synthesizedComp.ClusterName, synthesizedComp.Name, switchover.InstanceName, switchover.CandidateName); err != nil {
actionCtx := SwitchoverActionContext{
Namespace: synthesizedComp.Namespace,
ClusterName: synthesizedComp.ClusterName,
ComponentName: synthesizedComp.Name,
LifecycleActions: synthesizedComp.LifecycleActions.ComponentLifecycleActions,
TemplateVars: synthesizedComp.TemplateVars,
Comment thread
weicao marked this conversation as resolved.
}
if err = runtime.Switchover(reqCtx.Ctx, actionCtx, switchover.InstanceName, switchover.CandidateName); err != nil {
progressDetail.Status = opsv1alpha1.FailedProgressStatus
progressDetail.Message = fmt.Sprintf("component %s %s", compName, err.Error())
} else {
Expand Down
5 changes: 5 additions & 0 deletions pkg/operations/switchover_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -660,10 +660,12 @@ var _ = Describe("", func() {
ops := testops.NewOpsRequestObj("ops-switchover-"+testCtx.GetRandomStr(), testCtx.DefaultNamespace,
shardingCluster.Name, opsv1alpha1.SwitchoverType)
instanceName := fmt.Sprintf("%s-%d", its.Name, 1)
candidateName := fmt.Sprintf("%s-%d", its.Name, 0)
ops.Spec.SwitchoverList = []opsv1alpha1.Switchover{
{
ComponentName: shardingName,
InstanceName: instanceName,
CandidateName: candidateName,
},
}
opsRes.OpsRequest = testops.CreateOpsRequest(ctx, testCtx, ops)
Expand All @@ -685,6 +687,9 @@ var _ = Describe("", func() {
recorder.Action(gomock.Any(), gomock.Any()).Times(1).DoAndReturn(func(ctx context.Context, req kbagentproto.ActionRequest) (kbagentproto.ActionResponse, error) {
GinkgoWriter.Printf("ActionRequest: %#v\n", req)
Expect(req.Parameters["KB_SWITCHOVER_CURRENT_NAME"]).Should(Equal(instanceName))
Expect(req.Parameters["KB_SWITCHOVER_CANDIDATE_NAME"]).Should(Equal(candidateName))
Expect(req.Parameters["KB_SWITCHOVER_CURRENT_FQDN"]).Should(ContainSubstring(shardingCluster.Name + "-" + shardComponentName + "-headless"))
Expect(req.Parameters["KB_SWITCHOVER_CANDIDATE_FQDN"]).Should(ContainSubstring(shardingCluster.Name + "-" + shardComponentName + "-headless"))
rsp := kbagentproto.ActionResponse{Message: "mock success"}
return rsp, nil
})
Expand Down
13 changes: 12 additions & 1 deletion pkg/operations/type.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,18 @@ type OpsRuntime interface {
ListInstances(namespace, clusterName, compName string) ([]Instance, error)
GenerateInstanceNameSet(clusterName, compName string, compReplicas int32, instances []appsv1.InstanceTemplate, offlineInstances []string) (map[string]string, error)
GenerateTemplateInstanceNames(clusterName, compName, templateName string, replicas int32, offlineInstances []string, ordinals appsv1.Ordinals) ([]string, error)
Switchover(ctx context.Context, namespace, clusterName, compName, instanceName, candidateName string) error
Switchover(ctx context.Context, actionCtx SwitchoverActionContext, instanceName, candidateName string) error
}

// SwitchoverActionContext is the resolved, stable input needed to dispatch a
// switchover lifecycle action. It deliberately excludes the controller's
// synthesized component implementation details.
type SwitchoverActionContext struct {
Namespace string
ClusterName string
ComponentName string
LifecycleActions *appsv1.ComponentLifecycleActions
TemplateVars map[string]string
}

type Workload interface {
Expand Down
Loading