Merge pull request #14 from vinyl-linux/segregate_erros

Ensure boot handles broken configs better

Makefile

@@ -49,7 +49,7 @@ $(CLIENT_BINARY): client client/*.go client/**/*.go
 # This avoids having to redownload dependencies and having to configure
 # toolchains/ go env vars
 $(TEST_BINARY): $(GRPC_FILES) $(CERTS) *.go go.mod go.sum
-	-go test -covermode=count -o $@ -tags sudo > /dev/null
+	go test -covermode=count -c -o $@ -tags sudo > /dev/null
 
 .PHONY: test
 test: $(TEST_BINARY)

dispatcher.go

@@ -11,8 +11,9 @@ import (
 )
 
 var (
-	errNoService       = status.Error(codes.InvalidArgument, "missing service name")
-	errServiceNotExist = status.Error(codes.InvalidArgument, "service does not exist")
+	errNoService        = status.Error(codes.InvalidArgument, "missing service name")
+	errServiceNotExist  = status.Error(codes.InvalidArgument, "service does not exist")
+	errServiceDodgyConf = status.Error(codes.FailedPrecondition, "service config is incorrect")
 )
 
 type Dispatcher struct {

dispatcher_test.go

@@ -29,10 +29,7 @@ func newDispatcher() Dispatcher {
 		panic(err)
 	}
 
-	supervisor, err := New(pwd + "/testdata/services")
-	if err != nil {
-		panic(err)
-	}
+	supervisor, _ := New(pwd + "/testdata/services")
 
 	return Dispatcher{supervisor, dispatcher.UnimplementedDispatcherServer{}}
 }
@@ -142,6 +139,19 @@ func TestDispatcher_Start(t *testing.T) {
 		}
 	})
 
+	t.Run("Service must not have a dodgy config", func(t *testing.T) {
+		_, err := d.Start(context.Background(), &dispatcher.Service{
+			Name: "broken",
+		})
+		if err == nil {
+			t.Fatalf("expected error")
+		}
+
+		if err.Error() != "rpc error: code = FailedPrecondition desc = service config is incorrect" {
+			t.Errorf("error: %#v should be %q", err.Error(), "service config is incorrect")
+		}
+	})
+
 	t.Run("Service must exist", func(t *testing.T) {
 		_, err := d.Start(context.Background(), &dispatcher.Service{
 			Name: "foo",
@@ -351,8 +361,17 @@ func TestDispatcher_ReadConfigs(t *testing.T) {
 	currentStatus := d.s.services["app"].status
 
 	_, err = d.ReadConfigs(context.Background(), new(emptypb.Empty))
-	if err != nil {
-		t.Errorf("unexpected error: %#v", err)
+	if err == nil {
+		t.Fatal("expected error, received none")
+	}
+
+	cpe, ok := err.(ConfigParseError)
+	if !ok {
+		t.Fatalf("unexpected error of type: %T", err)
+	}
+
+	if _, ok = cpe.errors["01-broken"]; !ok {
+		t.Fatalf("expected an error for service %q in %#v", "01-broken", cpe)
 	}
 
 	if !reflect.DeepEqual(currentStatus, d.s.services["app"].status) {

service.go

@@ -42,16 +42,13 @@ type Service struct {
 	// prior to restarting
 	desiredRunning bool
 
-	// isDirty is set to true if a call to Supervisor.LoadConfigs
+	// loadError is set if a call to Supervisor.LoadConfigs
 	// fails and so new config hasn't been picked up.
 	//
 	// we can assume that this means there's a new config that doesn't
 	// look right, since the existence of this service means it must
 	// have been right first time.
-	//
-	// but, in any case, isDirty is a flag that means 'Something went
-	// wrong parsing this service config, go check the logs'
-	isDirty bool
+	loadError string
 }
 
 func LoadService(dir string) (s *Service, err error) {

supervisor.go

@@ -75,15 +75,19 @@ func (s *Supervisor) LoadConfigs() (err error) {
 
 		svc, err = LoadService(filepath.Join(s.dir, entry.Name()))
 		if err != nil {
-			// log error, recover
-			svc.isDirty = true
 			cpe.Append(entry.Name(), err)
-
-			continue
+			svc.loadError = err.Error()
 		}
 
 		name := serviceName(entry.Name())
-		groupName := s.Config.ReconcileOverride(name, svc.Config.Grouping.GroupName)
+
+		var groupName string
+		if svc.loadError == "" {
+			groupName = s.Config.ReconcileOverride(name, svc.Config.Grouping.GroupName)
+		} else {
+			groupName = ""
+		}
+
 		_, ok := groupsServices[groupName]
 		if !ok {
 			groupsServices[groupName] = make([]string, 0)
@@ -101,15 +105,13 @@ func (s *Supervisor) LoadConfigs() (err error) {
 		services[name] = svc
 	}
 
-	if len(cpe.errors) > 0 {
-		return cpe
-	}
-
-	// Only assign new services and groups when everything loads,
-	// rather than accidentally returning broken state
 	s.groupsServices = groupsServices
 	s.services = services
 
+	if len(cpe.errors) > 0 {
+		err = cpe
+	}
+
 	return
 }
 
@@ -119,6 +121,10 @@ func (s *Supervisor) Start(name string, wait bool) error {
 		return errServiceNotExist
 	}
 
+	if svc.loadError != "" {
+		return errServiceDodgyConf
+	}
+
 	return svc.Start(wait)
 }
 
@@ -153,6 +159,12 @@ func (s *Supervisor) StartAll() {
 	var err error
 
 	for _, group := range s.Config.Groups {
+		// Ignore anything with an empty group; this signifies
+		// a config error
+		if group == "" {
+			continue
+		}
+
 		services, ok := s.groupsServices[group]
 		if !ok {
 			sugar.Errorw("group either has no services or does not exist",

supervisor_test.go

@@ -6,14 +6,12 @@ import (
 )
 
 func TestNew(t *testing.T) {
-	s, err := New("testdata/services")
-	if err != nil {
-		t.Errorf("unexpected error %#v", err)
-	}
+	s, _ := New("testdata/services")
 
 	t.Run("groups and service mappings are correct", func(t *testing.T) {
 		got := s.groupsServices
 		expect := map[string][]string{
+			"":       []string{"broken"},
 			"system": []string{"app", "app-cronjob", "app-oneoff"},
 		}
 

testdata/services/01-broken/.config.toml

@@ -0,0 +1,15 @@
+# 'App' service config file
+#
+
+type = "service"
+reload_signal = "nuffin"
+
+[user]
+user = "jspc"
+group = "jspc"
+
+[grouping]
+name = "system"
+
+[command]
+args = "-a -b -c 100"

testdata/services/01-broken/bin

@@ -0,0 +1,6 @@
+#!/usr/bin/env bash
+
+while true; do
+	date
+	sleep 1s
+done

testdata/services/01-broken/environment

@@ -0,0 +1 @@
+PATH=/bin:/sbin

testdata/services/01-broken/environment_overrides

@@ -0,0 +1 @@
+HELLO=world