Initial design ideas for Source

[nader-ziada]

Signed-off-by: Shash Reddy shashwathireddy@gmail.com

Design proposal for #11 #12 and #13

Proposed Changes

• Create a PipelineSource and SourceVersion
• remove ArtifactSourceBinding and use SourceBinding as both Input and Output
• Create an interface for Sources
• git and image implementation of Source
• passedConstraint to make sure a task uses the Source Version that passed a previous task

This PR is just to gather feedback about these design ideas.

[imjasonh]

Revision ?

In Build's GitSource today, we decided to accept branches, tags and arbitrary refs (including commits) in the same field, using Git's built-in semantics.

[nader-ziada]

make sense to be consistent

[tejal29]

Few tips:
We probably define interface Source method Type.

// SourceType defines a source type for all source
type SourceType string

var Git SourceType "git"
var Svn SourceType "svn"
type Source interface {
    Type() SourceType
    Version() string 
   // Pulls source code.
    Pull(copyLocation string) error
  // This combines a bunch of fields and then displays the source location
  // e.g. in case of git, it will be github url while in case of svn it will be a svn url etc.
   Location() string
}

And Then the GitSource can define its own fields like you have and implement methods

func (gs GitSource) Version() string {
    return gs.Commit
}

func (gs GitSource) Type() SourceType {
  return Git
} 

[tejal29]

That ways, other sources can be added easily

[imjasonh]

return []Param{}?

These could also be written on single lines to conserve vertical space:

func (GitSource) getParams() []Param { return []Param{} }

[nader-ziada]

It really should also be returning the actual values of any existing Params

[bobcatfish]

remove ArtifactSourceBinding and use SourceBinding as both Input and Output

This is a cool idea, and more like some of the coolness that concourse has - it makes a lot of sense, since the line between the two is so fuzzy anyway e.g. images could be either inputs or outputs.

• @tejal29 who is driving the artifacts and result store portion of the design, this would change the scope a bit.

passedConstraint to make sure a task uses the Source Version that passed a previous task

Interesting!! So this would actually tackle #11 as well.

It seems like this changes the model a bit: instead of passing a Source/image from task to task, you can declare it at the top level and then declare what stages it needs to pass 🤔

I feel like this makes the model a bit more complicated: we now have one more way of expressing the DAG (nextTask, prevTask, paramBindings AND passedConstraints), unless there's a way we can do away with paramBindings completely and replace them with passedConstraints? I'm not completely seeing the advantage of using passedConstraints over using paramBindings.

Would be helpful to see a list of pros and cons of both!

git and image implementation of Source

Do you have any thoughts about what one would do if they wanted to extend this model? I guess it would require rebuilding the controller, but you could extend it with anything that complies with the interface?

As we move forward it would also be great to see what this looks like in a TaskRun (e.g. the kritis task run)

My main questions:

• Would like to be convinced that sourcebindings + passedConstraints are > paramBindings
• Would I have to declare a Source for any image I intend to build and pass between Tasks in my Pipeline? (maybe that's a good thing!)

[bobcatfish]

maybe a bit here about how a SourceVersion would be intended to be used?

[nader-ziada]

SourceVersion is for the taskrun_controller to make sure the same version of the Source (eg: git commit) is used across the same pipeline run, will update the comment

[bobcatfish]

ah cool, so anything could have a version potentially, and for a git source, it happens to be the commit - pretty cool!

[nader-ziada]

yeah, exactly, it will mean different things for different types

[tejal29]

+1
Just a note, lets lock down a Source interface and then GitSource implement the interface.

[bobcatfish]

I think it would maybe be useful to separate the endpoint where the image is tored from the URL - we might want to have an endpoint, tag, and sha all available somehow

[shashwathi]

Thats would be the metadata for image source. We could consider adding metadata for source's interface to provide this information.

[tejal29]

How about we use the subset of fields Redhat Atomic Host Signature. https://github.com/aweiteka/image/blob/e5a20d98fe698732df2b142846d007b45873627f/docs/signature.md

 "identity": {
            "docker-reference": "busybox"
        },
        "image": {
            "docker-manifest-digest": "sha256:a59906e33509d14c036c8678d687bd4eec81ed7c4b8ce907b888c607f6a1e0e6"
        }, 

Some Missing fields are "Registry"

[shashwathi]

docker-reference could be combination of registry:docker-image-name as well.

[bobcatfish]

in the current design, this would make sense to put into the PipelineParams, i.e. replacing these sources in the example

[nader-ziada]

I wanted to make them outside of the tasks so it can be used across multiple tasks, but it would work either outside or in the PipelineParams

[bobcatfish]

I think we'd definitely want this in PipelineParams, unless we decide in the long run to do away with PipelineParams

[bobcatfish]

So is SourceRef the interface that Sources must comply with? Would a SourceRef be expected to have the functions that the git + image types above have? e.g. getVersion?

[nader-ziada]

i think so :)

[nader-ziada]

This is a cool idea, and more like some of the coolness that concourse has - it makes a lot of sense, > since the line between the two is so fuzzy anyway e.g. images could be either inputs or outputs.

yes images can be either inputs and outputs, maybe the name Source is not the best with it representing inputs and outputs. This would it make it easier to pass an image from the build to the test task.

Interesting!! So this would actually tackle #11 as well.

Like you said this would address #11 as well to make it easier for the code to track which Source is moving along between tasks in a PipelineRun

It seems like this changes the model a bit: instead of passing a Source/image from task to task, you > can declare it at the top level and then declare what stages it needs to pass 🤔

You would have to declare any image (Source) at the top level and tasks can use it as input or output, each image would have a different Source top level item. The passed is to enforce the image is not latest from that source repo (or registry) but the one that got built

I feel like this makes the model a bit more complicated: we now have one more way of expressing the > DAG (nextTask, prevTask, paramBindings AND passedConstraints), unless there's a way we can do
away with paramBindings completely and replace them with passedConstraints? I'm not completely > seeing the advantage of using passedConstraints over using paramBindings.

you are correct, we might not need the ParamBinding, the reason I added the passedConstraint is that it makes the dependency of artifacts more explicit, didn't know how ParamBindings can do that?

Do you have any thoughts about what one would do if they wanted to extend this model? I guess it
would require rebuilding the controller, but you could extend it with anything that complies with the
interface?

Not sure if we have to add something, but I though anything that implements the interface should work, I can try it out

Would like to be convinced that sourcebindings + passedConstraints are > paramBindings

will work on some more examples of the pipeline yaml to demonstrate it more

Would I have to declare a Source for any image I intend to build and pass between Tasks in my Pipeline? (maybe that's a good thing!)

yes, each image needs a Source

This PR is just to start the conversation about the Source part of the pipeline, and I agree it increases the scope a bit, but I think it provides a lot of flexibility and value going forward, its similar to the resource concept in Concourse which I have used a lot and appreciate the benefits of.

We are open to any modifications we come up while working on this together

[shashwathi]

I feel like this makes the model a bit more complicated: we now have one more way of expressing the > DAG (nextTask, prevTask, paramBindings AND passedConstraints), unless there's a way we can do
away with paramBindings completely and replace them with passedConstraints? I'm not completely > seeing the advantage of using passedConstraints over using paramBindings.

Adding to @pivotal-nader-ziada's point about the advantage of passedConstraints. I think it also will describes granular dependency across sources for more accurate DAG than using prevTask, nextTask which provide generic task dependency.

For example

apiVersion: pipeline.knative.dev/v1beta1
kind: Pipeline
metadata:
  name: passed-pipeline
  namespace: default
spec:
    tasks:
        - name: unit-test-kritis          # 1.  Run unit Tests
          taskRef:
              name: make
          inputSourceBindings:
              - inputName: workspace
                sourceKey: kritis
         inputSourceBindings:
              - inputName: workspace
                sourceKey: kritis-test-script
          params:
              - name: makeTarget
                value: test
        - name: integration-tests-kritis               # 2. Run integration Tests
          taskRef:
              name: build-push
          inputSourceBindings:
              - name: workspace
                sourceKey: kritis
          passedConstraint: [unit-test-kritis]
          inputSourceBindings:
              - inputName: workspace
                sourceKey: kritis-test-script
          params:
              - name: makeTarget
                value: integration-test
    sources:
      - name: kritis
        type: git
        params:
          - name: url
            value: https://github.com/grafeas/kritis
          - name: branch
            value: master
    - name: kritis-test-script
        type: git
        params:
          - name: url
            value: https://github.com/grafeas/kritis-test-scripts
          - name: branch
            value: master

It provides information that task integration-tests-kritis.source(kritis) is dependent on previous task but integration-tests-kritis is okay to pull latest commit on kritis-test-script. This is a common use case as a release author, I would like same version of application code passing through the pipeline but not the test code. This makes the dependency of sources across tasks very explicit. Another advantage is the fan-in and fan-out model is supported only with passedConstraints. I will add to the above example to demonstrate

apiVersion: pipeline.knative.dev/v1beta1
kind: Pipeline
metadata:
  name: passed-pipeline
  namespace: default
spec:
    tasks:
        - name: unit-test-kritis          # 1.  Run unit Tests
          taskRef:
              name: make
          inputSourceBindings:
              - inputName: workspace
                sourceKey: kritis
         inputSourceBindings:
              - inputName: workspace
                sourceKey: kritis-test-script
          params:
              - name: makeTarget
                value: test
        - name: integration-tests-kritis               # 2. Run integration Tests
           taskRef:
              name: build-push
           inputSourceBindings:
              - name: workspace
                sourceKey: kritis
           passedConstraint: [unit-test-kritis]            # fan-out after unit testing
           inputSourceBindings:
              - inputName: workspace
                sourceKey: kritis-test-script
           params:
              - name: makeTarget
                value: integration-test
        - name: stress-tests-kritis               # 2. Run stress Tests
           taskRef:
              name: build-push
           inputSourceBindings:
              - name: workspace
                sourceKey: kritis
           passedConstraint: [unit-test-kritis]            # fan-out after unit testing
           inputSourceBindings:
              - inputName: workspace
                sourceKey: kritis-test-script
           params:
              - name: makeTarget
                value: stress-test
           taskRef:       
              name: build-push           # 3. Build and push
           inputSourceBindings:
              - name: workspace
                sourceKey: kritis
           passedConstraint: [stress-test-kritis, integration-test-kritis]  # fan-in after all testing
           outputSourceBindings:
              - name: registry
                sourceKey: stagingRegistry
           params:
              - name: pathToDockerfile
                value: deploy/Dockerfile
    sources:
    ....

WDYT? @pivotal-nader-ziada @bobcatfish @imjasonh

[tejal29]

Ah i see you have defined Source interface here.

[tejal29]

Can you add more information on this method?
getPassingConstraints()

[nader-ziada]

good catch, that should not be here. We first thought about putting the constraint about a Source being passed from one task to another here, but moved it to SourceBinding. Missed removing it from here. Where do you think is a better place for that?

[tejal29]

I feel, getPassingConstrainsts should be defined at the interface level.
The reason being, for each source type, they are going to be constant. You can multiple sources of same source type defined in your cluster. You do not want to repeat the binding information for each sourcebinding.
Does that make sense?

[bobcatfish]

Thanks for putting in those examples @shashwathi , those really help make the ideas more clear!

This is a common use case as a release author, I would like same version of application code passing through the pipeline but not the test code.

I don't quite agree with this b/c I think the test code should be versioned along with the code it is testing and it would be very confusing to have a different commit's test code running.

The simplest would be to use the same version of the test code as the application code, and so my feeling is that by default a source should be used at the same version all the way through the pipeline.

This makes the dependency of sources across tasks very explicit.

I agree! It's interesting that you have to declare all your sources up front as well, which I think provides more clarity, e.g. in the previous kritis example halfway through the pipeline there's suddenly this image being passed around, but if you just glance at the pipeline you have no idea it's producing image. I think your suggestion is a definite improvement here!

Another advantage is the fan-in and fan-out model is supported only with passedConstraints.

Interesting! So it seems like we could potentially replace nextTask, prevTask AND paramBindings with passedConstraints!

Would it be possible to use passedConstraints even if a source is not involved?

[nader-ziada]

we will work on updating the PR with all the comments and adding a README with examples
@shashwathi

[bobcatfish]

Sounds great, thanks @pivotal-nader-ziada !!

I don't quite agree with this b/c I think the test code should be versioned along with the code it is testing and it would be very confusing to have a different commit's test code running.

Quick update that @shashwathi brought to light some really important usecases here, e.g. in a super long running pipeline you need to make a change to a repo, e.g. a test, and don't want to have to re-run the whole thing.

[shashwathi]

Would it be possible to use passedConstraints even if a source is not involved?

What would be the use case for this?

[bobcatfish]

Would it be possible to use passedConstraints even if a source is not involved?
What would be the use case for this?

It's hard to think of a good one, but here's the best that I've got: maybe you want to send a slack notification (via a Task) after a Task has run, e.g. after the integration tests pass, you send the message?

On the other hand, from today's discussion it sounds like we probably want to create some kind of notification specific design 🤔 so maybe this is a bad example.

[bobcatfish]

@shashwathi @pivotal-nader-ziada after thinking a bit more about our discussion on Friday, I've created a couple follow up issues:

#49 - Designing notifications
#50 - Partial pipeline execution

After thinking more about the usecase where user wants to update a source needed by a long running Pipeline as the Pipeline is executing, I am thinking:

• Instead of pulling from HEAD, what if we implement Design: Partial Pipeline execution #50 and allow the user to stop the Pipeline between Tasks and resume execution explicitly from a new commit?
• How often is a long running Pipeline broken up into many tasks? When I try to imagine use cases where a Pipeline is running for e.g. 15 hours, I can only think of cases where the long running portion is in one Task, e.g. a load test that runs over the weekend. In this case, being able to update HEAD of a repo 10 hours into the run wouldn't help, b/c the Task would have already started executing from a previous HEAD

[nader-ziada]

We (myself and @shashwathi) are in sync with the idea @imjasonh explained in our discussion that a PipelineRun is the minimum unit we can run and that we are not expecting long running pipelines, or trying to avoid that at least. In that case, the Partial pipeline execution might be a lower priority for now.
We like the notifications idea

[bobcatfish]

PipelineRun is the minimum unit we can run and that we are not expecting long running pipelines, or trying to avoid that at least. In that case, the Partial pipeline execution might be a lower priority for now.

That's reasonable @pivotal-nader-ziada - I'm fine with assuming that we'll avoid longer running pipelines for now, and revisiting this later.

What do you think @imjasonh ? I'm fine with resolving #50 as won't fix for now, we could always bring it back later if we need it.

[imjasonh]

If nobody needs #50 now then I'm okay with thinking about it more later, but I still think we'll need it eventually. 👍

[tejal29]

Overall i think this LGTM,
The only concern i have is with passingConstraint should be a source implementation detail and should be hidden to users in config.
To make the config easier, we can also drop inputSourceBindings and outputSourceBindings and just havesourceBindings. We should be able to use @aaron-prindle templating work to identify the names.
e.g.

      - name: push-kritis          
        taskRef:
            name: build-push
        sourceBindings:
            - name: $inputs.sources.workspace
              sourceKey: kritis
            - name: $outputs.artifacts.registry
              storeKey: stagingRegistry
              builtImage: kritis

[tejal29]

Sorry @shashwathi jumping on this late.

Adding to @pivotal-nader-ziada's point about the advantage of passedConstraints. I think it also will describes granular dependency across sources for more accurate DAG than using prevTask, nextTask which provide generic task dependency.

Maybe we all (at least i am not :) ) are not on the same page what passedConstraint means.
I was thinking passedConstraint as a way for sources to pass on to the next task "the version of code i just built". The Pipeline Controller can then pass this version to next task in the pipeline.
I do not see this as a way to provide task dependency. @pivotal-nader-ziada what were you thinking initially?

I feel passedConstraint should be eliminated from config and it should be an Source interface method which the controller will use internally.

[bobcatfish]

Maybe we all (at least i am not :) ) are not on the same page what passedConstraint means.
I was thinking passedConstraint as a way for sources to pass on to the next task "the version of code i just built".

@pivotal-nader-ziada and @shashwathi can go into more detail but I understood passedConstraints to do two things:

1. Ensure that the "version" of a Source/Resource used for a Task is the same as the version used for the Tasks in passedConstraints
2. Express the DAG, i.e. if task B needs a Source/Resource that specifies passedConstraints: [task A] then task A would need to run before task B

Re. (1) my preference is that we should always use the same version of the Source/Resource regardless, but I think that (2) is cleaner than what we had before, and @shashwathi 's exmaple in #39 (comment) shows how it can be used for both fan in and fan out

To make the config easier, we can also drop inputSourceBindings and outputSourceBindings and just have sourceBindings

Interesting idea!

At the moment, inputSourceBindings would specify data that needs to be mounted into containers run by the Task, and outputSourceBindings would declare outputs that the Task is expected to produce (more discussion in Knative Build Artifacts Doc) (plus both would be available for templating). I'm not sure how we would make those work with just sourceBindings?

[nader-ziada]

@tejal29 @bobcatfish

We are working on applying all the comments and adding examples that will make things more clear, coming soon (today)

passedConstraints to do two things:

Ensure that the "version" of a Source/Resource used for a Task is the same as the version used for > the Tasks in passedConstraints
Express the DAG, i.e. if task B needs a Source/Resource that specifies passedConstraints: [task A] > then task A would need to run before task B
Re. (1) my preference is that we should always use the same version of the Source/Resource > regardless, but I think that (2) is cleaner than what we had before, and @shashwathi 's exmaple in > #39 (comment) shows how it can be used for both fan in and fan out

yes, exactly, passed constraint would replace nextTask and prevTask and be used to drive the fan in and out as @bobcatfish explained above

I think the intputSourceBinding and outputSourceBinding just make the yaml more explicit and easier to read

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

[nader-ziada]

/check-cla

[tejal29]

yes, exactly, passed constraint would replace nextTask and prevTask and be used to drive the fan in and out as @bobcatfish explained above

In favor of replacing nextTask, prevTask by one keyword. I was playing with srewdriver.cd other day and i was going to propose requires as a construct instead.
Link: https://docs.screwdriver.cd/user-guide/configuration/workflow#defining-workflow-order

Just note, its implicit that we pass on the versions and sha of source built by a upstream task to downstream task. We can start the pipeline at any stage, but from there on, the next tasks use the source code at version built by prev task.

I think the intputSourceBinding and outputSourceBinding just make the yaml more explicit and easier to read.

Works!

[nader-ziada]

/hold cancel

[nader-ziada]

@bobcatfish

• we fixed the cla by force pushing the branch with just one author
• split out the resource
• changed the task input and output

everything looks good from our side, can you take a look if we missed anything and approve again since we had a new commit

we can add follow up issues for more changes

Thanks

[bobcatfish]

cool, i like that this is separate!

[bobcatfish]

i think that this reference belongs in the PipelineRun, since that would be the place where you'd bring everything together (otherwise everytime you use this Pipeline, you have to use the same resources). but we can iterate on this!

[bobcatfish]

Looking good! I'm going to lgtm this so we can go ahead and merge it :D

/lgtm

Thanks for fixing up the author issues @pivotal-nader-ziada , sorry about that :( now it looks like @shashwathi isn't officially an author of any commits :'(

How much does this interfere with your workflow? I want to follow up about this internally. Is there any chance you'd consider making use of something like github's co-author feature?

[bobcatfish]

Oh I'm also going to hit "update branch"

[bobcatfish]

/lgtm

NOTHING REALLY CHANGED @knative-prow-robot CMON

[shashwathi]

How much does this interfere with your workflow? I want to follow up about this internally. Is there any chance you'd consider making use of something like github's co-author feature?

Yeah we can try github co-author next time.

[willnorris]

I know we added support for GitHub co-author to our CLA checker, but I actually think it'll run into the same problem... it'll still complain about needing consent of the secondary author. We can handle this case of different author and committer (or even co-author) when the two individuals are covered by the same CLA. In fact, I'm pretty sure it was for Pivotal specifically that we added this feature.

I'll follow up privately to get some more information and try to debug what happened here. In this case in particular, there might be an easy fix. However, this is a use case we need to handle in the general case as well, but that's going to take a bit more time.

[jglick]

This repository does not seem to exist. Was it subsequently deleted? Or not publicly readable?

[bobcatfish]

funny you should mention this, i just noticed it myself! in #217 it's being updated to use a repo that exists - I think this was supposed to be an example of a case where someone has decided to put their tests in a separate repo (not something I'd recommend!)