Diagnostic client #2032

fcrisciani · 2017-12-08T19:10:12Z

decode internal state of service discovery and overlay
remediate in case of orphan entries
realign naming with moby/moby api
GetEntry should return error if the key is under deletion (added unit test)

ddebroy

Looks good. Had a couple of minor comments. One q: can the remediation be triggered automatically once in a while as part of a "garbage collection sweep" or is that too dangerous?

ddebroy · 2017-12-08T19:38:19Z

diagnose/client/main.go

+	if err != nil {
+		logrus.WithError(err).Fatalf("The connection failed")
+	}
+	httpIsOk(resp.Body)


You can directly check resp.StatusCode for 200/2xx codes which indicate HTTP OK rather than parsing resp.body for OK: https://golangcode.com/get-the-http-response-status-code/

I think can be a good enhancement, right now the server side did not really set any specific error code, but it would be good to implement a more proper api with error codes

ddebroy · 2017-12-08T19:40:23Z

diagnose/client/main.go

+	}
+	httpIsOk(resp.Body)
+
+	clsuterPeers := fetchNodePeers(*ipPtr, *portPtr, "")


clsuster typo

vieux · 2017-12-08T23:30:18Z

Makefile

@@ -52,6 +53,7 @@ cross-local:
 	@echo "🐳 $@"
 	go build -o "bin/dnet-$$GOOS-$$GOARCH" ./cmd/dnet
 	go build -o "bin/docker-proxy-$$GOOS-$$GOARCH" ./cmd/proxy
+	go build -o "bin/dignosticClient-$$GOOS-$$GOARCH" ./diagnose/client


nit: why put it in /diagnose/ and not /cmd/ ? like the others ?

vieux · 2017-12-08T23:33:57Z

diagnose/client/main.go

+			for _, k := range orphanKeys {
+				resp, err := http.Get(fmt.Sprintf(deleteEntry, ip, port, network, tableName, k))
+				if err != nil {
+					logrus.WithError(err).Fatalf("Failed deleting entry k:%s", k)


do we really want to fatal here ? since it will stop the program, it won't delete the remaining entries.

sure, changed in error

codecov-io · 2017-12-14T23:32:05Z

Codecov Report

❗ No coverage uploaded for pull request base (master@862df3a). Click here to learn what that means.
The diff coverage is 10.28%.

@@            Coverage Diff            @@
##             master    #2032   +/-   ##
=========================================
  Coverage          ?   40.46%           
=========================================
  Files             ?      138           
  Lines             ?    22172           
  Branches          ?        0           
=========================================
  Hits              ?     8971           
  Misses            ?    11884           
  Partials          ?     1317

Impacted Files	Coverage Δ
networkdb/networkdbdiagnostic.go	`0% <0%> (ø)`
agent.go	`5.02% <0%> (ø)`
networkdb/networkdb.go	`67.8% <100%> (ø)`
controller.go	`36.74% <52.94%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 862df3a...be91c3e. Read the comment docs.

thaJeztah · 2017-12-19T19:07:23Z

@fcrisciani can you include a README / Markdown file in this PR? Misty started working on one in docker/docs#5558, and can be used as a starting point

fcrisciani · 2018-01-09T22:52:38Z

@thaJeztah @ddebroy @vieux can you guys give another pass?

ddebroy

LGTM. Just a couple of minor string nits

ddebroy · 2018-01-10T00:23:01Z

cmd/diagnostic/README.md

+A message like the following will appear in the Docker host logs:
+
+```none
+Starting the diagnose server listening on <port> for commands


should the string diagnose be diagnostics ?

ddebroy · 2018-01-10T00:23:28Z

cmd/diagnostic/README.md

+A message like the following will appear in the Docker host logs:
+
+```none
+Disabling the diagnose server


should the string diagnose be diagnostics ?

ddebroy

LGTM

fcrisciani · 2018-01-23T22:11:14Z

@thaJeztah can you also give the final blessing?

ddebroy

Looks good with a couple of minor comments.

ddebroy · 2018-01-24T19:37:42Z

diagnostic/server.go

-// IsDebugEnable returns true when the debug is enabled
-func (s *Server) IsDebugEnable() bool {
+// IsDiagnosticEnable returns true when the debug is enabled
+func (s *Server) IsDiagnosticEnable() bool {


Minor nit: how about changing the name to IsDiagnosticEnable**d** like in controller
func (c *controller) IsDiagnosticEnabled() bool {

ddebroy · 2018-01-24T19:47:12Z

networkdb/networkdb.go

@@ -328,6 +328,9 @@ func (nDB *NetworkDB) GetEntry(tname, nid, key string) ([]byte, error) {
 	if err != nil {
 		return nil, err
 	}
+	if entry != nil && entry.deleting {
+		return nil, types.NotFoundErrorf("entry not found in table %s with network id %s and key %s", tname, nid, key)


Masking the existence into a "not found" message may be confusing if this message makes into logs but say the actual deletion does not trigger for a while. How about making the message something along the lines of: "entry found but in deleting state. returning not found" to keep things super clear for support engineers?

changing in: return nil, types.NotFoundErrorf("entry in table %s network id %s and key %s deleted and pending garbage collection", tname, nid, key)

thaJeztah

Left some comments 🤗

thaJeztah · 2018-01-11T11:10:30Z

cmd/diagnostic/Dockerfile

@@ -0,0 +1,8 @@
+FROM docker:17.12-rc-dind


non-rc tag 17.12-dind is now available (https://hub.docker.com/_/docker/)

I was also thinking why we needed the dind for this tool, but this is so that we can use it for older daemons, which do not have this functionality built-in, correct?

Should we have two versions of the image? one "minimal" (just the binary), and one dind?

thaJeztah · 2018-01-11T12:17:59Z

cmd/diagnostic/Dockerfile

+
+RUN apk add --no-cache curl
+
+WORKDIR /tool


Instead of installing in /tool, you could just install in /usr/local/bin

thaJeztah · 2018-01-11T12:38:54Z

cmd/diagnostic/README.md

+**Standalone network:**
+
+```bash
+$ debugClient -c sd -v -net n8a8ie6tb3wr2e260vxj8ncy4


s/debugClient/diagnosticClient/

thaJeztah · 2018-01-11T12:39:02Z

cmd/diagnostic/README.md

+**Overlay network:**
+
+```bash
+$ debugClient -port 2001 -c overlay -v -net n8a8ie6tb3wr2e260vxj8ncy4


s/debugClient/diagnosticClient/

thaJeztah · 2018-01-24T19:49:38Z

cmd/diagnostic/Dockerfile

+WORKDIR /tool
+
+COPY daemon.json /etc/docker/daemon.json
+COPY diagnosticClient /tool/diagnosticClient


For a follow-up; we should make this Dockerfile a multi-stage build, and actually build the client in it (I had a branch with that, but looks like I didn't push, and it's not on my laptop 😊

thaJeztah · 2018-01-24T19:52:06Z

cmd/diagnostic/README.md

+Remember that table operations have ownership, so any `create entry` will be persistent till
+the diagnostic container is part of the swarm.
+
+1.  Make sure that the node where the diagnostic client will run is not part of the swarm, if so do `docker swarm leave -f`


We'll need different steps for 17.12+ daemons (as they don't have to leave the swarm, just the diagnostic client to connect to them)

This is the container version so you will run the dind version and you will need to leave the swarm

Understood; thinking if we should have instructions (also using a containerised version) to use for 17.12 daemons (just bind-mounting /var/run/docker.sock e.g. and connecting to the running daemon)

added the containerized version of it, as dockereng/network-diagnostic:onlyclient that can be used as it with --net host, no need for the docker.sock

thaJeztah · 2018-01-24T19:52:48Z

cmd/diagnostic/README.md

+2.  To run the container, use a command like the following:
+
+    ```bash
+    $ docker container run --name net-diagnostic -d --privileged --network host fcrisciani/network-diagnostic


Can we put the image under an official organization? (dockereng/ or dockercore/ e.g.)

fcrisciani · 2018-01-25T23:56:00Z

Note to myself, after this gets merged the moby vendoring requires code change due to changes in method names

Align it to the moby/moby external api Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

- the client allows to talk to the diagnostic server and decode the internal values of the overlay and service discovery - the tool also allows to remediate in case of orphans entries - added README Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

thaJeztah

LGTM, but left two suggestions 👍

thaJeztah · 2018-01-26T18:57:19Z

cmd/diagnostic/README.md

+    `HUP` signal to the PID you found in the previous step.
+
+    ```bash
+    kill -HUP <pid-of-dockerd>


nit: this could be killall -HUP dockerd

thaJeztah · 2018-01-26T18:57:33Z

cmd/diagnostic/README.md

+    `HUP` signal to the PID you found in the previous step.
+
+    ```bash
+    kill -HUP <pid-of-dockerd>


vieux

LGTM

fcrisciani requested review from vieux, trapier and mavenugo December 8, 2017 19:10

ddebroy reviewed Dec 8, 2017

View reviewed changes

vieux reviewed Dec 8, 2017

View reviewed changes

fcrisciani force-pushed the debug-client branch 2 times, most recently from 84493e3 to 989af94 Compare December 14, 2017 23:14

fcrisciani force-pushed the debug-client branch 2 times, most recently from 3ba06ad to ef512ba Compare January 9, 2018 22:51

ddebroy approved these changes Jan 10, 2018

View reviewed changes

fcrisciani force-pushed the debug-client branch from ef512ba to ed0721b Compare January 11, 2018 00:33

ddebroy approved these changes Jan 23, 2018

View reviewed changes

fcrisciani changed the title ~~Diagnose client~~ Diagnostic client Jan 23, 2018

fcrisciani force-pushed the debug-client branch 3 times, most recently from 766b46b to 5b9614f Compare January 24, 2018 18:55

ddebroy reviewed Jan 24, 2018

View reviewed changes

thaJeztah requested changes Jan 24, 2018

View reviewed changes

fcrisciani force-pushed the debug-client branch 3 times, most recently from 198046b to 2ccffde Compare January 25, 2018 23:53

Flavio Crisciani added 2 commits January 25, 2018 16:09

Change diagnose module name to diagnostic

f8dc09a

Align it to the moby/moby external api Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

Diagnostic client

be91c3e

- the client allows to talk to the diagnostic server and decode the internal values of the overlay and service discovery - the tool also allows to remediate in case of orphans entries - added README Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

fcrisciani force-pushed the debug-client branch from 2ccffde to be91c3e Compare January 26, 2018 00:09

thaJeztah approved these changes Jan 26, 2018

View reviewed changes

vieux approved these changes Jan 26, 2018

View reviewed changes

fcrisciani merged commit 20dd462 into moby:master Jan 29, 2018

fcrisciani deleted the debug-client branch January 29, 2018 19:06

fcrisciani mentioned this pull request Jan 30, 2018

Libnetwork revendoring moby/moby#36137

Merged

trapier mentioned this pull request May 17, 2019

[Backport 17.06] NetworkDB qlen optimization #2379

Merged

Diagnostic client #2032

Diagnostic client #2032

Conversation

fcrisciani commented Dec 8, 2017 • edited Loading

ddebroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 14, 2017 • edited Loading

Codecov Report

thaJeztah commented Dec 19, 2017

fcrisciani commented Jan 9, 2018 • edited Loading

ddebroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

ddebroy left a comment

Choose a reason for hiding this comment

fcrisciani commented Jan 23, 2018

ddebroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcrisciani commented Jan 25, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vieux left a comment

Choose a reason for hiding this comment

fcrisciani commented Dec 8, 2017 •

edited

Loading

codecov-io commented Dec 14, 2017 •

edited

Loading

fcrisciani commented Jan 9, 2018 •

edited

Loading

ddebroy Jan 10, 2018 •

edited

Loading

ddebroy Jan 24, 2018 •

edited

Loading