Skip to content

Cleanup residual appregistry-server references#697

Merged
openshift-merge-robot merged 1 commit into
operator-framework:masterfrom
zcahana:appregistry-server-cleanup
Jul 8, 2021
Merged

Cleanup residual appregistry-server references#697
openshift-merge-robot merged 1 commit into
operator-framework:masterfrom
zcahana:appregistry-server-cleanup

Conversation

@zcahana
Copy link
Copy Markdown
Contributor

@zcahana zcahana commented Jul 8, 2021

Signed-off-by: Zvi Cahana zvic@il.ibm.com

Description of the change:

This PR complements #608 by cleaning up some additional appregistry-server references:

  • A reference in the upstream-registry-builder Dockerfile, which caused this image to fail build due to missing binary.
  • The Dockerfile used to build the appregistry-server image.

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
@openshift-ci openshift-ci Bot requested review from dinhxuanvu and njhale July 8, 2021 12:50
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jul 8, 2021

Hi @zcahana. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 8, 2021
@zcahana
Copy link
Copy Markdown
Contributor Author

zcahana commented Jul 8, 2021

/cc @ecordell @joelanford

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 8, 2021

Codecov Report

Merging #697 (df35b25) into master (eacb9a4) will not change coverage.
The diff coverage is n/a.

❗ Current head df35b25 differs from pull request most recent head 52ccb08. Consider uploading reports for the commit 52ccb08 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #697   +/-   ##
=======================================
  Coverage   49.11%   49.11%           
=======================================
  Files          96       96           
  Lines        8185     8185           
=======================================
  Hits         4020     4020           
  Misses       3388     3388           
  Partials      777      777           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eacb9a4...52ccb08. Read the comment docs.

@timflannagan
Copy link
Copy Markdown
Member

/ok-to-test
/lgtm

@openshift-ci openshift-ci Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 8, 2021
Copy link
Copy Markdown
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jul 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, zcahana

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:

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2021
@openshift-merge-robot openshift-merge-robot merged commit 4335a01 into operator-framework:master Jul 8, 2021
@zcahana zcahana deleted the appregistry-server-cleanup branch July 8, 2021 16:31
@zcahana
Copy link
Copy Markdown
Contributor Author

zcahana commented Jul 8, 2021

The quay build of upstream-registry-builder:latest still fails after merging this PR:
https://quay.io/repository/operator-framework/upstream-registry-builder?tab=info

I'm not sure why, since it now builds successfully on my machine.
Anyone with access to the quay build logs can check what's still failing?

@joelanford
Copy link
Copy Markdown
Member

It looks like failures have been ongoing for a few months. And its affecting other operator-registry images as well (e.g. upstream-opm-builder)

That log seems related to https://gitlab.alpinelinux.org/alpine/aports/-/issues/12396, but the failures have been ongoing longer than 3.14 has been released, so at this point, there maybe multiple factors involved in the failure.

What probably needs to happen is for us to migrate these image builds to a GH action such that:

  1. master branch and tag pushes run an action to build/push all the images
  2. PRs run an action to build (but not push) the images

This should give us more insight into the failures if they continue, and it'll help us identify the issue(s) as soon as they happen and block PRs that cause image build failures.

@zcahana
Copy link
Copy Markdown
Contributor Author

zcahana commented Jul 8, 2021

I've been playing with this locally and found this:
When I build using podman, it initially prompts me to select whether golang:1.16-alpine is quay.io/golang:1.16-alpine or rather docker.io/library/golang:1.16-alpine. If I choose quay, the build fails with a 404, whereas if I choose docker.io, the build succeeds (and the decision is stored at ~/.cache/containers/short-name-aliases.conf).

zvic@ZVICAHANA-D2LK:~/Workspace/operator-framework/operator-registry (master)$ podman build -t upstream-registry-builder:latest -f upstream-builder.Dockerfile .
STEP 1: FROM golang:1.16-alpine AS builder
✔ quay.io/golang:1.16-alpine
Trying to pull quay.io/golang:1.16-alpine...
STEP 2: FROM alpine:3
Error: error creating build container: Error initializing source docker://quay.io/golang:1.16-alpine: Error reading manifest 1.16-alpine in quay.io/golang: StatusCode: 404, <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final/...

Now, I don't know if this is what's been happening at the quay build machine (is there any hint in the logs for this?). But we can try to change the Dockerfiles and explicitly specify docker.io/library/golang:1.16-alpine. @joelanford WDYT?

@zcahana
Copy link
Copy Markdown
Contributor Author

zcahana commented Jul 8, 2021

I'm pretty sure the failures in upstream-registry-builder were initially due to the missing appregistry-server binary (dates match), and now due to the alpine 3.14 issue you mentioned. Also, the failures in operator-registry-server match the release date of alpine 3.14. The remaining piece of the puzzle is upstream-opm-builder, which only started failing after May 13, which doesn't ring any bell for me.

I'd say that a reasonable path forward here is to try and change golang:1.16-alpine --> golang:1.16 (i.e., don't use alpine which is considered highly experimental) and see if that revives upstream-registry-builder and operator-registry-server. Afterwards, it'd indeed be very useful to have these images built on GH and gate PRs on failures.

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. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants