Misc build/log changes #2623

joestringer · 2018-01-23T23:00:44Z

This PR pulls in a few misc Makefile and logging changes I've been trying out locally to improve debugging and developer workflows. Particularly noteworthy is the addition of the ginkgo build to the regular default make command. This means that to make, you now need ginkgo installed on your developer machine. This has been described in the documentation since commit 6771144 ("Documentation: Add ginkgo to developer guide"), so hopefully shouldn't be a surprise to anyone at this point.

aanm · 2018-01-23T23:56:54Z

daemon/daemon.go

@@ -1024,6 +1024,7 @@ func NewDaemon(c *Config) (*Daemon, error) {
 			return nil, fmt.Errorf("Unable to reserve IPv4 loopback address: %s", err)
 		}
 		d.loopbackIPv4 = loopbackIPv4
+		log.Infof("Loopback IPv4: %s", d.loopbackIPv4.String())


Can you change line 1014 to Infof?

aanm · 2018-01-23T23:59:45Z

test/Makefile

@@ -2,6 +2,13 @@ provision = true
 # If you set provision to false the test will run without compile the code
 # again.

+all: build


I'm on the fence here, does this means when I run make all on the root directory it will also build ginkgo? If yes can we move the build target to a test target?

Yes, that's the intention. It would build the tests, but not run ginkgo.

There's a couple of instances that I think motivate this:

Late last year there was a case where someone refactored code in pkg/ and ended up unintentionally breaking the building of tests.

Just this week I've done the same thing with PRs, eg renaming a variable and missing some cases.

I think that the easiest way to pick up on stuff like this is for developers to run make on their local machines before pushing, and for the default make command to also at least compile the tests.

The main argument I see against this is that it makes the build take longer. At this point it's a couple of seconds on my laptop.

@aanm What would you like the behaviour to be for make all and make -C test/ all ?

EDIT: I moved the ginkgo build into the root level tests-common, let me know what you think.

IMO make all would be to generate the binaries, make tests would execute the tests (including all the necessary ginkgo building). Gingko itself is not required to run cilium, that's why I think it doesn't make senses to build ginkgo as well

OK, the latest version should achieve this. Specifically:

make does not build the ginkgo tests.

make tests builds ginkgo tests, but does not run them (it runs only unit tests).

make -C test, cd test/ && make will build ginkgo tests but not run them.

make -C test/ test, make -C test/ run or make -C test/ k8s builds & runs ginkgo tests.

EDIT: For a bit of context, you might expect make tests to run the tests, but it only runs the unit tests. IMO running all of the runtime and k8s tests on a local machine is not a reasonable thing to do off this target, considering the resource requirements and time to complete (dozens of minutes and would spin up perhaps 8 VMs, with each VM wanting 2-3GB of memory). Personally my workflow involves attempting specific runtime tests on my local machine via ginkgo foo, then relying on the CI on PRs to catch the rest of the issues.

@aanm Do you agree with this?

yes I think it looks good, thanks!

This address is used for services when directing traffic from a container to a service IP that is backed by itself. Log it so it's easier to grep around and figure out where it came from. Signed-off-by: Joe Stringer <joe@covalent.io>

tests/00-fmt.sh is invoked at build time to check the format of golang source code, so there's no need to invoke it again under runtime tests. Furthermore, the bash-based tests/ directory will be going away, so move this script to contrib/ while we're at it. Signed-off-by: Joe Stringer <joe@covalent.io>

Build the ginkgo tests as part of the regular make command, it doesn't take long and ensures that we don't end up inadvertently breaking ginkgo tests when refactoring common code. Signed-off-by: Joe Stringer <joe@covalent.io>

A few of the BPF object targets were only depending on the C source, and some were depending on nothing. Add the proper library dependencies to the make targets to ensure that they are rebuilt whenever library files change. Signed-off-by: Joe Stringer <joe@covalent.io>

Ensure this shows up in the cilium logs. Signed-off-by: Joe Stringer <joe@covalent.io>

joestringer · 2018-01-24T18:50:16Z

test-me-please

manalibhutiyani

LGTM!

Install cloud CLIs so that you can run cilium-cli from inside a container and connect to AKS / EKS / GKE clusters. This commit also changes in-cluster scripts to use bash instead of sh. Some of these scripts are not POSIX-compliant, and they don't work with sh that comes with Ubuntu. Ref: #2623 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

Add image-repo and image-tag parameters to the cilium-cli action that set up cilium-cli to run inside a container. Update aks-byocni.yaml to run cilium-cli inside a container using the action instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

Update gke.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

Update externalworklads.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

Update {eks,eks-tunnel}.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

Update multicluster.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

Now all the cloud workflows run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Also update eks-tunnel.yaml and remove the helm install step. I forgot to remove it in 5146f46d. Fixes: #2623 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Install cloud CLIs so that you can run cilium-cli from inside a container and connect to AKS / EKS / GKE clusters. This commit also changes in-cluster scripts to use bash instead of sh. Some of these scripts are not POSIX-compliant, and they don't work with sh that comes with Ubuntu. Ref: #2623 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Add image-repo and image-tag parameters to the cilium-cli action that set up cilium-cli to run inside a container. Update aks-byocni.yaml to run cilium-cli inside a container using the action instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Update gke.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Update externalworklads.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Update {eks,eks-tunnel}.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Update multicluster.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Now all the cloud workflows run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Also update eks-tunnel.yaml and remove the helm install step. I forgot to remove it in 5146f46d. Fixes: #2623 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Install cloud CLIs so that you can run cilium-cli from inside a container and connect to AKS / EKS / GKE clusters. This commit also changes in-cluster scripts to use bash instead of sh. Some of these scripts are not POSIX-compliant, and they don't work with sh that comes with Ubuntu. Ref: #2623 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Add image-repo and image-tag parameters to the cilium-cli action that set up cilium-cli to run inside a container. Update aks-byocni.yaml to run cilium-cli inside a container using the action instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Update gke.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Update externalworklads.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Update {eks,eks-tunnel}.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Update multicluster.yaml to run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Ref: #2623 Ref: #2627 Ref: cilium/design-cfps#9 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

[ cherry-picked from cilium/cilium-cli repository ] Now all the cloud workflows run cilium-cli inside a container instead of using cilium-cli-test-job-chart. Also update eks-tunnel.yaml and remove the helm install step. I forgot to remove it in 5146f46d. Fixes: #2623 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

joestringer added pending-review area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. labels Jan 23, 2018

joestringer requested review from a team January 23, 2018 23:00

joestringer requested review from a team as code owners January 23, 2018 23:00

aanm reviewed Jan 24, 2018

View reviewed changes

joestringer force-pushed the submit/misc-2018-01-23 branch 3 times, most recently from 2927e05 to 896fe4f Compare January 24, 2018 01:52

ianvernon approved these changes Jan 24, 2018

View reviewed changes

joestringer added 5 commits January 24, 2018 10:49

daemon: Log loopback IPv4 address

ba40a11

This address is used for services when directing traffic from a container to a service IP that is backed by itself. Log it so it's easier to grep around and figure out where it came from. Signed-off-by: Joe Stringer <joe@covalent.io>

test: Build ginkgo as part of 'make tests'

40db114

Build the ginkgo tests as part of the regular make command, it doesn't take long and ensures that we don't end up inadvertently breaking ginkgo tests when refactoring common code. Signed-off-by: Joe Stringer <joe@covalent.io>

daemon: Log IPv6 router address at info level

d872aac

Ensure this shows up in the cilium logs. Signed-off-by: Joe Stringer <joe@covalent.io>

joestringer force-pushed the submit/misc-2018-01-23 branch from 896fe4f to d872aac Compare January 24, 2018 18:50

manalibhutiyani approved these changes Jan 24, 2018

View reviewed changes

aanm merged commit 610d034 into cilium:master Jan 25, 2018

joestringer deleted the submit/misc-2018-01-23 branch January 25, 2018 01:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Misc build/log changes #2623

Misc build/log changes #2623

Uh oh!

joestringer commented Jan 23, 2018

Uh oh!

aanm Jan 23, 2018

Uh oh!

joestringer Jan 24, 2018 •

edited

Loading

Uh oh!

aanm Jan 23, 2018

Uh oh!

joestringer Jan 24, 2018 •

edited

Loading

Uh oh!

joestringer Jan 24, 2018 •

edited

Loading

Uh oh!

aanm Jan 24, 2018

Uh oh!

joestringer Jan 24, 2018 •

edited

Loading

Uh oh!

tgraf Jan 25, 2018

Uh oh!

aanm Jan 25, 2018

Uh oh!

joestringer commented Jan 24, 2018

Uh oh!

manalibhutiyani left a comment

Uh oh!

Uh oh!

Misc build/log changes #2623

Misc build/log changes #2623

Uh oh!

Conversation

joestringer commented Jan 23, 2018

Uh oh!

aanm Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

joestringer Jan 24, 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!

aanm Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

joestringer Jan 24, 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!

joestringer Jan 24, 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!

aanm Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

joestringer Jan 24, 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!

tgraf Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

aanm Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

joestringer commented Jan 24, 2018

Uh oh!

manalibhutiyani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joestringer Jan 24, 2018 •

edited

Loading

joestringer Jan 24, 2018 •

edited

Loading

joestringer Jan 24, 2018 •

edited

Loading

joestringer Jan 24, 2018 •

edited

Loading