ci-operator: add support for building operator index images by AlexNPavel · Pull Request #1084 · openshift/ci-tools

AlexNPavel · 2020-08-06T19:08:19Z

This PR adds support for building operator index images by adding an
operator_index field for images. The process is handled in a similar
manner to the bundle images, which require 2 steps, with the first step
involving building a new image to be used as a source image.

First, an image build is made that runs the opm command that generates
the index.db file and a dockerfile to build the corresponding index
image.

Second, a build is made with the previous build's image as its source,
using the dockerfile generated by opm.

This PR also adds validation for the images section of the config as
both operator_manifests and operator_index add potential for user
error.

AlexNPavel · 2020-08-06T19:09:17Z

The code is ready for review, but I'm marking it as WIP because I want to make sure it actually works when run against the cluster.

stevekuznetsov

Can we get an end-to-end test that builds a bundle and an index?

AlexNPavel · 2020-08-10T19:04:03Z

Current status of work: the index-gen image is being built correctly with the Dockerfile and database dir in the correct location in the image. ~~Currently trying to fix an issue with getting the openshift build to correctly find the dockerfile for the actual index build.~~ EDIT: Index image is getting built and seems to run correctly.

Other issues that need to be worked on:

The e2e tests don't work in the ci-tools CI. The existing credentials for the e2e test don't provide access to the local registries, which is necessary for the test. For local testing, I am using the regcred credentials that all normal ci-operator runs get in CI. We may need to use those credentials for the e2e tests here as well.
This currently depends on a @petr-muller's metering-operator for for manifests. I should be able to copy the manifests directory from there into the test directory in this repo and point the e2e test to the correct ref using the CI env vars so we don't depend on an external repo.

stevekuznetsov · 2020-08-10T19:38:47Z

@AlexNPavel sounds good -- do we still explicitly ask the user to configure the index build or implicitly create it when bundles are configured? I agree that updating the secret available to the e2e and moving static definitions (as minimal as possible) into this repo are good next steps

AlexNPavel · 2020-08-10T22:47:27Z

E2E tests for the bundle now use the manifests from the PR they are being run against.

AlexNPavel · 2020-08-10T23:22:56Z

This PR has been updated, rebased, and cleaned up. It should be ready once we update the pull credentials for the e2e tests.

AlexNPavel · 2020-08-10T23:24:48Z

@AlexNPavel sounds good -- do we still explicitly ask the user to configure the index build or implicitly create it when bundles are configured?

We still require it to be explicitly specified. I'm looking into how to handle it implicitly.

petr-muller · 2020-08-11T11:01:22Z

/assign

Will look! For the record, my intention was to first make the specifications explicit and after the functionality is validated I wanted to followup with a "infer indexes from bundle presence" story to have smaller iterations.

stevekuznetsov

Code looks good except for the explicit flow, couple comments

stevekuznetsov · 2020-08-11T15:40:15Z

+os::test::junit::declare_suite_end

+os::test::junit::declare_suite_start "e2e/simple/optional-operator"
+if [[ -z "${PULL_BASE_SHA:-}" ]]; then


Confused about these, and why $JOB_SPEC doesn't cover it?

It should. Changed to use that instead.

stevekuznetsov · 2020-08-11T15:41:54Z

+  os::log::fatal "\$PULL_PULL_SHA must be set for this test"
+fi
+export JOB_SPEC='{"type":"postsubmit","job":"branch-ci-openshift-ci-tools-master-ci-operator-e2e","buildid":"0","prowjobid":"uuid","refs":{"org":"openshift","repo":"ci-tools","base_ref":"master","base_sha":"'"${PULL_BASE_SHA}"'","pulls":[{"number":'"${PULL_NUMBER}"',"sha":"'"${PULL_PULL_SHA}"'","author":"AlexNPavel"}]}}'
+os::cmd::expect_success "ci-operator --image-import-pull-secret ${PULL_SECRET_DIR}/.dockerconfigjson --target [images] --config ${suite_dir}/optional-operators.yaml"


Could we somehow write a simple, lightweight tests step in this config that either runs in the index or uses my PR to depend on the index image and does something trivial to validate that the index is workable?

petr-muller

Code LGTM, not approving b/c I see Steve has some comments.

petr-muller · 2020-08-12T13:06:20Z

/test e2e

petr-muller

LGTM

This PR adds support for building operator index images by adding an `operator_index` field for images. The process is handled in a similar manner to the bundle images, which require 2 steps, with the first step involving building a new image to be used as a source image. First, an image build is made that runs the `opm` command that generates the `index.db` file and a dockerfile to build the corresponding index image. Second, a build is made with the previous build's image as its source, using the dockerfile generated by `opm`. This PR also adds validation for the `images` section of the config as both `operator_manifests` and `operator_index` add potential for user error.

stevekuznetsov · 2020-08-14T18:17:32Z

+	}
+
+	var expectedDockerfileSingleBundle = `
+FROM quay.io/operator-framework/upstream-opm-builder AS builder


Technically would be good to import these as base_images to the namespace so that they're considered when detecting the version of the job and so that we're not reliant on external registries/doing cluster-local pulls

stevekuznetsov · 2020-08-14T18:20:40Z

+os::test::junit::declare_suite_end

+os::test::junit::declare_suite_start "e2e/simple/optional-operator"
+if [[ -z "${PULL_BASE_SHA:-}" ]]; then


openshift-ci-robot · 2020-08-14T18:21:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexNPavel, 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~~ [AlexNPavel,stevekuznetsov]

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

This PR addresses a variety of comments from openshift#1084 that were unaddressed when the PR merged. These are the changes: - Make functions to create bundle image name and detect if image is bundle name - Add brief Godoc for api.Bundle - Use `find` command directly in bundle substitution instead of executing through bash - Use exact pullspecs from ResolvePullSpec, which also verifies that the image tag exists - Add image substitutions that get built in the pipeline to the required images for bundle-sub - Don't change global state (changing directory) in unit tests - Handle both yaml and yml file extensions for manifests - Handle escaping in dockerfile more cleanly - Remove extra, empty "" lines from beginning and end of generated dockerfiles - Expand replaceCommand unit tests by adding a .yml and .txt file

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 Aug 6, 2020

openshift-ci-robot requested review from alvaroaleman and petr-muller August 6, 2020 19:08

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

stevekuznetsov reviewed Aug 6, 2020

View reviewed changes

Comment thread pkg/api/types.go

Comment thread pkg/steps/bundle_source.go Outdated

Comment thread pkg/steps/index_generator.go Outdated

Comment thread pkg/steps/index_generator.go

AlexNPavel force-pushed the index-builder branch 10 times, most recently from 6e7cb69 to be0bbc3 Compare August 10, 2020 18:55

AlexNPavel force-pushed the index-builder branch from 236b3ba to 7ff5206 Compare August 10, 2020 23:15

openshift-ci-robot assigned petr-muller Aug 11, 2020

stevekuznetsov reviewed Aug 11, 2020

View reviewed changes

petr-muller reviewed Aug 11, 2020

View reviewed changes

AlexNPavel force-pushed the index-builder branch 3 times, most recently from f46cf00 to e035012 Compare August 12, 2020 18:02

AlexNPavel force-pushed the index-builder branch 2 times, most recently from c8c950e to a351a87 Compare August 12, 2020 18:05

petr-muller reviewed Aug 13, 2020

View reviewed changes

AlexNPavel force-pushed the index-builder branch from 2575178 to 3a16d37 Compare August 13, 2020 19:37

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2020

AlexNPavel added 2 commits August 13, 2020 12:43

move all operator image config under operator section of config

a8025c9

AlexNPavel force-pushed the index-builder branch from 3a16d37 to a8025c9 Compare August 13, 2020 19:44

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2020

stevekuznetsov reviewed Aug 13, 2020

View reviewed changes

Comment thread pkg/api/types.go

Comment thread pkg/api/types.go

Comment thread pkg/api/types.go Outdated

Comment thread pkg/defaults/defaults.go Outdated

Comment thread pkg/defaults/defaults.go

AlexNPavel changed the title ~~[WIP] ci-operator: add support for building operator index images~~ ci-operator: add support for building operator index images Aug 13, 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 13, 2020

add context_dir and remove manifests fields for operator

5b8a34d

AlexNPavel force-pushed the index-builder branch from 23edb64 to 5b8a34d Compare August 13, 2020 23:02

stevekuznetsov approved these changes Aug 14, 2020

View reviewed changes

openshift-ci-robot assigned stevekuznetsov Aug 14, 2020

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

openshift-merge-robot merged commit 6a89299 into openshift:master Aug 14, 2020

AlexNPavel deleted the index-builder branch August 21, 2020 00:41

AlexNPavel mentioned this pull request Aug 21, 2020

pkg/steps: some cleanups for index+bundle image building #1136

Merged

Conversation

AlexNPavel commented Aug 6, 2020

Uh oh!

AlexNPavel commented Aug 6, 2020

Uh oh!

stevekuznetsov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexNPavel commented Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevekuznetsov commented Aug 10, 2020

Uh oh!

AlexNPavel commented Aug 10, 2020

Uh oh!

AlexNPavel commented Aug 10, 2020

Uh oh!

AlexNPavel commented Aug 10, 2020

Uh oh!

petr-muller commented Aug 11, 2020

Uh oh!

stevekuznetsov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevekuznetsov Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

AlexNPavel Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

stevekuznetsov Aug 14, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

petr-muller commented Aug 12, 2020

Uh oh!

petr-muller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevekuznetsov Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stevekuznetsov Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AlexNPavel commented Aug 10, 2020 •

edited

Loading