Introduce multiple service group categories and maximumMaxReplicas for each of them #428
Conversation
pkg/hpa/service_test.go
Outdated
There was a problem hiding this comment.
You haven't added any tests for this enhancement. Please always make sure you add a test for every change basically. In this case, you have to add test cases at TestClient_UpdateHPAFromTortoiseRecommendation.
There was a problem hiding this comment.
Yes, I am still working on the tests.
AVSBharadwaj
force-pushed
the
multiple-service-groups-feature
branch
from
343c435 to
00134fe
Compare
AVSBharadwaj
force-pushed
the
multiple-service-groups-feature
branch
from
d6b89b9 to
c15336c
Compare
|
|
||
| // Ensure ServiceGroupNames in MaximumMaxReplicas match defined ServiceGroups | ||
| serviceGroupMap := make(map[string]bool) | ||
| for _, sg := range config.ServiceGroups { |
There was a problem hiding this comment.
| for _, sg := range config.ServiceGroups { | |
| for _, sg := range config.ServiceGroups { | |
| if sg.Name == "" { | |
| return fmt.Errorf("Name of the service group should not be empty") | |
| } |
| if seenServiceGroups[sg.Name] { | ||
| return fmt.Errorf("duplicate ServiceGroupName found: %s", sg.Name) | ||
| } |
There was a problem hiding this comment.
This duplicate check can be done in the above loop at L388.
|
|
||
| // Check all entries in MaximumMaxReplicasPerService have non-nil ServiceGroupName | ||
| for _, maxReplicas := range config.MaximumMaxReplicasPerService { | ||
| if maxReplicas.ServiceGroupName == "" { |
There was a problem hiding this comment.
this empty check could be done in the above loop at 393.
| if config.MaximumMinReplicas > minOfMaximumMaxReplicas { | ||
| return fmt.Errorf("MaximumMinReplicas should be less than or equal to MaximumMaxReplicas") | ||
| } | ||
| if config.PreferredMaxReplicas >= int(config.MaximumMaxReplicas) { | ||
| if config.PreferredMaxReplicas >= int(minOfMaximumMaxReplicas) { |
There was a problem hiding this comment.
I think config.MaximumMaxReplicas and minOfMaximumMaxReplicas should be validated with PreferredMaxReplicas and MaximumMinReplicas separately for better error messages.
There was a problem hiding this comment.
you should add test cases for all validations that you added.
| klog.InfoS("Namespace matched", "serviceGroup", serviceGroup.Name, "namespace", tortoiseNamespace) | ||
| return serviceGroup.Name | ||
| } | ||
| // TODO(avs): Add logic for matching labels in the future. |
There was a problem hiding this comment.
Why not within this PR? If you meant not to implement this within this PR, you should remove LabelSelector support from the config for now. (and add the one when you actually implement logic
code Implementation of this Issue: #426
Introduces:
default, fintech-service, critical-service, etcmaximumMaxReplicasvalue one for each of the service-categories mentioned above