Removal of caching for service networks #391

erikfuller · 2023-09-08T19:39:09Z

What type of PR is this?
cleanup

Which issue does this PR fix:
n/a

What does this PR do / Why do we need it:
This is the first of what will be several refactors to eliminate the local LatticeDataStore cache. Currently, use of the data store results in inconsistent behaviour across the codebase and encourages side-effect programming as many decisions are made based on the presence of or state of the data in the cache rather than using the Lattice APIs as the source of truth. Longer term, this approach should reduce the surface area for consistency bugs or race conditions.

This PR eliminates use of the data store for service network operations, instead calling the VPC Lattice API directly. This increases call volume to the ListServiceNetworks API, but centralizes this logic in a way that should be more reliably cacheable in future.

Additional changes include minor refactoring for readability, typos, or consistency in naming.

Testing done on this change:
Unit tests added and some modified or removed as appropriate.

ok  	github.com/aws/aws-application-networking-k8s/pkg/aws	0.976s	coverage: 24.2% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/aws/services	0.643s	coverage: 5.8% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/config	0.488s	coverage: 32.9% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/deploy	1.134s	coverage: 20.6% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/deploy/externaldns	2.572s	coverage: 77.1% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/deploy/lattice	1.599s	coverage: 85.5% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/gateway	2.248s	coverage: 74.7% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/latticestore	2.583s	coverage: 70.3% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/model/core	2.047s	coverage: 61.7% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/model/core/graph	1.997s	coverage: 17.2% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/utils	1.735s	coverage: 18.8% of statements
ok  	github.com/aws/aws-application-networking-k8s/controllers	0.661s	coverage: 0.0% of statements
ok  	github.com/aws/aws-application-networking-k8s/controllers/eventhandlers	0.970s	coverage: 54.8% of statements

Ran e2e tests.

Ran 29 of 29 Specs in 1881.602 seconds
SUCCESS! -- 29 Passed | 0 Failed | 0 Pending | 0 Skipped

Automation added to e2e:
No new e2e tests

Will this PR introduce any new dependencies?:
No

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

Does this PR introduce any user-facing change?:
No

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

pkg/utils/common.go

pkg/aws/services/vpclattice.go

pkg/deploy/lattice/service_manager.go

coveralls · 2023-09-08T21:41:20Z

Pull Request Test Coverage Report for Build 6126663228

95 of 119 (79.83%) changed or added relevant lines in 9 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.1%) to 38.98%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/deploy/stack_deployer.go	0	1	0.0%
pkg/aws/services/vpclattice.go	42	46	91.3%
controllers/gateway_controller.go	0	8	0.0%
pkg/aws/services/vpclattice_mocks.go	0	11	0.0%

Totals
Change from base Build 6114920742:	-0.1%
Covered Lines:	3990
Relevant Lines:	10236

💛 - Coveralls

…ding nil checks for result

mikhail-aws

You can address comments in following PR. The deleteServiceNetwork function that return string stands out.

mikhail-aws · 2023-09-08T23:58:36Z

pkg/deploy/lattice/service_manager.go

+	if snInfo == nil {
+		return fmt.Errorf("Service network %s for account %s not found", snName, m.cloud.Config().AccountId)
+	}
+


I think we should look again into returning nil as not found later. We may find that there is no code that tolerates "not found" result. That will save us from doing double error checking - for err and for nil.

I agree. Currently there were only two places, but as I do more resource types I'll see which pattern makes more sense.

mikhail-aws · 2023-09-09T00:01:14Z

pkg/deploy/lattice/service_network_synthesizer.go

+
+	// TODO need to check if service network is referenced by gateway in other namespace
+	gwList := &gateway_api.GatewayList{}
+	s.Client.List(context.TODO(), gwList)


nit: use context from function argument - ctx, not context.TODO()

This was a straight refactor-to-method, but happy to update when I see these.

mikhail-aws · 2023-09-09T00:06:05Z

pkg/deploy/lattice/service_network_synthesizer.go

+
+	err := s.serviceNetworkManager.Delete(ctx, resServiceNetwork.Spec.Name)
+	if err != nil {
+		return LATTICE_RETRY


nit: return or log original error. For example:

return fmt.Errorf("%w: %w", RetryErr, err)

https://github.com/aws/aws-application-networking-k8s/blob/main/pkg/deploy/lattice/error.go#L13

Example: https://github.com/aws/aws-application-networking-k8s/blob/main/pkg/deploy/lattice/service_manager.go#L207-L214

This was also a straight copy/paste/new method refactor, so for now I'll just add a log line. I'm hesitant to change too much of the flow logic at once.

mikhail-aws · 2023-09-09T00:07:08Z

pkg/deploy/lattice/service_network_synthesizer.go

 	}
+}
+
+func (s *serviceNetworkSynthesizer) deleteServiceNetwork(ctx context.Context, resServiceNetwork *latticemodel.ServiceNetwork) string {


should return error

I don't disagree, but I was trying to minimize logic changes in the refactor. Previously, this code only set a return string.

Removal of caching for service networks

7695d6d

mikhail-aws suggested changes Sep 8, 2023

View reviewed changes

liwenwu-amazon reviewed Sep 8, 2023

View reviewed changes

pkg/deploy/lattice/service_manager.go Show resolved Hide resolved

Removal of caching for service networks - Addressing PR feedback

eb4e7f3

Removal of caching for service networks - Addressing PR feedback - ad…

1b6f883

…ding nil checks for result

mikhail-aws approved these changes Sep 9, 2023

View reviewed changes

erikfuller merged commit 6355a03 into aws:main Sep 11, 2023

erikfuller mentioned this pull request Sep 13, 2023

Removal of caching for services #394

Merged

Removal of caching for service networks #391

Removal of caching for service networks #391

Uh oh!

Conversation

erikfuller commented Sep 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 6126663228

💛 - Coveralls

Uh oh!

mikhail-aws left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Sep 8, 2023 •

edited

Loading