Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 35 additions & 24 deletions cmd/cli/commands/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ import (
"os"
"strconv"

"github.com/docker/model-runner/cmd/cli/pkg/types"

"github.com/docker/cli/cli-plugins/hooks"
"github.com/docker/model-runner/cmd/cli/commands/completion"
"github.com/docker/model-runner/cmd/cli/desktop"
"github.com/docker/model-runner/cmd/cli/pkg/standalone"
"github.com/docker/model-runner/cmd/cli/pkg/types"
"github.com/docker/model-runner/pkg/inference"
"github.com/spf13/cobra"
)

Expand All @@ -21,7 +22,7 @@ func newStatusCmd() *cobra.Command {
Use: "status",
Short: "Check if the Docker Model Runner is running",
RunE: func(cmd *cobra.Command, args []string) error {
standalone, err := ensureStandaloneRunnerAvailable(cmd.Context(), asPrinter(cmd), false)
runner, err := ensureStandaloneRunnerAvailable(cmd.Context(), asPrinter(cmd), false)
if err != nil {
return fmt.Errorf("unable to initialize standalone model runner: %w", err)
}
Expand All @@ -40,7 +41,7 @@ func newStatusCmd() *cobra.Command {
}

if formatJson {
return jsonStatus(standalone, status, backendStatus)
return jsonStatus(asPrinter(cmd), runner, status, backendStatus)
} else {
textStatus(cmd, status, backendStatus)
}
Expand Down Expand Up @@ -69,44 +70,54 @@ func textStatus(cmd *cobra.Command, status desktop.Status, backendStatus map[str
}
}

func jsonStatus(standalone *standaloneRunner, status desktop.Status, backendStatus map[string]string) error {
func makeEndpoint(host string, port int) string {
return "http://" + net.JoinHostPort(host, strconv.Itoa(port)) + "/v1/"
}

func jsonStatus(printer standalone.StatusPrinter, runner *standaloneRunner, status desktop.Status, backendStatus map[string]string) error {
type Status struct {
Running bool `json:"running"`
Backends map[string]string `json:"backends"`
Endpoint string `json:"endpoint"`
Running bool `json:"running"`
Backends map[string]string `json:"backends"`
Kind string `json:"kind"`
Endpoint string `json:"endpoint"`
EndpointHost string `json:"endpointHost"`
}
var endpoint string
var endpoint, endpointHost string
kind := modelRunner.EngineKind()
switch kind {
case types.ModelRunnerEngineKindDesktop:
endpoint = "http://model-runner.docker.internal/engines/v1/"
endpoint = "http://model-runner.docker.internal/v1/"
endpointHost = "http://localhost" + inference.ExperimentalEndpointsPrefix + "/v1/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with other cases like ModelRunnerEngineKindMobyManual and to avoid re-implementing the URL construction logic, it's better to use modelRunner.URL("/v1/") to get the host endpoint URL. The modelRunner context is already configured with the correct base URL for host-to-model-runner communication in the Desktop case. This change improves maintainability by centralizing URL construction.

Suggested change
endpointHost = "http://localhost" + inference.ExperimentalEndpointsPrefix + "/v1/"
endpointHost = modelRunner.URL("/v1/")

case types.ModelRunnerEngineKindMobyManual:
endpoint = modelRunner.URL("/engines/v1/")
endpoint = modelRunner.URL("/v1/")
endpointHost = endpoint
case types.ModelRunnerEngineKindCloud:
fallthrough
case types.ModelRunnerEngineKindMoby:
if standalone.gatewayIP == "" {
standalone.gatewayIP = "127.0.0.1"
if runner.gatewayIP == "" {
runner.gatewayIP = "127.0.0.1"
}

if standalone.gatewayPort == 0 {
standalone.gatewayPort = 12434
if runner.gatewayPort == 0 {
runner.gatewayPort = standalone.DefaultControllerPortCloud
}

endpoint = "http://" + net.JoinHostPort(standalone.gatewayIP, strconv.Itoa(int(standalone.gatewayPort))) + "/engines/v1/"
endpoint = makeEndpoint(runner.gatewayIP, int(runner.gatewayPort))
endpointHost = makeEndpoint("127.0.0.1", standalone.DefaultControllerPortCloud)
Comment on lines 94 to +102
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

There's a potential nil pointer dereference here. The runner variable can be nil if ensureStandaloneRunnerAvailable returns a nil runner (e.g., for non-standalone setups). If kind is ModelRunnerEngineKindCloud, the code proceeds to access runner.gatewayIP, which would cause a panic if runner is nil. Adding a nil check for runner will make the code more robust and prevent a crash.

Suggested change
case types.ModelRunnerEngineKindCloud:
fallthrough
case types.ModelRunnerEngineKindMoby:
if standalone.gatewayIP == "" {
standalone.gatewayIP = "127.0.0.1"
if runner.gatewayIP == "" {
runner.gatewayIP = "127.0.0.1"
}
if standalone.gatewayPort == 0 {
standalone.gatewayPort = 12434
if runner.gatewayPort == 0 {
runner.gatewayPort = standalone.DefaultControllerPortCloud
}
endpoint = "http://" + net.JoinHostPort(standalone.gatewayIP, strconv.Itoa(int(standalone.gatewayPort))) + "/engines/v1/"
endpoint = makeEndpoint(runner.gatewayIP, int(runner.gatewayPort))
endpointHost = makeEndpoint("127.0.0.1", standalone.DefaultControllerPortCloud)
case types.ModelRunnerEngineKindCloud:
if runner == nil {
return fmt.Errorf("internal error: standalone runner is required for cloud engine kind but was not provided")
}
if runner.gatewayIP == "" {
runner.gatewayIP = "127.0.0.1"
}
if runner.gatewayPort == 0 {
runner.gatewayPort = standalone.DefaultControllerPortCloud
}
endpoint = makeEndpoint(runner.gatewayIP, int(runner.gatewayPort))
endpointHost = makeEndpoint("127.0.0.1", standalone.DefaultControllerPortCloud)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh that's true in case of setting MODEL_RUNNER_NO_AUTO_INSTALL

case types.ModelRunnerEngineKindMoby:
endpoint = makeEndpoint("host.docker.internal", standalone.DefaultControllerPortMoby)
endpointHost = makeEndpoint("127.0.0.1", standalone.DefaultControllerPortMoby)
default:
return fmt.Errorf("unhandled engine kind: %v", kind)
}
s := Status{
Running: status.Running,
Backends: backendStatus,
Endpoint: endpoint,
Running: status.Running,
Backends: backendStatus,
Kind: kind.String(),
Endpoint: endpoint,
EndpointHost: endpointHost,
}
marshal, err := json.Marshal(s)
if err != nil {
return err
}
fmt.Println(string(marshal))
printer.Println(string(marshal))
return nil
}

Expand Down
79 changes: 79 additions & 0 deletions cmd/cli/commands/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,19 @@ package commands

import (
"bytes"
"encoding/json"
"fmt"
"io"
"net/http"
"strconv"
"strings"
"testing"

"github.com/docker/cli/cli-plugins/hooks"
"github.com/docker/model-runner/cmd/cli/desktop"
mockdesktop "github.com/docker/model-runner/cmd/cli/mocks"
"github.com/docker/model-runner/cmd/cli/pkg/standalone"
"github.com/docker/model-runner/cmd/cli/pkg/types"
"github.com/docker/model-runner/pkg/inference"
"github.com/stretchr/testify/require"
"go.uber.org/mock/gomock"
Expand Down Expand Up @@ -126,3 +130,78 @@ func TestStatus(t *testing.T) {
})
}
}

func TestJsonStatus(t *testing.T) {
tests := []struct {
name string
engineKind types.ModelRunnerEngineKind
urlPrefix string
expectedKind string
expectedEndpoint string
expectedHostEnd string
}{
{
name: "Docker Desktop",
engineKind: types.ModelRunnerEngineKindDesktop,
urlPrefix: "http://localhost" + inference.ExperimentalEndpointsPrefix,
expectedKind: "Docker Desktop",
expectedEndpoint: "http://model-runner.docker.internal/v1/",
expectedHostEnd: "http://localhost" + inference.ExperimentalEndpointsPrefix + "/v1/",
},
{
name: "Docker Engine",
engineKind: types.ModelRunnerEngineKindMoby,
urlPrefix: "http://localhost:" + strconv.Itoa(standalone.DefaultControllerPortMoby),
expectedKind: "Docker Engine",
expectedEndpoint: makeEndpoint("host.docker.internal", standalone.DefaultControllerPortMoby),
expectedHostEnd: makeEndpoint("127.0.0.1", standalone.DefaultControllerPortMoby),
},
{
name: "Docker Cloud",
engineKind: types.ModelRunnerEngineKindCloud,
urlPrefix: "http://localhost:" + strconv.Itoa(standalone.DefaultControllerPortCloud),
expectedKind: "Docker Cloud",
expectedEndpoint: makeEndpoint("127.0.0.1", standalone.DefaultControllerPortCloud),
expectedHostEnd: makeEndpoint("127.0.0.1", standalone.DefaultControllerPortCloud),
},
{
name: "Docker Engine (Manual Install)",
engineKind: types.ModelRunnerEngineKindMobyManual,
urlPrefix: "http://localhost:8080",
expectedKind: "Docker Engine (Manual Install)",
expectedEndpoint: "http://localhost:8080/v1/",
expectedHostEnd: "http://localhost:8080/v1/",
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
modelRunner = desktop.NewContextForMockWithKind(nil, test.engineKind, test.urlPrefix)

var output string
printer := desktop.NewSimplePrinter(func(msg string) {
output = msg
})
status := desktop.Status{Running: true}
backendStatus := map[string]string{"llama.cpp": "running"}

// Cloud kind needs a runner for gateway IP/port
var runner *standaloneRunner
if test.engineKind == types.ModelRunnerEngineKindCloud {
runner = &standaloneRunner{}
}

err := jsonStatus(printer, runner, status, backendStatus)
require.NoError(t, err)

var result map[string]any
err = json.Unmarshal([]byte(output), &result)
require.NoError(t, err)

require.Equal(t, test.expectedKind, result["kind"])
require.Equal(t, test.expectedEndpoint, result["endpoint"])
require.Equal(t, test.expectedHostEnd, result["endpointHost"])
require.Equal(t, true, result["running"])
})
}
}
14 changes: 14 additions & 0 deletions cmd/cli/desktop/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,20 @@ func NewContextForMock(client DockerHttpClient) *ModelRunnerContext {
}
}

// NewContextForMockWithKind is a ModelRunnerContext constructor exposed only for the
// purposes of mock testing with a specific engine kind.
func NewContextForMockWithKind(client DockerHttpClient, kind types.ModelRunnerEngineKind, rawURLPrefix string) *ModelRunnerContext {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we could update NewContextForTest to have the kind parameter instead of creating this new constructor

urlPrefix, err := url.Parse(rawURLPrefix)
if err != nil {
panic("error occurred while parsing URL: " + err.Error())
}
return &ModelRunnerContext{
kind: kind,
urlPrefix: urlPrefix,
client: client,
}
}

// NewContextForTest creates a ModelRunnerContext for integration testing
// with a custom URL endpoint. This is intended for use in integration tests
// where the Model Runner endpoint is dynamically created (e.g., testcontainers).
Expand Down
Loading