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

Add EP on enabling local source code support #717

Merged

Conversation

qu1queee
Copy link
Contributor

@qu1queee qu1queee commented Apr 5, 2021

Changes

Fixes #716

Provide the enhancement proposal document around the local source code feature.

A prototype example is documented in here. This should allow you to run a BuildRun using local source code feature via the Shipwright/CLI.

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

Release Notes

Add EP on enabling local source code feature

@qu1queee qu1queee added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 5, 2021
@openshift-ci-robot openshift-ci-robot added the release-note Label for when a PR has specified a release note label Apr 5, 2021
@qu1queee qu1queee force-pushed the qu1queee/ep_local_feature branch 3 times, most recently from e6f7780 to 0d1c2d5 Compare April 5, 2021 23:04
Copy link
Member

@otaviof otaviof left a comment

Choose a reason for hiding this comment

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

I just learnt about Mink approach, interesteting that we can easily bundle up local data, and upload as a layer in a container registry. Defintely something to keep in mind for later use!

However, for the local source-code upload feature, I think we should further explore the same approach that Tekton relies on. At least the following aspects of it:

  • A mechanism that can be secluded inside the cluster, preferably a ephemeral type of location, so right after the build is done the data would be wiped out. Here use cases like intellectual-property limitations would benefit of, plus avoid using the container-registry as a intermediary storage;
  • A drop-in-replacement for PipelineResource, on which we are able to upload local data before the Build-Strategy's build take place;

Some time ago, we had input from @rhuss (issue #97 comment) on how to upload data before a given build starts, I think we should take this in consideration again.

@zhangtbj
Copy link
Contributor

zhangtbj commented Apr 7, 2021

We did some investigations and solution comparison before.

There are some reasons we choose Mink instead of other solutions:

  • If we upload to container registry, we don't need to maintain any upload service in our Shipwright side, which is the easiest way for us.
  • Tekton can support this image way directly, so we don't need too much change in our server side logic
  • Store in the end user container registry can reduce the security compliance problem because we don't store any user source on the Shipwright side
  • For the other upload service solutions, such as kubectl cp, we need to consider the kube api upload performance if there are more local builds, kubectl cp call kube api which is not good; single or central upload service also need to consider the upload type like http or grpc, etc... and consider the performance and upload service maintain.

After lots of comparison, we think container registry (Mink) way is better than others.

@otaviof
Copy link
Member

otaviof commented Apr 7, 2021

We did some investigations and solution comparison before.

Indeed, we have the track record in issue #97.

There are some reasons we choose Mink instead of other solutions:

* If we upload to container registry, we don't need to maintain any upload service in our Shipwright side, which is the easiest way for us.

I think we should also take in consideration that users/companies might not want (or are able to) store their source-code outside of VCS systems (like GitHub).

Additionally, should we expose the container registry to arbitrary user uploads? During the development workflow the users might need to upload their data too often, ending up consuming too much space.

* Tekton can support this image way directly, so we don't need too much change in our server side logic

Tekton can support Kubernetes volumes directly. I think we should explore having users data in a typical Kubernetes volume, so we can work with settings like emptyDir and allowing users to choose in the future.

* Store in the end user container registry can reduce the security compliance problem because we don't store any user source on the Shipwright side

Aligned with my initial comment, certain ecosystems are design to only explose compiled artifacts. In this case we would be exposing the actual source-code as well.

Also, by using a Kubernetes abstraction like volumes, we can use both Shipwright operator control and common elements from Tekton, which gives us more tooling to tackle the use-case.

* For the other upload service solutions, such as `kubectl cp`, we need to consider the kube api upload performance if there are more local builds, `kubectl cp` call kube api which is not good; single or central upload service also need to consider the upload type like http or grpc, etc... and consider the performance and upload service maintain.

I think it's early to claim better performance if we haven't make a cohesive profiling of both designs. As far as I can recall, I haven't seen such data to be able to conclude the registry upload would be simply faster, please correct me if we had such profiling.

Furthermore, there are several flavours of container-registries so the performance (and reliability) can vary wildely.

We should discuss the performance implications, I would like to understand why it's being taken as a decision factor so early in the process.

After lots of comparison, we think container registry (Mink) way is better than others.

Thanks for bringing Mink on the radar. My arguments here are focused on the storage layer proposed.

@zhangtbj
Copy link
Contributor

zhangtbj commented Apr 8, 2021

Yep, I am thinking if we can choose different storages for local source code, like registry or volume.

Aligned with my initial comment, certain ecosystems are design to only explose compiled artifacts. In this case we would be exposing the actual source-code as well.

Yep, we should support different types and we can support step by step.

When I talking about the performance, it is related the Kubernetes system performance, not the build or upload performance. If there are many kube api long connections, which may make the kube system or master slower.

@qu1queee qu1queee force-pushed the qu1queee/ep_local_feature branch 6 times, most recently from 91420d6 to 08969e6 Compare April 20, 2021 18:46
@qu1queee
Copy link
Contributor Author

@otaviof I added you as an approver of this EP, thanks for your feedback. One important comment is that this bundle approach will not constrain us on other mechanism to support local source code. In other words, we can have multiple flavors of what you call "the storage layer", where bundles is only one. If you see value in a secluded mechanism inside the cluster, we can always provide an EP for it. This EP is just about using container images as a way to store source code, in order to make it available later at runtime, on our generated pods.

As far as I know, Tekton does not have a way to support local source code, and this proposal is probably the less invasive one to achieve that, in terms of API changes.

Regarding your concerns on:

Registry storage

Similar to what @imjasonh mentioned, this is definitely a concern but we can have clean-up routines for it. Which can be configurable depending on the user selection.

Tekton Volumes

Similar to what @rhuss mentioned, this might be useful for us in the future. At the moment we do not have any story that involves mounting volumes. This EP does not block us from doing that, so I think its fine to move on, as long as we consider this.

Performance comparisons

As @zhangtbj mentioned there are many ways to achieve this feature, e.g. streaming the source code via a port-forward ( security concerns ), mounting source code in configmaps ( they have size limits ). I´m not saying they are not ideal, but all different options will have pros and cons. This EP proposes to rely on pushing/pulling to a container registry, should we restrict ourselves from this feature because of performance concerns? I dont think so. I think we should be aware on that we have room for optimizing the bundles approach, but we should do this in the implementation and further development cycles.

@qu1queee qu1queee requested a review from zhangtbj April 20, 2021 19:25
@qu1queee qu1queee deleted the qu1queee/ep_local_feature branch May 20, 2021 10:58
@adambkaplan adambkaplan added this to the release-v0.5.0 milestone Jun 10, 2021
@sbose78
Copy link
Member

sbose78 commented Jun 10, 2021

Do we have a GitHub issue tracking the development of this feature?

@qu1queee
Copy link
Contributor Author

qu1queee commented Jul 7, 2021

HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 13, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.

Add `github.com/google/go-containerregistry`.

Add `github.com/schollz/progressbar/v3`.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 14, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.

Add `github.com/google/go-containerregistry`.

Add `github.com/schollz/progressbar/v3`.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 14, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.

Add `github.com/google/go-containerregistry`.

Add `github.com/schollz/progressbar/v3`.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 14, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.

Add `github.com/google/go-containerregistry`.

Add `github.com/schollz/progressbar/v3`.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 14, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.

Add `github.com/google/go-containerregistry`.

Add `github.com/schollz/progressbar/v3`.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 14, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.

Add `github.com/google/go-containerregistry`.

Add `github.com/schollz/progressbar/v3`.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 14, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.

Add `github.com/google/go-containerregistry`.

Add `github.com/schollz/progressbar/v3`.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 14, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.

Add `github.com/google/go-containerregistry`.

Add `github.com/schollz/progressbar/v3`.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 20, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 20, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 20, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 23, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 23, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 30, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Jul 30, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Aug 10, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Sep 1, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Sep 1, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Sep 6, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Sep 6, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Sep 7, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Sep 7, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
HeavyWombat added a commit to HeavyWombat/cli that referenced this pull request Sep 10, 2021
Reference shipwright-io/build#717

Add `bundle` package that contains convenience code to push a local
source code directory as a bundle image to a container registry.

Add new flags for container image and local directory settings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Label for when a PR has specified a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support local source code in Shipwright