Fix ASA tenancy #3818

la4ezar · 2024-04-17T08:31:54Z

Description
When we delete a runtime that has ASA for it we should delete the associated scenarios labels and ASA for it. The problem is that the scenarios label is in the parent tenant and we are using the child tenant which comes from the context. But there are no such records in the database(for the child) and we don't process the scenarios labels, ASAs, and don't unassign the runtime from the formation.

Changes proposed in this pull request:

Fix ASA tenancy

Related issue(s)

...

Pull Request status

# Conflicts: # chart/compass/values.yaml

# Conflicts: # chart/compass/values.yaml # components/director/internal/domain/formation/service.go

…nancy

StanislavStefanov · 2024-05-10T14:41:19Z

components/director/internal/domain/formation/service.go

@@ -1499,7 +1503,7 @@ func (s *service) DeleteAutomaticScenarioAssignment(ctx context.Context, in *mod
 		return errors.Wrap(err, "while deleting the Assignment")
 	}

-	if err = s.asaEngine.RemoveAssignedScenario(ctx, in, s.UnassignFormation); err != nil {
+	if err = s.asaEngine.RemoveAssignedScenario(ctx, in, s.UnassignFormationIgnoreASA); err != nil {


Using UnassignFormationIgnoreASA instead of UnassignFormation is not needed as in RemoveAssignedScenario the ASA is being deleted before using the processing function for unassign.

StanislavStefanov · 2024-05-10T14:54:08Z

components/director/internal/domain/formation/unassign_formation_test.go

@@ -267,6 +267,7 @@ func TestServiceUnassignFormation(t *testing.T) {
 			ObjectType                    graphql.FormationObjectType
 			ObjectID                      string
 			InputFormation                model.Formation
+			ShouldSkipASA                 bool


Add test where the value is true and the scenario is comming from ASA to validate that it will be deleted.

StanislavStefanov · 2024-05-10T14:55:11Z

components/director/internal/domain/runtime/resolver.go

@@ -862,35 +863,46 @@ func (r *Resolver) EventingConfiguration(ctx context.Context, obj *graphql.Runti
 // deleteAssociatedScenarioAssignments ensures that scenario assignments which are responsible for creation of certain runtime labels are deleted,


Update the comment as the ASAs are not actualy deleted now

StanislavStefanov · 2024-05-10T15:03:39Z

components/director/internal/domain/runtime/resolver.go

+	for _, parentTenantID := range tntMapping.Parents {
+		ctxWithParentTenant := tenant.SaveToContext(ctx, parentTenantID, "")
+
+		scenariosLbl, err := r.runtimeService.GetLabel(ctxWithParentTenant, runtimeID, model.ScenariosKey)


The scenarioLabel will soon be removed. Instead of relying on it we should list the ASAs where the Tenant property of the ASA is amoung the tntMapping.Parents. Then we can remove the assigned scenarios using the ASAs. This will reduce the complexity and will remove unneeded calls to the DB.

StanislavStefanov · 2024-05-10T15:07:31Z

components/director/internal/domain/formation/service.go

@@ -1509,7 +1513,7 @@ func (s *service) DeleteAutomaticScenarioAssignment(ctx context.Context, in *mod
 // RemoveAssignedScenarios removes all the scenarios that are coming from any of the provided ASAs
 func (s *service) RemoveAssignedScenarios(ctx context.Context, in []*model.AutomaticScenarioAssignment) error {


Can we rename the method to something like UnassignFormationsCommingFromASA. The current name is a bit missleading.

Same applies to the asaEngine method as well

StanislavStefanov · 2024-05-10T15:16:29Z

components/director/internal/domain/scenarioassignment/repository.go

+
+	conditions := repo.Or(repo.ConditionTreesFromConditions(scenarioConditions)...)
+
+	if err := r.conditionLister.ListConditionTree(ctx, resource.AutomaticScenarioAssigment, tenantID, &collection, conditions); err != nil {


You can use the standard lister with repo.NewInConditionForStringValues(scenarioColumn, scenarioNames) instead of this one

StanislavStefanov · 2024-05-10T15:20:48Z

components/director/internal/domain/runtime/resolver.go

@@ -862,35 +863,46 @@ func (r *Resolver) EventingConfiguration(ctx context.Context, obj *graphql.Runti
 // deleteAssociatedScenarioAssignments ensures that scenario assignments which are responsible for creation of certain runtime labels are deleted,
 // if runtime doesn't have the scenarios label or is part of a scenario for which no scenario assignment exists => noop
 func (r *Resolver) deleteAssociatedScenarioAssignments(ctx context.Context, runtimeID string) error {


Rename the method as we no longer delete the ASAs

ognyvrac · 2024-05-13T09:26:22Z

/hold

StanislavStefanov · 2024-05-13T14:55:43Z

components/director/internal/domain/runtime/resolver.go

-		}
-
-		scenarios, err := labelPkg.ValueToStringsSlice(scenariosLbl.Value)
+		scenarios, err := r.asaEngine.GetScenariosFromMatchingASAs(ctx, runtimeID, graphql.FormationObjectTypeRuntime)


use ctxWithParentTenant

StanislavStefanov · 2024-05-13T14:57:41Z

components/director/internal/domain/scenarioassignment/repository.go

@@ -37,6 +38,7 @@ type repository struct {
 	creator         repo.CreatorGlobal
 	singleGetter    repo.SingleGetter
 	lister          repo.Lister
+	conditionLister repo.ConditionTreeLister


this lister is unused

# Conflicts: # chart/compass/values.yaml # components/director/internal/domain/root_resolver.go

kyma-bot · 2024-05-15T13:13:39Z

@la4ezar: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pre-main-compass-gke-benchmark	`fdf5ed3`	link	false	`/test pre-main-compass-gke-benchmark`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

kyma-bot · 2024-05-16T11:56:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: emmakarova, ognyvrac, StanislavStefanov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [StanislavStefanov,emmakarova,ognyvrac]
~~components/director/internal/domain/certsubjectmapping/OWNERS~~ [StanislavStefanov,ognyvrac]
~~components/director/internal/domain/formation/OWNERS~~ [StanislavStefanov,ognyvrac]
~~components/director/internal/domain/formationconstraint/OWNERS~~ [StanislavStefanov,ognyvrac]
~~components/director/internal/domain/formationtemplateconstraintreferences/OWNERS~~ [StanislavStefanov,ognyvrac]
~~components/director/internal/tenantfetchersvc/OWNERS~~ [emmakarova]
~~tests/director/tests/formation/OWNERS~~ [StanislavStefanov,ognyvrac]
~~tests/director/tests/runtime/OWNERS~~ [StanislavStefanov,ognyvrac]
~~tests/ord-service/OWNERS~~ [StanislavStefanov,ognyvrac]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Initial implementation.

8a8dad5

la4ezar requested review from a team as code owners April 17, 2024 08:31

kyma-bot requested review from DimitarPetrov and KirilKabakchiev April 17, 2024 08:32

kyma-bot added 🦖 team-raptor Team Raptor Label cla: yes Indicates the PR's author has signed the CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 17, 2024

Lachezar Bogomilov added 5 commits April 17, 2024 11:37

Update values.yaml

5d77607

Merge branch 'main' into fix-asa-tenancy

c7319f8

# Conflicts: # chart/compass/values.yaml

Make linter happy.

a7faf37

Make linter happy.

dada7a3

Initial implementation.

5d61264

kyma-bot added 🦅 team-falcon Team Falcon Label size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 24, 2024

la4ezar and others added 10 commits April 24, 2024 10:27

Merge branch 'main' into fix-asa-tenancy

a783fda

Partial implementation of Unassign when there is ASA.

4a4057e

Partial implementation of Unassign when there is ASA.

fb8a9f5

Full implementation of Unassign when there is ASA and unit tests.

5c9dbae

Merge branch 'main' into fix-asa-tenancy

c82251b

# Conflicts: # chart/compass/values.yaml # components/director/internal/domain/formation/service.go

Merge branch 'main' into fix-asa-tenancy

9b883ed

Merge branch 'main' into fix-asa-tenancy

a63cba7

Fix tenancy, add unit tests and adapt e2e tests.

6e912c7

Merge remote-tracking branch 'origin/fix-asa-tenancy' into fix-asa-te…

17e11fd

…nancy

Merge branch 'main' into fix-asa-tenancy

2b89b62

PetarTodorovv added the 👋 request review Review required label May 10, 2024

PetarTodorovv assigned StanislavStefanov May 10, 2024

Merge branch 'main' into fix-asa-tenancy

fdf5ed3

StanislavStefanov suggested changes May 10, 2024

View reviewed changes

StanislavStefanov reviewed May 10, 2024

View reviewed changes

Lachezar Bogomilov and others added 3 commits May 13, 2024 12:21

Address PR comments.

5910187

Merge branch 'main' into fix-asa-tenancy

eabb405

Improve unit tests.

65fe858

kyma-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2024

Remove dead code.

5d90d9c

StanislavStefanov suggested changes May 13, 2024

View reviewed changes

StanislavStefanov reviewed May 13, 2024

View reviewed changes

Lachezar Bogomilov added 2 commits May 15, 2024 16:06

Address PR comments.

eae767a

Merge branch 'main' into fix-asa-tenancy

0750268

# Conflicts: # chart/compass/values.yaml # components/director/internal/domain/root_resolver.go

Lachezar Bogomilov and others added 2 commits May 15, 2024 16:43

Adapt unit test after merge.

34ba3e9

Delete chart/compass/values.yaml

397b71b

StanislavStefanov approved these changes May 16, 2024

View reviewed changes

kyma-bot added the lgtm Looks good to me! label May 16, 2024

ognyvrac approved these changes May 16, 2024

View reviewed changes

kyma-bot assigned ognyvrac May 16, 2024

emmakarova approved these changes May 16, 2024

View reviewed changes

kyma-bot assigned emmakarova May 16, 2024

kyma-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2024

la4ezar removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 16, 2024

kyma-bot merged commit ea62a40 into main May 16, 2024
7 checks passed

la4ezar deleted the fix-asa-tenancy branch May 16, 2024 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ASA tenancy #3818

Fix ASA tenancy #3818

la4ezar commented Apr 17, 2024

StanislavStefanov May 10, 2024

StanislavStefanov May 10, 2024

StanislavStefanov May 10, 2024

StanislavStefanov May 10, 2024

StanislavStefanov May 10, 2024

StanislavStefanov May 10, 2024

StanislavStefanov May 10, 2024

StanislavStefanov May 10, 2024

ognyvrac commented May 13, 2024

StanislavStefanov May 13, 2024

StanislavStefanov May 13, 2024

kyma-bot commented May 15, 2024 •

edited

Loading

kyma-bot commented May 16, 2024

		@@ -862,35 +863,46 @@ func (r Resolver) EventingConfiguration(ctx context.Context, obj graphql.Runti
		// deleteAssociatedScenarioAssignments ensures that scenario assignments which are responsible for creation of certain runtime labels are deleted,

		@@ -1509,7 +1513,7 @@ func (s service) DeleteAutomaticScenarioAssignment(ctx context.Context, in mod
		// RemoveAssignedScenarios removes all the scenarios that are coming from any of the provided ASAs
		func (s service) RemoveAssignedScenarios(ctx context.Context, in []model.AutomaticScenarioAssignment) error {


		conditions := repo.Or(repo.ConditionTreesFromConditions(scenarioConditions)...)

		if err := r.conditionLister.ListConditionTree(ctx, resource.AutomaticScenarioAssigment, tenantID, &collection, conditions); err != nil {

Fix ASA tenancy #3818

Fix ASA tenancy #3818

Conversation

la4ezar commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ognyvrac commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyma-bot commented May 15, 2024 • edited Loading

kyma-bot commented May 16, 2024

kyma-bot commented May 15, 2024 •

edited

Loading