Cleanup backend namer workflow #861

skmatti · 2019-09-20T18:08:59Z

This is part 2 of V2 Namer Migration:#858 .

This does not change naming policy for backend resources. A light weight back-end namer interface is stubbed into each service port to retrieve back-end resource names. The current workflow passes in namer object to retrieve backend resources. This is not required anymore with new workflow.

k8s-ci-robot · 2019-09-20T18:09:08Z

Hi @skmatti. 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.

skmatti · 2019-09-20T18:09:37Z

/assign freehan

freehan · 2019-09-20T18:12:01Z

/ok-to-test

freehan · 2019-09-30T17:49:44Z

pkg/utils/namer/fenamer.go

+type LegacyIngressFrontendNamer struct {
+	ing    *v1beta1.Ingress
+	namer  LegacyFrontendNamer
+	lbName string


Since lbName is highly coupled with namer.Namer, I would recommend the following 2 options:

make lbName a specific type. (Can add a TODO and do it separately)

calculate lbName on the fly.

freehan · 2019-09-30T17:50:15Z

pkg/utils/namer/fenamer.go

+// LegacyIngressFrontendNamer implements IngressFrontendNamer. This is a wrapper on top of namer.Namer.
+type LegacyIngressFrontendNamer struct {
+	ing    *v1beta1.Ingress
+	namer  LegacyFrontendNamer


Anything in this package, you do not need to call Namer via interface.

I would recommend anything outside of this package to use an interface to interact with Namer.

freehan · 2019-09-30T17:52:08Z

pkg/utils/namer/interfaces.go

+}
+
+// LegacyFrontendNamer wraps frontend naming policy of namer.Namer.
+type LegacyFrontendNamer interface {


I would recommend not to duplicate the methods (e.g. ForwardingRule):

Only open the interface for the following methods. And use it in the L7s.

type LegacyFrontendNamerHelper interface { // LoadBalancer constructs a loadbalancer name from the given ingress key. LoadBalancer(ingKey string) string // LoadBalancerFromLbName reconstructs the full loadbalancer name, given the // lbName portion from NameComponents. LoadBalancerFromLbName(lbName string) string // ParseName parses the resource name of a resource generated by the namer. ParseName(resourceName string) *NameComponents // NameBelongsToCluster checks if a given resourceName is tagged with this // cluster's UID. NameBelongsToCluster(resourceName string) bool // ForwardingRule returns the name of the gce forwarding rule for given lbName and protocol. }

freehan · 2019-09-30T17:55:09Z

pkg/controller/controller.go

@@ -417,7 +417,7 @@ func (lbc *LoadBalancerController) SyncLoadBalancer(state interface{}) error {

 // GCLoadBalancers implements Controller.
 func (lbc *LoadBalancerController) GCLoadBalancers(toKeep []*v1beta1.Ingress) error {
-	return lbc.l7Pool.GC(toLbNames(toKeep))
+	return lbc.l7Pool.GC(utils.ToIngressKeys(toKeep))


GC takes names specifically. This is another reason to use strong type for lbName.

ToIngressKeys seems to lack of the Ingress-gce filtering capability of toLbNames

freehan · 2019-09-30T18:05:33Z

pkg/loadbalancers/l7.go

@@ -50,7 +50,6 @@ const (

 // L7RuntimeInfo is info passed to this module from the controller runtime.
 type L7RuntimeInfo struct {
-	// Name is the name of a loadbalancer.
 	Name string


did you forgot to remove Name?

freehan · 2019-09-30T18:06:15Z

pkg/loadbalancers/l7s.go

 type L7s struct {
 	cloud            *gce.Cloud
-	namer            *namer.Namer
+	namer            namer_util.LegacyFrontendNamer


use the LegacyFrontendNamerHelper

freehan · 2019-09-30T18:06:30Z

pkg/loadbalancers/l7s.go

 	"k8s.io/klog"
 	"k8s.io/legacy-cloud-providers/gce"
 )

 // L7s implements LoadBalancerPool.
+// TODO(smatti): replace *namer_util.Namer with an interface.


freehan · 2019-09-30T18:07:23Z

pkg/loadbalancers/l7s.go

 }

 // Namer returns the namer associated with the L7s.
-func (l *L7s) Namer() *namer.Namer {
+func (l *L7s) Namer() namer_util.LegacyFrontendNamer {


I do not think this function is used anywhere

freehan · 2019-09-30T18:13:10Z

pkg/utils/namer/fenamer_test.go

+// TestLegacyIngressFrontendNamer tests that the migrated legacy namer returns
+// expected values for various frontend resource names. This also tests that the
+// migrated legacy namer returns same values as old namer for frontend resource names.
+func TestLegacyIngressFrontendNamer(t *testing.T) {


I would recommend adding a few more cases:

many chars above the truncation threshold

1 char above the truncation threshold

1 char below the truncation threshold

chars just fits the 63 char limit

freehan

LGTM overall. Just one nit.

/assign bowei@ for another pair of eyes.

freehan · 2019-10-09T20:38:21Z

pkg/backends/ig_linker.go

 	}
 }

 // Link implements Link.
 func (l *instanceGroupLinker) Link(sp utils.ServicePort, groups []GroupKey) error {
 	var igLinks []string
 	for _, group := range groups {
-		ig, err := l.instancePool.Get(l.namer.InstanceGroup(), group.Zone)
+		ig, err := l.instancePool.Get(sp.IGName(), group.Zone)


how about dropping the IGName function and just use BackendName?

sorry. please skip it.

pkg/utils/namer/interfaces.go

skmatti · 2019-10-09T20:57:22Z

/assign @bowei

freehan

/lgtm

/assign bowei@ for approval

freehan · 2019-10-09T21:02:30Z

pkg/backends/ig_linker.go

 	}
 }

 // Link implements Link.
 func (l *instanceGroupLinker) Link(sp utils.ServicePort, groups []GroupKey) error {
 	var igLinks []string
 	for _, group := range groups {
-		ig, err := l.instancePool.Get(l.namer.InstanceGroup(), group.Zone)
+		ig, err := l.instancePool.Get(sp.IGName(), group.Zone)


sorry. please skip it.

k8s-ci-robot · 2019-10-09T21:03:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, skmatti

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~~ [freehan]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 20, 2019

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 20, 2019

k8s-ci-robot assigned freehan Sep 20, 2019

k8s-ci-robot requested review from bowei and MrHohn September 20, 2019 18:09

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 Sep 20, 2019

skmatti mentioned this pull request Sep 20, 2019

V2 Frontend Naming Scheme for Ingress-GCE L7 #858

Closed

4 tasks

skmatti force-pushed the benamer-cleanup branch 4 times, most recently from 592da1d to 90ad989 Compare September 30, 2019 17:35

freehan reviewed Sep 30, 2019

View reviewed changes

skmatti force-pushed the benamer-cleanup branch from 90ad989 to f5f7735 Compare October 2, 2019 23:49

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 2, 2019

skmatti force-pushed the benamer-cleanup branch 3 times, most recently from ecc7d82 to d57f5d8 Compare October 3, 2019 17:20

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 3, 2019

freehan reviewed Oct 9, 2019

View reviewed changes

Cleanup backend namer workflow

f0fc341

skmatti force-pushed the benamer-cleanup branch from d57f5d8 to f0fc341 Compare October 9, 2019 20:54

k8s-ci-robot assigned bowei Oct 9, 2019

freehan reviewed Oct 9, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2019

k8s-ci-robot merged commit 0d42a4c into kubernetes:master Oct 9, 2019

skmatti deleted the benamer-cleanup branch October 9, 2019 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup backend namer workflow #861

Cleanup backend namer workflow #861

skmatti commented Sep 20, 2019 •

edited

Loading

k8s-ci-robot commented Sep 20, 2019

skmatti commented Sep 20, 2019

freehan commented Sep 20, 2019

freehan Sep 30, 2019

freehan Sep 30, 2019

freehan Sep 30, 2019

freehan Sep 30, 2019

freehan Sep 30, 2019

freehan Sep 30, 2019

freehan Sep 30, 2019

freehan Sep 30, 2019

freehan Sep 30, 2019

freehan left a comment

freehan Oct 9, 2019

freehan Oct 9, 2019

skmatti commented Oct 9, 2019

freehan left a comment

freehan Oct 9, 2019

k8s-ci-robot commented Oct 9, 2019

Cleanup backend namer workflow #861

Cleanup backend namer workflow #861

Conversation

skmatti commented Sep 20, 2019 • edited Loading

k8s-ci-robot commented Sep 20, 2019

skmatti commented Sep 20, 2019

freehan commented Sep 20, 2019

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

freehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skmatti commented Oct 9, 2019

freehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 9, 2019

skmatti commented Sep 20, 2019 •

edited

Loading