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
9 changes: 8 additions & 1 deletion pkg/controllers/v1alpha1/databackup/status_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (

"github.com/fluid-cloudnative/fluid/api/v1alpha1"
"github.com/fluid-cloudnative/fluid/pkg/common"
"github.com/fluid-cloudnative/fluid/pkg/dataflow"
"github.com/fluid-cloudnative/fluid/pkg/dataoperation"
"github.com/fluid-cloudnative/fluid/pkg/runtime"
"github.com/fluid-cloudnative/fluid/pkg/utils"
Expand Down Expand Up @@ -52,7 +53,13 @@ func (o *OnceHandler) GetOperationStatus(ctx runtime.ReconcileRequestContext, op
return
}

// TODO: inject nodeaffinity like other data operations when using job instead of pod
if kubeclient.IsSucceededPod(backupPod) && result.NodeAffinity == nil {
result.NodeAffinity, err = dataflow.GenerateNodeAffinityFromPod(backupPod)

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.

I traced where the annotations this reads actually come from, and I don't think this branch ever fires in a real cluster. GenerateNodeAffinityFromPod returns early unless the pod carries dataflow-affinity.fluid.io.inject=true plus the affinity.dataflow.fluid.io.* node-label annotations. Those annotations are only ever written by DataOpJobReconciler in pkg/controllers/v1alpha1/fluidapp/dataflowaffinity/, whose ManagedResource() returns &batchv1.Job{} and which writes the labels onto the Job (injectPodNodeLabelsToJob). DataBackup runs as a Pod, not a Job — the pod template in charts/fluid-databackup/*/templates/databackup.yaml sets no such annotations, and there's no pod-level reconciler that backfills them. So for a real DataBackup, pod.Annotations[AnnotationDataFlowAffinityInject] != "true" and this returns (nil, nil); result.NodeAffinity stays nil. The new unit tests only pass because they hand-set those annotations on a fake pod. Where is the backup pod supposed to get the inject/affinity annotations? If there isn't a source yet, this needs the pod-side injection (equivalent of the Job reconciler) to actually deliver the feature.

if err != nil {
ctx.Log.V(1).Info("NodeAffinity not injected", "reason", err.Error())
err = nil

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.

The the affinity label is not set, wait for next reconcile error is logged at V(1) and then cleared (err = nil), after which this same pass goes on to set Phase = Complete. Once the status is terminal there's no follow-up reconcile to backfill the affinity, so the "wait for next reconcile" wording doesn't match what happens for a backup. Separately, reusing the named return err inside this nested block is a bit fragile — a local variable (e.g. affinityErr) makes the intent clearer and avoids accidental leakage if this function grows later.

}
}
Comment on lines +56 to +62

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.

medium

Reusing the named return parameter err for a transient error inside a nested block can be error-prone and may lead to accidental side effects if the function is refactored in the future. It is safer and more idiomatic in Go to use a local variable (e.g., affinityErr) to handle this transient error.

Suggested change
if kubeclient.IsSucceededPod(backupPod) && result.NodeAffinity == nil {
result.NodeAffinity, err = dataflow.GenerateNodeAffinityFromPod(backupPod)
if err != nil {
ctx.Log.V(1).Info("NodeAffinity not injected", "reason", err.Error())
err = nil
}
}
if kubeclient.IsSucceededPod(backupPod) && result.NodeAffinity == nil {
nodeAffinity, affinityErr := dataflow.GenerateNodeAffinityFromPod(backupPod)
if affinityErr != nil {
ctx.Log.V(1).Info("NodeAffinity not injected", "reason", affinityErr.Error())
} else {
result.NodeAffinity = nodeAffinity
}
}


var finishTime time.Time
if len(backupPod.Status.Conditions) != 0 {
Expand Down
71 changes: 71 additions & 0 deletions pkg/controllers/v1alpha1/databackup/status_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,5 +184,76 @@ var _ = Describe("OnceHandler", func() {
Expect(opStatus.Conditions).To(HaveLen(1))
Expect(opStatus.Conditions[0].LastTransitionTime.Time).To(BeTemporally("~", conditionTime.Time, time.Second))
})

It("should inject NodeAffinity when backup pod succeeds with dataflow annotations", func() {
pod := &corev1.Pod{
ObjectMeta: v1.ObjectMeta{
Name: mockDataBackup.GetName() + "-pod",
Namespace: mockDataBackup.GetNamespace(),
Annotations: map[string]string{
common.AnnotationDataFlowAffinityInject: "true",
common.AnnotationDataFlowCustomizedAffinityPrefix + "node-label": "node-value",
},
},
Status: corev1.PodStatus{
Phase: corev1.PodSucceeded,
Conditions: []corev1.PodCondition{
{
Type: corev1.PodReady,
Status: corev1.ConditionFalse,
LastTransitionTime: v1.Now(),
},
},
},
}
c := fake.NewFakeClientWithScheme(testScheme, pod, mockDataBackup)
handler := &OnceHandler{dataBackup: mockDataBackup}
ctx := cruntime.ReconcileRequestContext{
NamespacedName: types.NamespacedName{Namespace: "default", Name: "test"},
Log: fake.NullLogger(),
Client: c,
}
result, err := handler.GetOperationStatus(ctx, &mockDataBackup.Status)
Expect(err).NotTo(HaveOccurred())
Expect(result.Phase).To(Equal(common.PhaseComplete))
Expect(result.NodeAffinity).NotTo(BeNil())
Expect(result.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution).NotTo(BeNil())
terms := result.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms
Expect(terms).To(HaveLen(1))
Expect(terms[0].MatchExpressions).To(HaveLen(1))
Expect(terms[0].MatchExpressions[0].Key).To(Equal("node-label"))
Expect(terms[0].MatchExpressions[0].Values).To(ContainElement("node-value"))
})

It("should not inject NodeAffinity when backup pod succeeds without dataflow annotations", func() {
pod := &corev1.Pod{
ObjectMeta: v1.ObjectMeta{
Name: mockDataBackup.GetName() + "-pod",
Namespace: mockDataBackup.GetNamespace(),
},
Status: corev1.PodStatus{
Phase: corev1.PodSucceeded,
Conditions: []corev1.PodCondition{
{
Type: corev1.PodReady,
Status: corev1.ConditionFalse,
LastTransitionTime: v1.Now(),
},
},
},
}
c := fake.NewFakeClientWithScheme(testScheme, pod, mockDataBackup)
handler := &OnceHandler{dataBackup: mockDataBackup}
ctx := cruntime.ReconcileRequestContext{
NamespacedName: types.NamespacedName{Namespace: "default", Name: "test"},
Log: fake.NullLogger(),
Client: c,
}
result, err := handler.GetOperationStatus(ctx, &mockDataBackup.Status)
Expect(err).NotTo(HaveOccurred())
Expect(result.Phase).To(Equal(common.PhaseComplete))
Expect(result.NodeAffinity).To(BeNil())
})

})
})
44 changes: 44 additions & 0 deletions pkg/dataflow/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,47 @@

return nodeAffinity, nil
}

// GenerateNodeAffinityFromPod generates a NodeAffinity from a Pod's annotations,
// using the same annotation-based logic as GenerateNodeAffinity for Jobs.
// This is used by DataBackup which runs as a Pod rather than a Job.
func GenerateNodeAffinityFromPod(pod *corev1.Pod) (*corev1.NodeAffinity, error) {

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.

GenerateNodeAffinityFromPod is a near-verbatim copy of GenerateNodeAffinity — both only read .Annotations. Consider extracting the shared body into something like generateNodeAffinityFromAnnotations(annotations map[string]string) (*corev1.NodeAffinity, error) and having both the Job and Pod entrypoints delegate to it, so the two paths can't drift apart.

if pod == nil {
return nil, nil
}
// not inject, i.e. feature gate not enabled
if v := pod.Annotations[common.AnnotationDataFlowAffinityInject]; v != "true" {

Check warning on line 77 in pkg/dataflow/helper.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this unnecessary variable declaration and use the expression directly in the condition.

See more on https://sonarcloud.io/project/issues?id=fluid-cloudnative_fluid&issues=AZ9GQSUnEfRLliMysmkD&open=AZ9GQSUnEfRLliMysmkD&pullRequest=6110
return nil, nil
}

annotations := pod.Annotations

nodeAffinity := &corev1.NodeAffinity{
RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{
NodeSelectorTerms: []corev1.NodeSelectorTerm{
{
MatchExpressions: nil,
},
},
},
}

hasInjectedLabels := false
for key, value := range annotations {
if strings.HasPrefix(key, common.AnnotationDataFlowCustomizedAffinityPrefix) {
nodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms[0].MatchExpressions =
append(nodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms[0].MatchExpressions,
corev1.NodeSelectorRequirement{
Key: strings.TrimPrefix(key, common.AnnotationDataFlowCustomizedAffinityPrefix),
Operator: corev1.NodeSelectorOpIn,
Values: []string{value},
})
hasInjectedLabels = true
}
}
if !hasInjectedLabels {
return nil, errors.New("the affinity label is not set, wait for next reconcile")
}

return nodeAffinity, nil
}
Comment on lines +69 to +111

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.

medium

The logic in GenerateNodeAffinityFromPod is almost identical to GenerateNodeAffinity for Jobs, resulting in significant code duplication. We can extract the common annotation-parsing logic into a helper function GenerateNodeAffinityFromAnnotations(annotations map[string]string) and reuse it in both functions. This improves maintainability and readability.

// GenerateNodeAffinityFromAnnotations generates a NodeAffinity from annotations.
func GenerateNodeAffinityFromAnnotations(annotations map[string]string) (*corev1.NodeAffinity, error) {
	if v := annotations[common.AnnotationDataFlowAffinityInject]; v != "true" {
		return nil, nil
	}

	nodeAffinity := &corev1.NodeAffinity{
		RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{
			NodeSelectorTerms: []corev1.NodeSelectorTerm{
				{
					MatchExpressions: nil,
				},
			},
		},
	}

	hasInjectedLabels := false
	for key, value := range annotations {
		if strings.HasPrefix(key, common.AnnotationDataFlowCustomizedAffinityPrefix) {
			nodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms[0].MatchExpressions = 
				append(nodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms[0].MatchExpressions, 
					corev1.NodeSelectorRequirement{
						Key:      strings.TrimPrefix(key, common.AnnotationDataFlowCustomizedAffinityPrefix),
						Operator: corev1.NodeSelectorOpIn,
						Values:   []string{value},
					})
			hasInjectedLabels = true
		}
	}
	if !hasInjectedLabels {
		return nil, errors.New("the affinity label is not set, wait for next reconcile")
	}

	return nodeAffinity, nil
}

// GenerateNodeAffinityFromPod generates a NodeAffinity from a Pod's annotations,
// using the same annotation-based logic as GenerateNodeAffinity for Jobs.
// This is used by DataBackup which runs as a Pod rather than a Job.
func GenerateNodeAffinityFromPod(pod *corev1.Pod) (*corev1.NodeAffinity, error) {
	if pod == nil {
		return nil, nil
	}
	return GenerateNodeAffinityFromAnnotations(pod.Annotations)
}

83 changes: 83 additions & 0 deletions pkg/dataflow/helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,86 @@
})
}
}

func TestGenerateNodeAffinityFromPod(t *testing.T) {

Check failure on line 132 in pkg/dataflow/helper_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 20 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=fluid-cloudnative_fluid&issues=AZ9GQSY7EfRLliMysmkE&open=AZ9GQSY7EfRLliMysmkE&pullRequest=6110

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.

This is a feat PR that adds a new user-visible behavior (populating DataBackup.Status.NodeAffinity so dataflow runAfter chains can schedule against it), but it only ships unit tests. The repo's e2e suite lives under test/gha-e2e/ (driven by .github/workflows/kind-e2e.yml), and there's no databackup/affinity scenario there. Please add an e2e case that runs a DataBackup and a dependent dataflow operation and asserts the affinity actually propagates end to end — that would also directly confirm or refute the annotation-source concern above, since a real run is exactly where the no-op would show up. If you believe an e2e case isn't feasible for this change, let's get a maintainer's call on that rather than skipping it.

tests := []struct {
name string
pod *v1.Pod
wantNil bool
wantErr bool
wantLen int
}{
{
name: "nil pod returns nil",
pod: nil,
wantNil: true,
wantErr: false,
},
{
name: "pod without inject annotation returns nil",
pod: &v1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "backup-pod",
Annotations: map[string]string{},
},
},
wantNil: true,
wantErr: false,
},
{
name: "pod with inject annotation but no affinity labels returns error",
pod: &v1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "backup-pod",
Annotations: map[string]string{
common.AnnotationDataFlowAffinityInject: "true",
},
},
},
wantNil: true,
wantErr: true,
},
{
name: "pod with inject annotation and affinity labels returns NodeAffinity",
pod: &v1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "backup-pod",
Annotations: map[string]string{
common.AnnotationDataFlowAffinityInject: "true",
common.AnnotationDataFlowCustomizedAffinityPrefix + common.K8sNodeNameLabelKey: "node01",
common.AnnotationDataFlowCustomizedAffinityPrefix + common.K8sZoneLabelKey: "zone01",
},
},
},
wantNil: false,
wantErr: false,
wantLen: 2,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := GenerateNodeAffinityFromPod(tt.pod)
if (err != nil) != tt.wantErr {
t.Errorf("GenerateNodeAffinityFromPod() error = %v, wantErr %v", err, tt.wantErr)
return
}
if tt.wantNil {
if got != nil {
t.Errorf("GenerateNodeAffinityFromPod() expected nil, got %v", got)
}
return
}
if got == nil {
t.Fatal("GenerateNodeAffinityFromPod() expected non-nil NodeAffinity")
}
terms := got.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms
if len(terms) != 1 {
t.Fatalf("expected 1 NodeSelectorTerm, got %d", len(terms))
}
if len(terms[0].MatchExpressions) != tt.wantLen {
t.Errorf("expected %d MatchExpressions, got %d", tt.wantLen, len(terms[0].MatchExpressions))
}
})
}
}
Loading