Skip to content

remark the workload for resourcebinding #6881

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+96 −2
Open

remark the workload for resourcebinding #6881

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bea8285
add IsBindingWorkload() for resourceBindingSpec
LivingCcj Oct 27, 2025
a9ec2e2
update rb.Spec which is single pod template workload
LivingCcj Nov 12, 2025
1acbc39
Merge branch 'master' into remark_workload_of_resourcebinding
LivingCcj Nov 14, 2025
45bda3a
update rb.spec IsWorkLoad function
LivingCcj Nov 14, 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
8 changes: 8 additions & 0 deletions pkg/apis/work/v1alpha2/binding_types_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,3 +213,11 @@ func (s *ResourceBindingSpec) SchedulePriorityValue() int32 {
}
return s.SchedulePriority.Priority
}

// IsBindingWorkload checks whether the ResourceBinding is binding a workload.
func (s *ResourceBindingSpec) IsBindingWorkload() bool {
if s.Replicas > 0 || s.ReplicaRequirements != nil || len(s.Components) > 0 {
return true
}
return false
Copy link

@gemini-code-assist gemini-code-assist bot Oct 27, 2025

Choose a reason for hiding this comment

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

medium

For better readability and conciseness, you can directly return the result of the boolean expression.

return s.Replicas > 0 || s.ReplicaRequirements != nil || len(s.Components) > 0

Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang Oct 29, 2025

Choose a reason for hiding this comment

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

+1

Copy link
Member

@RainbowMango RainbowMango Nov 3, 2025

Choose a reason for hiding this comment

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

I‘m ok with the current implementation, which I think is clear and easy to understand.

But I'm suggesting changing the method name, since Binding is a bit redundant. Like:

// IsWorkload returns true if the ResourceBinding represents a workload
// (e.g., Deployment, StatefulSet) that has replicas or replica requirements.
func (s *ResourceBindingSpec) IsWorkload() bool {
}

}
31 changes: 31 additions & 0 deletions pkg/apis/work/v1alpha2/binding_types_helper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,3 +441,34 @@ func TestResourceBindingSpec_SchedulingSuspended(t *testing.T) {
})
}
}

func TestResourceBindingSpec_IsBindingWorkload(t *testing.T) {
tests := []struct {
name string
spec *ResourceBindingSpec
want bool
}{
{
name: "binding a workload",
spec: &ResourceBindingSpec{
Replicas: 1,
},
want: true,
},
{
name: "not binding a workload when replicas is 0",
spec: &ResourceBindingSpec{
Replicas: 0,
},
want: false,
},
Comment on lines 453 to 466
Copy link

@gemini-code-assist gemini-code-assist bot Oct 27, 2025

Choose a reason for hiding this comment

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

high

The test cases for IsBindingWorkload are not comprehensive as they only cover the Replicas field. To ensure the function works correctly under all conditions, please add test cases for ReplicaRequirements and Components as well.

{
			name: "binding a workload by replicas",
			spec: &ResourceBindingSpec{
				Replicas: 1,
			},
			want: true,
		},
		{
			name: "binding a workload by replica requirements",
			spec: &ResourceBindingSpec{
				ReplicaRequirements: &ReplicaRequirements{},
			},
			want: true,
		},
		{
			name: "binding a workload by components",
			spec: &ResourceBindingSpec{
				Components: []Component{{Name: "test"}},
			},
			want: true,
		},
		{
			name: "not a workload",
			spec: &ResourceBindingSpec{
				Replicas: 0,
			},
			want: false,
		},

Copy link
Member

@RainbowMango RainbowMango Nov 3, 2025

Choose a reason for hiding this comment

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

  • binding represents a workload that has replicas
  • binding represents a workload that has replica requirements
  • binding represents a workload that has components

}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ret := tt.spec.IsBindingWorkload()
if ret != tt.want {
t.Fatalf("test case %s failed, want %v, got %v", tt.name, tt.want, ret)
}
})
}
}
9 changes: 6 additions & 3 deletions pkg/scheduler/core/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,12 @@ func AssignReplicas(
}

// If not workload, assign all clusters without considering replicas.
targetClusters := make([]workv1alpha2.TargetCluster, len(clusters))
for i, cluster := range clusters {
targetClusters[i] = workv1alpha2.TargetCluster{Name: cluster.Cluster.Name}
var targetClusters []workv1alpha2.TargetCluster
if !spec.IsBindingWorkload() {
for _, cluster := range clusters {
targetClusters = append(targetClusters, workv1alpha2.TargetCluster{Name: cluster.Cluster.Name})
}
}
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

When spec.IsBindingWorkload() returns true (indicating a workload), targetClusters remains nil instead of being initialized to an empty slice. This changes the return behavior from returning an initialized empty slice to returning nil, which could cause issues for callers expecting a non-nil slice. Initialize targetClusters to an empty slice using make([]workv1alpha2.TargetCluster, 0) or preserve the original pre-allocation logic for the workload case.

Copilot uses AI. Check for mistakes.
Copy link

@gemini-code-assist gemini-code-assist bot Oct 27, 2025

Choose a reason for hiding this comment

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

medium

Using append inside a loop can be inefficient due to potential reallocations if the slice grows. Since the number of clusters is known beforehand, it's more performant to pre-allocate the slice with make.

Suggested change
if !spec.IsBindingWorkload() {
for _, cluster := range clusters {
targetClusters = append(targetClusters, workv1alpha2.TargetCluster{Name: cluster.Cluster.Name})
}
}
if !spec.IsBindingWorkload() {
targetClusters = make([]workv1alpha2.TargetCluster, len(clusters))
for i, cluster := range clusters {
targetClusters[i] = workv1alpha2.TargetCluster{Name: cluster.Cluster.Name}
}
}

Copy link
Member

@RainbowMango RainbowMango Nov 3, 2025

Choose a reason for hiding this comment

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

+1


return targetClusters, nil
}
4 changes: 2 additions & 2 deletions pkg/scheduler/scheduler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ func (s *Scheduler) doScheduleBinding(namespace, name string) (err error) {
metrics.BindingSchedule(string(ReconcileSchedule), utilmetrics.DurationInSeconds(start), err)
return err
}
if rb.Spec.Replicas == 0 ||
if !rb.Spec.IsBindingWorkload() ||
rb.Spec.Placement.ReplicaSchedulingType() == policyv1alpha1.ReplicaSchedulingTypeDuplicated {
// Duplicated resources should always be scheduled. Note: non-workload is considered as duplicated
// even if scheduling type is divided.
Expand Down Expand Up @@ -496,7 +496,7 @@ func (s *Scheduler) doScheduleClusterBinding(name string) (err error) {
metrics.BindingSchedule(string(ReconcileSchedule), utilmetrics.DurationInSeconds(start), err)
return err
}
if crb.Spec.Replicas == 0 ||
if !crb.Spec.IsBindingWorkload() ||
crb.Spec.Placement.ReplicaSchedulingType() == policyv1alpha1.ReplicaSchedulingTypeDuplicated {
// Duplicated resources should always be scheduled. Note: non-workload is considered as duplicated
// even if scheduling type is divided.
Expand Down