Store LoadBalancerStatus on urlmap instead of storing it on the forwarding rule #149

nikhiljindal · 2018-03-27T00:08:19Z

Main changes:

Stop storing LoadBalancerStatus on forwarding rules
Start storing status on urlmaps
Update List and GetStatus to read status from both forwarding rule and urlmap
Update tests to test that both forwarding rule and urlmaps work fine with and without status.

We wont need special migration for existing MCI's.
Gradually as users update their MCI, status will be migrated from forwarding rule to urlmap.

User visible change:
If they run kubemci create again with existing ingress spec, they will expect it to not make any change.
With this, it will throw an error saying user needs to run the command with --force to update the forwarding rule and urlmap.
Will include it in release notes.

cc @csbell @G-Harmon

nikhiljindal · 2018-03-27T06:37:19Z

Will rebase this once #150 and #151 merge.

madhusudancs · 2018-03-27T18:50:10Z

/assign

madhusudancs

@nikhiljindal I wish this was broken down into multiple small PRs.

For the next iteration of this review, could you create separate subsequent commits for the addressed comments without amending the original commit? You can later fold the commits after the PR is LGTM'ed, i.e. just before merging it. Makes the review tedious if the original commit is amended.

madhusudancs · 2018-03-27T18:57:42Z

app/kubemci/cmd/remove_clusters_test.go

@@ -0,0 +1,111 @@
+// Copyright 2017 Google Inc.


s/2017/2018/

Sorry this whole test file felt unrelated. Fixed and moved to #152

madhusudancs · 2018-03-27T19:20:30Z

app/kubemci/cmd/remove_clusters_test.go

+		IngressFilename: "ingress.yaml",
+		GCPProject:      "gcp-project",
+	}
+	if err := validateRemoveClustersArgs(&options, []string{"lbname"}); err == nil {


In all the 4 cases above, don't you also want to check if the expected error was returned? It's fine if you do this in a follow up CL, but could you add a TODO in that case?

Also, won't this test be more readable and less tedious if you move all the test case definitions to the beginning like the way you do in url maps tests? Something like:

testCases := []struct{ opts removeClustersOptions args []string expectedErr error failMsg string }{ { opts: removeClustersOptions{}, args: []string{}, expectedErr: errors.New("Some error"), failMsg: "Expected error for emtpy options", }, { opts: removeClustersOptions{ IngressFilename: "ingress.yaml", GCPProject: "gcp-project", KubeconfigFilename: "kubeconfig", }, args: []string{"lbname"}, expectedErr: errors.New("Some other error"), failMsg: "Expected error for missing load balancer name", }, ... } for i, case := range testCases { if err := validateRemoveClustersArgs(&case.opts, opts.args); err == case.expectedErr { t.Errorf(case.failMsg) } }

Filed #153 to update all such validate args tests

madhusudancs · 2018-03-27T19:21:11Z

app/kubemci/cmd/remove_clusters_test.go

+	}
+
+	// validateRemoveClustersArgs should return an error with missing gcp project.
+	options := removeClustersOptions{


madhusudancs · 2018-03-27T19:28:37Z

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go

+		return "", fmt.Errorf("Load balancer %s does not exist", l.lbName)
+	}
+	// Failed to get status from both url map and forwarding rule.
+	return "", fmt.Errorf("failed to get status from both url map and forwarding rule. Error in extracting status from url map: %s. Error in extracting status from forwarding rule: %s", umErr, frErr)


I believe this error is shown to the user, right? Can this be a little more friendlier than printing out the url map and forwarding rule details? May be just say there was an error extracting the status and print all these details in the log?

To clarify this prints the error and not the url map and forwarding rule themselves.
I agree we should make error messages as friendly as possible but we also want to keep them meaningful so that the user can take necessary action to fix the problem.
For ex: if fetching the forwarding rule failed due to an auth problem, user should know that.

madhusudancs · 2018-03-27T19:30:22Z

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go

+	// This is because we first used to store the status on forwarding rules and then migrated to url maps.
+	// https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/145 has more details.
+	umBalancers, umErr := l.ums.ListLoadBalancerStatuses()
+	if umErr != nil {


Is it appropriate to return on any error? Or should you continue for some errors because you are merging?

ListLoadBalancerStatus returns an error only if its a real error and cant be ignored. If status is not found on forwarding rule, for ex, then it does not return an error. It returns status for MCIs that have their status on forwarding rules.

Updated to merge the errors from both url map and forwarding rule and return both at once

madhusudancs · 2018-03-27T19:49:22Z

app/kubemci/pkg/gcp/urlmap/urlmapsyncer_test.go

+	}
+}
+
+// removeStatus updates the existing url map of the given name toremove load balancer status.


s/toremove/to remove/

madhusudancs · 2018-03-27T19:50:37Z

app/kubemci/pkg/gcp/urlmap/urlmapsyncer_test.go

+
+	// Remove the status from url map's description to simulate old url maps that have status on forwarding rule.
+	// It should return a non-nil error in this case.
+	name := namer.URLMapName()


removeStatus() function below could be used here?

madhusudancs · 2018-03-27T19:52:53Z

vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/gce/gce_urlmap.go

@@ -67,9 +67,11 @@ func (gce *GCECloud) DeleteUrlMap(name string) error {
 }

 // ListUrlMaps lists all UrlMaps in the project.
-func (gce *GCECloud) ListUrlMaps() (*compute.UrlMapList, error) {
-	mc := newUrlMapMetricContext("list")
+func (gce *GCECloud) ListUrlMaps() ([]*compute.UrlMap, error) {


Remove all these changes to this file?

Moved all vendor/ changes to #150.
They were in this PR for tests to pass. Rebased to remove these changes now that #150 is merged.

madhusudancs · 2018-03-27T19:55:40Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go

+					typedFRS := frs.(*ForwardingRuleSyncer)
+					name := typedFRS.namer.HttpForwardingRuleName()
+					if err := addStatus(typedFRS, fr.lbName, name, ipAddr, clusters); err != nil {
+						t.Fatalf("%s", err)


Are all these fatal errors really fatal? In other words, can't you run the next test case, i.e. the next iteration of the loop if this fails?

right. Updated to Errorf

madhusudancs · 2018-03-27T19:56:22Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go

+	typedFRS := frs.(*ForwardingRuleSyncer)
+	name := typedFRS.namer.HttpForwardingRuleName()
+	if err := addStatus(typedFRS, lbName, name, ipAddr, clusters); err != nil {
+		t.Fatalf("unexpected error in adding status: %s", err)


There is no point continuing in this case. There is no next loop.

nikhiljindal · 2018-03-28T01:42:58Z

Thanks for the review @madhusudancs
Updated code as per comments.
Have pushed a different commit for latest changes.

The new commit also has some changes to FakeForwardingRuleSyncer to fix TestRemoveFromClusters in loadbalancersyncer_test.go due to rebasing on #146

madhusudancs

/lgtm

madhusudancs · 2018-03-28T06:09:15Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go

 				t.Errorf("expected no error in ensuring http forwarding rule, actual: %v", err)
 			}
+			name := typedFRS.namer.HttpForwardingRuleName()
+			// Add status to the forwarding rule to simulate old firewall rule which has status.


s/old firewall/old forwarding/ ?

Thanks. Found the same mistake at a few more places, including the title of this PR :)

madhusudancs · 2018-03-28T06:09:57Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go

 				t.Errorf("expected no error in ensuring https forwarding rule, actual: %v", err)
 			}
+			name := typedFRS.namer.HttpsForwardingRuleName()
+			// Add status to the forwarding rule to simulate old firewall rule which has status.


madhusudancs · 2018-03-28T06:11:15Z

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go

+	// This is because we first used to store the status on forwarding rules and then migrated to url maps.
+	// https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/145 has more details.
+	umBalancers, umErr := l.ums.ListLoadBalancerStatuses()
+	if umErr != nil {


…all rule

nikhiljindal · 2018-03-28T18:56:36Z

Thanks for the review @madhusudancs
Updated code as per comments. Also squashed commits

nikhiljindal · 2018-03-28T18:57:30Z

Will merge once the tests pass

madhusudancs self-assigned this Mar 27, 2018

madhusudancs suggested changes Mar 27, 2018

View reviewed changes

nikhiljindal force-pushed the storeStatus branch from f073cdf to 749cec1 Compare March 28, 2018 01:38

madhusudancs approved these changes Mar 28, 2018

View reviewed changes

Store LoadBalancerStatus on urlmap instead of storing it on the firew…

eaee605

…all rule

nikhiljindal force-pushed the storeStatus branch from 749cec1 to eaee605 Compare March 28, 2018 18:55

nikhiljindal changed the title ~~Store LoadBalancerStatus on urlmap instead of storing it on the firewall rule~~ Store LoadBalancerStatus on urlmap instead of storing it on the forwarding rule Mar 28, 2018

nikhiljindal merged commit 40e1c75 into GoogleCloudPlatform:master Mar 28, 2018

nikhiljindal mentioned this pull request Mar 29, 2018

Adding more unit tests for remove-clusters #158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store LoadBalancerStatus on urlmap instead of storing it on the forwarding rule #149

Store LoadBalancerStatus on urlmap instead of storing it on the forwarding rule #149

nikhiljindal commented Mar 27, 2018

nikhiljindal commented Mar 27, 2018

madhusudancs commented Mar 27, 2018

madhusudancs left a comment

madhusudancs Mar 27, 2018

nikhiljindal Mar 28, 2018

madhusudancs Mar 27, 2018

nikhiljindal Mar 28, 2018

madhusudancs Mar 27, 2018

nikhiljindal Mar 28, 2018

madhusudancs Mar 27, 2018

nikhiljindal Mar 28, 2018

madhusudancs Mar 27, 2018

nikhiljindal Mar 28, 2018

madhusudancs Mar 28, 2018

madhusudancs Mar 27, 2018

nikhiljindal Mar 28, 2018

madhusudancs Mar 27, 2018

nikhiljindal Mar 28, 2018

madhusudancs Mar 27, 2018

nikhiljindal Mar 28, 2018

madhusudancs Mar 27, 2018

nikhiljindal Mar 28, 2018

madhusudancs Mar 27, 2018

nikhiljindal Mar 28, 2018

nikhiljindal commented Mar 28, 2018

madhusudancs left a comment

madhusudancs Mar 28, 2018

nikhiljindal Mar 28, 2018

madhusudancs Mar 28, 2018

nikhiljindal Mar 28, 2018

madhusudancs Mar 28, 2018

nikhiljindal commented Mar 28, 2018

nikhiljindal commented Mar 28, 2018

Store LoadBalancerStatus on urlmap instead of storing it on the forwarding rule #149

Store LoadBalancerStatus on urlmap instead of storing it on the forwarding rule #149

Conversation

nikhiljindal commented Mar 27, 2018

nikhiljindal commented Mar 27, 2018

madhusudancs commented Mar 27, 2018

madhusudancs 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

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

nikhiljindal commented Mar 28, 2018

madhusudancs 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

nikhiljindal commented Mar 28, 2018

nikhiljindal commented Mar 28, 2018