Closed G-Harmon closed 6 years ago
Thanks for sending this @G-Harmon
Most comments are generic code style comments that apply all through:
Reviewed 5 of 5 files at r1, 5 of 5 files at r2, 2 of 2 files at r3. Review status: all files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.
app/kubemci/cmd/create.go, line 100 at r1 (raw file):
Also explain what happens on enabling validation? Like "Performs extra validation steps and throws an error before creating the load balancer"?
Done.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 110 at r1 (raw file):
Both this and the method below can be private? (Can remain private when moved to the validations package)
Done.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 113 at r1 (raw file):
use fmt.Errorf or make it V(2). Also print the rule?
Well, what I have here is just copied from ingToNodePort. Made them both Warningf(), seems unlikely to trigger?
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 117 at r1 (raw file):
nit: Add a space after ":" And same comment as above. make it fmt.Printf or make this V(2)? (making this V(2) seems better to me)
added space. But I'm inclined to keep it in the default log file... Why do you suggest either promoting OR demoting?
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 119 at r1 (raw file):
Start error message with small letter: https://github.com/golang/go/wiki/CodeReviewComments#error-strings.
done.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 124 at r1 (raw file):
fmt.Errorf
I don't follow- this isn't returning an error here.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 128 at r1 (raw file):
Also print the service name and namespace. Keep the error message consistent with non-default services?
Done.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 129 at r1 (raw file):
Append all the errors instead of returning on the first?
Done. (This required updating the unit test, since now more GETs are done and errors returned.)
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 133 at r1 (raw file):
unexpected: ing.spec.backend is nil. Multicluster ingress needs a user specified default backend yea, I didn't put much thought into that message. I'll use the one you pasted.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 141 at r1 (raw file):
node_port := int64(-1) sure.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 143 at r1 (raw file):
clientName over client_name: https://golang.org/doc/effective_go.html#mixed-caps
oops. fixed all the variables.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 144 at r1 (raw file):
"Checking cluster" is enough. Dont need client. Also am wondering if we should make this V(2). Am on the fence. wdyt?
I think it's valuable to add the cluster name because it gives context to logs inside the loop, like for the "ServicePort" Log. But okay, we can make it a Vlog.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 161 at r1 (raw file):
Also include the service name/namespace here
Done.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 178 at r1 (raw file):
I assume we will add many more validations, so better to define a new `validations` package, rather than adding all that code here. This can just call Validate(). That will improve readability and will also enable us to test the validations independently, without having to instantiate loadbalancersyncer.
okay, in addition to moving these 2 functions I added, I also had to move GetServiceNodePorts and getSvc.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 183 at r1 (raw file):
Turn on validation by default as per: https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/53?
oh right. I set the default to 'true' in create.go.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 596 at r1 (raw file):
V(2)? Also print the service name. And "using protocol" instead of "changing protocol".
done and done and done.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 99 at r1 (raw file):
umm why do we fetch it twice again? To determine the node port? We should reuse the nodeport computed during validation? Not to be done in this PR. Feel free to add a TODO and/or file an issue.
thanks for poking at this. I checked again, and it's not duplicating work. it's 1 time for the default backend and 1 time for the path rule.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 222 at r1 (raw file):
Add more detail on what error should it have been
Done.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 237 at r1 (raw file):
Add a line break
Done.
Comments from Reviewable
Review status: all files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 117 at r1 (raw file):
added space. But I'm inclined to keep it in the default log file... Why do you suggest either promoting OR demoting?
Sorry should have been clearer before. Meant either use V() if you want to use glog. Or use fmt.Printf if you want to print by default
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 124 at r1 (raw file):
I don't follow- this isn't returning an error here.
sorry meant fmt.Printf or use V() with glog
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 144 at r1 (raw file):
I think it's valuable to add the cluster name because it gives context to logs inside the loop, like for the "ServicePort" Log. But okay, we can make it a Vlog.
Also remove "client" in "client/cluster"? We use that as variable name in code. From users perspective, they are giving us a list of clusters
Comments from Reviewable
Reviewed 6 of 6 files at r4, 4 of 4 files at r5. Review status: all files reviewed at latest revision, 33 unresolved discussions.
app/kubemci/cmd/create.go, line 102 at r3 (raw file):
Also say that it is true by default
Done.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 144 at r1 (raw file):
Also remove "client" in "client/cluster"? We use that as variable name in code. From users perspective, they are giving us a list of clusters
Done.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 116 at r3 (raw file):
remove line break
Done.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 118 at r3 (raw file):
Sorry should have been clearer before. We do not use glog unless user explicitly uses verbose flag. Hence we dont use glog.Errorf directly. Using glog.V(2).Errorf is fine. For messages that we want to print without verbosity, we use fmt.Printf().
I removed some other glog.Errorf's I found. We chatted again offline... I checked on the behavior: Only glog.Errorf() shows up on the console by default. (And not sure how to get any of these logs into the /tmp/kubemci.INFO log.)
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 514 at r3 (raw file):
use V(2)
Done.
app/kubemci/pkg/kubeutils/utils.go, line 271 at r3 (raw file):
Add a line break
Done.
app/kubemci/pkg/validations/validations.go, line 32 at r3 (raw file):
Services - services
Done. (s/ServicesNodePortsSame/servicesNodePortsSame/, assuming that's what you meant)
app/kubemci/pkg/validations/validations.go, line 33 at r3 (raw file):
"on the same node port in all clusters"
Done.
app/kubemci/pkg/validations/validations.go, line 42 at r3 (raw file):
glog.V(4)
Done.
app/kubemci/pkg/validations/validations.go, line 48 at r3 (raw file):
V(4)
Done.
app/kubemci/pkg/validations/validations.go, line 52 at r3 (raw file):
fmt.Printf. Also keep this consistent with error for non backend service. I see we are not printing the error there
changed them to both be fmt.Errorf (and not print)
app/kubemci/pkg/validations/validations.go, line 76 at r3 (raw file):
This got renamed to NodePort, so the comment is not required now :)
nice!
app/kubemci/pkg/validations/validations.go, line 85 at r3 (raw file):
How about collecting all the clusters where nodePort is not same as firstCluster's node Port. For ex: "Clusters A, B and C have a different node ports for service foo than cluster D. All are expected to have same ....".
discussed offline. This returns an error at the first mismatch. I added a TODO at the top of this loop for improving it, if we ever get a request for it.
Comments from Reviewable
/test pull-kubernetes-multicluster-ingress-test
I rebased my patch, and that should fix the test failure we were seeing.
Thanks for all the fixes @G-Harmon /lgtm
For every Backend/Service in the Ingress, validates that each cluster is using the same port number for its NodePort.
This covers some of the work needed for #53.
cc @nikhiljindal @csbell @bowei
This change is![Reviewable](https://reviewable.io/review_button.svg)