Skip to content
This repository has been archived by the owner on Aug 12, 2024. It is now read-only.

manifests location for a plain+v0 bundle #653

Closed
fgiloux opened this issue Jul 7, 2023 · 13 comments
Closed

manifests location for a plain+v0 bundle #653

fgiloux opened this issue Jul 7, 2023 · 13 comments

Comments

@fgiloux
Copy link
Contributor

fgiloux commented Jul 7, 2023

According to the API

  • For an OCI image the manifests must be located at the root-level, i.e. /manifests directory for the provisioner to be able to locate the plain+v0 bundle.
  • For a Git repo there is a field directory to specify the location.
  • For a ConfigMap there is a field pathto specify the location.

What is the rational for the differences? It seems that the UX may improve by having the location set in a consistent way across the three options, i.e:

  • using the same name for the field, path seems the more natural choice
  • supporting the specification of the location for OCI images
@joelanford
Copy link
Member

For an OCI image, the idea was that the image should only ever contain the bundle. We didn't want to encourage people to pack a whole bunch of extraneous stuff in an image because we have to ship that image around and unpack it to extract the bundle.

For a git repo, yes, directory lets you specify a subdirectory of the git root to extract. It looks like the code clones the repo and then returns a bundle FS rooted at the specified directory. Not sure if there's a way to avoid needing to pull the entire commit if we know we don't need a specific subdirectory. The idea here is that someone could point rukpak at a directory: bundle in a standard SDK project.

The path in configmap is a path to a specific directory within the bundle. In the configmaps case, all of the specified paths are rooted at the bundle root.

I think these are the correct choices, but at a minimum it seems like we should beef up the docs to explain things more. FWIW, I created #645 because we don't have any docs right now for the configmaps source.

@fgiloux
Copy link
Contributor Author

fgiloux commented Jul 7, 2023

For an OCI image, the idea was that the image should only ever contain the bundle. We didn't want to encourage people to pack a whole bunch of extraneous stuff in an image because we have to ship that image around and unpack it to extract the bundle.

This may prevent reusing images. It is not that people would pack a whole bunch of random things just because they can. I don't see it as a big issue but I don't see why your argument is valid for an OCI image and not for a ConfigMap. You would still "ship that around" and mount it.

For a git repo, yes, directory lets you specify a subdirectory of the git root to extract. It

My point was why to call it directory for git and path for ConfigMaps? At the end of the day they are both pointing to a location on a filesystem.

@joelanford
Copy link
Member

My point was why to call it directory for git and path for ConfigMaps?

Maybe the name can converge, but they do have different meanings, so converging them might actually increase confusion.

  • git directory is the directory within the repo root that should be treated as the bundle root.
  • configmaps path is the directory within the bundle root where the configmap's data should be mounted as files.

@fgiloux
Copy link
Contributor Author

fgiloux commented Jul 7, 2023

This is indeed confusing.

What benefits do you expect from mounting multiple ConfigMaps? It seems to go in the opposite direction of immutability.
Why does the mount point need to be exposed to users? Isn't it implementation internals of the plain provisioner? Especially if you don't want the bundle file structure to be configurable.
In the pod spec it is named mountPath.

ReadOnly bool `json:"readOnly,omitempty" protobuf:"varint,2,opt,name=readOnly"`
// Path within the container at which the volume should be mounted.  Must
// not contain ':'.
MountPath string `json:"mountPath" protobuf:"bytes,3,opt,name=mountPath"`

Nevertheless a directory is not a path. Does it mean that for git you would accept /bundle but not /config/bundle?

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

This issue has become stale because it has been open 60 days with no activity. The maintainers of this repo will remove this label during issue triage or it will be removed automatically after an update. Adding the lifecycle/frozen label will cause this issue to ignore lifecycle events.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 6, 2023
@fgiloux
Copy link
Contributor Author

fgiloux commented Sep 6, 2023

still relevant

@joelanford
Copy link
Member

What benefits do you expect from mounting multiple ConfigMaps?

To overcome the etcd size limit of a single configmap.

Why does the mount point need to be exposed to users?

Different bundle formats have different requirements for where files need to exist relative to the bundle root. The ConfigMaps source is not specific to the plain provisioner.

@joelanford
Copy link
Member

@joelanford
Copy link
Member

It seems to go in the opposite direction of immutability.

Right now, for better or worse, there are validating webhooks in rukpak that ensure referenced config maps are immutable and cannot be deleted

@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 7, 2023
@fgiloux
Copy link
Contributor Author

fgiloux commented Sep 7, 2023

Different bundle formats have different requirements for where files need to exist relative to the bundle root. The ConfigMaps source is not specific to the plain provisioner.

Sorry I am not following why it needs to be exposed to users. I understand that for builds it matters as the Dockerfile, batch or whatever is used for the build and provided by the user needs to access the data.
For RukPak my understanding is that it is only internal code to the RukPak provisioners that accesses the content of the configmaps and applies it to Kubernetes. If different bundle formats have different requirements for where files need to exist relative to the bundle root can't the specific provisioners take care of it?

@ncdc
Copy link
Member

ncdc commented Sep 7, 2023

I would like to see us have both sensible defaults for where we expect files to live inside a source, as well as allowing the user to change things if the defaults don't work for them.

I imagine we'll want to have a knob to configure the path in a source to consider its root; this is the "from".

We'll also want to have a knob to configure the path in the local staging area where the source's files go; this is the "to".

This is what we're working toward in #703

@ncdc
Copy link
Member

ncdc commented Sep 7, 2023

If #703 is moving us in the right direction, please feel free to close this and move conversation there. If you don't think it is, let's chat! Thanks. 😄

@fgiloux
Copy link
Contributor Author

fgiloux commented Sep 7, 2023

@ncdc #703 is surely moving us in the right direction. I am happy to close this and continue the discussion there.

@fgiloux fgiloux closed this as completed Sep 7, 2023
@fgiloux fgiloux mentioned this issue Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants