From fd38ca55816081d86c8777c5b85ce7980a3b60b6 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 5 Nov 2024 15:10:16 +0800 Subject: [PATCH 1/8] add failure test for TestGetQualityOfService_ExecutionCreateRequest --- .../executions/quality_of_service_test.go | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go b/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go index 41a04ec2bc..82a3ced372 100644 --- a/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go +++ b/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go @@ -35,6 +35,15 @@ func getQualityOfServiceWithDuration(duration time.Duration) *core.QualityOfServ } } +func getQualityOfServiceWithNilDuration() *core.QualityOfService { + return &core.QualityOfService{ + Designation: &core.QualityOfService_Spec{ + Spec: &core.QualityOfServiceSpec{ + QueueingBudget: nil, + }, + }, + } +} func getMockConfig() runtimeInterfaces.Configuration { mockConfig := mocks.NewMockConfigurationProvider(nil, nil, nil, nil, nil, nil) provider := &runtimeIFaceMocks.QualityOfServiceConfiguration{} @@ -116,6 +125,23 @@ func TestGetQualityOfService_ExecutionCreateRequest(t *testing.T) { }) assert.Nil(t, err) assert.EqualValues(t, spec.QueuingBudget, 3*time.Minute) + + _, failError := allocator.GetQualityOfService(context.Background(), GetQualityOfServiceInput{ + Workflow: getWorkflowWithQosSpec(getQualityOfServiceWithDuration(4 * time.Minute)), + LaunchPlan: &admin.LaunchPlan{ + Spec: &admin.LaunchPlanSpec{ + QualityOfService: getQualityOfServiceWithDuration(2 * time.Minute), + }, + }, + ExecutionCreateRequest: &admin.ExecutionCreateRequest{ + Domain: "production", + Spec: &admin.ExecutionSpec{ + QualityOfService: getQualityOfServiceWithNilDuration(), + }, + }, + }) + assert.Error(t, failError) + } func TestGetQualityOfService_LaunchPlan(t *testing.T) { From 922ae834be673a9b779a4a849153a1868c6c7d28 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 5 Nov 2024 15:34:18 +0800 Subject: [PATCH 2/8] fix old launch plan nil pointer bug --- flyteadmin/pkg/manager/impl/executions/quality_of_service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flyteadmin/pkg/manager/impl/executions/quality_of_service.go b/flyteadmin/pkg/manager/impl/executions/quality_of_service.go index a96d99d3d6..1f9bf956f8 100644 --- a/flyteadmin/pkg/manager/impl/executions/quality_of_service.go +++ b/flyteadmin/pkg/manager/impl/executions/quality_of_service.go @@ -110,7 +110,7 @@ func (q qualityOfServiceAllocator) GetQualityOfService(ctx context.Context, inpu return QualityOfServiceSpec{}, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "Invalid custom quality of service set in launch plan [%v], failed to parse duration [%v] with: %v", input.LaunchPlan.Id, - input.ExecutionCreateRequest.Spec.QualityOfService.GetSpec().QueueingBudget, err) + input.LaunchPlan.Spec.QualityOfService.GetSpec().QueueingBudget, err) } return QualityOfServiceSpec{ QueuingBudget: duration, From 1751332e2d21d078084c468e3754aed0f5f40c6d Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 5 Nov 2024 15:40:30 +0800 Subject: [PATCH 3/8] add fail test case of TestGetQualityOfService_LaunchPlan --- .../impl/executions/quality_of_service_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go b/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go index 82a3ced372..af0582fcc2 100644 --- a/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go +++ b/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go @@ -163,6 +163,20 @@ func TestGetQualityOfService_LaunchPlan(t *testing.T) { }) assert.Nil(t, err) assert.EqualValues(t, spec.QueuingBudget, 2*time.Minute) + + _, failError := allocator.GetQualityOfService(context.Background(), GetQualityOfServiceInput{ + Workflow: getWorkflowWithQosSpec(getQualityOfServiceWithDuration(4 * time.Minute)), + LaunchPlan: &admin.LaunchPlan{ + Spec: &admin.LaunchPlanSpec{ + QualityOfService: getQualityOfServiceWithNilDuration(), + }, + }, + ExecutionCreateRequest: &admin.ExecutionCreateRequest{ + Domain: "production", + Spec: &admin.ExecutionSpec{}, + }, + }) + assert.Error(t, failError) } func TestGetQualityOfService_Workflow(t *testing.T) { From 0755eb1aa16bf5db1b424d1053456578c73c5154 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 5 Nov 2024 15:56:10 +0800 Subject: [PATCH 4/8] fix old workflow nil pointer bug --- flyteadmin/pkg/manager/impl/executions/quality_of_service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flyteadmin/pkg/manager/impl/executions/quality_of_service.go b/flyteadmin/pkg/manager/impl/executions/quality_of_service.go index 1f9bf956f8..4557a1d77f 100644 --- a/flyteadmin/pkg/manager/impl/executions/quality_of_service.go +++ b/flyteadmin/pkg/manager/impl/executions/quality_of_service.go @@ -129,7 +129,7 @@ func (q qualityOfServiceAllocator) GetQualityOfService(ctx context.Context, inpu return QualityOfServiceSpec{}, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "Invalid custom quality of service set in workflow [%v], failed to parse duration [%v] with: %v", workflowIdentifier, - input.ExecutionCreateRequest.Spec.QualityOfService.GetSpec().QueueingBudget, err) + input.Workflow.Closure.CompiledWorkflow.Primary.Template.Metadata.QualityOfService.GetSpec().QueueingBudget, err) } return QualityOfServiceSpec{ QueuingBudget: duration, From 6282eca9c6fd8177528511d16791246770ee2870 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 5 Nov 2024 15:56:40 +0800 Subject: [PATCH 5/8] add fail test case of TestGetQualityOfService_Workflow --- .../impl/executions/quality_of_service_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go b/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go index af0582fcc2..e478f40639 100644 --- a/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go +++ b/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go @@ -196,6 +196,19 @@ func TestGetQualityOfService_Workflow(t *testing.T) { }) assert.Nil(t, err) assert.EqualValues(t, spec.QueuingBudget, 4*time.Minute) + + _, failError := allocator.GetQualityOfService(context.Background(), GetQualityOfServiceInput{ + Workflow: getWorkflowWithQosSpec(getQualityOfServiceWithNilDuration()), + LaunchPlan: &admin.LaunchPlan{ + Spec: &admin.LaunchPlanSpec{}, + }, + ExecutionCreateRequest: &admin.ExecutionCreateRequest{ + Domain: "production", + Spec: &admin.ExecutionSpec{}, + }, + }) + assert.Error(t, failError) + } func TestGetQualityOfService_MatchableResource(t *testing.T) { From d51148b78e44c53616a4c27f27e308923200fbb0 Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 6 Nov 2024 11:25:43 +0800 Subject: [PATCH 6/8] fix old nil pointer bug --- flyteadmin/pkg/manager/impl/executions/quality_of_service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flyteadmin/pkg/manager/impl/executions/quality_of_service.go b/flyteadmin/pkg/manager/impl/executions/quality_of_service.go index 4557a1d77f..f40c4b0c81 100644 --- a/flyteadmin/pkg/manager/impl/executions/quality_of_service.go +++ b/flyteadmin/pkg/manager/impl/executions/quality_of_service.go @@ -154,7 +154,7 @@ func (q qualityOfServiceAllocator) GetQualityOfService(ctx context.Context, inpu return QualityOfServiceSpec{}, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "Invalid custom quality of service set in overridable matching attributes for [%v],"+ "failed to parse duration [%v] with: %v", workflowIdentifier, - input.ExecutionCreateRequest.Spec.QualityOfService.GetSpec().QueueingBudget, err) + qualityOfService.GetSpec().QueueingBudget, err) } return QualityOfServiceSpec{ QueuingBudget: duration, From 915c8dc59db35b8b64b0f0124fddb62f6bd19968 Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 6 Nov 2024 11:27:01 +0800 Subject: [PATCH 7/8] add error case test for quality of service from matchable attributes resource table --- .../executions/quality_of_service_test.go | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go b/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go index e478f40639..335b5d390c 100644 --- a/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go +++ b/flyteadmin/pkg/manager/impl/executions/quality_of_service_test.go @@ -2,6 +2,7 @@ package executions import ( "context" + "errors" "testing" "time" @@ -44,6 +45,7 @@ func getQualityOfServiceWithNilDuration() *core.QualityOfService { }, } } + func getMockConfig() runtimeInterfaces.Configuration { mockConfig := mocks.NewMockConfigurationProvider(nil, nil, nil, nil, nil, nil) provider := &runtimeIFaceMocks.QualityOfServiceConfiguration{} @@ -87,6 +89,35 @@ func addGetResourceFunc(t *testing.T, resourceManager interfaces.ResourceInterfa } } +func addGetErrorResourceFunc(t *testing.T, resourceManager interfaces.ResourceInterface, containSpec bool) { + resourceManager.(*managerMocks.MockResourceManager).GetResourceFunc = func(ctx context.Context, + request interfaces.ResourceRequest) (*interfaces.ResourceResponse, error) { + assert.EqualValues(t, request, interfaces.ResourceRequest{ + Project: workflowIdentifier.Project, + Domain: workflowIdentifier.Domain, + Workflow: workflowIdentifier.Name, + ResourceType: admin.MatchableResource_QUALITY_OF_SERVICE_SPECIFICATION, + }) + noSourceError := errors.New("no resource") + + noServiceResponse := &interfaces.ResourceResponse{ + Attributes: &admin.MatchingAttributes{ + Target: &admin.MatchingAttributes_QualityOfService{ + QualityOfService: getQualityOfServiceWithNilDuration(), + }, + }, + } + + result, error := &interfaces.ResourceResponse{}, noSourceError + + if containSpec { + result, error = noServiceResponse, nil + } + + return result, error + } +} + func getWorkflowWithQosSpec(qualityOfService *core.QualityOfService) *admin.Workflow { return &admin.Workflow{ Id: workflowIdentifier, @@ -228,6 +259,33 @@ func TestGetQualityOfService_MatchableResource(t *testing.T) { }) assert.Nil(t, err) assert.EqualValues(t, spec.QueuingBudget, 5*time.Minute) + + addGetErrorResourceFunc(t, &resourceManager, false) + _, failError := allocator.GetQualityOfService(context.Background(), GetQualityOfServiceInput{ + Workflow: getWorkflowWithQosSpec(nil), + LaunchPlan: &admin.LaunchPlan{ + Spec: &admin.LaunchPlanSpec{}, + }, + ExecutionCreateRequest: &admin.ExecutionCreateRequest{ + Domain: "production", + Spec: &admin.ExecutionSpec{}, + }, + }) + assert.Error(t, failError) + + addGetErrorResourceFunc(t, &resourceManager, true) + _, noSpecError := allocator.GetQualityOfService(context.Background(), GetQualityOfServiceInput{ + Workflow: getWorkflowWithQosSpec(nil), + LaunchPlan: &admin.LaunchPlan{ + Spec: &admin.LaunchPlanSpec{}, + }, + ExecutionCreateRequest: &admin.ExecutionCreateRequest{ + Domain: "production", + Spec: &admin.ExecutionSpec{}, + }, + }) + assert.Error(t, noSpecError) + } func TestGetQualityOfService_ConfigValues(t *testing.T) { From 8a2b43018fb899a0f5a9a7a395c5ddd06180e70d Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 6 Nov 2024 12:00:29 +0800 Subject: [PATCH 8/8] fix old nil pointer bug --- flyteadmin/pkg/manager/impl/executions/quality_of_service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flyteadmin/pkg/manager/impl/executions/quality_of_service.go b/flyteadmin/pkg/manager/impl/executions/quality_of_service.go index f40c4b0c81..e2a32debdd 100644 --- a/flyteadmin/pkg/manager/impl/executions/quality_of_service.go +++ b/flyteadmin/pkg/manager/impl/executions/quality_of_service.go @@ -163,7 +163,7 @@ func (q qualityOfServiceAllocator) GetQualityOfService(ctx context.Context, inpu logger.Debugf(ctx, "Determining quality of service tier from database override for [%s/%s/%s]", input.ExecutionCreateRequest.Project, input.ExecutionCreateRequest.Domain, input.ExecutionCreateRequest.Name) - qualityOfServiceTier = input.Workflow.Closure.CompiledWorkflow.Primary.Template.Metadata.QualityOfService.GetTier() + qualityOfServiceTier = qualityOfService.GetTier() } }