ci-operator: allow users to depend on images in steps #1044

stevekuznetsov · 2020-07-29T22:17:16Z

Users may not expect to find the pull spec for an image created at some
other point in the pipeline fed to their step via an environment
variable that they specify. The ci-operator will ensure that their step
only runs once the image is available.

Signed-off-by: Steve Kuznetsov [email protected]

stevekuznetsov · 2020-07-29T22:17:34Z

Need more unit & e2e testing but otherwise ready for review @bbguimaraes @petr-muller @droslean

stevekuznetsov · 2020-07-29T22:25:52Z

/retest

stevekuznetsov · 2020-07-31T00:50:21Z

This is updated with all testing, need to write a help page doc next. Code is ready for review.

stevekuznetsov · 2020-07-31T15:06:45Z

 error: error creating buildah builder: Error reading signatures: Get https://image-registry.openshift-image-registry.svc:5000/extensions/v2/ci-op-pz30hhsk/pipeline/signatures/sha256:c08cd73803e35c137ed6506026720b5ee2aff001fe53678c1fb9fb53ff9c3d31: dial tcp: lookup image-registry.openshift-image-registry.svc on 172.30.0.10:53: no such host

and

 error: build error: failed to pull image: Get https://docker-registry.default.svc:5000/v2/ci-op-pz30hhsk/pipeline/manifests/sha256:9d8495f2b58d6746031e7c948f9a366100ea79be1e53d6bbbfc2b264d4cc64e4: EOF

.... amazing

bbguimaraes

The PR looks really good, just some minor comments about the implementation.

I also don't see anything for container tests and steps in the registry. Are we planning to add those later or only support literal steps for now?

pkg/api/config.go

pkg/steps/multi_stage.go

stevekuznetsov · 2020-08-03T21:56:26Z

For my own TODO:

add validation at config load time that the values people put in their dependencies are valid
flesh out the helpdoc

stevekuznetsov · 2020-08-06T19:44:46Z

@bbguimaraes PTAL

stevekuznetsov · 2020-08-06T19:51:54Z

https://server-ci-tools-1044.apps.ci.l2s4.p1.openshiftapps.com/help/ci-operator#image-references

stevekuznetsov · 2020-08-10T15:22:16Z

/retest

pkg/webreg/webreg.go

alvaroaleman · 2020-08-10T21:49:07Z

pkg/webreg/webreg.go

+
+<table>
+  <tr>
+    <th style="white-space: nowrap"><code>ImageStream</code></th>


Having the title as code looks weird IMHO

pkg/webreg/webreg.go

petr-muller

Nothing really substantial, just the HTML in the docs needs a bit of cleanup. The code looks fine.

petr-muller · 2020-08-11T16:02:21Z

pkg/api/config_test.go

+
+func TestReleaseBuildConfiguration_ImageStreamFor(t *testing.T) {
+
+}


haha holy crap what a brain fart

petr-muller · 2020-08-11T16:03:15Z

pkg/api/config_test.go

+
+func TestReleaseBuildConfiguration_DependencyParts(t *testing.T) {
+
+}


Plan to fill these in? If not then they should be removed.

petr-muller · 2020-08-11T16:04:31Z

pkg/api/graph.go

+}
+
+// IsReleasePayloadStream determines if the ImageStream holds
+// release paylaod images.


nit: payload

pkg/webreg/webreg.go

stevekuznetsov · 2020-08-11T21:45:46Z

@alvaroaleman @petr-muller @bbguimaraes responded to comments

bbguimaraes · 2020-08-12T07:43:03Z

/lgtm
/hold
/retest

error: unable to read image registry.svc.ci.openshift.org/ocp/release@sha256:91663ecfdfa7f92440b8ad49a0d57d9487fed68ec4883741a294c6f8956d8411: Get https://registry.svc.ci.openshift.org/v2/ocp/release/manifests/sha256:91663ecfdfa7f92440b8ad49a0d57d9487fed68ec4883741a294c6f8956d8411: Get https://registry.svc.ci.openshift.org/openshift/token?scope=repository%3Aocp%2Frelease%3Apull: dial tcp 35.196.103.194:443: connect: no route to host

Hmm, the api.ci registry has been failing much more often recently…

stevekuznetsov · 2020-08-12T14:34:17Z

/hold cancel

There was a lot of confusion about these constants, even leading to one of them being called a stream, when it wasn't. These are the names of implicit releases, latest and initial, that we create from the tag specification if a user provides it. Signed-off-by: Steve Kuznetsov <[email protected]>

These functions operate on ImageStreams that hold the images that make up a release, they happen to be prefixed with "stable" but that is not a great name for them. Signed-off-by: Steve Kuznetsov <[email protected]>

Users may not expect to find the pull spec for an image created at some other point in the pipeline fed to their step via an environment variable that they specify. The ci-operator will ensure that their step only runs once the image is available. Signed-off-by: Steve Kuznetsov <[email protected]>

We can detect a large swath of errors in dependency declarations during static validation, which is preferrable to waiting for dynamic step graph build failure as it allows us to be specific about what is wrong, why, and to do so at config check-in time and not at run time. Signed-off-by: Steve Kuznetsov <[email protected]>

Signed-off-by: Steve Kuznetsov <[email protected]>

bbguimaraes · 2020-08-12T15:24:40Z

/lgtm

openshift-ci-robot · 2020-08-12T15:24:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bbguimaraes, stevekuznetsov

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [bbguimaraes,stevekuznetsov]

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

Tests generally install one OpenShift release, and then perform some actions to validate that release. Sometimes the actions include updating to a different release, or a series of different releases. With this commit, I'm pivoting to dependency naming that more clearly reflects these goals, instead of "RELEASE_IMAGE_LATEST", which was a reflection of the default release:latest value, and not an expression of the intended semantics. I've shifted to explicitly declaring these image dependencies, taking advantage of openshift/ci-tools@782008c873 (ci-operator: allow users to depend on images in steps, 2020-07-30, openshift/ci-tools#1044), instead of relying on the implicit RELEASE_IMAGE_LATEST and similar. I've dropped explicit dependencies that were not used in a step's associated commands, like RELEASE_IMAGE_LATEST in ipi-install since eb8eb32 (step-registry: add upgrade workflows, 2020-08-25, openshift#11247). I've dropped explicit dependency overrides that set the same value as the default. For example, there was no need to set: OPENSHIFT_UPGRADE_RELEASE_IMAGE: "release:latest" in openshift-upgrade-aws-loki, because release:latest is already the default in all steps that consume OPENSHIFT_UPGRADE_RELEASE_IMAGE. I've dropped the _OVERRIDE suffix from the dependencies, because while we may set that suffix when communicating with the openshift-install command, it's a legitimate knob and not a strange override in each steps' public API.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2020

openshift-ci-robot requested review from alvaroaleman and bbguimaraes July 29, 2020 22:17

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2020

stevekuznetsov force-pushed the skuznets/explicit-image-dependencies branch from 646c5c3 to 3dcc798 Compare July 31, 2020 00:48

bbguimaraes reviewed Aug 3, 2020

View reviewed changes

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

pkg/steps/multi_stage.go Outdated Show resolved Hide resolved

stevekuznetsov force-pushed the skuznets/explicit-image-dependencies branch from 3dcc798 to 89b6e1e Compare August 6, 2020 19:44

stevekuznetsov force-pushed the skuznets/explicit-image-dependencies branch from 89b6e1e to 8ef79b4 Compare August 6, 2020 19:52

stevekuznetsov changed the title ~~[wip] ci-operator: allow users to depend on images in steps~~ ci-operator: allow users to depend on images in steps Aug 7, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2020

stevekuznetsov force-pushed the skuznets/explicit-image-dependencies branch from ef18178 to 32b83a5 Compare August 7, 2020 15:56

alvaroaleman reviewed Aug 10, 2020

View reviewed changes

petr-muller reviewed Aug 11, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2020

openshift-ci-robot assigned bbguimaraes Aug 12, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2020

stevekuznetsov added 2 commits August 12, 2020 08:18

ci-operator: rename release stream functions

b219ef7

These functions operate on ImageStreams that hold the images that make up a release, they happen to be prefixed with "stable" but that is not a great name for them. Signed-off-by: Steve Kuznetsov <[email protected]>

stevekuznetsov added 5 commits August 12, 2020 08:18

help: document how tests should consume images

0e26b1b

Signed-off-by: Steve Kuznetsov <[email protected]>

update test regex matching for dependency SHAs

0547845

Signed-off-by: Steve Kuznetsov <[email protected]>

update helpdoc, add more tests

f7ceb5c

Signed-off-by: Steve Kuznetsov <[email protected]>

stevekuznetsov force-pushed the skuznets/explicit-image-dependencies branch from 8f69eb2 to f7ceb5c Compare August 12, 2020 15:18

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2020

openshift-merge-robot merged commit 3582d06 into openshift:master Aug 12, 2020

wking mentioned this pull request Oct 20, 2020

ci-operator/step-registry: Unify OPENSHIFT_*_RELEASE_IMAGE dependencies openshift/release#12959

Closed


		func TestReleaseBuildConfiguration_ImageStreamFor(t *testing.T) {

		}


		func TestReleaseBuildConfiguration_DependencyParts(t *testing.T) {

		}

ci-operator: allow users to depend on images in steps #1044

ci-operator: allow users to depend on images in steps #1044

Uh oh!

Conversation

stevekuznetsov commented Jul 29, 2020

Uh oh!

stevekuznetsov commented Jul 29, 2020

Uh oh!

stevekuznetsov commented Jul 29, 2020

Uh oh!

stevekuznetsov commented Jul 31, 2020

Uh oh!

stevekuznetsov commented Jul 31, 2020

Uh oh!

bbguimaraes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stevekuznetsov commented Aug 3, 2020

Uh oh!

stevekuznetsov commented Aug 6, 2020

Uh oh!

stevekuznetsov commented Aug 6, 2020

Uh oh!

stevekuznetsov commented Aug 10, 2020

Uh oh!

Uh oh!

alvaroaleman Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petr-muller left a comment

Choose a reason for hiding this comment

Uh oh!

petr-muller Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

stevekuznetsov Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

petr-muller Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

petr-muller Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevekuznetsov commented Aug 11, 2020

Uh oh!

bbguimaraes commented Aug 12, 2020

Uh oh!

stevekuznetsov commented Aug 12, 2020

Uh oh!

bbguimaraes commented Aug 12, 2020

Uh oh!

openshift-ci-robot commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants