Add DNS check to bootstrap #164

lilic · 2018-04-09T13:43:57Z

Sometimes DNS is not configured correctly, but during the launcher
process, DNS is required to setup all the needed resources. This creates
a check by creating a pod where nslookup is performed.

Closes #158.

Note: Opened as more of a preview PR, if we agree on this implementation will add a test or two to this PR. :)

Sometimes DNS is not configured correctly, but during the launcher process DNS is required to setup all the needed resources. This creates a check by creating a pod where nslookup is performed.

bootstrap/main.go

@@ -111,6 +111,21 @@ func mainImpl() {
 		}
 	}

+	// TODO: Should we check always?


bootstrap/main.go

+		// able to connect to the server to correctly setup the needed resources.
+		if !ok {
+			fmt.Println("DNS is not working in this Kubernetes cluster. We require correct DNS setup in the Kubernetes cluster.")
+			os.Exit(1)


pkg/kubectl/kubectl.go

+	ns := "weave"
+
+	// Create weave namespace, as this happens before any resources are created.
+	_, err := CreateNamespace(c, ns)


pkg/kubectl/kubectl.go

+	}
+
+	// Create pod to perform nslookup on a passed domain to check DNS is working.
+	_, err = Execute(c, "run", "-n", "weave", "--image", "busybox", "--command", podName, "nslookup --timeout=10", domain, "--restart=Never", "--pod-running-timeout=10s")


leth · 2018-04-09T14:23:25Z

Looks good! Just some minor questions!

dlespiau

Approach looks to me, a couple of additional comment.

pkg/kubectl/kubectl.go

+	}
+
+	// Create pod to perform nslookup on a passed domain to check DNS is working.
+	_, err = Execute(c, "run", "-n", "weave", "--image", "busybox", "--command", podName, "nslookup --timeout=10", domain, "--restart=Never", "--pod-running-timeout=10s")


pkg/kubectl/kubectl.go

+	}
+
+	// Get fresh pod data.
+	podJSON, err = Execute(c, "get", "pod", podName, "-ojson", "-n", ns)


pkg/kubectl/kubectl.go

+// ExecuteJSON execute kubectl <args> and returns the combined json stdout and err output.
+func ExecuteJSON(c client, args ...string) (string, error) {
+	return c.Execute(append(args, "-ojson"))
+}


lilic · 2018-04-10T12:49:54Z

@dlespiau @leth PTALA, thanks!

Not sure if we want add a note to the STDOUT saying we are performing checks or not. The last one right now before the DNS check is Installing Weave Cloud agents on minikube..., which might be slightly misleading or confusing when we then error out with There was an error while performing DNS check. DNS check failed. Timed out during DNS check. Thoughts, ideas, suggestions?
As mentioned elsewhere, the timeout cannot be set for nslookup, so right now we wait for 1 minute and timeout during the check. IMO if we can find a better way, or when we add other pre flight checks in the future we can use a different image rather then busybox, or build our own tool to do that. For now I would leave it as is, but am open for suggestions.

lilic · 2018-04-10T12:51:39Z

BTW not sure what's up with tests not running? :/ Could it be because I have this branch on my fork, did not have a look at circle ci setup yet in this project.

dlespiau · 2018-04-10T13:02:38Z

Ah yes, we push branches on the same repo. The reason is that we don't want anyone being able to start a test run by just opening a pull request (it could be a pull request with a commit changing what the tests will be running). The "Build forked pull requests" option in CircleCI is turned off for this project.

This way we make sure that only people with the rights to push to this repository can run test code in CI.

dlespiau · 2018-04-10T13:05:57Z

Adding a line on stdout saying that we're doing a pre-flight check sounds really useful "Checking the Kubernetes installation" or similar. It'll be especially useful for people having to wait for that 1 minute timeout, making it not awful!

lilic · 2018-04-10T13:07:36Z

Closing in favor of #166 to let the tests run.

Add DNS check to bootstrap

9136a3b

Sometimes DNS is not configured correctly, but during the launcher process DNS is required to setup all the needed resources. This creates a check by creating a pod where nslookup is performed.

lilic mentioned this pull request Apr 9, 2018

Add DNS status check #160

Closed

lilic commented Apr 9, 2018

View reviewed changes

bootstrap/main.go Outdated

@@ -111,6 +111,21 @@ func mainImpl() {

}

}

// TODO: Should we check always?

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

leth reviewed Apr 9, 2018

View reviewed changes

bootstrap/main.go Outdated

// able to connect to the server to correctly setup the needed resources.

if !ok {

fmt.Println("DNS is not working in this Kubernetes cluster. We require correct DNS setup in the Kubernetes cluster.")

os.Exit(1)

This comment was marked as abuse.

Sign in to view

leth reviewed Apr 9, 2018

View reviewed changes

pkg/kubectl/kubectl.go

ns := "weave"

// Create weave namespace, as this happens before any resources are created.

_, err := CreateNamespace(c, ns)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

leth reviewed Apr 9, 2018

View reviewed changes

dlespiau reviewed Apr 9, 2018

View reviewed changes

lilic added 4 commits April 9, 2018 17:01

Always check DNS

d396c06

ExitWithCapture instead of just exit

5c42f4c

Remove pod timeout as it does not work on pod creation

635a9c7

Extract ExecuteJSON

178d39f

dlespiau reviewed Apr 9, 2018

View reviewed changes

pkg/kubectl/kubectl.go Outdated

// ExecuteJSON execute kubectl <args> and returns the combined json stdout and err output.

func ExecuteJSON(c client, args ...string) (string, error) {

return c.Execute(append(args, "-ojson"))

}

This comment was marked as abuse.

Sign in to view

Adjust ExecuteJSON

70bc18e

lilic force-pushed the test-dns branch from ab9be28 to 70bc18e Compare April 10, 2018 11:58

lilic mentioned this pull request Apr 10, 2018

Add DNS check to bootstrap #166

Merged

lilic closed this Apr 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DNS check to bootstrap #164

Add DNS check to bootstrap #164

lilic commented Apr 9, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

leth commented Apr 9, 2018

dlespiau left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

lilic commented Apr 10, 2018

lilic commented Apr 10, 2018

dlespiau commented Apr 10, 2018

dlespiau commented Apr 10, 2018

lilic commented Apr 10, 2018

Add DNS check to bootstrap #164

Add DNS check to bootstrap #164

Conversation

lilic commented Apr 9, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

leth commented Apr 9, 2018

dlespiau left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

lilic commented Apr 10, 2018

lilic commented Apr 10, 2018

dlespiau commented Apr 10, 2018

dlespiau commented Apr 10, 2018

lilic commented Apr 10, 2018