-
Notifications
You must be signed in to change notification settings - Fork 128
sync docker credentials to container #947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import ( | |
| "time" | ||
|
|
||
| "github.com/containerd/errdefs" | ||
| dockercliconfig "github.com/docker/cli/cli/config" | ||
| gpupkg "github.com/docker/model-runner/cmd/cli/pkg/gpu" | ||
| "github.com/docker/model-runner/cmd/cli/pkg/types" | ||
| "github.com/moby/moby/api/types/container" | ||
|
|
@@ -28,17 +29,16 @@ import ( | |
| // controllerContainerName is the name to use for the controller container. | ||
| const controllerContainerName = "docker-model-runner" | ||
|
|
||
| // copyDockerConfigToContainer copies the Docker config file from the host to the container | ||
| // and sets up proper ownership and permissions for the modelrunner user. | ||
| // It does nothing for Desktop and Cloud engine kinds. | ||
| func copyDockerConfigToContainer(ctx context.Context, dockerClient *client.Client, containerID string, engineKind types.ModelRunnerEngineKind) error { | ||
| // SyncDockerConfigToContainer copies the host's ~/.docker/config.json into the | ||
| // running container. It is a no-op for Desktop and Cloud engine kinds. | ||
| func SyncDockerConfigToContainer(ctx context.Context, dockerClient *client.Client, containerID string, engineKind types.ModelRunnerEngineKind) error { | ||
|
areebahmeddd marked this conversation as resolved.
areebahmeddd marked this conversation as resolved.
|
||
| // Do nothing for Desktop and Cloud engine kinds | ||
| if engineKind == types.ModelRunnerEngineKindDesktop || engineKind == types.ModelRunnerEngineKindCloud || | ||
|
sourcery-ai[bot] marked this conversation as resolved.
|
||
| os.Getenv("_MODEL_RUNNER_TREAT_DESKTOP_AS_MOBY") == "1" { | ||
| return nil | ||
| } | ||
|
|
||
| dockerConfigPath := os.ExpandEnv("$HOME/.docker/config.json") | ||
| dockerConfigPath := filepath.Join(dockercliconfig.Dir(), "config.json") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked with the previous implementation and the current one they have different ways internally for example if someone hase set
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest comparing against what moby/containerd does and copying that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i used config.Dir() from docker/cli here because that's the same logic docker uses to locate its config directory for the moby/containerd point.. my understanding is that this part is handled on the cli side rather than by the daemon itself
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @areebahmeddd yes this might be the normal way but always make sure nothing breaks for backwards compatibility. if you check the implementations they differ a bit that's why i pointed it out.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you point out for me? 🤔
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so this is from the ... You see the implementation is different. thats what i was pointing to like it considers some extra env overrides may be the users are using now which might break
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i see .. let me try something |
||
| if s, err := os.Stat(dockerConfigPath); err != nil || s.Mode()&os.ModeType != 0 { | ||
| return nil | ||
| } | ||
|
|
@@ -622,7 +622,7 @@ func CreateControllerContainer(ctx context.Context, dockerClient *client.Client, | |
|
|
||
| // Copy Docker config file if it exists and we're the container creator. | ||
| if created && !vllmOnWSL { | ||
| if err := copyDockerConfigToContainer(ctx, dockerClient, resp.ID, engineKind); err != nil { | ||
| if err := SyncDockerConfigToContainer(ctx, dockerClient, resp.ID, engineKind); err != nil { | ||
| // Log warning but continue - don't fail container creation | ||
| printer.Printf("Warning: failed to copy Docker config: %v\n", err) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package standalone | ||
|
|
||
| import ( | ||
| "context" | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/docker/model-runner/cmd/cli/pkg/types" | ||
| ) | ||
|
|
||
| // TestSyncDockerConfigToContainer_NoopForDesktopAndCloud verifies that | ||
| // SyncDockerConfigToContainer skips Desktop and Cloud engine kinds. | ||
|
areebahmeddd marked this conversation as resolved.
|
||
| func TestSyncDockerConfigToContainer_NoopForDesktopAndCloud(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| dockerDir := filepath.Join(tmpDir, ".docker") | ||
| if err := os.MkdirAll(dockerDir, 0o700); err != nil { | ||
| t.Fatalf("failed to create .docker dir: %v", err) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(dockerDir, "config.json"), []byte(`{}`), 0o600); err != nil { | ||
| t.Fatalf("failed to write config.json: %v", err) | ||
| } | ||
| t.Setenv("HOME", tmpDir) | ||
|
|
||
| for _, engineKind := range []types.ModelRunnerEngineKind{ | ||
| types.ModelRunnerEngineKindDesktop, | ||
| types.ModelRunnerEngineKindCloud, | ||
| } { | ||
| t.Run(engineKind.String(), func(t *testing.T) { | ||
| err := SyncDockerConfigToContainer(context.Background(), nil, "container-id", engineKind) | ||
| if err != nil { | ||
| t.Fatalf("SyncDockerConfigToContainer(%v) returned unexpected error: %v", engineKind, err) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TestSyncDockerConfigToContainer_NoopWhenConfigMissing verifies that | ||
| // SyncDockerConfigToContainer skips when the host config file is absent. | ||
| func TestSyncDockerConfigToContainer_NoopWhenConfigMissing(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| t.Setenv("HOME", tmpDir) | ||
|
|
||
| err := SyncDockerConfigToContainer(context.Background(), nil, "container-id", types.ModelRunnerEngineKindMoby) | ||
| if err != nil { | ||
| t.Fatalf("SyncDockerConfigToContainer returned unexpected error for missing config: %v", err) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.