Sync one ingress #106

bowei · 2018-01-16T18:35:57Z

A bunch of patches:

A lot of clean up and movement
Sync one ingress (as opposed to all) in Checkpoint
Call GC explicitly when an Ingress is deleted
Shutdown() firewall when there are no more Ingresses

Also remove more state from LBC.

- If the Ingress was deleted, only perform a GC. - Only update the Ingress that was changed.

bowei · 2018-01-16T18:36:06Z

@G-Harmon

bowei · 2018-01-16T18:36:36Z

Look at this change on a commit wise basis, otherwise will be hard to review.

freehan · 2018-01-16T18:51:49Z

pkg/firewalls/firewalls.go

@@ -54,6 +54,7 @@ func NewFirewallPool(cloud Firewall, namer *utils.Namer) SingleFirewallPool {

 // Sync sync firewall rules with the cloud.
 func (fr *FirewallRules) Sync(nodePorts []int64, nodeNames []string) error {
+	glog.V(4).Infof("Sync(%v, %v)", nodePorts, nodeNames)


"Sync Firewall (%v, %v)"

It should be clear from the filename in the log line

freehan · 2018-01-17T01:23:04Z

pkg/controller/cluster_manager.go

 	}
-	if igErr != nil {
-		return igErr
+	if len(lbNames) == 0 {


merge it with the previous if statement?

Add a comment to say if there is no lbs. Remove instance group and firewall

freehan · 2018-01-17T01:28:02Z

pkg/controller/controller.go

 		Interface: kubeClient.Core().Events(""),
 	})
 	lbc := LoadBalancerController{
 		client:              kubeClient,
+		ctx:                 ctx,
+		ingLister:           StoreToIngressLister{Store: ctx.IngressInformer.GetStore()},
+		nodeLister:          ctx.NodeInformer.GetIndexer(),


Probably makes sense to throw nodeLister and getReadyNodes into Translator.

freehan · 2018-01-17T01:34:28Z

pkg/context/context.go

 	IngressInformer  cache.SharedIndexInformer
 	ServiceInformer  cache.SharedIndexInformer
 	PodInformer      cache.SharedIndexInformer
 	NodeInformer     cache.SharedIndexInformer
 	EndpointInformer cache.SharedIndexInformer
+
+	// Map of namespace => record.EventRecorder.
+	recorders map[string]record.EventRecorder


EventRecorder per namespace?

I'm a bit confused by this too. The service controller doesn't specify the namespace:

https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/service/service_controller.go#L118

Let's keep this for now, I'll remove in a later commit once we resolve the unit test log spam

nicksardo · 2018-01-17T01:51:57Z

pkg/controller/node.go

+)
+
+const (
+	nodeResyncPeriod = 1 * time.Second


Resync every second?

? what do you want me to do here? This is the existing policy.

Could we rename to something else? Looks like this is just the frequency that the PeriodicTaskQueue checks the queue, not at which items in the queue are re-enqueued. Am I mistaken?

I changed to nodeUpdatePeriod

nicksardo · 2018-01-17T01:59:18Z

pkg/controller/utils.go

+
+// getReadyNodeNames returns names of schedulable, ready nodes from the node lister.
+func getReadyNodeNames(lister listers.NodeLister) ([]string, error) {
+	nodeNames := []string{}


I know you didn't write this, but
Nit: var nodeNames []string

nicksardo · 2018-01-17T01:59:54Z

pkg/context/context.go

@@ -51,6 +51,26 @@ func NewControllerContext(kubeClient kubernetes.Interface, namespace string, res
 	return context
 }

+// HasSynced returns true if all relevant informers has been synced.
+func (ctx *ControllerContext) HasSynced() bool {
+


Extra space

nicksardo · 2018-01-17T02:04:13Z

pkg/controller/cluster_manager.go

@@ -105,6 +105,8 @@ func (c *ClusterManager) shutdown() error {
 // If in performing the checkpoint the cluster manager runs out of quota, a
 // googleapi 403 is returned.
 func (c *ClusterManager) Checkpoint(lbs []*loadbalancers.L7RuntimeInfo, nodeNames []string, backendServicePorts []backends.ServicePort, namedPorts []backends.ServicePort, firewallPorts []int64) ([]*compute.InstanceGroup, error) {
+	glog.V(4).Infof("Checkpoint %q, len(lbs)=%v, len(nodeNames)=%v, lne(backendServicePorts)=%v, len(namedPorts)=%v, len(firewallPorts)=%v", len(lbs), len(nodeNames), len(backendServicePorts), len(namedPorts), len(firewallPorts))


Provided only 5 values - expected 6.

Typo: "lne(backendServicePorts)"

G-Harmon · 2018-01-17T00:50:12Z

pkg/controller/cluster_manager.go

@@ -174,15 +174,17 @@ func (c *ClusterManager) GC(lbNames []string, nodePorts []backends.ServicePort)
 	}


// This method ignores googleapi 404 errors (StatusNotFound). (line 155)
Is this true? If so, where does it happen in this function?

G-Harmon · 2018-01-17T00:52:50Z

pkg/controller/translator/translator_test.go

+		}
+		for _, p := range res {
+			if _, ok := int64ToMap(tc.want)[p]; !ok {
+				t.Errorf("firewall port %v is missing, (got %v, want %v)", p, res, tc.want)


report value of 'defaultBackend'(?)

G-Harmon · 2018-01-17T18:28:19Z

pkg/controller/translator/translator_test.go

@@ -0,0 +1,317 @@
+/*
+Copyright 2017 The Kubernetes Authors.


s/2017/2018/

G-Harmon · 2018-01-17T18:36:07Z

pkg/controller/node.go

@@ -0,0 +1,72 @@
+/*
+Copyright 2017 The Kubernetes Authors.


We typically don't update the year as it causes too much spam.

This isn't an update. This is a new file. (I agree for changes to files the copyright isn't normally changed.)

G-Harmon · 2018-01-17T18:41:17Z

pkg/controller/node.go

+// NodeController synchronizes the state of the nodes to the unmanaged instance
+// groups.
+type NodeController struct {
+	lister cache.Indexer


newbie comment: is it worth commenting members? Is there style guidance on always commenting struct/class member variables? (In google c++, we say we should.)

good point, yes in general unless extremely obvious.

G-Harmon · 2018-01-17T18:48:33Z

pkg/controller/translator/translator.go

 	portMap := map[int64]bool{}
 	for _, p := range svcPorts {
-		if p.NEGEnabled {
+		if t.negEnabled && p.NEGEnabled {


Is this a bug fix? If so, should it go in its own commit?

nicksardo · 2018-01-29T20:14:59Z

pkg/controller/node.go

+
+	ctx.NodeInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
+		AddFunc:    c.queue.Enqueue,
+		DeleteFunc: c.queue.Enqueue,


Can we add a TODO here that we need to improve this NodeController?

We don't react on the UpdateFunc because it's supposedly very noisy. However, when new nodes are added, they are typically NotReady. This means in practice that instance groups are synced when the NodeInformer re-calls AddFunc (every 10 minutes).

bowei · 2018-01-29T21:23:35Z

/retest

bowei added 10 commits January 16, 2018 10:31

Rename "networkendpointgroup" to "neg"

7546cab

Move HasSynced to be internal to the controller context

31c3f3d

Split off L7s and L7 into their own files

eb5b097

Improve logging changes in firewalls

2a78584

Move node specific sync out into its own controller

f670858

Also remove more state from LBC.

Move translator specific unit test into translator

ae56add

Cleanup unit test to be more readable

c9a3f8f

Only sync the Ingress that was changed

e140cb1

- If the Ingress was deleted, only perform a GC. - Only update the Ingress that was changed.

Make event recorders have the right namespaces

7aa6519

Remove firewall if there are no more L7 lbs

4a831f3

bowei requested a review from nicksardo January 16, 2018 18:35

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 16, 2018

freehan reviewed Jan 17, 2018

View reviewed changes

nicksardo reviewed Jan 17, 2018

View reviewed changes

G-Harmon reviewed Jan 17, 2018

View reviewed changes

nicksardo mentioned this pull request Jan 23, 2018

Emit event on TLS errors #112

Merged

nicksardo reviewed Jan 29, 2018

View reviewed changes

bowei merged commit 572e8e5 into kubernetes:master Jan 29, 2018

bowei deleted the sync-one-ingress branch January 29, 2018 22:30

nicksardo mentioned this pull request Feb 6, 2018

Sync ingress-specific backends and minor logging changes #123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync one ingress #106

Sync one ingress #106

bowei commented Jan 16, 2018

bowei commented Jan 16, 2018

bowei commented Jan 16, 2018

freehan Jan 16, 2018

bowei Jan 29, 2018

freehan Jan 17, 2018

bowei Jan 29, 2018

freehan Jan 17, 2018

freehan Jan 17, 2018

nicksardo Jan 17, 2018 •

edited

Loading

bowei Jan 29, 2018

nicksardo Jan 29, 2018

nicksardo Jan 17, 2018

bowei Jan 29, 2018

nicksardo Jan 29, 2018

bowei Jan 29, 2018

nicksardo Jan 17, 2018

bowei Jan 29, 2018

nicksardo Jan 17, 2018

bowei Jan 29, 2018

nicksardo Jan 17, 2018

bowei Jan 29, 2018

G-Harmon Jan 17, 2018

G-Harmon Jan 17, 2018

G-Harmon Jan 17, 2018

G-Harmon Jan 17, 2018

bowei Jan 29, 2018

G-Harmon Jan 30, 2018

G-Harmon Jan 17, 2018

bowei Jan 29, 2018

G-Harmon Jan 17, 2018

nicksardo Jan 29, 2018

bowei Jan 29, 2018

bowei commented Jan 29, 2018

		@@ -174,15 +174,17 @@ func (c *ClusterManager) GC(lbNames []string, nodePorts []backends.ServicePort)
		}

Sync one ingress #106

Sync one ingress #106

Conversation

bowei commented Jan 16, 2018

bowei commented Jan 16, 2018

bowei commented Jan 16, 2018

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

nicksardo Jan 17, 2018 • edited Loading

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

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

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

bowei commented Jan 29, 2018

nicksardo Jan 17, 2018 •

edited

Loading