services: key services by server id; auto start cli service flags (#6425

)

* services: key services by server id; auto start cli service flags

Before this, services provided as CLI flag values were not automatically
started, so user module code needed to explicitly start them (or
implicitly start them via a service binding), which is somewhat
surprising and boilerplatey since there's not really any cases where a
user wouldn't want the host service to already be running. This is
debatable though.

Now, the CLI calls start on host services provided as flag args.

In order for this to work, there was also a change needed in the
internal services implementation to key services by the ServerID rather
than ClientID. The difference is that now the state of a service is
shared across a whole session, which includes the CLI and all modules it
invokes (directly or transitively). Otherwise even if the CLI auto
starts the service it will still not be running when the module code
uses it.

That change is also fairly debatable, not a very clear cut best answer
here. Previously, every module could start an identical service and get
their own instance of it. But now modules in a session starting an
identical service will get a shared instance of it. Both behaviors are
logical and possibly desired in different use cases.

I think the main argument for changing to key by ServerID is that it
makes it possible for services to be shared across a session, but still
leaves open the possibilty for them to not be shared by having users
include some "module-specific" key in the service's definition that
causes it to not be de-duped. This is as opposed to the previous use of
ClientID that made it impossible for services to be shared.

One other possible solution here would be to only key host services
(i.e. the one started by the CLI here) by the server ID, but use
ClientID and retain the previous behavior for other services. That feels
a bit too complicated (both to explain and to implement) to be worth it,
but is probably possible if it ends up making more sense.

Signed-off-by: Erik Sipsma <erik@dagger.io>

* update service unit test to account for client->server id

Signed-off-by: Erik Sipsma <erik@dagger.io>

---------

Signed-off-by: Erik Sipsma <erik@dagger.io>

cmd/dagger/flags.go

@@ -382,8 +382,12 @@ func (v *serviceValue) Set(s string) error {
 	return nil
 }
 
-func (v *serviceValue) Get(_ context.Context, c *dagger.Client) (any, error) {
-	return c.Host().Service(v.ports, dagger.HostServiceOpts{Host: v.host}), nil
+func (v *serviceValue) Get(ctx context.Context, c *dagger.Client) (any, error) {
+	svc, err := c.Host().Service(v.ports, dagger.HostServiceOpts{Host: v.host}).Start(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("failed to start service: %w", err)
+	}
+	return svc, nil
 }
 
 // AddFlag adds a flag appropriate for the argument type. Should return a

core/integration/module_call_test.go

@@ -17,12 +17,15 @@ func TestModuleDaggerCallArgTypes(t *testing.T) {
 	t.Run("service args", func(t *testing.T) {
 		t.Parallel()
 
-		modGen := c.Container().From(golangImage).
-			WithMountedFile(testCLIBinPath, daggerCliFile(t, c)).
-			WithWorkdir("/work").
-			With(daggerExec("mod", "init", "--name=test", "--sdk=go")).
-			WithNewFile("main.go", dagger.ContainerWithNewFileOpts{
-				Contents: `package main
+		t.Run("used as service binding", func(t *testing.T) {
+			t.Parallel()
+
+			modGen := c.Container().From(golangImage).
+				WithMountedFile(testCLIBinPath, daggerCliFile(t, c)).
+				WithWorkdir("/work").
+				With(daggerExec("mod", "init", "--name=test", "--sdk=go")).
+				WithNewFile("main.go", dagger.ContainerWithNewFileOpts{
+					Contents: `package main
 import (
 	"context"
 )
@@ -36,19 +39,69 @@ func (m *Test) Fn(ctx context.Context, svc *Service) (string, error) {
 		Stdout(ctx)
 }
 `,
-			})
+				})
 
-		logGen(ctx, t, modGen.Directory("."))
+			logGen(ctx, t, modGen.Directory("."))
 
-		httpServer, _ := httpService(ctx, t, c, "im up")
-		endpoint, err := httpServer.Endpoint(ctx)
-		require.NoError(t, err)
+			httpServer, _ := httpService(ctx, t, c, "im up")
+			endpoint, err := httpServer.Endpoint(ctx)
+			require.NoError(t, err)
 
-		out, err := modGen.
-			WithServiceBinding("testserver", httpServer).
-			With(daggerCall("fn", "--svc", "tcp://"+endpoint)).Stdout(ctx)
-		require.NoError(t, err)
-		require.Equal(t, strings.TrimSpace(out), "im up")
+			out, err := modGen.
+				WithServiceBinding("testserver", httpServer).
+				With(daggerCall("fn", "--svc", "tcp://"+endpoint)).Stdout(ctx)
+			require.NoError(t, err)
+			require.Equal(t, strings.TrimSpace(out), "im up")
+		})
+
+		t.Run("used directly", func(t *testing.T) {
+			t.Parallel()
+
+			modGen := c.Container().From(golangImage).
+				WithMountedFile(testCLIBinPath, daggerCliFile(t, c)).
+				WithWorkdir("/work").
+				With(daggerExec("mod", "init", "--name=test", "--sdk=go")).
+				WithNewFile("main.go", dagger.ContainerWithNewFileOpts{
+					Contents: `package main
+import (
+	"context"
+	"fmt"
+	"strings"
+)
+
+type Test struct {}
+
+func (m *Test) Fn(ctx context.Context, svc *Service) (string, error) {
+	ports, err := svc.Ports(ctx)
+	if err != nil {
+		return "", err
+	}
+	var out []string
+	out = append(out, fmt.Sprintf("%d exposed ports:", len(ports)))
+	for _, port := range ports {
+		number, err := port.Port(ctx)
+		if err != nil {
+			return "", err
+		}
+		out = append(out, fmt.Sprintf("- TCP/%d", number))
+	}
+	return strings.Join(out, "\n"), nil
+}
+`,
+				})
+
+			logGen(ctx, t, modGen.Directory("."))
+
+			httpServer, _ := httpService(ctx, t, c, "im up")
+			endpoint, err := httpServer.Endpoint(ctx)
+			require.NoError(t, err)
+
+			out, err := modGen.
+				WithServiceBinding("testserver", httpServer).
+				With(daggerCall("fn", "--svc", "tcp://"+endpoint)).Stdout(ctx)
+			require.NoError(t, err)
+			require.Equal(t, strings.TrimSpace(out), "1 exposed ports:\n- TCP/8000")
+		})
 	})
 
 	t.Run("list args", func(t *testing.T) {

core/service.go

@@ -450,7 +450,7 @@ func (svc *Service) startContainer(
 			Ports:   ctr.Ports,
 			Key: ServiceKey{
 				Digest:   dig,
-				ClientID: clientMetadata.ClientID,
+				ServerID: clientMetadata.ServerID,
 			},
 			Stop: stopSvc,
 			Wait: func(ctx context.Context) error {
@@ -573,7 +573,7 @@ func (svc *Service) startTunnel(ctx context.Context, id *idproto.ID) (running *R
 		Service: svc,
 		Key: ServiceKey{
 			Digest:   dig,
-			ClientID: clientMetadata.ClientID,
+			ServerID: clientMetadata.ServerID,
 		},
 		Host:  dialHost,
 		Ports: ports,
@@ -656,7 +656,7 @@ func (svc *Service) startReverseTunnel(ctx context.Context, id *idproto.ID) (run
 			Service: svc,
 			Key: ServiceKey{
 				Digest:   dig,
-				ClientID: clientMetadata.ClientID,
+				ServerID: clientMetadata.ServerID,
 			},
 			Host:  fullHost,
 			Ports: checkPorts,

core/services.go

@@ -66,7 +66,7 @@ type RunningService struct {
 // ServiceKey is a unique identifier for a service.
 type ServiceKey struct {
 	Digest   digest.Digest
-	ClientID string
+	ServerID string
 }
 
 // NewServices returns a new Services.
@@ -96,7 +96,7 @@ func (ss *Services) Get(ctx context.Context, id *idproto.ID) (*RunningService, e
 
 	key := ServiceKey{
 		Digest:   dig,
-		ClientID: clientMetadata.ClientID,
+		ServerID: clientMetadata.ServerID,
 	}
 
 	notRunningErr := fmt.Errorf("service %s is not running", network.HostHash(dig))
@@ -146,7 +146,7 @@ func (ss *Services) Start(ctx context.Context, id *idproto.ID, svc Startable) (*
 
 	key := ServiceKey{
 		Digest:   dig,
-		ClientID: clientMetadata.ClientID,
+		ServerID: clientMetadata.ServerID,
 	}
 
 dance:
@@ -265,7 +265,7 @@ func (ss *Services) Stop(ctx context.Context, id *idproto.ID) error {
 
 	key := ServiceKey{
 		Digest:   dig,
-		ClientID: clientMetadata.ClientID,
+		ServerID: clientMetadata.ServerID,
 	}
 
 	ss.l.Lock()
@@ -305,7 +305,7 @@ func (ss *Services) StopClientServices(ctx context.Context, client *engine.Clien
 
 	eg := new(errgroup.Group)
 	for _, svc := range ss.running {
-		if svc.Key.ClientID != client.ClientID {
+		if svc.Key.ServerID != client.ServerID {
 			continue
 		}
 

core/services_test.go

@@ -58,7 +58,7 @@ func TestServicesStartHappy(t *testing.T) {
 	})
 }
 
-func TestServicesStartHappyDifferentClients(t *testing.T) {
+func TestServicesStartHappyDifferentServers(t *testing.T) {
 	t.Parallel()
 
 	ctx := context.Background()
@@ -68,9 +68,9 @@ func TestServicesStartHappyDifferentClients(t *testing.T) {
 
 	svc := newStartable("fake")
 
-	startOne := func(t *testing.T, stub *fakeStartable, clientID string) {
+	startOne := func(t *testing.T, stub *fakeStartable, serverID string) {
 		ctx := engine.ContextWithClientMetadata(ctx, &engine.ClientMetadata{
-			ClientID: clientID,
+			ServerID: serverID,
 		})
 
 		expected := stub.Succeed()
@@ -88,11 +88,11 @@ func TestServicesStartHappyDifferentClients(t *testing.T) {
 	}
 
 	t.Run("start one", func(t *testing.T) {
-		startOne(t, svc, "client-1")
+		startOne(t, svc, "server-1")
 	})
 
 	t.Run("start another", func(t *testing.T) {
-		startOne(t, svc, "client-2")
+		startOne(t, svc, "server-2")
 	})
 }
 
@@ -325,7 +325,7 @@ func (f *fakeStartable) Succeed() *core.RunningService {
 	running := &core.RunningService{
 		Key: core.ServiceKey{
 			Digest:   f.digest,
-			ClientID: "doesnt-matter",
+			ServerID: "doesnt-matter",
 		},
 		Host: f.name + "-host",
 	}