Skip to content

Commit 44f709d

Browse files
AVSBharadwajAVSBharadwaj
committed
add config validations and tests for these validations
1 parent 0862823 commit 44f709d

File tree

4 files changed

+211
-41
lines changed

4 files changed

+211
-41
lines changed

‎internal/controller/tortoise_controller_test.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import (
2424
"sigs.k8s.io/yaml"
2525

2626
"github.com/mercari/tortoise/api/v1beta3"
27-
config_file "github.com/mercari/tortoise/pkg/config"
2827
"github.com/mercari/tortoise/pkg/deployment"
2928
"github.com/mercari/tortoise/pkg/features"
3029
"github.com/mercari/tortoise/pkg/hpa"
@@ -261,7 +260,7 @@ func startController(ctx context.Context) func() {
261260
cli, err := vpa.New(mgr.GetConfig(), recorder)
262261
Expect(err).ShouldNot(HaveOccurred())
263262

264-
hpaS, err := hpa.New(mgr.GetClient(), recorder, 0.95, 90, 25, time.Hour, nil, 1000, 10000, 3, []config_file.ServiceGroup{}, []config_file.MaximumMaxReplicasPerGroup{}, ".*-exclude-metric")
263+
hpaS, err := hpa.New(mgr.GetClient(), recorder, 0.95, 90, 25, time.Hour, nil, 1000, 10000, 3, nil, nil, ".*-exclude-metric")
265264
Expect(err).ShouldNot(HaveOccurred())
266265
reconciler := &TortoiseReconciler{
267266
Scheme: scheme,

‎pkg/config/config.go‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -459,40 +459,49 @@ func validate(config *Config) error {
459459
}
460460

461461
// Ensure ServiceGroupNames in MaximumMaxReplicas match defined ServiceGroups
462-
serviceGroupMap := make(map[string]bool)
463-
for _, sg := range config.ServiceGroups {
464-
serviceGroupMap[sg.Name] = true
465-
}
466-
467-
for _, maxReplicas := range config.MaximumMaxReplicasPerService {
468-
if maxReplicas.ServiceGroupName != "" {
469-
if _, exists := serviceGroupMap[maxReplicas.ServiceGroupName]; !exists {
470-
return fmt.Errorf("ServiceGroupName %s in MaximumMaxReplicas is not defined in ServiceGroups", maxReplicas.ServiceGroupName)
471-
}
472-
}
473-
}
474-
475462
// Ensure no duplicates in ServiceGroups
476463
seenServiceGroups := make(map[string]bool)
464+
serviceGroupMap := make(map[string]bool)
477465
for _, sg := range config.ServiceGroups {
466+
if sg.Name == "" {
467+
return fmt.Errorf("name of the service group should not be empty")
468+
}
478469
if seenServiceGroups[sg.Name] {
479470
return fmt.Errorf("duplicate ServiceGroupName found: %s", sg.Name)
480471
}
481472
seenServiceGroups[sg.Name] = true
473+
serviceGroupMap[sg.Name] = true
482474
}
483475

476+
// Ensure MaximumMaxReplicasPerService is defined in ServiceGroups
484477
// Check all entries in MaximumMaxReplicasPerService have non-nil ServiceGroupName
485478
for _, maxReplicas := range config.MaximumMaxReplicasPerService {
486479
if maxReplicas.ServiceGroupName == "" {
487480
return fmt.Errorf("ServiceGroupName should not be nil in MaximumMaxReplicasPerService entries")
488481
}
482+
if _, exists := serviceGroupMap[maxReplicas.ServiceGroupName]; !exists {
483+
return fmt.Errorf("ServiceGroupName %s in MaximumMaxReplicas is not defined in ServiceGroups", maxReplicas.ServiceGroupName)
484+
}
485+
}
486+
487+
// Validate MaximumMinReplicas against default first, then service groups
488+
if config.MaximumMinReplicas > config.MaximumMaxReplicas {
489+
return fmt.Errorf("MaximumMinReplicas (%v) should be less than or equal to MaximumMaxReplicas (%v)", config.MaximumMinReplicas, config.MaximumMaxReplicas)
490+
}
491+
for _, group := range config.MaximumMaxReplicasPerService {
492+
if config.MaximumMinReplicas > group.MaximumMaxReplica {
493+
return fmt.Errorf("MaximumMinReplicas (%v) should be less than or equal to MaximumMaxReplica (%v) for service group %s", config.MaximumMinReplicas, group.MaximumMaxReplica, group.ServiceGroupName)
494+
}
489495
}
490496

491-
if config.MaximumMinReplicas > minOfMaximumMaxReplicas {
492-
return fmt.Errorf("MaximumMinReplicas should be less than or equal to MaximumMaxReplicas")
497+
// Validate PreferredMaxReplicas against default first, then service groups
498+
if config.PreferredMaxReplicas >= int(config.MaximumMaxReplicas) {
499+
return fmt.Errorf("PreferredMaxReplicas (%v) should be less than MaximumMaxReplicas (%v)", config.PreferredMaxReplicas, config.MaximumMaxReplicas)
493500
}
494-
if config.PreferredMaxReplicas >= int(minOfMaximumMaxReplicas) {
495-
return fmt.Errorf("PreferredMaxReplicas should be less than MaximumMaxReplicas")
501+
for _, group := range config.MaximumMaxReplicasPerService {
502+
if config.PreferredMaxReplicas >= int(group.MaximumMaxReplica) {
503+
return fmt.Errorf("PreferredMaxReplicas (%v) should be less than MaximumMaxReplica (%v) for service group %s", config.PreferredMaxReplicas, group.MaximumMaxReplica, group.ServiceGroupName)
504+
}
496505
}
497506
if config.PreferredMaxReplicas <= config.MinimumMinReplicas {
498507
return fmt.Errorf("PreferredMaxReplicas should be greater than or equal to MinimumMinReplicas")

‎pkg/config/config_test.go‎

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -578,6 +578,190 @@ func Test_validate(t *testing.T) {
578578
},
579579
wantErr: true,
580580
},
581+
// ServiceGroup validation test cases
582+
{
583+
name: "invalid ServiceGroup - empty service group name",
584+
config: &Config{
585+
RangeOfMinMaxReplicasRecommendationHours: 1,
586+
GatheringDataPeriodType: "weekly",
587+
HPATargetUtilizationMaxIncrease: 5,
588+
MinimumMinReplicas: 3,
589+
MaximumMinReplicas: 10,
590+
MaximumMaxReplicas: 100,
591+
PreferredMaxReplicas: 30,
592+
MaxAllowedScalingDownRatio: 0.8,
593+
ServiceGroups: []ServiceGroup{
594+
{
595+
Name: "", // Empty name should be invalid
596+
},
597+
},
598+
},
599+
wantErr: true,
600+
},
601+
{
602+
name: "invalid ServiceGroup - duplicate service group names",
603+
config: &Config{
604+
RangeOfMinMaxReplicasRecommendationHours: 1,
605+
GatheringDataPeriodType: "weekly",
606+
HPATargetUtilizationMaxIncrease: 5,
607+
MinimumMinReplicas: 3,
608+
MaximumMinReplicas: 10,
609+
MaximumMaxReplicas: 100,
610+
PreferredMaxReplicas: 30,
611+
MaxAllowedScalingDownRatio: 0.8,
612+
ServiceGroups: []ServiceGroup{
613+
{Name: "frontend"},
614+
{Name: "frontend"}, // Duplicate name should be invalid
615+
},
616+
},
617+
wantErr: true,
618+
},
619+
// MaximumMaxReplicasPerGroup validation test cases
620+
{
621+
name: "invalid MaximumMaxReplicasPerService - empty ServiceGroupName",
622+
config: &Config{
623+
RangeOfMinMaxReplicasRecommendationHours: 1,
624+
GatheringDataPeriodType: "weekly",
625+
HPATargetUtilizationMaxIncrease: 5,
626+
MinimumMinReplicas: 3,
627+
MaximumMinReplicas: 10,
628+
MaximumMaxReplicas: 100,
629+
PreferredMaxReplicas: 30,
630+
MaxAllowedScalingDownRatio: 0.8,
631+
ServiceGroups: []ServiceGroup{
632+
{Name: "frontend"},
633+
},
634+
MaximumMaxReplicasPerService: []MaximumMaxReplicasPerGroup{
635+
{
636+
ServiceGroupName: "", // Empty ServiceGroupName should be invalid
637+
MaximumMaxReplica: 50,
638+
},
639+
},
640+
},
641+
wantErr: true,
642+
},
643+
{
644+
name: "invalid MaximumMaxReplicasPerService - ServiceGroupName not defined in ServiceGroups",
645+
config: &Config{
646+
RangeOfMinMaxReplicasRecommendationHours: 1,
647+
GatheringDataPeriodType: "weekly",
648+
HPATargetUtilizationMaxIncrease: 5,
649+
MinimumMinReplicas: 3,
650+
MaximumMinReplicas: 10,
651+
MaximumMaxReplicas: 100,
652+
PreferredMaxReplicas: 30,
653+
MaxAllowedScalingDownRatio: 0.8,
654+
ServiceGroups: []ServiceGroup{
655+
{Name: "frontend"},
656+
},
657+
MaximumMaxReplicasPerService: []MaximumMaxReplicasPerGroup{
658+
{
659+
ServiceGroupName: "backend", // Not defined in ServiceGroups
660+
MaximumMaxReplica: 50,
661+
},
662+
},
663+
},
664+
wantErr: true,
665+
},
666+
{
667+
name: "invalid MaximumMaxReplicasPerService - negative MaximumMaxReplica",
668+
config: &Config{
669+
RangeOfMinMaxReplicasRecommendationHours: 1,
670+
GatheringDataPeriodType: "weekly",
671+
HPATargetUtilizationMaxIncrease: 5,
672+
MinimumMinReplicas: 3,
673+
MaximumMinReplicas: 10,
674+
MaximumMaxReplicas: 100,
675+
PreferredMaxReplicas: 30,
676+
MaxAllowedScalingDownRatio: 0.8,
677+
ServiceGroups: []ServiceGroup{
678+
{Name: "frontend"},
679+
},
680+
MaximumMaxReplicasPerService: []MaximumMaxReplicasPerGroup{
681+
{
682+
ServiceGroupName: "frontend",
683+
MaximumMaxReplica: -5, // Negative value should be invalid
684+
},
685+
},
686+
},
687+
wantErr: true,
688+
},
689+
{
690+
name: "invalid MaximumMinReplicas greater than service group MaximumMaxReplica",
691+
config: &Config{
692+
RangeOfMinMaxReplicasRecommendationHours: 1,
693+
GatheringDataPeriodType: "weekly",
694+
HPATargetUtilizationMaxIncrease: 5,
695+
MinimumMinReplicas: 3,
696+
MaximumMinReplicas: 15, // Greater than service group MaximumMaxReplica
697+
MaximumMaxReplicas: 100,
698+
PreferredMaxReplicas: 30,
699+
MaxAllowedScalingDownRatio: 0.8,
700+
ServiceGroups: []ServiceGroup{
701+
{Name: "frontend"},
702+
},
703+
MaximumMaxReplicasPerService: []MaximumMaxReplicasPerGroup{
704+
{
705+
ServiceGroupName: "frontend",
706+
MaximumMaxReplica: 10, // Less than MaximumMinReplicas
707+
},
708+
},
709+
},
710+
wantErr: true,
711+
},
712+
{
713+
name: "invalid PreferredMaxReplicas greater than service group MaximumMaxReplica",
714+
config: &Config{
715+
RangeOfMinMaxReplicasRecommendationHours: 1,
716+
GatheringDataPeriodType: "weekly",
717+
HPATargetUtilizationMaxIncrease: 5,
718+
MinimumMinReplicas: 3,
719+
MaximumMinReplicas: 10,
720+
MaximumMaxReplicas: 100,
721+
PreferredMaxReplicas: 25, // Greater than service group MaximumMaxReplica
722+
MaxAllowedScalingDownRatio: 0.8,
723+
ServiceGroups: []ServiceGroup{
724+
{Name: "frontend"},
725+
},
726+
MaximumMaxReplicasPerService: []MaximumMaxReplicasPerGroup{
727+
{
728+
ServiceGroupName: "frontend",
729+
MaximumMaxReplica: 20, // Less than PreferredMaxReplicas
730+
},
731+
},
732+
},
733+
wantErr: true,
734+
},
735+
{
736+
name: "valid ServiceGroups and MaximumMaxReplicasPerService configuration",
737+
config: &Config{
738+
RangeOfMinMaxReplicasRecommendationHours: 1,
739+
GatheringDataPeriodType: "weekly",
740+
HPATargetUtilizationMaxIncrease: 5,
741+
MinimumTargetResourceUtilization: 65,
742+
MaximumTargetResourceUtilization: 90,
743+
MinimumMinReplicas: 3,
744+
MaximumMinReplicas: 10,
745+
MaximumMaxReplicas: 100,
746+
PreferredMaxReplicas: 30,
747+
MaxAllowedScalingDownRatio: 0.8,
748+
ServiceGroups: []ServiceGroup{
749+
{Name: "frontend"},
750+
{Name: "backend"},
751+
},
752+
MaximumMaxReplicasPerService: []MaximumMaxReplicasPerGroup{
753+
{
754+
ServiceGroupName: "frontend",
755+
MaximumMaxReplica: 50,
756+
},
757+
{
758+
ServiceGroupName: "backend",
759+
MaximumMaxReplica: 80,
760+
},
761+
},
762+
},
763+
wantErr: false,
764+
},
581765
}
582766
for _, tt := range tests {
583767
t.Run(tt.name, func(t *testing.T) {

‎pkg/hpa/service.go‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -254,28 +254,6 @@ func (c *Service) syncHPAMetricsWithTortoiseAutoscalingPolicy(ctx context.Contex
254254
return currenthpa, tortoise, hpaEdited
255255
}
256256

257-
// TODO: move this to configuration
258-
var globalRecommendedHPABehavior = &v2.HorizontalPodAutoscalerBehavior{
259-
ScaleUp: &v2.HPAScalingRules{
260-
Policies: []v2.HPAScalingPolicy{
261-
{
262-
Type: v2.PercentScalingPolicy,
263-
Value: 100,
264-
PeriodSeconds: 60,
265-
},
266-
},
267-
},
268-
ScaleDown: &v2.HPAScalingRules{
269-
Policies: []v2.HPAScalingPolicy{
270-
{
271-
Type: v2.PercentScalingPolicy,
272-
Value: 2,
273-
PeriodSeconds: 90,
274-
},
275-
},
276-
},
277-
}
278-
279257
// Determine which service group is applicable for a given Tortoise using its selectors.
280258
func (c *Service) determineServiceGroup(tortoise *autoscalingv1beta3.Tortoise) string {
281259
tortoiseNamespace := tortoise.Namespace

0 commit comments

Comments
 (0)