[BUILD-372] SHIP-0021: Local Source Upload #934

otaviof · 2021-11-03T11:58:46Z

Changes

Local Source Upload (SHIP-0021)

This PR feature local source upload (SHIP-0021), creating the workflow changes to replace the git clone step, by waiting for the content uploaded by the user. The upload is stored in the same original location, the /workspace/source mount point.

To accomplish this task, we employ the waiter nested application, which holds the POD running while the upload is performed.

In order to trigger the local source upload, the user will need the following BuildRun, for instance:

---
apiVersion: shipwright.io/v1alpha1
kind: BuildRun
spec:
  sources:
    - name: local-copy
      type: LocalCopy
      timeout: 60s

The data upload will be performed by our CLI application.

Remote Artifacts

Given the fact there are two types of BuildSource supported, the Remote Artifacts is now defined by the type attribute. The "HTTP" type is introduced for this reason.

---
apiVersion: shipwright.io/v1alpha1
kind: Build
spec:
  sources:
    - name: remote-artifact
      type: HTTP
      url: https://.../

Testing

To run the controller and simulate the changes locally, you'll need a waiter image in place. For that export WAITER_CONTAINER_IMAGE, as per:

export WAITER_CONTAINER_IMAGE="ghcr.io/otaviof/waiter:latest"
make local

After the changes here are merged, then we can use the regular waiter image.

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

action required: `.spec.sources` contains a new attribute `type`, for Remote Artifacts the `type` is "HTTP". The newly introduced type is `LocalCopy`.

SaschaSchwarze0

Hi @otaviof looks good mostly. Suggesting that you quickly prepare a separate PR for the base image as this needs to be built first.

.github/workflows/base-images.yaml

.ko.yaml

cmd/waiter/main.go

cmd/waiter/waiter.go

pkg/config/config.go

SaschaSchwarze0 · 2022-01-06T16:36:43Z

pkg/reconciler/buildrun/resources/sources.go

-// AmendTaskSpecWithSources adds steps, results and volumes for spec.source and spec.sources
+// isLocalCopy appends all "Sources" in a single slice, and if any entry is typed "LocalCopy" it
+// returns first LocalCopy typed BuildSource found, or nil.
+func isLocalCopy(build *buildv1alpha1.Build, buildRun *buildv1alpha1.BuildRun) *buildv1alpha1.BuildSource {


The name of the function suggests that it returns a boolean. I suggest to chose a name like findLocalCopyBuildSource.

Indeed, a more complete name makes more sense here. I adopted your suggestion, with a minor modification. Please consider.

pkg/reconciler/buildrun/resources/sources/local_copy.go

SaschaSchwarze0 · 2022-01-06T16:49:21Z

test/e2e/e2e_local_source_upload_test.go

+	// making sure the taskrun is schedule and becomes a pod, since the build-controller will transit
+	// the object status from empty to unknown, when the actual build starts
+	Eventually(func() *corev1.ConditionStatus {
+		status := getBuildRunStatus(buildRunName)
+		Logf("BuildRun '%s' status '%v'...", buildRunName, status)
+		return status
+	}, time.Duration(60*time.Second), 3*time.Second).
+		Should(Equal(&unknownCondition), "BuildRun should reach unknown condition")
+
+	// asserting the waiter step will end up in timeout, in other words, it ends as failed
+	Eventually(func() *corev1.ConditionStatus {
+		status := getBuildRunStatus(buildRunName)
+		Expect(status).ToNot(BeNil())
+		Logf("BuildRun '%s' status '%s'...", buildRunName, *status)
+		return status
+	}, time.Duration(90*time.Second), 10*time.Second).
+		Should(Equal(&falseCondition), "BuildRun should end up in timeout")


I suggest to wait for the Reason value (Running in the first block, Failed in the second one) and assert the status. Basically it will be unknown already when the BuildRun is Pending and it can theoretically stay there for a while (waiting for the pod to be scheduled and its images pulled).

Please consider latest changes, I'm using your suggestion and checking the actual "Reason", inside the Condition slice.

I wonder if we should also extend the first block timeout to account for the image pulling time as well.

Right, especially with a lower bandwidth, it might not be enough. For this purpose, there is also the timeout multiplyer that you could add here as well. Like here:

build/test/e2e/validators_test.go

Line 140 in 973ef83

}, time.Duration(1100*getTimeoutMultiplier())*time.Second, 5*time.Second).Should(Equal(trueCondition), "BuildRun did not succeed")

Thanks for the snippet, it's in place now.

.gitignore

SaschaSchwarze0

Stumbled over one more reference to quay.io.

pkg/config/config.go

gabemontero

reviewing your PR on a per commit basis @otaviof (since I'm multi tasking a bit here this afternoon) ... if anything, I wanted to show I had at least started on this

only minor comments for commit 377dfb0

will take on the bigger dd72409 in a subsequent iteration

thanks

pkg/reconciler/buildrun/resources/sources.go

gabemontero · 2022-01-13T19:12:27Z

pkg/apis/build/v1alpha1/build_source.go

@@ -11,9 +11,12 @@ import (
 // BuildSourceType enumerates build source type names.
 type BuildSourceType string

-// LocalCopy defines a source type that waits for user input, as in a local copy into a POD.
+// LocalCopy source type when the user will upload local content.


not officially asking for a change, but am wondering .... is it truly obvious that "uploading local content" means it is NOT a remote artifact?

Probably the answer is "yes it is obvious", but at least at this precise moment, I have a small percent of uncertainty

But especially when we start adding remote artifact types other than HTTP, perhaps distinguishing that local source is not one of the remote artifacts becomes a bit more important.

I won't push on it further @otaviof but if you are amenable to making it really clear by all means

Probably the answer is "yes it is obvious", but at least at this precise moment, I have a small percent of uncertainty

I think you touch a important subject.

To be honest, at this point, I don't think the local-copy nor the remote-artifacts are depicted well enough. I'm rewording the doc comments for the BuildSource types, trying to contrast "upload" vs. "download". Please consider.

the comments in question are clearer @otaviof

after reading your comment here and in the code, I think I might even go a step further, and

change constants starting with RemoteArtifacts... to DownloadedArtfacts...

change LocalCopy to UploadedArtifacts

WDYT?

gabemontero

ok here is my second pass on the waiter commit @otaviof

a few questions, and perhaps an alternative to your existing waiter implementation that is "simpler" in that it is mainly based off of existing golang utilities (assuming the pid in the file is not required down the line)

cmd/waiter/README.md

cmd/waiter/main.go

cmd/waiter/waiter.go

pkg/apis/build/v1alpha1/build_source.go

cmd/waiter/README.md

rolfedh

Made some minor suggestions. Otherwise, LGTM.

otaviof · 2022-01-18T13:37:04Z

Made some minor suggestions. Otherwise, LGTM.

Thanks, appreciated!

SaschaSchwarze0

Just two more questions regarding the e2e test.

test/e2e/e2e_local_source_upload_test.go

SaschaSchwarze0 · 2022-01-19T16:41:18Z

test/e2e/e2e_local_source_upload_test.go

+	"github.com/shipwright-io/build/test/utils"
+)
+
+var _ = Describe("For a Kubernetes cluster with Tekton and build installed", func() {


It would be cool to have an e2e test which causes the waiter to successfully complete. Not sure if that's doable.

I agree! The complete test workflow will initially happen on the CLI repository (as part of PR #86), since it depends on the client to utilize the local source upload feature end-to-end. As per:

https://github.com/otaviof/cli/blob/5c0cadc4c623920cafc43d1b77dbc481660129ad/test/e2e/upload.bats#L54-L75

Given the approach is consolidated on the CLI side, we can then reuse it here, if we conclude we want it here to.

What do you think about this approach, and, should we create a issue to keep track of it?

Yes, an issue sounds fine.

gabemontero · 2022-01-19T18:55:26Z

Other than a response to my idea of actually changing the names of the structs LocalCopy and RemoteArtifacts (where I don't consider doing the suggestion a blocker), my review comments are resolved @otaviof

I'll

/approve

and let @SaschaSchwarze0 handle LGTM with his remaining comments.

@adambkaplan did you want a pass at this before it merges?

openshift-ci · 2022-01-19T18:55:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [gabemontero]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Co-authored-by: Sascha Schwarze <schwarzs@de.ibm.com> Co-authored-by: Rolfe Dlugy-Hegwer <rolfedh@users.noreply.github.com>

Implementing "type" on the BuildSource instances employed for the Remote Artifacts support. Amending tests.

SaschaSchwarze0

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2021

openshift-ci bot requested review from sbose78 and shahulsonhal November 3, 2021 11:58

openshift-ci bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Nov 3, 2021

otaviof force-pushed the ship-0021-waiter branch from 7c061e3 to b0401c6 Compare November 9, 2021 08:34

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2021

otaviof force-pushed the ship-0021-waiter branch from 5255b56 to 9405356 Compare November 11, 2021 07:27

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2021

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2021

otaviof force-pushed the ship-0021-waiter branch from b3dadd3 to d529d40 Compare December 14, 2021 10:58

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2021

otaviof force-pushed the ship-0021-waiter branch from 3488c85 to 1e1770b Compare December 16, 2021 08:37

otaviof mentioned this pull request Dec 19, 2021

[BUILD-373] SHIP-0021: Local Source Upload shipwright-io/cli#86

Merged

4 tasks

otaviof changed the title ~~[WIP] SHIP-0021: Waiter~~ [WIP] SHIP-0021: Local Source Upload Dec 20, 2021

otaviof changed the title ~~[WIP] SHIP-0021: Local Source Upload~~ [BUILD-372] SHIP-0021: Local Source Upload Dec 21, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2021

SaschaSchwarze0 requested changes Jan 6, 2022

View reviewed changes

otaviof mentioned this pull request Jan 7, 2022

[BUILD-372] SHIP-0021: Waiter's Base Image #977

Merged

4 tasks

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2022

otaviof force-pushed the ship-0021-waiter branch from 0896b8d to ff2c72b Compare January 10, 2022 07:11

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2022

otaviof force-pushed the ship-0021-waiter branch from ff2c72b to 1cc44f1 Compare January 11, 2022 05:45

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2022

SaschaSchwarze0 requested changes Jan 11, 2022

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

otaviof force-pushed the ship-0021-waiter branch 6 times, most recently from 87b03f9 to c7b5042 Compare January 13, 2022 10:28

openshift-ci bot assigned alicerum Jan 13, 2022

gabemontero reviewed Jan 13, 2022

View reviewed changes

otaviof force-pushed the ship-0021-waiter branch from e280160 to 7085f2c Compare January 14, 2022 05:38

gabemontero reviewed Jan 14, 2022

View reviewed changes

cmd/waiter/README.md Outdated Show resolved Hide resolved

cmd/waiter/README.md Outdated Show resolved Hide resolved

cmd/waiter/main.go Show resolved Hide resolved

cmd/waiter/waiter.go Show resolved Hide resolved

pkg/apis/build/v1alpha1/build_source.go Show resolved Hide resolved

otaviof requested a review from gabemontero January 17, 2022 10:49

rolfedh reviewed Jan 18, 2022

View reviewed changes

cmd/waiter/README.md Outdated Show resolved Hide resolved

rolfedh reviewed Jan 18, 2022

View reviewed changes

cmd/waiter/README.md Outdated Show resolved Hide resolved

rolfedh approved these changes Jan 18, 2022

View reviewed changes

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2022

otaviof force-pushed the ship-0021-waiter branch from 10e3335 to 42d4985 Compare January 18, 2022 13:36

otaviof force-pushed the ship-0021-waiter branch from 42d4985 to 7c81013 Compare January 18, 2022 13:41

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2022

otaviof force-pushed the ship-0021-waiter branch from 7c81013 to b97643c Compare January 19, 2022 08:21

SaschaSchwarze0 reviewed Jan 19, 2022

View reviewed changes

gabemontero mentioned this pull request Jan 19, 2022

convert waiter from file monitor to timer/ticker combination #985

Closed

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2022

SaschaSchwarze0 added this to the release-v0.8.0 milestone Jan 19, 2022

otaviof and others added 3 commits January 20, 2022 08:03

Waiter.

5710ebd

Co-authored-by: Sascha Schwarze <schwarzs@de.ibm.com> Co-authored-by: Rolfe Dlugy-Hegwer <rolfedh@users.noreply.github.com>

Remote Artifacts

7283713

Implementing "type" on the BuildSource instances employed for the Remote Artifacts support. Amending tests.

Vendoring in dependencies.

f12fc44

otaviof force-pushed the ship-0021-waiter branch from b97643c to f12fc44 Compare January 20, 2022 07:04

SaschaSchwarze0 approved these changes Jan 20, 2022

View reviewed changes

openshift-ci bot assigned SaschaSchwarze0 Jan 20, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2022

openshift-merge-robot merged commit 9ceb2f0 into shipwright-io:main Jan 20, 2022

SaschaSchwarze0 added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 20, 2022

otaviof deleted the ship-0021-waiter branch January 20, 2022 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUILD-372] SHIP-0021: Local Source Upload #934

[BUILD-372] SHIP-0021: Local Source Upload #934

otaviof commented Nov 3, 2021 •

edited

Loading

SaschaSchwarze0 left a comment

SaschaSchwarze0 Jan 6, 2022

otaviof Jan 12, 2022

SaschaSchwarze0 Jan 6, 2022 •

edited

Loading

otaviof Jan 12, 2022 •

edited

Loading

SaschaSchwarze0 Jan 12, 2022

otaviof Jan 12, 2022

SaschaSchwarze0 left a comment

gabemontero left a comment

gabemontero Jan 13, 2022

otaviof Jan 14, 2022

gabemontero Jan 14, 2022

gabemontero left a comment

rolfedh left a comment

otaviof commented Jan 18, 2022

SaschaSchwarze0 left a comment

SaschaSchwarze0 Jan 19, 2022

otaviof Jan 20, 2022 •

edited

Loading

SaschaSchwarze0 Jan 20, 2022

gabemontero commented Jan 19, 2022

openshift-ci bot commented Jan 19, 2022

SaschaSchwarze0 left a comment

[BUILD-372] SHIP-0021: Local Source Upload #934

[BUILD-372] SHIP-0021: Local Source Upload #934

Conversation

otaviof commented Nov 3, 2021 • edited Loading

Changes

Local Source Upload (SHIP-0021)

Remote Artifacts

Testing

Submitter Checklist

Release Notes

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaSchwarze0 Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

otaviof Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

gabemontero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero left a comment

Choose a reason for hiding this comment

rolfedh left a comment

Choose a reason for hiding this comment

otaviof commented Jan 18, 2022

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otaviof Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero commented Jan 19, 2022

openshift-ci bot commented Jan 19, 2022

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

otaviof commented Nov 3, 2021 •

edited

Loading

SaschaSchwarze0 Jan 6, 2022 •

edited

Loading

otaviof Jan 12, 2022 •

edited

Loading

otaviof Jan 20, 2022 •

edited

Loading