Update remove-clusters to remove clusters from urlmaps status as well #151

nikhiljindal · 2018-03-27T06:36:35Z

Ref #58 (remove-clusters) and #145 (storing status on url map instead of forwarding rule).

Follow up to #146 which added a kubemci remove-clusters command. That PR updated the status stored on forwarding rule.
This PR updates that code to update the status on url map as well.

The status could be stored on forwarding rule (for old MCIs) or url map (for new MCIs).

cc @G-Harmon @csbell @madhusudancs

perotinus · 2018-03-27T17:48:25Z

I'll take a look at this one.

perotinus · 2018-03-27T17:53:32Z

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

+	return fmt.Errorf("errors in updating both http and https forwarding rules, http rule error: %s, https rule error: %s", httpErr, httpsErr)
+}
+
+// canIgnoreStatusUpdateError returns true iff either of the following is true:


nit: I think it might be better to expand "iff" to "if and only if". I'm always concerned that people will think that "iff" is a typo and miss out on the meaning.

perotinus · 2018-03-27T17:54:04Z

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

+// * err is nil
+// * err has http.StatusNotFound code
+func canIgnoreStatusUpdateError(err error) bool {
+	if err == nil || utils.IsHTTPErrorCode(err, http.StatusNotFound) {


return err == nil || utils.IsHTTPErrorCode(err, http.StatusNotFound)

Right :) Updated!

perotinus · 2018-03-27T17:57:24Z

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

+	httpErr := s.removeClustersFromStatus("http", s.namer.HttpForwardingRuleName(), clusters)
+	// If both are not found, then return that.
+	if utils.IsHTTPErrorCode(httpErr, http.StatusNotFound) && utils.IsHTTPErrorCode(httpsErr, http.StatusNotFound) {
+		return httpErr


Shouldn't this be a combined error that explains that both are not found?

Also, why is it OK if only one is not found?

By default users create http only ingress, so only HttpForwardingRule will exist.
Similarly for https only ingress, only HttpsForwardingRule will exist.
Generally speaking, if this was able to update the status's that it found, it returns success.

For combining: the error only has the NotFound status code, so combining wont change anything.
We handle both NotFound specially since we want to surface "load balancer does not exist" errors.

perotinus · 2018-03-27T17:58:48Z

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

-			fmt.Println(httpProtocol, "forwarding rule not found. Nothing to update. Continuing")
-			return nil
+			// Return this error as is.
+			return err
 		}
 		err := fmt.Errorf("error in fetching existing forwarding rule %s: %s", name, err)
 		fmt.Println(err)


Shouldn't this be Printf("%v\n", err)

um why?
I dont need any special formatting. Println will print the error fine.
Example: https://play.golang.org/p/CEl9eX_esTh

Ah, OK. I think this was a mental holdover on my part from doing C-based language work for years, where the arguments to "PrintX" methods were always strings. Apologies!

perotinus · 2018-03-27T18:01:47Z

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

-		if err := frs.RemoveClustersFromStatus([]string{"cluster1"}); err != nil {
-			t.Errorf("unexpected error in updating status to remove clusters: %s", err)
+		err := frs.RemoveClustersFromStatus([]string{"cluster1"})
+		if c.shouldErr != (err != nil) {


Maybe create a local variable for err != nil

hasErr := err != nil if c.ShouldErr != hasErr

I think its fine here without it but good point to keep in mind for places with more complex code.

perotinus · 2018-03-27T18:04:54Z

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

 	return err
 }

+func (l *LoadBalancerSyncer) removeClustersFromStatus(lbName string, removeClusters []string) error {


perotinus · 2018-03-27T18:07:24Z

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

-	// Also note that it continues silently if either HTTP or HTTPS forwarding rules do not exist.
-	if err := s.removeClustersFromStatus("https", s.namer.HttpsForwardingRuleName(), clusters); err != nil {
-		return fmt.Errorf("unexpected error in updating the https forwarding rule status: %s", err)
+	httpsErr := s.removeClustersFromStatus("https", s.namer.HttpsForwardingRuleName(), clusters)


How does this relate to the URL map change? It seems independent.

Strictly speaking yes its independent. The reason its here is that the interface is tighter now and consistent with UrlMapSyncer.RemoveClustersFromStatus. Both return NotFound if the urlmap or forwarding rule does not exist.

perotinus · 2018-03-27T19:43:56Z

app/kubemci/pkg/gcp/status/loadbalancerstatus.go

+
+// RemoveClusters removes the given list of clusters from the given status string.
+// Returns NotFoundError if the given statusStr is not a valid LoadBalancerStatus.
+func RemoveClusters(statusStr string, removeClusters []string) (string, error) {


nit: clustersToRemove

Done, here and everywhere

perotinus · 2018-03-27T19:45:20Z

app/kubemci/pkg/gcp/status/loadbalancerstatus.go

+	status.Clusters = newClusters
+	newDesc, err := status.ToString()
+	if err != nil {
+		return statusStr, fmt.Errorf("unexpected error in converting status to string. status: %s, err: %s", status, err)


Is this a NotFoundError?

No. This is error in serializing Status struct to string. This should ideally never be hit.

perotinus · 2018-03-27T20:06:32Z

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

@@ -117,6 +118,32 @@ func (s *URLMapSyncer) DeleteURLMap() error {
 	}
 }

+// See interface for comment.
+func (s *URLMapSyncer) RemoveClustersFromStatus(clusters []string) error {
+	fmt.Println("Removing clusters", clusters, "from url map")


Should this use Printf?

um why? We use Println throughout the code base for simple output strings that dont require special formatting.

nikhiljindal · 2018-03-27T22:34:00Z

Thanks for the review @perotinus
Pushed a new commit as per comments.
PTAL. Will squash commits after lgtm before merging.

nikhiljindal · 2018-03-28T21:59:00Z

Rebased to remove merge conflicts and fixed test.
@perotinus PTAL

perotinus · 2018-03-29T00:44:44Z

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

-			fmt.Println(httpProtocol, "forwarding rule not found. Nothing to update. Continuing")
-			return nil
+			// Return this error as is.
+			return err
 		}
 		err := fmt.Errorf("error in fetching existing forwarding rule %s: %s", name, err)
 		fmt.Println(err)


Ah, OK. I think this was a mental holdover on my part from doing C-based language work for years, where the arguments to "PrintX" methods were always strings. Apologies!

perotinus · 2018-03-29T00:51:39Z

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

-			t.Errorf("unexpected error in updating status to remove clusters: %s", err)
+		err := frs.RemoveClustersFromStatus([]string{"cluster1"})
+		if c.shouldErr != (err != nil) {
+			t.Errorf("unexpected error in updating status to remove clusters, expected err != nil: %v, actual err: %s", c.shouldErr, err)


Hmm. I think the test output and this code would be more readable if you had two branches. If this test failed and I had to look at it without having seen it before, it would take me some time to parse out all of the boolean conditions to actually understand the failure, which is not ideal in a test.

If you wanted to leave one branch, I think the failure output would be clearer if you replaced expected err != nil: %v with expected err %s nil: and a branch that set either == or != as the replacement for %s.

perotinus · 2018-03-29T00:58:51Z

/lgtm

nikhiljindal · 2018-03-29T01:14:00Z

Thanks for the review @perotinus

Updated as per comments and squashed commits.
Will merge once tests pass

nikhiljindal mentioned this pull request Mar 27, 2018

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

Merged

nikhiljindal assigned perotinus Mar 27, 2018

perotinus reviewed Mar 27, 2018

View reviewed changes

nikhiljindal force-pushed the removeCluster branch from e5a47ce to 3423bcc Compare March 27, 2018 22:32

nikhiljindal force-pushed the removeCluster branch 2 times, most recently from 275da99 to 3f49d05 Compare March 28, 2018 21:58

perotinus approved these changes Mar 29, 2018

View reviewed changes

Update remove-clusters to remove clusters from urlmaps status as well

a40d474

nikhiljindal force-pushed the removeCluster branch from 3f49d05 to a40d474 Compare March 29, 2018 01:13

nikhiljindal merged commit 4b94013 into GoogleCloudPlatform:master Mar 29, 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

Update remove-clusters to remove clusters from urlmaps status as well #151

Update remove-clusters to remove clusters from urlmaps status as well #151

nikhiljindal commented Mar 27, 2018

perotinus commented Mar 27, 2018

perotinus Mar 27, 2018

nikhiljindal Mar 27, 2018

perotinus Mar 27, 2018

nikhiljindal Mar 27, 2018

perotinus Mar 27, 2018

nikhiljindal Mar 27, 2018

perotinus Mar 27, 2018

nikhiljindal Mar 27, 2018

perotinus Mar 29, 2018

perotinus Mar 27, 2018

nikhiljindal Mar 27, 2018

perotinus Mar 27, 2018

nikhiljindal Mar 27, 2018

perotinus Mar 27, 2018

nikhiljindal Mar 27, 2018

perotinus Mar 27, 2018

nikhiljindal Mar 27, 2018

perotinus Mar 27, 2018

nikhiljindal Mar 27, 2018

perotinus Mar 27, 2018

nikhiljindal Mar 27, 2018

nikhiljindal commented Mar 27, 2018

nikhiljindal commented Mar 28, 2018

perotinus Mar 29, 2018

perotinus Mar 29, 2018

perotinus commented Mar 29, 2018

nikhiljindal commented Mar 29, 2018

Update remove-clusters to remove clusters from urlmaps status as well #151

Update remove-clusters to remove clusters from urlmaps status as well #151

Conversation

nikhiljindal commented Mar 27, 2018

perotinus commented Mar 27, 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

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 27, 2018

nikhiljindal commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perotinus commented Mar 29, 2018

nikhiljindal commented Mar 29, 2018