Adding a remove-clusters command

[nikhiljindal]

As per #58, adding a new kubemci remove-clusters command that users can use to remove an existing MCI from some clusters.

Things left to be done:

• This still causes downtime. Filed Store LoadBalancerStatus on a resource other than forwarding rule #145 for that.
• This doesnt use --force yet.
• Adding more tests.

This PR has already become too big, so will do those in follow up PRs.

cc @csbell @G-Harmon

---

This change is 

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

[googlebot]

CLAs look good, thanks!

[nikhiljindal]

Pushed a new commit to add a few more unit tests

[nikhiljindal]

Gentle ping for review

[G-Harmon]

Reviewed 16 of 17 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 17 unresolved discussions.

---

app/kubemci/cmd/remove_clusters.go, line 59 at r2 (raw file):

// Overwrite values when they differ from what's requested. If
// the resource does not exist, or is already the correct
// value, then 'force' is a no-op.
ForceUpdate bool
Is "Force" meaningful for remove-clusters?

---

app/kubemci/cmd/remove_clusters.go, line 62 at r2 (raw file):

// Optional.
It's inconsistent that some members say "required", some say "optional", and some don't say either....

---

app/kubemci/cmd/remove_clusters.go, line 101 at r2 (raw file):

params
nit: s/params/options/ ? or s/params/flags/ ?

---

app/kubemci/cmd/remove_clusters.go, line 148 at r2 (raw file):

	// Delete ingress resource from clusters
	err = ingress.NewIngressSyncer().DeleteIngress(&ing, clients)

shouldn't we check NewIngressSyncer() for returning nil? (I know it doesn't know, but isn't the pattern to check for nil from a New* func?)

---

app/kubemci/pkg/gcp/backendservice/backendservicesyncer.go, line 273 at r2 (raw file):

}

func (b *BackendServiceSyncer) desiredBackendServiceWithoutClusters(existingBE *compute.BackendService, removeIGLinks []string) *compute.BackendService {

minor: can you add a comment please for my benefit?

---

app/kubemci/pkg/gcp/backendservice/backendservicesyncer_test.go, line 221 at r2 (raw file):

if len(be.Backends) != 1 {
t.Fatalf("expected the backend service to have 1 backend for ig: %s, actual: %v", ig2Link, be.Backends)
}
 
Can you please verify that the right ig is left?

---

app/kubemci/pkg/gcp/backendservice/fake_backendservicesyncer.go, line 66 at r2 (raw file):

func (h *FakeBackendServiceSyncer) RemoveFromClusters(ports []ingressbe.ServicePort, removeIGLinks []string) error {
	// TODO: Implement this.

put in a glog.Fatal? Would be better for folks to know that this doesn't work if they try it.

---

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 101 at r2 (raw file):

func (s *FirewallRuleSyncer) removeFromClusters(lbName string, removeIGLinks map[string][]string) error {
I don't understand why you have both RemoveFromClusters and the helper removeFromClusters. In this case, the functions have the same signature and there's loop in the outer func.

---

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 257 at r2 (raw file):

// TODO(nikhiljindal): Fix this if it becomes a problem.
File an Issue so we can point to a known issue if necessary?

---

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go, line 212 at r2 (raw file):

// Update HTTPS status first and then HTTP.
// This ensures that get-status returns the correct status even if HTTPS is updated but updating HTTP fails.
// This is because get-status reads from HTTP if it exists.
// Also note that it continues silently if either HTTP or HTTPS forwarding rules do not exist.
I don't quite follow all this logic, but might not be a big deal.

---

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go, line 219 at r2 (raw file):

}

// ensureForwardingRule ensures a forwarding rule exists as per the given input parameters.

copy-pasted comment needs updating.

---

app/kubemci/pkg/gcp/instances/fake_instancegetter.go, line 24 at r2 (raw file):

Zone: igUrl,
seems fishy to use the same URL for Name and Zone and Tag.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 138 at r2 (raw file):

portsand
s/portsand/ports and/

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 266 at r2 (raw file):

// DeleteLoadBalancer deletes the GCP resources associated with the L7 GCP load balancer for the given ingress.
func (l *LoadBalancerSyncer) RemoveFromClusters(ing *v1beta1.Ingress, removeClients map[string]kubeclient.Interface, forceUpdate bool) error {
	// TODO(nikhiljindal): Dont require the ingress yaml from users. Just the name should be enough. We can fetch ingress YAML from one of the clusters.

Why is this TODO placed right here? I would either put this in the function comment, or next to ing's first use.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 291 at r2 (raw file):

igsForBE := []string{}
for k := range removeIGLinks {
igsForBE = append(igsForBE, removeIGLinks[k]...)
}
What's this doing?

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 383 at r2 (raw file):

// Also note that this returns all named ports on the instance group and not just the ones relevant to the given ingress.
Is this a problem? or, what problem might this cause?

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 366 at r2 (raw file):

	ing, err := setupLBCForCreateIng(lbc, nodePort, igName, igZone, zoneLink, ipAddress)
	if err != nil {
		t.Fatalf("%s", err)

nit: include some text besides the error

---

Comments from Reviewable

[nikhiljindal]

Review status: all files reviewed at latest revision, 17 unresolved discussions.

---

app/kubemci/cmd/remove_clusters.go, line 59 at r2 (raw file):

Previously, G-Harmon wrote…

// Overwrite values when they differ from what's requested. If
// the resource does not exist, or is already the correct
// value, then 'force' is a no-op.
ForceUpdate bool
Is "Force" meaningful for remove-clusters?

#58 has an idea of what we can use it for.

---

app/kubemci/cmd/remove_clusters.go, line 62 at r2 (raw file):

Previously, G-Harmon wrote…

// Optional.
It's inconsistent that some members say "required", some say "optional", and some don't say either....

Good catch. Fixed so that required params explicitly say Required.

---

app/kubemci/cmd/remove_clusters.go, line 101 at r2 (raw file):

Previously, G-Harmon wrote…

params
nit: s/params/options/ ? or s/params/flags/ ?

Done.

---

app/kubemci/cmd/remove_clusters.go, line 148 at r2 (raw file):

Previously, G-Harmon wrote…

shouldn't we check NewIngressSyncer() for returning nil? (I know it doesn't know, but isn't the pattern to check for nil from a New* func?)

Done.

---

app/kubemci/pkg/gcp/backendservice/backendservicesyncer.go, line 273 at r2 (raw file):

Previously, G-Harmon wrote…

minor: can you add a comment please for my benefit?

Sure, done

---

app/kubemci/pkg/gcp/backendservice/backendservicesyncer_test.go, line 221 at r2 (raw file):

Previously, G-Harmon wrote…

if len(be.Backends) != 1 {
t.Fatalf("expected the backend service to have 1 backend for ig: %s, actual: %v", ig2Link, be.Backends)
}
 
Can you please verify that the right ig is left?

Done.

---

app/kubemci/pkg/gcp/backendservice/fake_backendservicesyncer.go, line 66 at r2 (raw file):

Previously, G-Harmon wrote…

put in a glog.Fatal? Would be better for folks to know that this doesn't work if they try it.

Implemented it :)

---

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 101 at r2 (raw file):

Previously, G-Harmon wrote…

func (s *FirewallRuleSyncer) removeFromClusters(lbName string, removeIGLinks map[string][]string) error {
I don't understand why you have both RemoveFromClusters and the helper removeFromClusters. In this case, the functions have the same signature and there's loop in the outer func.

Makes it easier to prefix "error in removing clusters from firewall rule" to all errors.

---

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 257 at r2 (raw file):

This assumes that the ordering of target tags has not changed on these instances.
// A potential solution to that problem is to recompute the target tags for the existing clusters,
// rather than removing the ones for old clusters.
// But we do not want to change existing tags for clusters that are already working.
// Ideally network tags should only be appended to, so this should not be a problem.
// TODO(nikhiljindal): Fix this if it becomes a problem.

Good idea. Filed #147 and added link to comments in code.

---

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go, line 212 at r2 (raw file):

Previously, G-Harmon wrote…

// Update HTTPS status first and then HTTP.
// This ensures that get-status returns the correct status even if HTTPS is updated but updating HTTP fails.
// This is because get-status reads from HTTP if it exists.
// Also note that it continues silently if either HTTP or HTTPS forwarding rules do not exist.
I don't quite follow all this logic, but might not be a big deal.

Reworded. Hopefully its better now

---

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go, line 219 at r2 (raw file):

Previously, G-Harmon wrote…

copy-pasted comment needs updating.

Done.

---

app/kubemci/pkg/gcp/instances/fake_instancegetter.go, line 24 at r2 (raw file):

Previously, G-Harmon wrote…

Zone: igUrl,
seems fishy to use the same URL for Name and Zone and Tag.

Its a fake :)

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 138 at r2 (raw file):

Previously, G-Harmon wrote…

portsand
s/portsand/ports and/

Done.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 266 at r2 (raw file):

Previously, G-Harmon wrote…

Why is this TODO placed right here? I would either put this in the function comment, or next to ing's first use.

Good point. Moved to function comment. Did the same for DeleteLoadBalancer, which had the same comment.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 291 at r2 (raw file):

Previously, G-Harmon wrote…

igsForBE := []string{}
for k := range removeIGLinks {
igsForBE = append(igsForBE, removeIGLinks[k]...)
}
What's this doing?

This is converting a map of clustername to IGs for that cluster into a flat array of all IGs.
Added comment in code.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 383 at r2 (raw file):

Previously, G-Harmon wrote…

// Also note that this returns all named ports on the instance group and not just the ones relevant to the given ingress.
Is this a problem? or, what problem might this cause?

That depends on the caller :)
This method clarifies that it returns all the ports.

---

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 366 at r2 (raw file):

Previously, G-Harmon wrote…

nit: include some text besides the error

Done at all places in this test.

---

Comments from Reviewable

[nikhiljindal]

Thanks for the detailed review @G-Harmon
Updated as per comments. PTAL.

[G-Harmon]

---

Reviewed 9 of 9 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

app/kubemci/pkg/gcp/backendservice/fake_backendservicesyncer.go, line 66 at r2 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

Implemented it :)

nice!

---

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 101 at r2 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

Makes it easier to prefix "error in removing clusters from firewall rule" to all errors.

haha, ok.

---

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 257 at r2 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

This assumes that the ordering of target tags has not changed on these instances.
// A potential solution to that problem is to recompute the target tags for the existing clusters,
// rather than removing the ones for old clusters.
// But we do not want to change existing tags for clusters that are already working.
// Ideally network tags should only be appended to, so this should not be a problem.
// TODO(nikhiljindal): Fix this if it becomes a problem.

Good idea. Filed #147 and added link to comments in code.

thanks

---

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go, line 212 at r2 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

Reworded. Hopefully its better now

yes, i follow now.

---

Comments from Reviewable