Fix target group leaking and e2etest #510

solmonk · 2023-11-16T07:16:06Z

What type of PR is this?

bug

Which issue does this PR fix:

Use strongly consistent list call on ListTargetGroups, fixes target group leaking issue (which seems to happen more often when we have a lot of existing target groups)
Correctly handle when there are multiple target groups matching tags in findTargetGroup(). This can happen in the middle of TG protocol update, etc. Target groups only become unique after matching more fields including protocol. Changed related e2e test to make sure unused target groups are getting replaced by this logic.
Fix wrong e2e test cases on delete_target_group_test, deleting a deployment still results in target deregistration as it does not delete service.
Fix bug where e2e test will pick up existing other hosted zone and delete it
Made a few tests ordered. We are running tests sequentially anyways, and even in parallel execution we will likely keep one file per worker.
Made a few tweaks to be ready for parallel execution, it seems working with 2 procs but not sure if it is reliable.

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:

Testing done on this change:
make e2e-test

Automation added to e2e:

Will this PR introduce any new dependencies?:

Will this break upgrades or downgrades. Has updating a running cluster been tested?:

Does this PR introduce any user-facing change?:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

mikhail-aws · 2023-11-16T07:29:20Z

test/suites/integration/byoc_test.go

+	if aws.StringValue(hz.Name) != name {
+		return nil, nil
+	}


why name should not be equal? list zones should apply dns name filter
out, err := client.ListHostedZonesByName(&route53.ListHostedZonesByNameInput{DNSName: &name})

"Fix bug where e2e test will pick up existing other hosted zone and delete it"

There was intention to use existing hosted zone if test was terminated and zone leaked. Test re-run should pick-up previous zone and cleanup after. Otherwise need to manually delete them from route53.

ListHostedZonesByName always returns all hosted zones in the account. DNSName field only guarantees the first element that is getting picked up, and you still get other hosted zones in the following elements.

zijun726911 · 2023-11-16T16:35:02Z

pkg/deploy/lattice/target_group_manager.go

+			switch status {
+			case vpclattice.TargetGroupStatusCreateInProgress, vpclattice.TargetGroupStatusDeleteInProgress:
+				return nil, errors.New(LATTICE_RETRY)
+			case vpclattice.TargetGroupStatusDeleteFailed, vpclattice.TargetGroupStatusActive:


Do we need to return err for TargetGroupStatusDeleteFailed tg?

I don't think so, they do exist.

zijun726911 · 2023-11-16T16:44:54Z

test/pkg/test/framework.go

-				ResourceArn: service.Arn,
-			})
+			managed, err := env.Cloud.IsArnManaged(ctx, *service.Arn)
 			if err == nil { // for err != nil, it is possible that this service own by other account, and it is shared to current account by RAM


nit: change variable name to managedByCurrentCluster?

Seems this comment description is not really precise:
// for err != nil, it is possible that this service own by other account, and it is shared to current account by RAM

should be // for err != nil, it is possible that this lattice service is own by other cluster
not just limit to the RAM share

its failing to fetch tags so most of the time it will be other account, not other cluster within the same account. I can update the comment though.

zijun726911 · 2023-11-16T16:46:44Z

test/pkg/test/framework.go

 			}
 		}

 		retrievedTargetGroups, _ := env.LatticeClient.ListTargetGroupsAsList(ctx, &vpclattice.ListTargetGroupsInput{})


Do we need to pass vpcIdentifier in vpclattice.ListTargetGroupsInput{} ?

this makes sense

zijun726911 · 2023-11-16T16:47:31Z

test/pkg/test/framework.go

-					continue
-				}
+			managed, err := env.Cloud.IsArnManaged(ctx, *tg.Arn)
+			if err == nil { // for err != nil, it is possible that this service own by other account, and it is shared to current account by RAM


Need to change this comment, it is checking target group existing

missed that, will change

zijun726911 · 2023-11-16T17:27:48Z

pkg/deploy/lattice/target_group_manager.go

+			IpAddressType: latticeTg.Config.IpAddressType,
+			Type:          latticeTg.Type,
+			VpcIdentifier: latticeTg.Config.VpcIdentifier,
+		}, nil) // we already know that tags match


Just wondering should we add a Protocol(or other tg fields) tags for TargetGroup?
So that to make sure one FindResourcesByTags() api call could definitely exactly match one TG?

(not in this PR)

Not sure if it is necessary. Actually.. I wanted to remove as many tags as possible. I "had to" add protocolVersion in the tag because it is not available in TGSummary, so it needed extra Get() call to be figured out. Ideally VPC Lattice should just return protocolVersion in the summary instead.

zijun726911

LGTM, are we safe to add the --procs param in the Makefile?

solmonk · 2023-11-16T17:50:31Z

I have thought about that but I think it is at experimental stage yet. Will add that once I have more confidence

Doyoon Kim added 2 commits November 15, 2023 22:59

Fix target group leaking and e2etest

316edff

Fix grpcurl runner

bbb4da2

mikhail-aws reviewed Nov 16, 2023

View reviewed changes

Doyoon Kim added 3 commits November 15, 2023 23:51

Fix unit test

5d6c501

Fix more flaky/buggy e2e test cases

6feed90

Rollback conditional

d9de907

zijun726911 reviewed Nov 16, 2023

View reviewed changes

Address PR comments

854164f

zijun726911 approved these changes Nov 16, 2023

View reviewed changes

zijun726911 merged commit 6a6c8b5 into aws:main Nov 16, 2023

solmonk mentioned this pull request Nov 17, 2023

End-to-End Test Performance Enhancements #451

Open

Fix target group leaking and e2etest #510

Fix target group leaking and e2etest #510

Uh oh!

Conversation

solmonk commented Nov 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zijun726911 Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zijun726911 left a comment

Choose a reason for hiding this comment

Uh oh!

solmonk commented Nov 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zijun726911 Nov 16, 2023 •

edited

Loading