Implement Writable for the Volume plugin #771
Conversation
MikaelSmith
force-pushed
the
volume-write
branch
from
6324667 to
0ff1dc9
Compare
ghost
requested review from
| // VolumeWrite satisfies the Interface required by Write to write content to a file. | ||
| func (d *FS) VolumeWrite(ctx context.Context, path string, b []byte) error { | ||
| activity.Record(ctx, "Writing %v bytes to %v on %v", len(b), path, plugin.ID(d.executor)) | ||
| command := d.selectShellCommand([]string{"cp", "/dev/stdin", path}, []string{"$input | Set-Content '" + path + "'"}) |
There was a problem hiding this comment.
Do we print the command that's running? If not, then we probably should. That would make it easier to make sense of the stdout:/stderr: output.
plugin/docker/volume.go
Outdated
| type doInContainerFn = func(containerDeleter) error | ||
|
|
||
| // Executes the callback while handling start and cleanup of an existing container. | ||
| func (v *volume) doInTemporaryContainer(ctx context.Context, cid string, fn doInContainerFn) error { |
There was a problem hiding this comment.
Could createContainer's return signature be extended to (cid, deleteFunc, error), and make the post-condition be that it returns a running container? If so, then I don't think we need doInTemporaryContainer.
There was a problem hiding this comment.
Yeah I think so. At one point doInTemporaryContainer handled some of the defer calls, but that wasn't flexible enough.
MikaelSmith
force-pushed
the
volume-write
branch
2 times, most recently
from
ce95deb to
bcccf34
Compare
|
I think this is ready again. |
plugin/docker/volume.go
Outdated
| } | ||
| defer cleanup() | ||
|
|
||
| // Create a tar of the file contents and upload it. |
There was a problem hiding this comment.
Why are we tar'ing and copying? Some clarification would help. Seems weird that this is different from the Kubernetes PVC implementation.
There was a problem hiding this comment.
Other than the clarification, PR looks good.
There was a problem hiding this comment.
Added. I suppose I could've done it with execs like everything else. That would avoid needing to pass in the permissions.
There was a problem hiding this comment.
How hard would it be to do that? The inconsistency between this and Kube PVC is kind of weird.
There was a problem hiding this comment.
I'll explore cleaning this up similar to Kubernetes.
I didn't add the same logic to identify an existing container because
- Docker scheduling is a lot faster, so it's not much overhead compared to starting a new process
- Docker doesn't have anything like ReadWriteOnce, so we can always get read/write access
Standardize and share more of the setup and teardown. Signed-off-by: Michael Smith <michael.smith@puppet.com>
Volumes now support writing content to files. Resolves puppetlabs-toy-chest#675. Signed-off-by: Michael Smith <michael.smith@puppet.com>
Remove logging that's redundant because the API or FUSE layer already logs the same info. Ensure each call to Exec logs what it's executing. Signed-off-by: Michael Smith <michael.smith@puppet.com>
MikaelSmith
force-pushed
the
volume-write
branch
from
bcccf34 to
86d05a8
Compare
Volumes now support writing content to files. Resolves #675.
Depends on #773 to ensure writes are immediately visible.