WIP: Implement migration from L4 Legacy Target Pool based services to L4 RBS #1721

panslava · 2022-05-26T15:03:27Z

Switch target of existing forwarding rule from target pool to regional
backend service
Clean up GCP resources used only by legacy service: target pool,
health checks, firewall rules for health checks

k8s-ci-robot · 2022-05-26T15:03:36Z

Hi @panslava. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2022-05-26T15:03:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: panslava
To complete the pull request process, please assign mrhohn after the PR has been reviewed.
You can assign the PR to them by writing /assign @mrhohn in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

kl52752

I don't see and testcases for this feature.

pkg/l4lb/l4netlbcontroller.go

kl52752 · 2022-05-27T08:00:35Z

pkg/loadbalancers/forwarding_rules.go

@@ -452,6 +466,19 @@ func (l4netlb *L4NetLB) deleteForwardingRule(name string, version meta.Version)
 	}
 }

+// ConfigsEqual check if two forwarding rules are equal in configurations, except for backend targets


and description :)
For ILB and NetLB we have different "configs" if this function is only used by NetLB we can skip AllowGlobalAccess and Subnetwork checks (and changed the name to point to ExternalConfig)

This is used not only for NetLB, I just took a part of Equals(fr1, fr2) (which was used by both ILB, NetLB) and moved it to separate function, so Equals now still calls ConfigsEqual

pkg/loadbalancers/l4netlb.go

pkg/utils/utils.go

pkg/composite/composite.go

cezarygerard · 2022-05-27T09:36:18Z

pkg/loadbalancers/forwarding_rules.go

+
+		// Check if we transition from Target Pool Legacy Service to RBS based one.
+		// If configs are not equal, we should delete forwarding rule and recreate.
+		if existingFwdRule.Target != "" && ConfigsEqual(fr, existingFwdRule) {


is this 'if' enought to identify transition from Target Pool Legacy Service to RBS based?

This is mostly business question, we can discuss it, but all the other functionality should stay pretty the same, so if needed we can change this condition

pkg/loadbalancers/forwarding_rules.go

cezarygerard · 2022-05-27T09:43:13Z

pkg/loadbalancers/l4netlb.go

+	lbRefStr := fmt.Sprintf("%v(%v)", loadBalancerName, serviceName)
+
+	errs := utilerrors.AggregateGoroutines(
+		func() error {


plese used named functions

cezarygerard · 2022-05-27T09:43:18Z

pkg/loadbalancers/l4netlb.go

+			err := utils.IgnoreHTTPNotFound(l4netlb.cloud.DeleteFirewall(fwName))
+			return err
+		},
+		func() error {


plese used named functions

cezarygerard · 2022-05-27T09:45:03Z

pkg/loadbalancers/l4netlb.go

+			// They both use legacy naming scheme, so they are not useful for RBS service, and it is safe to delete them
+			healthChecksToDelete := []string{loadBalancerName, gce.MakeNodesHealthCheckName(l4netlb.namer.ClusterID())}
+
+			for _, hcToDelete := range healthChecksToDelete {


plese used named functions

you can optimize this even furthrer by adding each HC deletion as one func in AggregateGoroutines

wait for PR/1705 to be merged

cezarygerard · 2022-05-27T09:48:56Z

pkg/loadbalancers/l4netlb_test.go

 )

+func ensureFrontend(l4netlb *L4NetLB, nodeNames []string, svc *v1.Service) *L4NetLBSyncResult {


why do you need this?

put int after TestXXX funcs

pkg/utils/utils.go

- create a common singleton-like struct fot l4 health checks - new struct holds mutex for common resources (healthchecks and their firewall rules used for - [bugfix] delete shared healtcheck firewall rules safely - cross-check between ILB and NLB healthchecks is firewall rules are in use Logging New log line idicating firewall rule not deleted due to cross-check: "Failed to delete health check firewall rule %s: health check in use." Testing - healthcheck management is mostly covered (by existing tests), they required little update. - added test cases for sahred firewall rule deletion(lack of), named TestHealthCheckFirewallDeletionWithILB and TestHealthCheckFirewallDeletionWithNetLB - run test manual tests

panslava · 2022-05-27T12:51:32Z

Rebased on this #1705 , waiting to be merged

kl52752 · 2022-05-31T06:14:39Z

/ok-to-test

EnsureL4HealthCheck: replace lengthy return value list in with named struct Improve firewall rule comparison Added debug logs Improved go fmt Renamed function names And many more small ones

k8s-ci-robot · 2022-05-31T18:35:53Z

@panslava: PR needs rebase.

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.

- Switch target of existing forwarding rule from target pool to regional backend service - Clean up GCP resources used only by legacy service: target pool, health checks, firewall rules for health checks

k8s-ci-robot · 2022-06-02T08:34:33Z

@panslava: 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
pull-ingress-gce-test	`e7cb75c`	link	true	`/test pull-ingress-gce-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

k8s-triage-robot · 2022-09-01T14:04:57Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 26, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 26, 2022

k8s-ci-robot requested a review from MrHohn May 26, 2022 15:03

k8s-ci-robot requested a review from thockin May 26, 2022 15:03

panslava force-pushed the legacy-to-rbs-migration branch from 7fcc292 to cf60083 Compare May 26, 2022 15:05

kl52752 reviewed May 27, 2022

View reviewed changes

panslava force-pushed the legacy-to-rbs-migration branch 5 times, most recently from d8188e6 to a0f8a02 Compare May 27, 2022 09:42

cezarygerard reviewed May 27, 2022

View reviewed changes

panslava force-pushed the legacy-to-rbs-migration branch 2 times, most recently from 85344cc to 39f0c90 Compare May 27, 2022 10:51

k8s-ci-robot added 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 May 27, 2022

panslava force-pushed the legacy-to-rbs-migration branch 4 times, most recently from cf109d6 to 6400ba5 Compare May 30, 2022 13:23

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 31, 2022

Rewrite L4 healthchecks: Apply review comments

f8353dc

EnsureL4HealthCheck: replace lengthy return value list in with named struct Improve firewall rule comparison Added debug logs Improved go fmt Renamed function names And many more small ones

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2022

panslava force-pushed the legacy-to-rbs-migration branch from 6400ba5 to 05c7fd9 Compare June 1, 2022 08:40

panslava added 2 commits June 1, 2022 17:17

WIP: refactor l4netlb, split functions

630b188

Implement migration from L4 Legacy Target Pool based services to L4 RBS

e7cb75c

- Switch target of existing forwarding rule from target pool to regional backend service - Clean up GCP resources used only by legacy service: target pool, health checks, firewall rules for health checks

panslava force-pushed the legacy-to-rbs-migration branch from 05c7fd9 to e7cb75c Compare June 2, 2022 08:33

panslava changed the title ~~Implement migration from L4 Legacy Target Pool based services to L4 RBS~~ WIP: Implement migration from L4 Legacy Target Pool based services to L4 RBS Jun 3, 2022

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 1, 2022

panslava closed this Sep 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Implement migration from L4 Legacy Target Pool based services to L4 RBS #1721

WIP: Implement migration from L4 Legacy Target Pool based services to L4 RBS #1721

panslava commented May 26, 2022

k8s-ci-robot commented May 26, 2022

k8s-ci-robot commented May 26, 2022

kl52752 left a comment

kl52752 May 27, 2022

panslava May 27, 2022

cezarygerard May 27, 2022

panslava May 28, 2022

cezarygerard May 27, 2022

cezarygerard May 27, 2022

cezarygerard May 27, 2022

cezarygerard May 27, 2022

panslava commented May 27, 2022

kl52752 commented May 31, 2022

k8s-ci-robot commented May 31, 2022

k8s-ci-robot commented Jun 2, 2022

k8s-triage-robot commented Sep 1, 2022

		)

		func ensureFrontend(l4netlb L4NetLB, nodeNames []string, svc v1.Service) *L4NetLBSyncResult {

WIP: Implement migration from L4 Legacy Target Pool based services to L4 RBS #1721

WIP: Implement migration from L4 Legacy Target Pool based services to L4 RBS #1721

Conversation

panslava commented May 26, 2022

k8s-ci-robot commented May 26, 2022

k8s-ci-robot commented May 26, 2022

kl52752 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panslava commented May 27, 2022

kl52752 commented May 31, 2022

k8s-ci-robot commented May 31, 2022

k8s-ci-robot commented Jun 2, 2022

k8s-triage-robot commented Sep 1, 2022