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 Task for publishing tekton pipeline images + yaml #632

Merged
merged 4 commits into from
Mar 22, 2019

Conversation

bobcatfish
Copy link
Collaborator

Changes

Add a Task which invokes ko to build and publish all images and yaml
config required for installing Tekton Pipelines.

This Task will:

  • Build and publish the "base image" using Kaniko
  • Generate a .ko.yaml
  • Invoke ko to build/publish images and generate a release.yaml
  • Parse the release.yaml for built images; ensuring that the expected
    images were built (and no more)
  • Tag the built images with the correct version and also tag in all
    regions (us, asia, eu)

This should be the same functionality that could previously be seen in
https://github.com/tektoncd/pipeline/blob/master/hack/release.sh
(which used
https://github.com/knative/test-infra/blob/master/scripts/release.sh).
We can remove release.sh once we have completed #530 as well.

Some functionality has been implemented in a python script, which has
its own tests. Since it is currently difficult to update the pull
request test logic to do additional things (such as run python unit
tests), I'm hoping we are okay with waiting until #532 to add
automatic running of these tests).

Fixes #528
Fixes #529

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide
for more details.

Release Notes

n/a

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 19, 2019
@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 19, 2019
images = []
with open(path) as f:
for line in f:
match = re.search(base + ".*@sha256:[0-9a-f]*", line)

Choose a reason for hiding this comment

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

u shuld use DIGGEST_MARKER here ;)

Copy link
Member

@nader-ziada nader-ziada left a comment

Choose a reason for hiding this comment

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

great PR @bobcatfish

I don't know enough to comment on the python code, otherwise some minor comments 👍

set -e
set -x

cat <<EOF > /workspace/go/src/github.com/tektoncd/pipeline/.ko.yaml
Copy link
Member

Choose a reason for hiding this comment

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

github.com/tektoncd/pipeline/cmd/entrypoint is missing from the ko file created here

also there is a thing called defaultBaseImage but I don't think it will save that much

Copy link
Member

Choose a reason for hiding this comment

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

@bobcatfish what is the default base image if we don't override ?

Copy link
Member

Choose a reason for hiding this comment

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

if you don't specify any base image, ko will use distroless

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

adding a comment about the default!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added entrypoint image :D

- "get"
- "github.com/google/go-containerregistry/cmd/ko"

# TODO(#631): publish a `ko` image (which has golang)
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to have a Dockerfile for an image that has go and ko and just publish that instead of having steps for it, it can even be published in the task or before hand

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah definitely!! I was thinking of leaving that for #631 but you're probably right, it's worth doing something better than this in the meantime XD

cp -R /usr/local/go /workspace/golang/localgo

- name: ensure-release-dirs-exist
image: busybox
Copy link
Member

Choose a reason for hiding this comment

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

when the bucket was empty, I got the error

error exit status 1; cmd_output CommandException: No URLs matched: gs://pipeline-test-123456/*\n","stacktrace":"main.main\n\t/Users/pivotal/go/src/github.com/tektoncd/pipeline/cmd/gsutil/main.go:94\nruntime.main\n\t/usr/local/Cellar/go/1.12/libexec/src/runtime/proc.go:200"

but that might be a bug in the resource itself

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh interesting - so you'd created the bucket but nothing was in it? ill try that out too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hm okay so it seems like if you use the dir flag and the bucket is empty, you get this error:

Error executing command "gsutil cp -r gs://christies-empty-bucket/* /workspace/bucket" ; error exit status 1; cmd_output CommandException: No URLs matched: gs://christies-empty-bucket/*
"   
  stacktrace:  "main.main
	/Users/christiewilson/Code/go/src/github.com/tektoncd/pipeline/cmd/gsutil/main.go:94
runtime.main
	/usr/local/Cellar/go/1.10.3/libexec/src/runtime/proc.go:198"   
  ts:  1553120763.622061   

My feeling is that this is a bug in the resource; I think we should be able to handle checking out an empty bucket that we intend to put content into.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created #646

hack/release.md Outdated Show resolved Hide resolved
@nader-ziada
Copy link
Member

/test pull-tekton-pipeline-integration-tests

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Looking good 💃 Few questions as @pivotal-nader-ziada 👼

hack/release.md Outdated Show resolved Hide resolved

#### Production credentials

TODO(dlorenc, bobcatfish): We need to setup a group which users can be added to, as well as guidelines
Copy link
Member

Choose a reason for hiding this comment

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

💯 😻

tekton/koparse/koparse.py Outdated Show resolved Hide resolved
@bobcatfish
Copy link
Collaborator Author

I0319 00:22:06.449] =============================
I0319 00:22:06.461] ----------------------------------------------
I0319 00:22:06.461] ---- Checking links in the markdown files ----
I0319 00:22:06.462] ----------------------------------------------
I0319 00:22:14.228] hack/release.md
I0319 00:22:14.228] 	ERROR	tekton.README.md
I0319 00:22:14.228] 		Stat hack/tekton.README.md: no such file or directory

lol whaaaaat

@bobcatfish
Copy link
Collaborator Author

I haven't finished responding to all the feedback yet but I did:

  • Switch to Python 3
  • Add a Dockerfile for the ko image (and a Task to build it!)

So those bits are ready for another look :D

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Small comments otherwise looks good 🕺

os.path.abspath(__file__)), "test_release.yaml")
PATH_TO_WRONG_FILE = os.path.join(os.path.dirname(
os.path.abspath(__file__)), "koparse.py")
BUILT_IMAGES = [
Copy link
Member

Choose a reason for hiding this comment

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

We need cmd/entrypoint too 👼

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually the test data (which is the 0.1 release) doesnt have the entrypoint image! 😇

but this reminds me, i forgot to address @pivotal-nader-ziada 's comment and add the entrypoint image to the ko.yaml so I'll do that now 🤦‍♀️ 😅

"gcr.io/knative-releases/github.com/knative/build-pipeline/cmd/webhook@sha256:cca7069a11aaf0d9d214306d456bc40b2e33e5839429bf07c123ad964d495d8a",
]
EXPECTED_IMAGES = [
"gcr.io/knative-releases/github.com/knative/build-pipeline/cmd/kubeconfigwriter",
Copy link
Member

Choose a reason for hiding this comment

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

here too 😅

set -e
set -x

cat <<EOF > /workspace/go/src/github.com/tektoncd/pipeline/.ko.yaml
Copy link
Member

Choose a reason for hiding this comment

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

@bobcatfish what is the default base image if we don't override ?

@nader-ziada
Copy link
Member

@bobcatfish PR #643 update the release script, you may want to include that change in your task

@bobcatfish
Copy link
Collaborator Author

Okay I think I've addressed all comments @pivotal-nader-ziada @vdemeester , ready for another look :D

Add a `Task` which invokes `ko` to build and publish all images and yaml
config required for installing Tekton Pipelines.

This Task will:
* Build and publish the "base image" using Kaniko
* Generate a .ko.yaml
* Invoke ko to build/publish images and generate a release.yaml
* Parse the release.yaml for built images; ensuring that the expected
  images were built (and no more)
* Tag the built images with the correct version and also tag in all
  regions (us, asia, eu)

This should be the same functionality that could previously be seen in
https://github.com/tektoncd/pipeline/blob/master/hack/release.sh
(which used
https://github.com/knative/test-infra/blob/master/scripts/release.sh).
We can remove release.sh once we have completed tektoncd#530 as well.

Some functionality has been implemented in a python script, which has
its own tests. Since it is currently difficult to update the pull
request test logic to do additional things (such as run python unit
tests), I'm hoping we are okay with waiting until tektoncd#532 to add
automatic running of these tests).

Use actual production bucket and registry by default (tektoncd#527)

Fixes tektoncd#528
Fixes tektoncd#529
python 2.7 will no longer be maintained in 2020 (so soon now!!) and also
now we get cool type annotations 😎
Instead of doing a lot of hacks to make sure we have all the tools we
need to auth + invoke ko, let's build an image that has what we need in
advance.

Eventually we should be able to build this image and refer to the built
image in our steps (tektoncd#639) but for now we'll have to hardcode it.

We may also improve this image in tektoncd#631, or decide to move away from `ko`
entirely.
Without `SOURCE_DATE_EPOCH` `ko` will use a hardcoded value of 48 years
ago as the timestamp for a created image. This was done so that repeated
build are reproducible, i.e. if you build the same thing twice, nothing
will change, not even the creation time.

SOURCE_DATE_EPOCH will set the creation timestamp that `ko` will use.

See google/go-containerregistry#1

This was added to `release.sh` (which we are moving away from in favor
of dogfodding via Tasks) in tektoncd#643
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 💃

@bobcatfish
Copy link
Collaborator Author

/test pull-tekton-pipeline-integration-tests

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, vdemeester

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 [bobcatfish,vdemeester]

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

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2019
@bobcatfish
Copy link
Collaborator Author

I0321 17:57:39.418]  detail: u"Deploy error: Not all instances running in IGM after 25.116998368s. Expect 1. Current errors: [ZONE_RESOURCE_POOL_EXHAUSTED_WITH_DETAILS]: Instance 'gke-kpipeline-e2e-cls110-default-pool-7aa6058d-24gs' creation failed: The zone 'projects/tekton-prow-5/zones/us-west1-c' does not have enough resources available to fulfill the request.  '(resource type:compute)'. - ; ."

Well my current strategy of ignoring this error and hoping it goes away doesn't seem to be working XD

bobcatfish added a commit to bobcatfish/pipeline that referenced this pull request Mar 21, 2019
In tektoncd#632 I've been running into
ZONE_RESOURCE_POOL_EXHAUSTED_WITH_DETAILS errors saying `us-west1-c`
doesn't have enough resources for our test clusters. It seems like this
is outside of our control (and even tho we are using a regional
`us-west1` cluster, the setup stubbornly insists on trying to use
`us-west`-c`.

So instead let's try `us-central` for these very scientific reasons:

1. It's the only region with 4 zones at https://cloud.google.com/compute/docs/regions-zones/
2. Knative is using it and seems fine XD
@bobcatfish
Copy link
Collaborator Author

I've manually applied the us-central1 change from #651 so trying this again:

/test pull-tekton-pipeline-integration-tests

tekton-robot pushed a commit that referenced this pull request Mar 21, 2019
In #632 I've been running into
ZONE_RESOURCE_POOL_EXHAUSTED_WITH_DETAILS errors saying `us-west1-c`
doesn't have enough resources for our test clusters. It seems like this
is outside of our control (and even tho we are using a regional
`us-west1` cluster, the setup stubbornly insists on trying to use
`us-west`-c`.

So instead let's try `us-central` for these very scientific reasons:

1. It's the only region with 4 zones at https://cloud.google.com/compute/docs/regions-zones/
2. Knative is using it and seems fine XD
@bobcatfish
Copy link
Collaborator Author

bobcatfish commented Mar 21, 2019

timeout_test.go:222: Error waiting for TaskRun run-giraffe to finish: timed out waiting for the condition

o.o

kind of ironic XD

/test pull-tekton-pipeline-integration-tests

@bobcatfish
Copy link
Collaborator Author

lol so many tests failing with race detected during execution of test 🏎 interesting...

@bobcatfish
Copy link
Collaborator Author

/me flails

/test pull-tekton-pipeline-integration-tests

@bobcatfish
Copy link
Collaborator Author

/test pull-tekton-pipeline-integration-tests

@tekton-robot tekton-robot merged commit ec13b3c into tektoncd:master Mar 22, 2019
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. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants