Skip to content
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

feat(desktop): synchronized file share integration #11614

Merged
merged 1 commit into from Mar 20, 2024

Conversation

milas
Copy link
Member

@milas milas commented Mar 13, 2024

What I did
Initial integration between Docker Desktop's "Synchronized File Shares" functionality and Compose.

When running under Docker Desktop, File Shares will be created/used for any bind mount paths. (There's a missing feature check that still needs to be added, but ALL desktop functionality is currently gated behind the COMPOSE_EXPERIMENTAL_DESKTOP env var.)

Compose will wait for the File Share to reach steady state, i.e. having completed initial synchronization. The progress is shown, similar to how layer pulls for images are displayed.

Related issue
JIRA:

(not mandatory) A picture of a cute animal, if possible in relation to what you did
happy looking platypus on someone's hand

@milas milas added kind/feature area/desktop Features that are integrated with Docker Desktop labels Mar 13, 2024
@milas milas requested review from xenoscopic and a team March 13, 2024 19:37
@milas milas self-assigned this Mar 13, 2024
@milas milas requested review from ndeloof, glours and jhrotko and removed request for a team March 13, 2024 19:37
Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 6.64137% with 492 lines in your changes are missing coverage. Please review.

Project coverage is 55.65%. Comparing base (1b5fa3b) to head (eaabe65).

Files Patch % Lines
internal/desktop/file_shares.go 0.00% 264 Missing ⚠️
internal/desktop/client.go 0.00% 112 Missing ⚠️
internal/paths/paths.go 16.66% 49 Missing and 1 partial ⚠️
pkg/compose/create.go 18.42% 29 Missing and 2 partials ⚠️
pkg/progress/tty.go 26.31% 24 Missing and 4 partials ⚠️
pkg/compose/down.go 0.00% 4 Missing and 1 partial ⚠️
pkg/watch/watcher_naive.go 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11614      +/-   ##
==========================================
- Coverage   57.58%   55.65%   -1.93%     
==========================================
  Files         140      142       +2     
  Lines       11805    12241     +436     
==========================================
+ Hits         6798     6813      +15     
- Misses       4322     4740     +418     
- Partials      685      688       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

LGTM
My understanding was we wanted file share to be used only for watch sync, not for all bind mounts, maybe I missed some discussion :)

Copy link

@xenoscopic xenoscopic left a comment

Choose a reason for hiding this comment

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

LGTM, my only real concern is the project label values, but maybe Compose's normalization is sufficient? If we can add label validation to the backend, then it's a little less worrying that we might ingest an invalid value.

Comment on lines +53 to +55
if share.Labels["com.docker.compose.project"] == projectName {
toRemove = append(toRemove, share)
}

Choose a reason for hiding this comment

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

We could extend the file shares API with a mechanism to remove these using a label selector. I'm not sure if that would make things more robust; it might make status updates more difficult.

}

func (c *Client) ListFileShares(ctx context.Context) ([]FileShareSession, error) {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, backendURL("/mutagen/file-shares"), http.NoBody)

Choose a reason for hiding this comment

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

Maybe we should add some comments in the backend that these URL patterns need to stay fixed. I guess we could add some sort of service discovery mechanism, but that seems like overkill.

req := CreateFileShareRequest{
HostPath: path,
Labels: map[string]string{
"com.docker.compose.project": m.projectName,

Choose a reason for hiding this comment

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

We may need to sanitize m.projectName to comply with Mutagen's (i.e. Kubernetes') label value syntax (maybe it already is, I'm not sure). I don't think it will affect the loop-based approach you have above, but it will break LabelSelector-based selection in the Mutagen synchronization manager (possibly/probably for other sessions too).

Initial integration between Docker Desktop's "Synchronized File Shares"
functionality and Compose.

When running under Docker Desktop, File Shares will be created/used for
any bind mount paths. (There's a missing feature check that still needs
to be added, but ALL desktop functionality is currently gated behind the
`COMPOSE_EXPERIMENTAL_DESKTOP` env var.)

Compose will wait for the File Share to reach steady state, i.e. having
completed initial synchronization. The progress is shown, similar to how
layer pulls for images are displayed.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
@milas milas merged commit db4ed89 into docker:main Mar 20, 2024
28 checks passed
@milas milas deleted the feat/desktop-sfs branch March 20, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/desktop Features that are integrated with Docker Desktop kind/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants