Trigger server #39

FrankPetrilli · 2018-01-12T16:06:59Z

We'll deploy it with a on-start generated shared secret, which can be retrieved from the master node.
It'll be packaged as a systemd .service file so that it runs on startup, etc.
Packaged as RPM/DEB via FPM

Tests now run successfully with this version.

We'll deploy it with a on-start generated shared secret, which can be retrieved from the master node. It'll be packaged as a systemd .service file so that it runs on startup, etc. Packaged as RPM/DEB via FPM Tests now run successfully with this version.

FrankPetrilli · 2018-01-12T16:09:48Z

RPM/DEB build artifacts:

added QmXrSm9mp3ewBFkUWLbdkkn8f6fwg3SroAbRNj9FumJKbN triggerserver-1.0.0-1.x86_64.rpm
added QmWRWsubcxEpsPwfkSQ62wdrbuWsb3UB3613XaiMCibfYK triggerserver_1.0.0_amd64.deb

hsanjuan

In this form the server still suffers the problem that it's going to run whatever ipfs/ipfs-cluster docker container there is.

Instead, it should run the tests with the code that it's being currently being tested (in a PR).

As I pointed out somewhere else, that means that a container needs to be built, published, and the deployment file needs to be pointed to it.

hsanjuan · 2018-01-15T14:49:59Z

ipfs-cluster/trigger-server/README.md

@@ -0,0 +1,11 @@
+To build RPM/DEB


I guess this needs more docs. What is trigger-server, how is it built, how is it deployed, where etc.

hsanjuan · 2018-01-15T14:55:20Z

ipfs-cluster/trigger-server/build.sh

@@ -0,0 +1,16 @@
+if [ -z $1 ]


#!/bin/bash set -e set -x

hsanjuan · 2018-01-15T14:58:22Z

ipfs-cluster/trigger-server/build.sh

+	exit 1
+fi
+VERSION=$1
+if [[ ! $(docker images | grep fpm) ]]


Improve this grep so it doesn't have the chance to match something unrelated (`docker images --format '{{ .Repository }}' ...)

hsanjuan · 2018-01-15T14:59:03Z

ipfs-cluster/trigger-server/build.sh

+	docker build -t="fpm" github.com/jordansissel/fpm/
+fi
+go build triggerserver.go
+docker run -v $(pwd):/data --rm fpm /bin/sh -c "cd /data && fpm -s dir -t rpm -n triggerserver -v $VERSION -p /data/ --deb-no-default-config-files triggerserver=/usr/bin/ triggerserver.service=/usr/lib/systemd/system/"


make the image name a variable at the top

hsanjuan · 2018-01-15T15:02:25Z

ipfs-cluster/trigger-server/build.sh

+fi
+go build triggerserver.go
+docker run -v $(pwd):/data --rm fpm /bin/sh -c "cd /data && fpm -s dir -t rpm -n triggerserver -v $VERSION -p /data/ --deb-no-default-config-files triggerserver=/usr/bin/ triggerserver.service=/usr/lib/systemd/system/"
+docker run -v $(pwd):/data --rm fpm /bin/sh -c "cd /data && fpm -s dir -t deb -n triggerserver -v $VERSION -p /data/ --deb-no-default-config-files triggerserver=/usr/bin/ triggerserver.service=/lib/systemd/system/"


it is customary to add custom units to /etc/systemd/system/ and leave /lib/systemd for the system.

hsanjuan · 2018-01-15T15:19:36Z

ipfs-cluster/trigger-server/triggerserver.go

+		// This command takes a while, so we'll hang out here while it runs
+		cmd := exec.Command("bash", "runner.sh", RUNNER_NUM_NODES, RUNNER_NUM_PINS)
+		cmd.Dir = RUNNER_PATH
+		out, err := cmd.CombinedOutput()


find a way to stream the output of the command as it happens. Seems https://golang.org/pkg/os/exec/#Cmd.StdinPipe might help. May want to run with a command context so you can cancel the command if you cannot write anymore.

Also, should make sure the deployment is cleaned up from the cluster after the tests have run so they can run again without finding anything dirty.

hsanjuan · 2018-01-15T15:27:29Z

ipfs-cluster/trigger-server/triggerserver.go

+}
+
+func main() {
+    secret_bytes, readErr := ioutil.ReadFile(CONFIG_PATH)


since the config file needs several sections, probably json is a good format.

hsanjuan · 2018-01-15T15:28:33Z

ipfs-cluster/trigger-server/triggerserver.go

+    secret_bytes, readErr := ioutil.ReadFile(CONFIG_PATH)
+    if readErr != nil {
+        log.Printf("Missing secret configuration in %s. We'll generate one for you...\n", CONFIG_PATH)
+	rand.Seed(time.Now().UnixNano())


customary to initialize seeds in init(). Might as well use a proper random source from crypto package. Just need to read enough bytes into a buffer and SHA it for example.

hsanjuan · 2018-01-15T15:32:22Z

ipfs-cluster/trigger-server/triggerserver.go

+	}
+    }
+    secret = string(secret_bytes)
+    log.Printf("Starting with secret as %s on port %d\n", secret, PORT)


better not log secrets

hsanjuan · 2018-01-15T15:33:25Z

ipfs-cluster/trigger-server/triggerserver.go

+	https://github.com/ipfs/kubernetes-ipfs/
+	`)
+    })
+    log.Println(http.ListenAndServe(":" + strconv.FormatInt(PORT, 10), nil))


fmt.Sprintf("%d", PORT). Saves one import.

Major changes: - Output is now streamed from the daemon rather than being dumped at the end. - At the end of the run, data is dumped to IPFS and you are provided an IFPS link to the output. - `golint` now passes sucessfully - The server now responds with 401 to unauthorized users - User can now specify what they want to run - Config is now in JSON format - Config is generated as JSON on first load

FrankPetrilli · 2018-01-17T21:28:29Z

@hsanjuan, will you check my recent changes?

hsanjuan · 2018-01-18T18:14:39Z

ipfs-cluster/trigger-server/README.md

@@ -1,3 +1,9 @@
+# What is trigger server
+
+The trigger server's purpose is defined in ipfs/ipfs-cluster/issues/244. The goal is to create an endpoint


That issue is very long, and deals with other things. In any case, the user should not have to look up an issue in a different repository to find out what the project does, when reading's the project readme.

hsanjuan · 2018-01-18T18:16:38Z

ipfs-cluster/trigger-server/README.md

+The trigger server's purpose is defined in ipfs/ipfs-cluster/issues/244. The goal is to create an endpoint
+which can be triggered remotely and automated to run the tests, obtaining their results.
+
+


Explain that it runs in the kubernetes master? Explain the authentication mechanism and how to set it up? Explain how to put it there after you have the packages? explain that it includes a systemd unit file? how to obtain credentials or where to reach out if needed...

hsanjuan · 2018-01-18T18:19:18Z

ipfs-cluster/trigger-server/README.md

+The trigger server's purpose is defined in ipfs/ipfs-cluster/issues/244. The goal is to create an endpoint
+which can be triggered remotely and automated to run the tests, obtaining their results.
+
+
 To build RPM/DEB
 ---



As an additional note, we don't need new issues for every commit addressing issues in a PR. Commit messages should be more descriptive than this, but it's fine too if you just write WIP, and at the end of the review process squash it all together in a single commit, but in any case an additional issue is not needed.

hsanjuan · 2018-01-18T18:38:34Z

ipfs-cluster/trigger-server/triggerserver.go

+    // This command takes a while, so we'll hang out here while it runs
+    cmd := exec.Command("bash", "-c", fmt.Sprintf("%s%s %s", config.RunnerPath, config.Runner, config.RunnerArgs))
+    cmd.Dir = config.RunnerPath
+    pipeReader, pipeWriter := io.Pipe()


Perhaps implement a custom io.Writer. The Write method in it just writes the bytes to both the response writer and your out buffer. That way you don't need a go-routine on the side reading from one reader and writing somewhere else.

hsanjuan · 2018-01-18T18:43:12Z

ipfs-cluster/trigger-server/triggerserver.go

+
+func init() {
+    rand.Seed(time.Now().UnixNano())
+    configFile, readErr := ioutil.ReadFile(configPath)


configuration handling to ->main(), but do create functions for it.. DefaultConfig(), ReadConfig(), WriteConfig().. they can live in config.go

hsanjuan · 2018-01-18T18:46:15Z

ipfs-cluster/trigger-server/triggerserver.go

+	cwd, _ := os.Getwd()
+	config.Secret = randLetters(30)
+	config.Port = 8082
+	config.RunnerPath = cwd


probably need a way to specify several runners for several projects

hsanjuan · 2018-01-18T18:46:30Z

ipfs-cluster/trigger-server/triggerserver.go

-        fmt.Fprintf(rw, `Kubernetes-IPFS Trigger Server
-	https://github.com/ipfs/kubernetes-ipfs/
-	`)
+	    json.Unmarshal(configFile, &config)


error check

and I think golint would ask you to return from the if and not have an else clause.

FrankPetrilli assigned hsanjuan and ZenGround0 Jan 12, 2018

FrankPetrilli requested review from hsanjuan and ZenGround0 January 13, 2018 23:54

hsanjuan suggested changes Jan 15, 2018

View reviewed changes

FrankPetrilli added 5 commits January 17, 2018 09:47

Working toward #42

dfca914

Resolve #46

c0512fa

Resolve #41

2761a2c

Merge branch 'master' into frankpetrilli/trigger_improvement

73743b5

hsanjuan changed the title ~~https://github.com/ipfs/ipfs-cluster/issues/244~~ Trigger server Jan 18, 2018

hsanjuan suggested changes Jan 18, 2018

View reviewed changes

hsanjuan unassigned ZenGround0 Apr 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trigger server #39

Trigger server #39

FrankPetrilli commented Jan 12, 2018 •

edited

Loading

FrankPetrilli commented Jan 12, 2018

hsanjuan left a comment

hsanjuan Jan 15, 2018

hsanjuan Jan 15, 2018

hsanjuan Jan 15, 2018

hsanjuan Jan 15, 2018

hsanjuan Jan 15, 2018

hsanjuan Jan 15, 2018

hsanjuan Jan 15, 2018

hsanjuan Jan 15, 2018

hsanjuan Jan 15, 2018

hsanjuan Jan 15, 2018

FrankPetrilli commented Jan 17, 2018

hsanjuan Jan 18, 2018

hsanjuan Jan 18, 2018

hsanjuan Jan 18, 2018

hsanjuan Jan 18, 2018

hsanjuan Jan 18, 2018

hsanjuan Jan 18, 2018

hsanjuan Jan 18, 2018

hsanjuan Jan 18, 2018

		@@ -1,3 +1,9 @@
		# What is trigger server

		The trigger server's purpose is defined in ipfs/ipfs-cluster/issues/244. The goal is to create an endpoint

		The trigger server's purpose is defined in ipfs/ipfs-cluster/issues/244. The goal is to create an endpoint
		which can be triggered remotely and automated to run the tests, obtaining their results.

Trigger server #39

Are you sure you want to change the base?

Trigger server #39

Conversation

FrankPetrilli commented Jan 12, 2018 • edited Loading

FrankPetrilli commented Jan 12, 2018

hsanjuan 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

FrankPetrilli commented Jan 17, 2018

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

FrankPetrilli commented Jan 12, 2018 •

edited

Loading