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

As a shipwright developer, I want to ensure BuildRuns can support the Build Spec #677

Closed
qu1queee opened this issue Mar 18, 2021 · 30 comments · Fixed by #1016
Closed

As a shipwright developer, I want to ensure BuildRuns can support the Build Spec #677

qu1queee opened this issue Mar 18, 2021 · 30 comments · Fixed by #1016
Milestone

Comments

@qu1queee
Copy link
Contributor

qu1queee commented Mar 18, 2021

Idea:

For workflows on top of Shipwright , I want to allow them to generate a BuildRun with an embedded Build Spec definition.

This will allow workflows, e.g. CLI to have a command that can generate a container image from a single CRD. This will avoid the workflow to orchestrate two resources.

@qu1queee qu1queee added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Mar 18, 2021
@duglin
Copy link

duglin commented Mar 18, 2021

This will also allow for people to override the Build Spec fields on a per BuildRun basis (if they specify both a ref to a Build Spec and an embedded Build Spec). E.g. I want to use "that" build spec, but change just these 3 properties.

@SaschaSchwarze0
Copy link
Member

This will also allow for people to override the Build Spec fields on a per BuildRun basis (if they specify both a ref to a Build Spec and an embedded Build Spec). E.g. I want to use "that" build spec, but change just these 3 properties.

Imo this feature would bring more complexity (to implement, to test, to document) than value. I think enforcing either of them (inline buildSpec or reference to Build) to be defined in a BuildRun is good enough for most scenarios.

The overwriting of certain Build fields in a BuildRun is provided at the moment through explicit fields in the BuildRun (like spec.timeout or spec.output.image).

@imjasonh
Copy link
Contributor

Is this coming from a user request?

This will allow workflows, e.g. CLI to have a command that can generate a container image from a single CRD. This will avoid the workflow to orchestrate two resources.

I believe the proposed benefit of Shipwright to users is that they can set up a build definition once, then stamp out runs of that build (ideally triggered from source changes, #51). I think the value of being able to specify BuildRuns directly without specifying a Build first weakens that.

I'm not totally opposed to being able to run a BuildRun in isolation, but I don't think it's a high priority, especially not while we don't have triggered BuildRuns, which seem much higher value to end users.

Tekton supports TaskRuns with inline taskSpecs, but that was motivated by some user need -- indeed, Shipwright takes advantage of that at its core -- and I think makes sense because the conceptual diff of a TaskRun and its underlying Pod is smaller than the diff between a BuildRun and a TaskRun. You can even specify a PipelineRun entirely inline, but I don't think any sane person does that, or at least they don't stay sane for long. 😄

If there's user pain coming from having to define Builds before creating BuildRuns, I'd love to understand their workflow. Do they also want to be able to inline BuildStrategies 🤢 ? Maybe they just want to create TaskRuns directly.

@qu1queee
Copy link
Contributor Author

yes, this is coming from feedback I got from the teams building a workflow on top of Shipwright, mainly CLI inside IBM. Not feedback from users. Users get an abstraction on this via a workflow.

The argument is, that workflows like a cli require two separate steps for triggering the image build execution, and the orchestration of this is exposed to downtimes, e.g. if the cli call closes abruptly after the Build was created but not the BuildRun

@qu1queee
Copy link
Contributor Author

We already have workflows doing the orchestration of two resources, like the build-load via a single command, and that worked perfectly fine, e.g.:

$ build-load buildruns --cluster-build-strategy=kaniko --source-url=https://github.com/EmilyEmily/docker-simple --output-image-url=docker.io/eeeoo/om-img:ga  --output-secret-ref=secretfoobar --namespace=dev --skip-delete

Priority wise this is not high. I also agree with @SaschaSchwarze0 , I think the aggregated value we get by doing this is minimal, but I wanted to get more feedback on this, therefore this issue.

@duglin
Copy link

duglin commented Mar 18, 2021

The pain comes from it being a multi-step process. While there is definitely the need for "create the build template and then run it over and over w/o having to specify the details each time", there is also the desire to just do a one-off build. Think of it as the user wants to just do a one-time "docker build" and nothing else. Yes they can do that today in multiple steps or by creating their own "workflow" on top of the primitives, but the issue is around how hard we make it for them - so more of a UX issue.

I can't speak to the priority of this but I do know that we have lots of users who will be coming from a "docker build" world w/o any triggers (or the triggers are part of their pre-built ci/cd flows) and while we could produce a product specific solution that hides the complexity of managing multiple CRs and orchestrating them, we'd like interop and portability if possible.

@imjasonh
Copy link
Contributor

I hadn't been aware of the usage of Shipwright as a remote docker build replacement, that certainly changes the scope of the project. I had envisioned it mainly as a simple extensible repo-watcher-image-stamper-outer, where you do some one-time setup (define SAs, Builds, choose a BuildStrategy) then sit back as images get pumped out.

If we do intend to make it a remote one-time build executor as well, then the one-shot BuildRun makes sense. I just don't know how that should be prioritized against the trigger-focused workflow.

@qu1queee
Copy link
Contributor Author

I still struggling to understand the high complexity of dealing with two steps, specially because k8s comes with a lot of functionalities that allow you to understand when something is ready, so that you can move to the next step.

I´m wondering if what @duglin is looking for, can be tackle in a different way.

I think the assumption now is that in order to reach the resource that triggers the execution(BuildRun), you need to execute two steps: first a Build , then a BuildRun. But we could trigger the execution via a Build object creation because of some use cases we have.

We already have an issue for this, see #82. I think the initial idea was that by defining a particular annotation on your Build object, we would ensure that a BuildRun gets generated on the fly, without any other user intervention. This is intended for the use case of triggers(e.g. Tekton triggers support). Also, this seems that will fit for any workflow, where they want to have a single command that can cover a config definition + execution as a single step.

@duglin would that approach fulfill what you are looking for?

@qu1queee
Copy link
Contributor Author

@imjasonh I think the we need some sort of one-shot approach because of #82. But is more likely to be a one-shot Build and not a one-shot BuildRun. #82 is an old one, we might need to revisit the assumptions there, but I do see the value there for the triggers scenario.

@imjasonh
Copy link
Contributor

Re #82 I think its title is a bit confusing. You wouldn't need to involve Tekton Triggers to start a BuildRun right away after a new Build is created, that's just something the controller could do directly ("I see a new Build, I store it, I immediately create a BuildRun that references it"). The issue title says "trigger" but I think we could just say "start" instead to reduce confusion.

I don't think that's a good solution for the case I think @duglin wants though, because you'd end up with a bunch of superfluous (potentially duplicate) Build objects that only existed to start a BuildRun. Something could clean up those Build objects, but then you've just added more orchestration.

@imjasonh
Copy link
Contributor

@imjasonh I think the we need some sort of one-shot approach because of #82. But is more likely to be a one-shot Build and not a one-shot BuildRun. #82 is an old one, we might need to revisit the assumptions there, but I do see the value there for the triggers scenario.

When you say "one-shot Build" do you mean "a Build that exists only to start its initial BuildRun, then is deleted"? That seems more complex than just creating a BuildRun that can execute without referencing a Build. At least in that case the BuildRun spec would store the user's requested steps. A Build object that disappears after you run it once loses useful data about what the user wanted the build to do.

@qu1queee
Copy link
Contributor Author

@imjasonh the issue might be confusing, but the essence of the issue is around

I have a Build, now I want to automatically trigger a single BuildRun because of some event, I dont care how

Isnt the above elegible to cover the scenario @duglin is after?

@imjasonh
Copy link
Contributor

My issue is with the overloading of the term "trigger" -- it really just means "I want a BuildRun to be created right away", which doesn't have to involve Tekton Triggers at all.

@imjasonh
Copy link
Contributor

imjasonh commented Mar 18, 2021

Isnt the above elegible to cover the scenario @duglin is after?

To summarize above points, if @duglin's docker build-esque CLI works by creating Builds that immediately create and execute BuildRuns, either:

  • we end up with a bunch of extra Build objects everywhere, or
  • we delete the Builds and lose useful data about what the BuildRun was expected to do.

@qu1queee
Copy link
Contributor Author

qu1queee commented Mar 18, 2021

When you say "one-shot Build" do you mean "a Build that exists only to start its initial BuildRun, then is deleted"?

I think that if we need a one-shot Build, it will need to trigger the creation of the BuildRun on a CREATE event of the Build. So yes, it basically exists only to starts its initial BuildRun. I do not see further use cases where an existing Build should create a BuildRun automatically. And the only scenario where this have been fitting perfectly is for events on updates of github repositories, so that X generates a Build A and we generate a BuildRun A automatically on the creation of Build A.

When Build A gets generated, we can support specific annotations(for putting an example) that will let us know if we should cleanup resources after the BuildRun runs to completion or if we should keep them around.

EDIT:

When Build A gets generated, we can support specific annotations(for putting an example) that will let us know if we should cleanup resources after the BuildRun runs to completion or if we should keep them around.

Quoting myself. This allow users to decide if they want:

  • a bunch of extra Build objects everywhere
  • lose useful data about what the BuildRun was expected to do

@imjasonh
Copy link
Contributor

imjasonh commented Mar 18, 2021

@qu1queee and I synced offline and I think we've come to an understanding (please correct me if I'm wrong):

Triggering (which is not the goal of this issue, and we shouldn't keep discussing it in this issue) should be accomplished either by having Tekton Triggers :

  • create Builds that are configured to immediately create BuildRuns (@qu1queee 's preference)
  • create BuildRuns that reference Builds that already exist (my preference)

Like I said, triggering is not the goal of this issue, we won't discuss it anymore 😄

For one-shot CLI-driven image builds, the CLI in question should either be able to:

  • create Builds that are configured to immediately create BuildRuns (@qu1queee's preference)
  • create BuildRuns that can have the full source+strategy+image config inlined in buildSpec (my preference, and it sounds like @duglin's preference)

In both of my preferred approaches, Builds are expected to be long-lived stateless objects that tend not to duplicate information among each other, and as such there's no need to configure garbage collection of Builds. BuildRuns should (edit: and do!) store a snapshot of the Build resource at the time they were created since Builds can be modified, and BuildRuns store execution success and outputs.

@qu1queee and I agree that between automatically triggering BuildRuns and one-shot CLI-driven scenarios, we should prioritize triggering BuildRun creation, but we probably do want some way to one-shot a BuildRun as well, to better integrate with other tooling.

@duglin
Copy link

duglin commented Mar 18, 2021

Thanks for chatting and the summary.

At a super high level, as long as it's a single step process then whether that step is "create a build" or "create a buildrun" doesn't matter much to me - my goal is met.

But, not one to avoid stating an opinion :-) yes I do think creating a buldrun makes more sense than creating a build. For a few reasons:

  • I believe that BuildRun should have a full BuildSpec inlined to allow the user to override any aspect of the build definition. Some folks have said that certain fields can be overridden today, and that's great. But why just those? I'm pretty sure that no matter which subset is picked someone will want to override one of the ones excluded. So why stop them?
  • Given that POV, and my belief that Build is really meant to be a "template" for a BuildRun, if the user is going to choose the fast path option and do everyone in one step, it doesn't make a lot of sense to focus on the "template" instead of the "thing that will run" - the BuildRun. Especially, if you can specify everything in the BuildRun - meaning you don't need the template.
  • finally, if the user specifies a Build object as part of the one-step operation, then they need to go and find the BuildRun to find the status of the build. That might feel awkward to create one object but then have to query a different one to see the status.

@qu1queee
Copy link
Contributor Author

I believe that BuildRun should have a full BuildSpec inlined to allow the user to override any aspect of the build definition. Some folks have said that certain fields can be overridden today, and that's great. But why just those? I'm pretty sure that no matter which subset is picked someone will want to override one of the ones excluded. So why stop them?

mainly because the conceptual integrity of a BuildRun was never intended to be the "know-what" to build, but rather the trigger of the execution. This is why we also have been very picky on what can a BuildRun override from a Build. I dont think your idea is bad, it just does not fit with the overall vision/system design of a Build and a BuildRun. @sbose78 can provide his views on this one.

Given that POV, and my belief that Build is really meant to be a "template" for a BuildRun, if the user is going to choose the fast path option and do everyone in one step, it doesn't make a lot of sense to focus on the "template" instead of the "thing that will run" - the BuildRun. Especially, if you can specify everything in the BuildRun - meaning you don't need the template.

Sure, that make sense. And I think this is why this is @imjasonh preferred option, and Im convinced. However, at the moment we do not have a strong use case to do this, besides the one you are bringing in this issue.

@imjasonh
Copy link
Contributor

mainly because the conceptual integrity of a BuildRun was never intended to be the "know-what" to build, but rather the trigger of the execution. This is why we also have been very picky on what can a BuildRun override from a Build.

I noticed that BuildRun can override the Build's output image (but seemingly only the URL, and not the credentials?), I'd be curious to understand why only that field is overrideable, and not source or strategy.

If/when we do decide to allow BuildRuns to specify more/all fields from Build that it needs to execute, we should decide whether that looks like:

  1. specifying a BuildSpec in BuildRunSpec, or
  2. just specifying the necessary fields (Source, Strategy, Output (Params?))

If (1), we need to deprecate BuildRun .spec.output. If (2), a BuildRun starts to look a lot like a Build, which might be confusing to users.

Either way, agree this is relatively lower priority than many near-term feature requests (triggers, params, etc.)

@qu1queee
Copy link
Contributor Author

I noticed that BuildRun can override the Build's output image (but seemingly only the URL, and not the credentials?),

output should come with an image ref, and a secretRef.

I'd be curious to understand why only that field is overrideable, and not source or strategy.

I think you want to read #218, see the PR review comments, there is a lot of insights there around the integrity of a Build and how to avoid breaking it by doing overrides.

@imjasonh
Copy link
Contributor

I noticed that BuildRun can override the Build's output image (but seemingly only the URL, and not the credentials?),

output should come with an image ref, and a secretRef.

Yep, but only the ImageURL field is ever referenced (here), never the BuildRun's overriding secretRef. I'm not sure whether that's intentional or an oversight.

(this is offtopic, sorry)

I think you want to read #218, see the PR review comments, there is a lot of insights there around the integrity of a Build and how to avoid breaking it by doing overrides.

Thanks, that's helpful, I'll read up.

@qu1queee
Copy link
Contributor Author

@imjasonh right, I think you found a bug, can you file an issue pls.

@imjasonh
Copy link
Contributor

@imjasonh right, I think you found a bug, can you file an issue pls.

#684

@gabemontero
Copy link
Member

yes, this is coming from feedback I got from the teams building a workflow on top of Shipwright, mainly CLI inside IBM. Not feedback from users. Users get an abstraction on this via a workflow.

The argument is, that workflows like a cli require two separate steps for triggering the image build execution, and the orchestration of this is exposed to downtimes, e.g. if the cli call closes abruptly after the Build was created but not the BuildRun

I'm catching this discussion after it has already progressed a little, but fwiw, OpenShift's internal CI also follows a similar pattern with its use of OpenShift Builds, and the analogous types there to the Shipwright Build types discussed here, wrt skipping steps that a developer would employ for generating builds from the command line via CLI.

@adambkaplan adambkaplan added this to the release-v0.6.0 milestone May 19, 2021
@adambkaplan
Copy link
Member

There's consensus that this feature is worth having. Details should be ironed out via an enhancement proposal.

@adambkaplan
Copy link
Member

Moving this to v0.7.0, as this requires a SHIP.

@adambkaplan
Copy link
Member

@jaideepr97 has volunteered to at least write a proof of concept. This will help drive the conversation for the eventual SHIP.

@sbose78
Copy link
Member

sbose78 commented Nov 4, 2021

Would this ever wander into areas where an admin only allows users (using rbac) to execute builds defined by her (ie, create BuildRuns for pre-defined Builds) for 'security' reasons ?

Enabling the embedded build spec will not allow such policies to be enforced.

@SaschaSchwarze0
Copy link
Member

Would this ever wander into areas where an admin only allows users (using rbac) to execute builds defined by her (ie, create BuildRuns for pre-defined Builds) for 'security' reasons ?

Enabling the embedded build spec will not allow such policies to be enforced.

Interesting aspect. Maybe a candidate for policy enforcement tools. At a minimum, we could have a global feature flag.

@adambkaplan adambkaplan modified the milestones: release-v0.8.0, Backlog Jan 26, 2022
@adambkaplan adambkaplan added ship-required and removed good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Feb 23, 2022
@adambkaplan
Copy link
Member

Per #677 (comment), removing the "good first issue" label and adding the "ship-required" label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants