[jjo] added network_services_controller ginkgo testing #348

jjo · 2018-03-21T21:10:48Z

FYI no new functionality, but just golang interface hooks,
to be able to do BDD network_services_controller testing.

created a shim/mostly-nil LinuxNetworking go interface,
(to be able to replace it with mocks for testing)
hooked all linux networking subsys calls thru it

@reviewer: note that network_services_controller.go
modifications to existing code are indeed e.g.
replacing:
```
ipvsClusterVipSvc, err := ipvsAddService(...)
```
by:
```
ipvsClusterVipSvc, err := nsc.ln.ipvsAddService(...)
```
added network_services_controller_moq.go autogenerated
with moq
using ginkgo, created network_services_controller_test.go
and other autogenerated supporting files
updated vendor/ deps for
- github.com/onsi/ginkgo
- github.com/onsi/gomega
added network_services_controller_test.go testing for:
- Context("service no endpoints with externalIPs", ...)
- Context("service no endpoints with loadbalancer IPs with annotation", ...)
- Context("service no endpoints with loadbalancer IPs without annotation", ...)
- Context("service no endpoints with loadbalancer without IPs", ...)
- Context("node has endpoints for service", ...)

jjo · 2018-03-22T12:07:47Z

@murali-reddy appreciate review, time permits.

jjo · 2018-03-27T21:40:21Z

@murali-reddy updated to include PR #355 + network_services_controller test case for it.

jjo · 2018-04-04T17:21:55Z

@murali-reddy: rebased against latest master.

andrewsykim · 2018-04-07T18:57:11Z

@jjo really excited about IPVS tests! Would it be possible to put all the vendor changes into 1 commit?

FYI no new functionality, but just golang interface hooks, to be able to do BDD network_services_controller testing. * created a shim/mostly-nil LinuxNetworking go interface, (to be able to replace it with mocks for testing) hooked all linux networking subsys calls thru it `@reviewer`: note that `network_services_controller.go` modifications to existing code are indeed e.g. replacing: ~~~ ipvsClusterVipSvc, err := ipvsAddService(...) ~~~ by: ~~~ ipvsClusterVipSvc, err := nsc.ln.ipvsAddService(...) ~~~ * added `network_services_controller_moq.go` autogenerated with `moq` * using `ginkgo`, created `network_services_controller_test.go` and other autogenerated supporting files * added `network_services_controller_test.go` testing for: - Context("service no endpoints with externalIPs", ...) - Context("service no endpoints with loadbalancer IPs with annotation", ...) - Context("service no endpoints with loadbalancer IPs without annotation", ...) - Context("service no endpoints with loadbalancer without IPs", ...) - Context("node has endpoints for service", ...) * updated `vendor/` deps for - github.com/onsi/ginkgo - github.com/onsi/gomega ( in separated, next commit )

jjo · 2018-04-07T23:11:02Z

really excited about IPVS tests! Would it be possible to put all the vendor changes into 1 commit?

@andrewsykim thanks for the feedback :), just did it:
reworked changes into just 2 commits, 2nd one with glide.yaml + vendor/ additions.

andrewsykim · 2018-04-08T01:20:35Z

app/controllers/network_services_controller.go

-		vip := &netlink.Addr{IPNet: &net.IPNet{IP: svc.clusterIP, Mask: net.IPv4Mask(255, 255, 255, 255)}, Scope: syscall.RT_SCOPE_LINK}
-		err := netlink.AddrAdd(dummyVipInterface, vip)
-		if err != nil && err.Error() != IFACE_HAS_ADDR {
-			glog.Errorf("Failed to assign cluster ip %s to dummy interface %s", svc.clusterIP.String(), err.Error())


glog.Errorf line was removed, was that on purpose?

Note that 4 lines construct (i.e. ipAddrAdd() + error check + glog ) was moved
to func (ln *linuxNetworking) ipAddrAdd(iface netlink.Link, ip string) error
at network_services_controller.go:102 interface method
(ditto ipAddrDel()), to de-dup it from the ~3 places it was previously present.

FWIW if you check linuxNetworking object methods there, above two
are the only ones "reworked" a bit (other ones being just stubbed thru
LinuxNetworking interface).

andrewsykim · 2018-04-08T01:24:42Z

app/controllers/network_services_controller_test.go

+package controllers
+
+import (
+	"fmt"


Group std imports together

go fmt is sorting them that way (and there's a check at Makefile I
want to obey ;)) .

gofmt doesn't account for how imports are grouped so gofmt still passes if you group stdlib separately, example: https://github.com/cloudnativelabs/kube-router/blob/master/app/controllers/network_routes_controller.go#L4-L15. The grouping is just good practice since it's easier to find packages that way.

Gotcha, done.

andrewsykim · 2018-04-08T01:26:04Z

app/controllers/network_services_controller_test.go

+	"time"
+)
+
+type LinuxNetworkingMocker interface {


This seems like a duplicate of LinuxNetworking can we just use that?

Good catch, done ( fwiw I had slightly preferred explicitly having there
the *Mock() methods as a way to document~ish what subset has
been mocked).

andrewsykim · 2018-04-08T01:28:56Z

@jjo PR looks good, just had some minor comments. It would be great if you can document how the mock code was generated (ideally a make command).

jjo · 2018-04-08T23:32:26Z

@andrewsykim wrote: PR looks good, just had some minor comments. It would be great if you can document how the mock code was generated (ideally a make command).

Good point -- added to Makefile w/some docs about it.

@andrewsykim PTAL, thanks!

andrewsykim · 2018-04-09T04:01:54Z

@jjo thanks! I'll review it again tomorrow, do you mind rebasing it seems there are commits from other PRs here (ed34187)

…only these in vendor/

jjo · 2018-04-09T10:14:27Z

@andrewsykim wrote: ... I'll review it again tomorrow, do you mind rebasing it seems there are commits from other PRs here

Thanks @andrewsykim!, rebased.

andrewsykim · 2018-04-09T12:40:16Z

LGTM! cc @murali-reddy for a final review

murali-reddy · 2018-04-09T14:06:10Z

LGTM as well. Thanks @jjo for the contribution.

jjo · 2018-04-09T14:07:47Z

w00T!, thank you @andrewsykim @murali-reddy for the support and reviews :)

jjo force-pushed the jjo-network_services_controller_test-wip branch from 78c0b77 to 360658e Compare March 22, 2018 04:52

jjo changed the title ~~[WIP] added network_services_controller testing~~ [jjo] added network_services_controller ginkgo testing Mar 22, 2018

jjo force-pushed the jjo-network_services_controller_test-wip branch 4 times, most recently from b3cf545 to 9d367d5 Compare March 27, 2018 00:18

jjo force-pushed the jjo-network_services_controller_test-wip branch from 30ae3e7 to 408d0a2 Compare April 4, 2018 17:21

andrewsykim reviewed Apr 8, 2018

View reviewed changes

jjo added 6 commits April 9, 2018 07:10

network services controller testing via moq and ginkgo

f7f25d8

add network_services_test case for LoadBalancer w/o IPs

d938630

no-change-commit: glide.yaml: pin ginkgo and gomega versions, update …

57dc45b

…only these in vendor/

golint fixes

3808d91

fixes from @andrewsykim review

fe0c35e

group std imports

845e4f9

jjo force-pushed the jjo-network_services_controller_test-wip branch from 588b642 to 845e4f9 Compare April 9, 2018 10:13

murali-reddy merged commit ed0dc39 into cloudnativelabs:master Apr 9, 2018

andrewsykim mentioned this pull request Apr 9, 2018

refactor to use shared informers #373

Merged

[jjo] added network_services_controller ginkgo testing #348

[jjo] added network_services_controller ginkgo testing #348

Uh oh!

Conversation

jjo commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjo commented Mar 22, 2018

Uh oh!

jjo commented Mar 27, 2018

Uh oh!

jjo commented Apr 4, 2018

Uh oh!

andrewsykim commented Apr 7, 2018

Uh oh!

jjo commented Apr 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewsykim Apr 8, 2018

Choose a reason for hiding this comment

Uh oh!

jjo Apr 8, 2018

Choose a reason for hiding this comment

Uh oh!

andrewsykim Apr 8, 2018

Choose a reason for hiding this comment

Uh oh!

jjo Apr 8, 2018

Choose a reason for hiding this comment

Uh oh!

andrewsykim Apr 9, 2018

Choose a reason for hiding this comment

Uh oh!

jjo Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewsykim Apr 8, 2018

Choose a reason for hiding this comment

Uh oh!

jjo Apr 8, 2018

Choose a reason for hiding this comment

Uh oh!

andrewsykim commented Apr 8, 2018

Uh oh!

jjo commented Apr 8, 2018

Uh oh!

andrewsykim commented Apr 9, 2018

Uh oh!

jjo commented Apr 9, 2018

Uh oh!

andrewsykim commented Apr 9, 2018

Uh oh!

murali-reddy commented Apr 9, 2018

Uh oh!

jjo commented Apr 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jjo commented Mar 21, 2018 •

edited

Loading

jjo commented Apr 7, 2018 •

edited

Loading

jjo Apr 9, 2018 •

edited

Loading