discoverd: Die sdutil! #933

archseer · 2015-02-02T23:41:12Z

This is ready for merge. I'm writing a quick prototype of how the CLI-side interactions with these services would go.

archseer · 2015-02-02T23:44:18Z

receiver/flynn-receive.go

+				Service: &host.PortService{
+					Name:   app.Name + "-web",
+					Create: true,
+					Check:  &host.PortServiceCheck{Type: "http"},


HTTP check by default. Works for majority of cases, but might not fit where the user has a resource that is not available under /, however I think we can just note that in the usage manual, and then have instructions on how to change the service checker using the CLI.

We could add a loose check that accepts any status code (maybe status code -1), what do you think?

I think that could work. We could also just use the TCP checker?

Yeah, probably easier to just do TCP and won't show up in the logs, we can always change it later.

archseer · 2015-02-02T23:57:02Z

@titanous I'm thinking of renaming the types PortService -> Service, PortServiceChecker -> Checker/HealthChecker. WDYT? They are under the host type namespace, so they won't conflict with let's say, Checker from the discoverd package.

titanous · 2015-02-03T00:47:24Z

host/containerinit/init.go

@@ -535,9 +619,6 @@ func Main() {
 		log.Fatalf("Unable to unmarshal config: %v", err)
 	}

-	// Propagate the plugin-specific container env variable
-	config.Env["container"] = os.Getenv("container")
-


Why was this removed?

Is it used anywhere? I think it's a leftover from the original code.

It sets an environment variable that should be passed on. It is 'used' in the sense that that env var should be available.

Yes, I figured that os.Getenv("container") == "", since I couldn't find anything setting container

libvirt-lxc sets it. Check out the environment of a container.

titanous · 2015-02-03T00:48:46Z

Changing the type names is fine, HealthChecker sounds good.

titanous · 2015-02-03T00:50:20Z

host/containerinit/init.go

+	}
+	portStr := strconv.Itoa(port.Port)
+	if portStr == "0" {
+		portStr = env["PORT"]


Do this before converting to a string.

env["PORT"] is a string, so I need to convert that to an integer then.

Ah, misunderstood this. Can't we just serialize the info after port assignment?

What do you mean by that?

titanous · 2015-02-03T00:53:18Z

This should have a bunch of integration tests covering each option of the service check, especially stuff like KillDown and thresholds.

archseer · 2015-02-03T21:02:38Z

@titanous Do we need to test all cases though? I'd test the KillDown + StartTimeout, but all of the Monitor and Checker options are already tested in the discoverd unit tests.

titanous · 2015-02-03T21:12:46Z

A sanity check or two wouldn't hurt, but you don't need to go overboard.

archseer · 2015-02-05T03:10:53Z

@titanous Tests pass.

titanous · 2015-02-05T03:17:47Z

host/containerinit/init.go

+			}
+		}
+	}
+	localAddr := fmt.Sprintf("127.0.0.1:%v", port.Port)


Do we need this? Can't we just use addr?

I ran into problems originally, but I think that might have happened because of other problems. Now that we have tests, I'll give it a try.

titanous · 2015-02-09T05:02:30Z

test/apps/ping/main.go

+	port := os.Getenv("PORT")
+	addr := ":" + port
+
+	hb, err := discoverd.AddServiceAndRegister(service, addr)


Why does this service register with discoverd?

Ouch, totally missed that, I think I copied the boilerplate code from the TCP app...

archseer · 2015-02-09T11:28:30Z

@titanous Addressed again. Note that TestFailure takes 11s for some reason, the current == nil event is sent straight away, but the down event is delayed.

titanous · 2015-02-09T18:40:58Z

@archseer Yes, sounds like a threshold/timeout issue.

archseer · 2015-02-11T05:45:33Z

@titanous Ready for another pass, should pass CI after the server is restarted.

titanous · 2015-02-11T18:58:39Z

discoverd/health/register_test.go

@@ -136,7 +136,11 @@ func (RegisterSuite) TestRegister(c *C) {
 			Events:    make(chan MonitorEvent),
 		}
 		hb := reg.Register()
-		defer hb.Close()
+		defer func() {
+			go func() { <-unregisterChan }()


What does this goroutine do?

1559e5f calls the wrapped heartbeater's close method, which then triggers the mocked Close() method, that sends a notification to the unregisterChan. I'm making sure the channel gets drained here, so that the function does not block indefinitely.

titanous · 2015-02-12T03:53:18Z

LGTM

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

It's no longer used or implemented. Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

Now that the services can be registered using containerinit and configuration, we can get rid of this utility -- this way users don't need to create custom images that include sdutil if they want to register their service. Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

discoverd: Die sdutil!

archseer reviewed Feb 2, 2015
View reviewed changes

titanous reviewed Feb 3, 2015
View reviewed changes

archseer force-pushed the discoverd-die-sdutil branch 4 times, most recently from e02e0f6 to 09c8393 Compare February 3, 2015 21:00

archseer force-pushed the discoverd-die-sdutil branch from 09c8393 to a86d3c3 Compare February 3, 2015 21:04

archseer force-pushed the discoverd-die-sdutil branch 8 times, most recently from f7fb2ca to 04404d9 Compare February 5, 2015 02:50

titanous reviewed Feb 5, 2015
View reviewed changes

titanous reviewed Feb 9, 2015
View reviewed changes

archseer force-pushed the discoverd-die-sdutil branch from 147014a to b9b32dd Compare February 9, 2015 05:41

archseer force-pushed the discoverd-die-sdutil branch 3 times, most recently from 0bafbb5 to 499f551 Compare February 11, 2015 05:44

archseer force-pushed the discoverd-die-sdutil branch from 499f551 to 1559e5f Compare February 11, 2015 05:49

titanous reviewed Feb 11, 2015
View reviewed changes

Blaž Hrastnik added 10 commits February 12, 2015 13:01

host: Add types for service discovery configuration.

1db8ef2

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

host: Consistently use the imageRoot constant.

f6365b6

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

controller, host: Get rid of Port.RangeEnd.

2e57f88

It's no longer used or implemented. Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

containerinit: Use Register to healthcheck and register services.

7583980

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

receiver: Use the service declaration instead of sdutil.

0067e45

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

bootstrap: Declare gitreceive and dashboard services.

161a11e

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

test: Add 'ping' server test application.

5ff79be

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

test: Add healthcheck tests.

8414819

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

discoverd: Close the wrapped heartbeater on Register.Close().

97afdf0

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

archseer force-pushed the discoverd-die-sdutil branch from 1559e5f to 97afdf0 Compare February 12, 2015 04:01

archseer pushed a commit that referenced this pull request Feb 12, 2015

Merge pull request #933 from flynn/discoverd-die-sdutil

474cd75

discoverd: Die sdutil!

archseer merged commit 474cd75 into master Feb 12, 2015

archseer deleted the discoverd-die-sdutil branch February 12, 2015 04:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

discoverd: Die sdutil! #933

discoverd: Die sdutil! #933

archseer commented Feb 2, 2015

archseer Feb 2, 2015

titanous Feb 3, 2015

archseer Feb 3, 2015

titanous Feb 3, 2015

archseer commented Feb 2, 2015

titanous Feb 3, 2015

archseer Feb 3, 2015

titanous Feb 3, 2015

archseer Feb 3, 2015

titanous Feb 3, 2015

titanous commented Feb 3, 2015

titanous Feb 3, 2015

archseer Feb 3, 2015

titanous Feb 3, 2015

archseer Feb 3, 2015

titanous commented Feb 3, 2015

archseer commented Feb 3, 2015

titanous commented Feb 3, 2015

archseer commented Feb 5, 2015

titanous Feb 5, 2015

archseer Feb 8, 2015

titanous Feb 9, 2015

archseer Feb 9, 2015

archseer commented Feb 9, 2015

titanous commented Feb 9, 2015

archseer commented Feb 11, 2015

titanous Feb 11, 2015

archseer Feb 12, 2015

titanous commented Feb 12, 2015

discoverd: Die sdutil! #933

discoverd: Die sdutil! #933

Conversation

archseer commented Feb 2, 2015

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

archseer commented Feb 2, 2015

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

titanous commented Feb 3, 2015

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

titanous commented Feb 3, 2015

archseer commented Feb 3, 2015

titanous commented Feb 3, 2015

archseer commented Feb 5, 2015

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

archseer commented Feb 9, 2015

titanous commented Feb 9, 2015

archseer commented Feb 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

titanous commented Feb 12, 2015