Skip to content

Commit 293306a

Browse files
AVSBharadwajAVSBharadwaj
committed
Change design to include backward compatibility
1 parent 01be401 commit 293306a

File tree

7 files changed

+77
-186
lines changed

7 files changed

+77
-186
lines changed

‎api/autoscaling/v2/webhook_suite_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ var _ = BeforeSuite(func() {
168168
eventRecorder := mgr.GetEventRecorderFor("tortoise-controller")
169169
tortoiseService, err := tortoise.New(mgr.GetClient(), eventRecorder, config.RangeOfMinMaxReplicasRecommendationHours, config.TimeZone, config.TortoiseUpdateInterval, config.GatheringDataPeriodType)
170170
Expect(err).NotTo(HaveOccurred())
171-
hpaService, err := hpa.New(mgr.GetClient(), eventRecorder, config.ReplicaReductionFactor, config.MaximumTargetResourceUtilization, 100, time.Hour, 1000, 3, "", config)
171+
hpaService, err := hpa.New(mgr.GetClient(), eventRecorder, config.ReplicaReductionFactor, config.MaximumTargetResourceUtilization, 100, time.Hour, 1000, 10000, 3, config.ServiceGroups, config.MaximumMaxReplicasPerService, "")
172172
Expect(err).NotTo(HaveOccurred())
173173

174174
hpaWebhook := New(tortoiseService, hpaService)

‎cmd/main.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ func main() {
152152
os.Exit(1)
153153
}
154154

155-
hpaService, err := hpa.New(mgr.GetClient(), eventRecorder, config.ReplicaReductionFactor, config.MaximumTargetResourceUtilization, config.HPATargetUtilizationMaxIncrease, config.HPATargetUtilizationUpdateInterval, config.MaximumMinReplicas, int32(config.MinimumMinReplicas), config.HPAExternalMetricExclusionRegex, config)
155+
hpaService, err := hpa.New(mgr.GetClient(), eventRecorder, config.ReplicaReductionFactor, config.MaximumTargetResourceUtilization, config.HPATargetUtilizationMaxIncrease, config.HPATargetUtilizationUpdateInterval, config.MaximumMinReplicas, config.MaximumMaxReplicas, int32(config.MinimumMinReplicas), config.ServiceGroups, config.MaximumMaxReplicasPerService, config.HPAExternalMetricExclusionRegex)
156156
if err != nil {
157157
setupLog.Error(err, "unable to start hpa service")
158158
os.Exit(1)
@@ -176,7 +176,7 @@ func main() {
176176
config.MinimumMemoryRequestPerContainer,
177177
config.MaximumCPURequest,
178178
config.MaximumMemoryRequest,
179-
config.GetDefaultMaximumMaxReplica(),
179+
config.MaximumMaxReplicas,
180180
config.MaxAllowedScalingDownRatio,
181181
config.BufferRatioOnVerticalResource,
182182
config.FeatureFlags,

‎internal/controller/tortoise_controller_test.go‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import (
2323
"sigs.k8s.io/yaml"
2424

2525
"github.com/mercari/tortoise/api/v1beta3"
26-
configfile "github.com/mercari/tortoise/pkg/config"
26+
config_file "github.com/mercari/tortoise/pkg/config"
2727
"github.com/mercari/tortoise/pkg/deployment"
2828
"github.com/mercari/tortoise/pkg/features"
2929
"github.com/mercari/tortoise/pkg/hpa"
@@ -251,18 +251,7 @@ func startController(ctx context.Context) func() {
251251
cli, err := vpa.New(mgr.GetConfig(), recorder)
252252
Expect(err).ShouldNot(HaveOccurred())
253253

254-
// Define a dummy config with maximumMaxReplica set to 10000 for the default group
255-
defaultGroupName := "default"
256-
dummyConfig := &configfile.Config{
257-
MaximumMaxReplicas: []configfile.MaximumMaxReplicasPerGroup{
258-
{
259-
ServiceGroupName: &defaultGroupName,
260-
MaximumMaxReplica: 10000, // Set the value you need
261-
},
262-
},
263-
// Add other default values if your function logic depends on them
264-
}
265-
hpaS, err := hpa.New(mgr.GetClient(), recorder, 0.95, 90, 25, time.Hour, 1000, 3, ".*-exclude-metric", dummyConfig)
254+
hpaS, err := hpa.New(mgr.GetClient(), recorder, 0.95, 90, 25, time.Hour, 1000, 10000, 3, []config_file.ServiceGroup{}, []config_file.MaximumMaxReplicasPerGroup{}, ".*-exclude-metric")
266255
Expect(err).ShouldNot(HaveOccurred())
267256
reconciler := &TortoiseReconciler{
268257
Scheme: scheme,

‎pkg/config/config.go‎

Lines changed: 15 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package config
22

33
import (
44
"fmt"
5-
"math"
65
"os"
76
"time"
87

@@ -168,6 +167,11 @@ type Config struct {
168167
// a tortoise will ignore `PreferredMaxReplicas`, and increase the number of replicas.
169168
// This feature is controlled by the feature flag `VerticalScalingBasedOnPreferredMaxReplicas`.
170169
PreferredMaxReplicas int `yaml:"PreferredMaxReplicas"`
170+
// MaximumMaxReplicas is the maximum maxReplica that tortoise can give to the HPA (default: 100)
171+
// Note that this is very dangerous. If you set this value too low, the HPA may not be able to scale up the workload.
172+
// The motivation is to use it has a hard limit to prevent the HPA from scaling up the workload too much in cases of Tortoise's bug, abnormal traffic increase, etc.
173+
// If some Tortoise hits this limit, the tortoise controller emits an error log, which may or may not imply you have to change this value.
174+
MaximumMaxReplicas int32 `yaml:"MaximumMaxReplicas"`
171175
// MaximumCPURequest is the maximum CPU cores that the tortoise can give to the container resource request (default: 10)
172176
MaximumCPURequest string `yaml:"MaximumCPURequest"`
173177
// MaximumMemoryRequest is the maximum memory bytes that the tortoise can give to the container resource request (default: 10Gi)
@@ -260,11 +264,9 @@ type Config struct {
260264

261265
// serviceGroups defines a list of service category names.
262266
ServiceGroups []ServiceGroup `yaml:"ServiceGroups"`
263-
// MaximumMaxReplicas is the maximum maxReplicas that tortoise can give to the HPA per group (default: 100)
264-
// Note that this is very dangerous. If you set this value too low, the HPA may not be able to scale up the workload.
265-
// The motivation is to use it has a hard limit to prevent the HPA from scaling up the workload too much in cases of Tortoise's bug, abnormal traffic increase, etc.
266-
// If some Tortoise hits this limit, the tortoise controller emits an error log, which may or may not imply you have to change this value.
267-
MaximumMaxReplicas []MaximumMaxReplicasPerGroup `yaml:"MaximumMaxReplicas"`
267+
// MaximumMaxReplicasPerService is the maximum maxReplicas that tortoise can give to the HPA per service category.
268+
// If the service category is not found in this list, tortoise uses the default value which is the value set in MaximumMaxReplicas.
269+
MaximumMaxReplicasPerService []MaximumMaxReplicasPerGroup `yaml:"MaximumMaxReplicasPerService"`
268270

269271
// FeatureFlags is the list of feature flags (default: empty = all alpha features are disabled)
270272
// See the list of feature flags in features.go
@@ -294,7 +296,6 @@ type ServiceGroup struct {
294296
}
295297

296298
func defaultConfig() *Config {
297-
defaultGroupName := "default"
298299
return &Config{
299300
RangeOfMinMaxReplicasRecommendationHours: 1,
300301
GatheringDataPeriodType: "weekly",
@@ -316,22 +317,15 @@ func defaultConfig() *Config {
316317
HPATargetUtilizationMaxIncrease: 5,
317318
HPATargetUtilizationUpdateInterval: time.Hour * 24,
318319
MaximumMinReplicas: 10,
320+
MaximumMaxReplicas: 100,
319321
MaxAllowedScalingDownRatio: 0.8,
320322
IstioSidecarProxyDefaultCPU: "100m",
321323
IstioSidecarProxyDefaultMemory: "200Mi",
322324
MinimumCPULimit: "0",
323325
ResourceLimitMultiplier: map[string]int64{},
324-
ServiceGroups: []ServiceGroup{
325-
// This is an empty slice, indicating that no service groups are defined by default.
326-
},
327-
MaximumMaxReplicas: []MaximumMaxReplicasPerGroup{
328-
// This is the default maximum maxReplicas limit for all services.
329-
{
330-
ServiceGroupName: &defaultGroupName, // Applies to all services by default.
331-
MaximumMaxReplica: 100, // Default max replica limit.
332-
},
333-
},
334-
BufferRatioOnVerticalResource: 0.1,
326+
ServiceGroups: []ServiceGroup{},
327+
MaximumMaxReplicasPerService: []MaximumMaxReplicasPerGroup{},
328+
BufferRatioOnVerticalResource: 0.1,
335329
}
336330
}
337331

@@ -359,16 +353,6 @@ func ParseConfig(path string) (*Config, error) {
359353
return config, nil
360354
}
361355

362-
// GetDefaultMaxReplica returns the default maximum max replicas from the configuration.
363-
func (cfg *Config) GetDefaultMaximumMaxReplica() int32 {
364-
for _, maxReplicaGroup := range cfg.MaximumMaxReplicas {
365-
if maxReplicaGroup.ServiceGroupName != nil && *maxReplicaGroup.ServiceGroupName == "default" {
366-
return maxReplicaGroup.MaximumMaxReplica
367-
}
368-
}
369-
return 100 // Choose a last resort default value if "default" is not found
370-
}
371-
372356
func validate(config *Config) error {
373357
if config.RangeOfMinMaxReplicasRecommendationHours > 24 || config.RangeOfMinMaxReplicasRecommendationHours < 1 {
374358
return fmt.Errorf("RangeOfMinMaxReplicasRecommendationHours should be between 1 and 24")
@@ -387,27 +371,12 @@ func validate(config *Config) error {
387371
return fmt.Errorf("MinimumMinReplicas should be less than MaximumMinReplicas")
388372
}
389373

390-
// Check that there is at least one MaximumMaxReplicas
391-
if len(config.MaximumMaxReplicas) == 0 {
392-
return fmt.Errorf("MaximumMaxReplicas must have at least one configuration entry")
393-
}
394-
395374
// Find the minimum value of MaximumMaxReplicas across all service groups
396-
minOfMaximumMaxReplicas := int32(math.MaxInt32) // Start with the largest possible int32 value
397-
var defaultFound bool
398-
for _, group := range config.MaximumMaxReplicas {
375+
minOfMaximumMaxReplicas := config.MaximumMaxReplicas // Start with the default value of MaximumMaxReplicas
376+
for _, group := range config.MaximumMaxReplicasPerService {
399377
if group.MaximumMaxReplica < minOfMaximumMaxReplicas {
400378
minOfMaximumMaxReplicas = group.MaximumMaxReplica
401379
}
402-
// Check for "default" entry
403-
if group.ServiceGroupName != nil && *group.ServiceGroupName == "default" {
404-
defaultFound = true
405-
}
406-
}
407-
408-
// Ensure that there is an entry with "default" for MaximumMaxReplicas
409-
if !defaultFound {
410-
return fmt.Errorf("There must be at least one MaximumMaxReplicas entry with ServiceGroupName set to \"default\"")
411380
}
412381

413382
// Check for non-negative values
@@ -421,13 +390,8 @@ func validate(config *Config) error {
421390
serviceGroupMap[sg.Name] = true
422391
}
423392

424-
for _, maxReplicas := range config.MaximumMaxReplicas {
393+
for _, maxReplicas := range config.MaximumMaxReplicasPerService {
425394
if maxReplicas.ServiceGroupName != nil {
426-
// Allow "default" to bypass the check of matching ServiceGroups.
427-
if *maxReplicas.ServiceGroupName == "default" {
428-
continue
429-
}
430-
// If not default, ensure it exists in the serviceGroupMap.
431395
if _, exists := serviceGroupMap[*maxReplicas.ServiceGroupName]; !exists {
432396
return fmt.Errorf("ServiceGroupName %s in MaximumMaxReplicas is not defined in ServiceGroups", *maxReplicas.ServiceGroupName)
433397
}

‎pkg/config/config_test.go‎

Lines changed: 35 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
)
88

99
func TestParseConfig(t *testing.T) {
10-
defaultGroupName := "default"
1110
type args struct {
1211
path string
1312
}
@@ -41,17 +40,13 @@ func TestParseConfig(t *testing.T) {
4140
TortoiseUpdateInterval: 1 * time.Hour,
4241
HPATargetUtilizationMaxIncrease: 10,
4342
MaximumMinReplicas: 10,
43+
MaximumMaxReplicas: 100,
44+
HPATargetUtilizationUpdateInterval: 3 * time.Hour,
45+
IstioSidecarProxyDefaultCPU: "100m",
46+
IstioSidecarProxyDefaultMemory: "200Mi",
47+
MaxAllowedScalingDownRatio: 0.5,
4448
ServiceGroups: []ServiceGroup{},
45-
MaximumMaxReplicas: []MaximumMaxReplicasPerGroup{
46-
{
47-
ServiceGroupName: &defaultGroupName,
48-
MaximumMaxReplica: 100,
49-
},
50-
},
51-
HPATargetUtilizationUpdateInterval: 3 * time.Hour,
52-
IstioSidecarProxyDefaultCPU: "100m",
53-
IstioSidecarProxyDefaultMemory: "200Mi",
54-
MaxAllowedScalingDownRatio: 0.5,
49+
MaximumMaxReplicasPerService: []MaximumMaxReplicasPerGroup{},
5550
MinimumCPURequestPerContainer: map[string]string{
5651
"istio-proxy": "100m",
5752
"hoge-agent": "120m",
@@ -91,21 +86,17 @@ func TestParseConfig(t *testing.T) {
9186
TortoiseUpdateInterval: 15 * time.Second,
9287
HPATargetUtilizationMaxIncrease: 5,
9388
MaximumMinReplicas: 10,
89+
MaximumMaxReplicas: 100,
90+
HPATargetUtilizationUpdateInterval: 24 * time.Hour,
91+
IstioSidecarProxyDefaultCPU: "100m",
92+
IstioSidecarProxyDefaultMemory: "200Mi",
93+
MaxAllowedScalingDownRatio: 0.8,
94+
MinimumCPURequestPerContainer: map[string]string{},
95+
MinimumMemoryRequestPerContainer: map[string]string{},
96+
ResourceLimitMultiplier: map[string]int64{},
9497
ServiceGroups: []ServiceGroup{},
95-
MaximumMaxReplicas: []MaximumMaxReplicasPerGroup{
96-
{
97-
ServiceGroupName: &defaultGroupName,
98-
MaximumMaxReplica: 100,
99-
},
100-
},
101-
HPATargetUtilizationUpdateInterval: 24 * time.Hour,
102-
IstioSidecarProxyDefaultCPU: "100m",
103-
IstioSidecarProxyDefaultMemory: "200Mi",
104-
MaxAllowedScalingDownRatio: 0.8,
105-
MinimumCPURequestPerContainer: map[string]string{},
106-
MinimumMemoryRequestPerContainer: map[string]string{},
107-
ResourceLimitMultiplier: map[string]int64{},
108-
BufferRatioOnVerticalResource: 0.1,
98+
MaximumMaxReplicasPerService: []MaximumMaxReplicasPerGroup{},
99+
BufferRatioOnVerticalResource: 0.1,
109100
},
110101
},
111102
{
@@ -139,21 +130,17 @@ func TestParseConfig(t *testing.T) {
139130
TortoiseUpdateInterval: 15 * time.Second,
140131
HPATargetUtilizationMaxIncrease: 5,
141132
MaximumMinReplicas: 10,
133+
MaximumMaxReplicas: 100,
134+
HPATargetUtilizationUpdateInterval: 24 * time.Hour,
135+
IstioSidecarProxyDefaultCPU: "100m",
136+
IstioSidecarProxyDefaultMemory: "200Mi",
137+
MaxAllowedScalingDownRatio: 0.8,
138+
MinimumCPURequestPerContainer: map[string]string{},
139+
MinimumMemoryRequestPerContainer: map[string]string{},
140+
ResourceLimitMultiplier: map[string]int64{},
142141
ServiceGroups: []ServiceGroup{},
143-
MaximumMaxReplicas: []MaximumMaxReplicasPerGroup{
144-
{
145-
ServiceGroupName: &defaultGroupName,
146-
MaximumMaxReplica: 100,
147-
},
148-
},
149-
HPATargetUtilizationUpdateInterval: 24 * time.Hour,
150-
IstioSidecarProxyDefaultCPU: "100m",
151-
IstioSidecarProxyDefaultMemory: "200Mi",
152-
MaxAllowedScalingDownRatio: 0.8,
153-
MinimumCPURequestPerContainer: map[string]string{},
154-
MinimumMemoryRequestPerContainer: map[string]string{},
155-
ResourceLimitMultiplier: map[string]int64{},
156-
BufferRatioOnVerticalResource: 0.1,
142+
MaximumMaxReplicasPerService: []MaximumMaxReplicasPerGroup{},
143+
BufferRatioOnVerticalResource: 0.1,
157144
},
158145
},
159146
}
@@ -172,7 +159,6 @@ func TestParseConfig(t *testing.T) {
172159
}
173160

174161
func Test_validate(t *testing.T) {
175-
defaultGroupName := "default"
176162
tests := []struct {
177163
name string
178164
config *Config
@@ -225,12 +211,7 @@ func Test_validate(t *testing.T) {
225211
HPATargetUtilizationMaxIncrease: 99,
226212
MinimumMinReplicas: 2,
227213
MaximumMinReplicas: 20,
228-
MaximumMaxReplicas: []MaximumMaxReplicasPerGroup{
229-
{
230-
ServiceGroupName: &defaultGroupName,
231-
MaximumMaxReplica: 10,
232-
},
233-
},
214+
MaximumMaxReplicas: 10,
234215
},
235216
wantErr: true,
236217
},
@@ -242,31 +223,21 @@ func Test_validate(t *testing.T) {
242223
HPATargetUtilizationMaxIncrease: 99,
243224
MinimumMinReplicas: 2,
244225
MaximumMinReplicas: 20,
245-
MaximumMaxReplicas: []MaximumMaxReplicasPerGroup{
246-
{
247-
ServiceGroupName: &defaultGroupName,
248-
MaximumMaxReplica: 100,
249-
},
250-
},
251-
PreferredMaxReplicas: 101,
226+
MaximumMaxReplicas: 100,
227+
PreferredMaxReplicas: 101,
252228
},
253229
wantErr: true,
254230
},
255231
{
256-
name: "invalid PreferredMaxReplicas less than minimum",
232+
name: "invalid PreferredMaxReplicas less than MinimumMinReplicas",
257233
config: &Config{
258234
RangeOfMinMaxReplicasRecommendationHours: 2,
259235
GatheringDataPeriodType: "daily",
260236
HPATargetUtilizationMaxIncrease: 99,
261237
MinimumMinReplicas: 5,
262238
MaximumMinReplicas: 20,
263-
MaximumMaxReplicas: []MaximumMaxReplicasPerGroup{
264-
{
265-
ServiceGroupName: &defaultGroupName,
266-
MaximumMaxReplica: 100,
267-
},
268-
},
269-
PreferredMaxReplicas: 4,
239+
MaximumMaxReplicas: 100,
240+
PreferredMaxReplicas: 4,
270241
},
271242
wantErr: true,
272243
},
@@ -278,15 +249,9 @@ func Test_validate(t *testing.T) {
278249
HPATargetUtilizationMaxIncrease: 99,
279250
MinimumMinReplicas: 5,
280251
MaximumMinReplicas: 20,
281-
ServiceGroups: []ServiceGroup{},
282-
MaximumMaxReplicas: []MaximumMaxReplicasPerGroup{
283-
{
284-
ServiceGroupName: &defaultGroupName,
285-
MaximumMaxReplica: 100,
286-
},
287-
},
288-
PreferredMaxReplicas: 6,
289-
MaxAllowedScalingDownRatio: 1.1,
252+
MaximumMaxReplicas: 100,
253+
PreferredMaxReplicas: 6,
254+
MaxAllowedScalingDownRatio: 1.1,
290255
},
291256
wantErr: true,
292257
},

0 commit comments

Comments
 (0)