Skip to content

Commit dc03ac4

Browse files
moko-poimoko-poi
committed
Fix incorrect StatusFilter usage in CapacityBufferPodListProcessor
1 parent 7b95cb0 commit dc03ac4

File tree

5 files changed

+216
-21
lines changed

5 files changed

+216
-21
lines changed

‎cluster-autoscaler/capacitybuffer/common/common.go‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121

2222
v1 "k8s.io/autoscaler/cluster-autoscaler/apis/capacitybuffer/autoscaling.x-k8s.io/v1alpha1"
2323

24+
"k8s.io/apimachinery/pkg/api/meta"
2425
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2526
)
2627

@@ -48,7 +49,7 @@ func SetBufferAsReadyForProvisioning(buffer *v1.CapacityBuffer, PodTemplateRef *
4849
Reason: "atrtibutesSetSuccessfully",
4950
LastTransitionTime: metav1.Time{Time: time.Now()},
5051
}
51-
buffer.Status.Conditions = []metav1.Condition{readyCondition}
52+
meta.SetStatusCondition(&buffer.Status.Conditions, readyCondition)
5253
}
5354

5455
// SetBufferAsNotReadyForProvisioning updates the passed buffer object with the rest of the attributes and sets its condition to not ready with the passed error
@@ -69,7 +70,7 @@ func SetBufferAsNotReadyForProvisioning(buffer *v1.CapacityBuffer, PodTemplateRe
6970
Reason: "error",
7071
LastTransitionTime: metav1.Time{Time: time.Now()},
7172
}
72-
buffer.Status.Conditions = []metav1.Condition{notReadyCondition}
73+
meta.SetStatusCondition(&buffer.Status.Conditions, notReadyCondition)
7374
}
7475

7576
func mapEmptyProvStrategyToDefault(ps *string) *string {
@@ -82,21 +83,23 @@ func mapEmptyProvStrategyToDefault(ps *string) *string {
8283

8384
// UpdateBufferStatusToFailedProvisioing updates the status of the passed buffer and set Provisioning to false with the passes reason and message
8485
func UpdateBufferStatusToFailedProvisioing(buffer *v1.CapacityBuffer, reason, errorMessage string) {
85-
buffer.Status.Conditions = []metav1.Condition{{
86+
failedCondition := metav1.Condition{
8687
Type: ProvisioningCondition,
8788
Status: ConditionFalse,
8889
Message: errorMessage,
8990
Reason: reason,
9091
LastTransitionTime: metav1.Time{Time: time.Now()},
91-
}}
92+
}
93+
meta.SetStatusCondition(&buffer.Status.Conditions, failedCondition)
9294
}
9395

9496
// UpdateBufferStatusToSuccessfullyProvisioing updates the status of the passed buffer and set Provisioning to true with the passes reason
9597
func UpdateBufferStatusToSuccessfullyProvisioing(buffer *v1.CapacityBuffer, reason string) {
96-
buffer.Status.Conditions = []metav1.Condition{{
98+
successCondition := metav1.Condition{
9799
Type: ProvisioningCondition,
98100
Status: ConditionTrue,
99101
Reason: reason,
100102
LastTransitionTime: metav1.Time{Time: time.Now()},
101-
}}
103+
}
104+
meta.SetStatusCondition(&buffer.Status.Conditions, successCondition)
102105
}

‎cluster-autoscaler/capacitybuffer/filters/status_include_filter.go‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package filter
18+
19+
import (
20+
v1 "k8s.io/autoscaler/cluster-autoscaler/apis/capacitybuffer/autoscaling.x-k8s.io/v1alpha1"
21+
)
22+
23+
// statusIncludeFilter includes buffers with the defined conditions (opposite of statusFilter)
24+
type statusIncludeFilter struct {
25+
conditions map[string]string
26+
}
27+
28+
// NewStatusIncludeFilter creates an instance of statusIncludeFilter that includes only the buffers with conditions in passed conditions.
29+
func NewStatusIncludeFilter(conditionsToInclude map[string]string) *statusIncludeFilter {
30+
return &statusIncludeFilter{
31+
conditions: conditionsToInclude,
32+
}
33+
}
34+
35+
// Filter filters the passed buffers to include only those with the specified status conditions
36+
func (f *statusIncludeFilter) Filter(buffersToFilter []*v1.CapacityBuffer) ([]*v1.CapacityBuffer, []*v1.CapacityBuffer) {
37+
var included []*v1.CapacityBuffer
38+
var excluded []*v1.CapacityBuffer
39+
40+
for _, buffer := range buffersToFilter {
41+
if f.hasAllConditions(buffer) {
42+
included = append(included, buffer)
43+
} else {
44+
excluded = append(excluded, buffer)
45+
}
46+
}
47+
return included, excluded
48+
}
49+
50+
// hasAllConditions checks if the buffer has all the specified conditions
51+
func (f *statusIncludeFilter) hasAllConditions(buffer *v1.CapacityBuffer) bool {
52+
bufferConditions := buffer.Status.Conditions
53+
matchCount := 0
54+
55+
for condType, condStatus := range f.conditions {
56+
for _, condition := range bufferConditions {
57+
if condition.Type == condType && string(condition.Status) == condStatus {
58+
matchCount++
59+
break
60+
}
61+
}
62+
}
63+
64+
// All specified conditions must be present
65+
return matchCount == len(f.conditions)
66+
}
67+
68+
// CleanUp cleans up the filter's internal structures.
69+
func (f *statusIncludeFilter) CleanUp() {
70+
}

‎cluster-autoscaler/capacitybuffer/filters/status_include_filter_test.go‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package filter
18+
19+
import (
20+
"testing"
21+
22+
"github.com/stretchr/testify/assert"
23+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
24+
v1 "k8s.io/autoscaler/cluster-autoscaler/apis/capacitybuffer/autoscaling.x-k8s.io/v1alpha1"
25+
"k8s.io/autoscaler/cluster-autoscaler/capacitybuffer/common"
26+
"k8s.io/autoscaler/cluster-autoscaler/capacitybuffer/testutil"
27+
)
28+
29+
func TestStatusIncludeFilter(t *testing.T) {
30+
tests := []struct {
31+
name string
32+
conditionsToInclude map[string]string
33+
buffers []*v1.CapacityBuffer
34+
expectedIncludedBuffers []*v1.CapacityBuffer
35+
expectedExcludedBuffers []*v1.CapacityBuffer
36+
}{
37+
{
38+
name: "Empty conditions map, include all",
39+
conditionsToInclude: map[string]string{},
40+
buffers: []*v1.CapacityBuffer{
41+
getTestBufferWithCondition(testutil.SomePodTemplateRefName, testutil.GetConditionReady()),
42+
},
43+
expectedIncludedBuffers: []*v1.CapacityBuffer{
44+
getTestBufferWithCondition(testutil.SomePodTemplateRefName, testutil.GetConditionReady()),
45+
},
46+
expectedExcludedBuffers: []*v1.CapacityBuffer{},
47+
},
48+
{
49+
name: "One buffer, include only ready for provisioning",
50+
conditionsToInclude: map[string]string{common.ReadyForProvisioningCondition: common.ConditionTrue},
51+
buffers: []*v1.CapacityBuffer{
52+
getTestBufferWithCondition(testutil.SomePodTemplateRefName, testutil.GetConditionReady()),
53+
},
54+
expectedIncludedBuffers: []*v1.CapacityBuffer{
55+
getTestBufferWithCondition(testutil.SomePodTemplateRefName, testutil.GetConditionReady()),
56+
},
57+
expectedExcludedBuffers: []*v1.CapacityBuffer{},
58+
},
59+
{
60+
name: "Two buffers, one included",
61+
conditionsToInclude: map[string]string{common.ReadyForProvisioningCondition: common.ConditionTrue},
62+
buffers: []*v1.CapacityBuffer{
63+
getTestBufferWithCondition(testutil.SomePodTemplateRefName, testutil.GetConditionReady()),
64+
getTestBufferWithCondition(testutil.AnotherPodTemplateRefName, testutil.GetConditionNotReady()),
65+
},
66+
expectedIncludedBuffers: []*v1.CapacityBuffer{
67+
getTestBufferWithCondition(testutil.SomePodTemplateRefName, testutil.GetConditionReady()),
68+
},
69+
expectedExcludedBuffers: []*v1.CapacityBuffer{
70+
getTestBufferWithCondition(testutil.AnotherPodTemplateRefName, testutil.GetConditionNotReady()),
71+
},
72+
},
73+
{
74+
name: "Buffer with no conditions, excluded",
75+
conditionsToInclude: map[string]string{common.ReadyForProvisioningCondition: common.ConditionTrue},
76+
buffers: []*v1.CapacityBuffer{
77+
getTestBufferWithCondition(testutil.SomePodTemplateRefName, []metav1.Condition{}),
78+
},
79+
expectedIncludedBuffers: []*v1.CapacityBuffer{},
80+
expectedExcludedBuffers: []*v1.CapacityBuffer{
81+
getTestBufferWithCondition(testutil.SomePodTemplateRefName, []metav1.Condition{}),
82+
},
83+
},
84+
{
85+
name: "Multiple conditions required, all must match",
86+
conditionsToInclude: map[string]string{
87+
common.ReadyForProvisioningCondition: common.ConditionTrue,
88+
common.ProvisioningCondition: common.ConditionTrue,
89+
},
90+
buffers: []*v1.CapacityBuffer{
91+
getTestBufferWithCondition(testutil.SomePodTemplateRefName, []metav1.Condition{
92+
{Type: common.ReadyForProvisioningCondition, Status: common.ConditionTrue},
93+
{Type: common.ProvisioningCondition, Status: common.ConditionTrue},
94+
}),
95+
getTestBufferWithCondition(testutil.AnotherPodTemplateRefName, testutil.GetConditionReady()),
96+
},
97+
expectedIncludedBuffers: []*v1.CapacityBuffer{
98+
getTestBufferWithCondition(testutil.SomePodTemplateRefName, []metav1.Condition{
99+
{Type: common.ReadyForProvisioningCondition, Status: common.ConditionTrue},
100+
{Type: common.ProvisioningCondition, Status: common.ConditionTrue},
101+
}),
102+
},
103+
expectedExcludedBuffers: []*v1.CapacityBuffer{
104+
getTestBufferWithCondition(testutil.AnotherPodTemplateRefName, testutil.GetConditionReady()),
105+
},
106+
},
107+
}
108+
for _, test := range tests {
109+
t.Run(test.name, func(t *testing.T) {
110+
statusIncludeFilter := NewStatusIncludeFilter(test.conditionsToInclude)
111+
included, excluded := statusIncludeFilter.Filter(test.buffers)
112+
assert.ElementsMatch(t, test.expectedIncludedBuffers, included)
113+
assert.ElementsMatch(t, test.expectedExcludedBuffers, excluded)
114+
})
115+
}
116+
}

‎cluster-autoscaler/processors/capacitybuffer/pod_list_processor.go‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,10 @@ const (
4242
// CapacityBufferPodListProcessor processes the pod lists before scale up
4343
// and adds buffres api virtual pods.
4444
type CapacityBufferPodListProcessor struct {
45-
client *client.CapacityBufferClient
46-
statusFilter buffersfilter.Filter
47-
podTemplateGenFilter buffersfilter.Filter
48-
provStrategies map[string]bool
49-
buffersRegistry *capacityBuffersFakePodsRegistry
45+
client *client.CapacityBufferClient
46+
statusFilter buffersfilter.Filter
47+
provStrategies map[string]bool
48+
buffersRegistry *capacityBuffersFakePodsRegistry
5049
}
5150

5251
// capacityBuffersFakePodsRegistry a struct that keeps the status of capacity buffer
@@ -73,13 +72,11 @@ func NewCapacityBufferPodListProcessor(client *client.CapacityBufferClient, prov
7372
}
7473
return &CapacityBufferPodListProcessor{
7574
client: client,
76-
statusFilter: buffersfilter.NewStatusFilter(map[string]string{
75+
statusFilter: buffersfilter.NewStatusIncludeFilter(map[string]string{
7776
common.ReadyForProvisioningCondition: common.ConditionTrue,
78-
common.ProvisioningCondition: common.ConditionTrue,
7977
}),
80-
podTemplateGenFilter: buffersfilter.NewPodTemplateGenerationChangedFilter(client),
81-
provStrategies: provStrategiesMap,
82-
buffersRegistry: buffersRegistry,
78+
provStrategies: provStrategiesMap,
79+
buffersRegistry: buffersRegistry,
8380
}
8481
}
8582

@@ -91,8 +88,7 @@ func (p *CapacityBufferPodListProcessor) Process(autoscalingCtx *ca_context.Auto
9188
return unschedulablePods, nil
9289
}
9390
buffers = p.filterBuffersProvStrategy(buffers)
94-
_, buffers = p.statusFilter.Filter(buffers)
95-
_, buffers = p.podTemplateGenFilter.Filter(buffers)
91+
buffers, _ = p.statusFilter.Filter(buffers)
9692

9793
totalFakePods := []*apiv1.Pod{}
9894
p.clearCapacityBufferRegistry()

‎cluster-autoscaler/processors/capacitybuffer/pod_list_processor_test.go‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -163,9 +163,19 @@ func TestPodListProcessor(t *testing.T) {
163163
for buffer, condition := range test.expectedBuffersProvCondition {
164164
buffer, err := fakeBuffersClient.AutoscalingV1alpha1().CapacityBuffers(corev1.NamespaceDefault).Get(context.TODO(), buffer, metav1.GetOptions{})
165165
assert.Equal(t, err, nil)
166-
assert.Equal(t, len(buffer.Status.Conditions), 1)
167-
assert.Equal(t, string(buffer.Status.Conditions[0].Type), string(condition.Type))
168-
assert.Equal(t, string(buffer.Status.Conditions[0].Status), string(condition.Status))
166+
// After the fix, conditions are properly updated using meta.SetStatusCondition
167+
// which means both ReadyForProvisioning and Provisioning conditions can coexist
168+
assert.GreaterOrEqual(t, len(buffer.Status.Conditions), 1)
169+
// Find the expected condition type in the buffer status
170+
foundCondition := false
171+
for _, c := range buffer.Status.Conditions {
172+
if c.Type == condition.Type {
173+
assert.Equal(t, string(condition.Status), string(c.Status))
174+
foundCondition = true
175+
break
176+
}
177+
}
178+
assert.True(t, foundCondition, fmt.Sprintf("Expected condition %s not found in buffer %s", condition.Type, buffer.Name))
169179
}
170180
})
171181
}

0 commit comments

Comments
 (0)