-
Notifications
You must be signed in to change notification settings - Fork 67
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
Ability to duplicate a manifest to another location #95
Comments
Looks like a decent proposal:
|
Ack to your first two bullets. Not sure I quite follow the third. By "I don't I think perhaps you are saying both, since Hrm, might we want different drivers for source (implied in the context) and destination (would need to be passed?) Perhaps destination should also be a(n empty) |
"I don't think Resource needs a Copyable interface"
I'm not sure how tightly we should integrate with drivers. Perhaps, only as much is needed to avoid repeating logic in getting resources from a given environment. I guess that would mean there is a version that takes paths and instantiates the environments (contexts) from paths and another that works solely from environments. Does that make sense? |
I take it your use of "environment" assume #78 has happened and will try to interpret with that in mind and use that term throughout. So I think you are proposing two functions. First a
That would build an
Or should this be:
In either case this will then iterate over the My original proposal had those
Which makes it impossible to integrate with drivers at all. Maybe this is signposting me towards the idea that the duplication should be happening in To do this you would need to be able to "readish" operations on An alternative model just occurred to me:
i.e. take the |
@ijc Yes, I am using "environment" because "context" is confusing in the context of the existence of the context package. That proposal looks good. As far as driver access goes, I think it is reasonable to expose something on environment to allow implementations to access platforms directly for performance optimizations. One other thing to consider is the behavior: do this "sync" (match the set) or "copy" (destination is now a superset of both src and dst). Are they destructive? |
There was at least 3 in #95 (comment) ;-) Do you have a favourite?
In the name of simplicity (and because I'm in inherently lazy) I'd prefer to mandate that the destination not exist at all upon entry for now. If we come up with a need to "overlay" src onto dst that should be doable in a backwards compatible way I think. |
There all my favo[u]rite! I guess I was saying the general direction looks good. I think the focus should be on the top-level, path-based function I think the main implementation would be something like this:
Basically, its a helper for doing the easy thing and then you can use the other parts when something special is required. However, I do see the benefit of making the "environment" one-sided, in that you sync to a local directory, but this may have limitations if you are syncing two environments that are only accessible from their drivers. Stepping back, I may have incorrectly rejected one of the original ideas to have this on the manifest, because something like this makes a lot of sense:
The above would let you apply the manifest to an environment, retrieving the content from the given provider, which might let you get direct access to fds for using things like @dmcgowan Any thoughts here? |
I'm unclear if hashing is a necessary part of the hardlink stuff. The So it's not the I think we certainly want to DTRT with hardlinks in this functionality. |
I finally started looking into implementing this week (before I got distracted again)... It looks like One thing which seems to be missing is timestamps, adding that would require reving the protobuf descriptions I think. Need to think a bit harder about whether that is a requirement for my use case (copy up from underlying image into a volumes for cri-containerd) at first glance I think it probably is... |
I think we need to add timestamsp, regardless. cc @AkihiroSuda I know there was a PR for this. |
Oh sorry for leaving the PR #88 for months.. |
@AkihiroSuda I think we were undecided on #88 but I think it is clearly a necessity now. |
My use case for this was to achieve the same thing which containerd/cri#535 did much more simply. Given that I no longer have a pressing need for this functionality. Probably this issue can be closed, unless you would prefer to keep it around for the design discussion should this need arise again in a different context? |
Background
Following on from containerd/containerd#1482 (comment)
Docker style volumes (those declared with
VOLUME /path
in aDockerfile
) have the property that they are initialised using the contents/path
in underlying image. In order to produce a helper which can allow containerd clients to support such functionality a safe mechanism to duplicate a filesystem trees is required.Safe here means at least:
Since continuity can safely and fully represent a filesystem tree extending it to allow duplicating that tree seems reasonable.
Proposal
I propose adding a single method to the existing
Resource
interface:This method will (given
p := resource.Path()
) duplicatesrcroot/p
todstroot/p
, preserving permissions and ownership. If theResource
implements theXattrer
interface thenxattrs
will also be preserved. Likewise if theResource
implementsHardlinkable
then all hardlinks will be created. (similarly for any other interfaces I've missed or which are added in the future)In normal expected usage
srcroot
would be the root of aContext
. Therefore a new method will also be added to theContext
interfaces:This will
BuildManifest(self)
, sort the result usingByPath
and then iterate callingresource.Copy(self.path, dstroot)
for each resource.Variations
Copying
Xattrer
andHardlinkable
as responsiblity of callerThe support for
Xattrer
andHardlinkable
in each implementation ofResource
seems likely to be quite similar, which likely implies the addition of a helper for each.We could therefore consider excluding handling of those from the
Resource.Copy
method and instead push the responsibility back to the callers (so,Context.Copy
here) using those same helpers.Perhaps:
I lean towards requiring
Resource.Copy
to completely copy all known properties since this seems simpler. The downside is that callers cannot then opt out of copying certain properties.Copyable
interfaceRather than adding a new method to
Resource
we could add a new interface:However since all Resources would have to support the method in order for the overall functionality to be reliably usable there doesn't seem much point.
The text was updated successfully, but these errors were encountered: