Skip to content

Commit e2ab367

Browse files
naemononaemono
authored
Ignore remote_cluster_client role when calculating autoscaling status. (#7269) (#7287)
* Ignore remote_cluster_client role when calculating autoscaling status. * Ignore remote_cluster_client role in ML validations. * Add test for ignoring remote_cluster_client role in ML validations. --------- Signed-off-by: Michael Montgomery <[email protected]> (cherry picked from commit e23251c)
1 parent a541745 commit e2ab367

File tree

4 files changed

+121
-5
lines changed

4 files changed

+121
-5
lines changed

‎pkg/controller/autoscaling/elasticsearch/driver.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,9 @@ func newStatusBuilder(log logr.Logger, autoscalingPolicies v1alpha1.AutoscalingP
7878
sort.Strings(roles)
7979

8080
for _, role := range roles {
81+
if role == string(esv1.RemoteClusterClientRole) {
82+
continue
83+
}
8184
policies := policiesByRole[role]
8285
if len(policies) < 2 {
8386
// This role is declared in only one autoscaling policy

‎pkg/controller/autoscaling/elasticsearch/driver_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,17 +25,17 @@ func Test_newStatusBuilder(t *testing.T) {
2525
want []v1alpha1.AutoscalingPolicyStatus
2626
}{
2727
{
28-
name: "Initialize new status with overlapping policies",
28+
name: "Initialize new status with overlapping policies ignores remote_cluster_client role",
2929
args: args{
3030
autoscalingPolicies: []v1alpha1.AutoscalingPolicySpec{
3131
{
32-
NamedAutoscalingPolicy: v1alpha1.NamedAutoscalingPolicy{Name: "policy1", AutoscalingPolicy: v1alpha1.AutoscalingPolicy{Roles: []string{"role1", "role2"}}},
32+
NamedAutoscalingPolicy: v1alpha1.NamedAutoscalingPolicy{Name: "policy1", AutoscalingPolicy: v1alpha1.AutoscalingPolicy{Roles: []string{"role1", "role2", "remote_cluster_client"}}},
3333
},
3434
{
35-
NamedAutoscalingPolicy: v1alpha1.NamedAutoscalingPolicy{Name: "policy2", AutoscalingPolicy: v1alpha1.AutoscalingPolicy{Roles: []string{"role2", "role3", "role5"}}},
35+
NamedAutoscalingPolicy: v1alpha1.NamedAutoscalingPolicy{Name: "policy2", AutoscalingPolicy: v1alpha1.AutoscalingPolicy{Roles: []string{"role2", "role3", "role5", "remote_cluster_client"}}},
3636
},
3737
{
38-
NamedAutoscalingPolicy: v1alpha1.NamedAutoscalingPolicy{Name: "policy3", AutoscalingPolicy: v1alpha1.AutoscalingPolicy{Roles: []string{"role4", "role2", "role3"}}},
38+
NamedAutoscalingPolicy: v1alpha1.NamedAutoscalingPolicy{Name: "policy3", AutoscalingPolicy: v1alpha1.AutoscalingPolicy{Roles: []string{"role4", "role2", "role3", "remote_cluster_client"}}},
3939
},
4040
},
4141
},

‎pkg/controller/autoscaling/elasticsearch/validation/webhook_test.go‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -507,6 +507,92 @@ func TestValidateElasticsearchAutoscaler(t *testing.T) {
507507
},
508508
wantValidationError: ptr.String("ElasticsearchAutoscaler.autoscaling.k8s.elastic.co \"esa\" is invalid: Elasticsearch.spec.nodeSets[0]: Invalid value: []string{\"volume1\", \"volume2\"}: autoscaling supports only one volume claim"),
509509
},
510+
{
511+
name: "ML policy with roles [ml, remote_cluster_client] succeeds",
512+
args: args{
513+
es: es(map[string]string{}, map[string][]string{"nodeset-data-1": {"data", "remote_cluster_client"}, "ml": {"ml", "remote_cluster_client"}}, nil, "8.0.0"),
514+
esa: v1alpha1.ElasticsearchAutoscaler{
515+
ObjectMeta: metav1.ObjectMeta{Name: "esa", Namespace: "ns"},
516+
Spec: v1alpha1.ElasticsearchAutoscalerSpec{
517+
ElasticsearchRef: v1alpha1.ElasticsearchRef{
518+
Name: "es",
519+
},
520+
AutoscalingPolicySpecs: commonv1alpha1.AutoscalingPolicySpecs{
521+
{
522+
NamedAutoscalingPolicy: commonv1alpha1.NamedAutoscalingPolicy{
523+
Name: "data",
524+
AutoscalingPolicy: commonv1alpha1.AutoscalingPolicy{
525+
Roles: []string{"data", "remote_cluster_client"},
526+
Deciders: nil,
527+
},
528+
},
529+
AutoscalingResources: defaultResources,
530+
},
531+
{
532+
NamedAutoscalingPolicy: commonv1alpha1.NamedAutoscalingPolicy{
533+
Name: "ml",
534+
AutoscalingPolicy: commonv1alpha1.AutoscalingPolicy{
535+
Roles: []string{"ml", "remote_cluster_client"},
536+
Deciders: nil,
537+
},
538+
},
539+
AutoscalingResources: defaultResources,
540+
},
541+
},
542+
},
543+
},
544+
checker: yesCheck,
545+
},
546+
wantValidationError: nil,
547+
},
548+
{
549+
name: "2 ML policies with roles [ml, remote_cluster_client] and [ml] fails",
550+
args: args{
551+
es: es(map[string]string{}, map[string][]string{"nodeset-data-1": {"data", "remote_cluster_client"}, "ml1": {"ml", "remote_cluster_client"}, "ml2": {"ml"}}, nil, "8.0.0"),
552+
esa: v1alpha1.ElasticsearchAutoscaler{
553+
ObjectMeta: metav1.ObjectMeta{Name: "esa", Namespace: "ns"},
554+
Spec: v1alpha1.ElasticsearchAutoscalerSpec{
555+
ElasticsearchRef: v1alpha1.ElasticsearchRef{
556+
Name: "es",
557+
},
558+
AutoscalingPolicySpecs: commonv1alpha1.AutoscalingPolicySpecs{
559+
{
560+
NamedAutoscalingPolicy: commonv1alpha1.NamedAutoscalingPolicy{
561+
Name: "data",
562+
AutoscalingPolicy: commonv1alpha1.AutoscalingPolicy{
563+
Roles: []string{"data", "remote_cluster_client"},
564+
Deciders: nil,
565+
},
566+
},
567+
AutoscalingResources: defaultResources,
568+
},
569+
{
570+
NamedAutoscalingPolicy: commonv1alpha1.NamedAutoscalingPolicy{
571+
Name: "ml1",
572+
AutoscalingPolicy: commonv1alpha1.AutoscalingPolicy{
573+
Roles: []string{"ml", "remote_cluster_client"},
574+
Deciders: nil,
575+
},
576+
},
577+
AutoscalingResources: defaultResources,
578+
},
579+
{
580+
NamedAutoscalingPolicy: commonv1alpha1.NamedAutoscalingPolicy{
581+
Name: "ml2",
582+
AutoscalingPolicy: commonv1alpha1.AutoscalingPolicy{
583+
Roles: []string{"ml"},
584+
Deciders: nil,
585+
},
586+
},
587+
AutoscalingResources: defaultResources,
588+
},
589+
},
590+
},
591+
},
592+
checker: yesCheck,
593+
},
594+
wantValidationError: ptr.String("ElasticsearchAutoscaler.autoscaling.k8s.elastic.co \"esa\" is invalid: spec.policies[2].name: Invalid value: \"ml\": ML nodes must be in a dedicated NodeSet"),
595+
},
510596
}
511597
for _, tt := range tests {
512598
t.Run(tt.name, func(t *testing.T) {

‎pkg/controller/common/autoscaling/validations.go‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ func ValidateAutoscalingPolicies(
106106
) field.ErrorList {
107107
var errs field.ErrorList
108108
policyNames := set.Make()
109+
mlPolicyCount := 0
109110
rolesSet := make([][]string, 0, len(autoscalingPolicies))
110111
for i, autoscalingSpec := range autoscalingPolicies {
111112
// The name field is mandatory.
@@ -139,8 +140,22 @@ func ValidateAutoscalingPolicies(
139140
}
140141
}
141142

143+
if stringsutil.StringInSlice(string(esv1.MLRole), autoscalingSpec.Roles) {
144+
mlPolicyCount++
145+
}
146+
147+
if mlPolicyCount > 1 {
148+
errs = append(
149+
errs,
150+
field.Invalid(
151+
autoscalingSpecPath(i, "name"), strings.Join(autoscalingSpec.Roles, ","),
152+
"ML nodes must be in a dedicated NodeSet",
153+
),
154+
)
155+
}
156+
142157
// Machine learning nodes must be in a dedicated tier.
143-
if stringsutil.StringInSlice(string(esv1.MLRole), autoscalingSpec.Roles) && len(autoscalingSpec.Roles) > 1 {
158+
if stringsutil.StringInSlice(string(esv1.MLRole), autoscalingSpec.Roles) && len(ignoreRemoteClusterClientRole(autoscalingSpec.Roles)) > 1 {
144159
errs = append(
145160
errs,
146161
field.Invalid(
@@ -188,9 +203,21 @@ func ValidateAutoscalingPolicies(
188203
// Validate storage
189204
errs = validateQuantities(errs, autoscalingSpecPath, autoscalingSpec.StorageRange, i, "storage", minStorage)
190205
}
206+
191207
return errs
192208
}
193209

210+
// ignoreRemoteClusterClientRole will ignore the 'remote_cluster_client' role in a given slice of roles.
211+
func ignoreRemoteClusterClientRole(roles []string) []string {
212+
var updatedRoles []string
213+
for _, role := range roles {
214+
if role != string(esv1.RemoteClusterClientRole) {
215+
updatedRoles = append(updatedRoles, role)
216+
}
217+
}
218+
return updatedRoles
219+
}
220+
194221
// validateQuantities ensures that a quantity range is valid.
195222
func validateQuantities(
196223
errs field.ErrorList,

0 commit comments

Comments
 (0)