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

remove pull secret copy, defer to image registry #249

Merged

Conversation

gabemontero
Copy link
Contributor

With @ricardomaraschini 's openshift/image-registry#226 merged here is my change here the change that remove's samples operator's copying of the install pull secret to the openshift namespace

I still have my prometheus metrics around whether the pull secret has TBR credentials, but it now just looks at the install pull secret directly.

Have not tried it in a local cluster yet so marked WIP for now

@openshift/openshift-team-developer-experience FYI

@openshift-ci-robot 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 Mar 18, 2020
@openshift-ci-robot
Copy link
Contributor

[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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 18, 2020
@gabemontero
Copy link
Contributor Author

@ricardomaraschini ... the image-ecosystem failures are around imagestream imports failing because of credentials.

In some of the test output I seen message like Mar 18 14:08:37.236: INFO: after seeing FailedImageImports for ruby a manual image-import was submitted

And when I examine the imagestreams in the must gather located at https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-samples-operator/249/pull-ci-openshift-cluster-samples-operator-master-e2e-aws-image-ecosystem/922/artifacts/e2e-aws-image-ecosystem/must-gather.tar

I see consistently see this across all the imagestreams:

        message: 'Internal error occurred: registry.redhat.io/3scale-amp23/apicast-gateway:latest:
          Get https://registry.redhat.io/v2/3scale-amp23/apicast-gateway/manifests/latest:
          unauthorized: Please login to the Red Hat Registry using your Customer Portal
          credentials. Further instructions can be found here: https://access.redhat.com/RegistryAuthentication'

I would contend your image-registry change is not working as expected.

@gabemontero
Copy link
Contributor Author

in talking with @ricardomaraschini it looks like this PR would need openshift/openshift-apiserver#83 and openshift/cluster-openshift-apiserver-operator#284 as well

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2020
@gabemontero
Copy link
Contributor Author

/retest

pkg/util/util.go Outdated Show resolved Hide resolved
@@ -487,7 +477,15 @@ func (h *Handler) CreateDefaultResourceIfNeeded(cfg *v1.Config) (*v1.Config, err
func (h *Handler) initConditions(cfg *v1.Config) *v1.Config {
now := kapis.Now()
util.Condition(cfg, v1.SamplesExist)
util.Condition(cfg, v1.ImportCredentialsExist)
creds := util.Condition(cfg, v1.ImportCredentialsExist)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not deleting this condition in case customers are coding to it

could entertain marking deprecated in openshift/api

@gabemontero
Copy link
Contributor Author

ci build pain wit its internal imagestreams with e2e-aws and e2e-aws-upgrade

we'll see what happens with the other 2 in flight e2es

@gabemontero
Copy link
Contributor Author

yeah @dmage @ricardomaraschini I'm still getting unauthorized: Please login to the Red Hat Registry using your Customer Portal credentials. errors on the sample imagestreams if the samples operator does not copy the install pull secret

@ricardomaraschini
Copy link

yeah @dmage @ricardomaraschini I'm still getting unauthorized: Please login to the Red Hat Registry using your Customer Portal credentials. errors on the sample imagestreams if the samples operator does not copy the install pull secret

Very interesting. How does the update work on CI? I mean, how can we be sure that we did have the new version of the apiserver when this test happened?

@ricardomaraschini
Copy link

Yeah, it seems like it is running the new version(judging by the artifacts). No idea why it is not working at this stage.

@gabemontero
Copy link
Contributor Author

Yeah, it seems like it is running the new version(judging by the artifacts). No idea why it is not working at this stage.

@ricardomaraschini @dmage perhaps it makes sense when you guys have cycles to use cluster bot on slack to launch a cluster from this PR and start investigating.

@ricardomaraschini
Copy link

/test e2e-aws-operator

@gabemontero @dmage I ran e2e tests using this PR branch on a cluster and they succeeded. I am not sure if these tests use the hypershift sidecar and if that might be causing issue(I don't think we are mounting the node credentials there).

[rmarasch@loaner ~]$ oc version
Client Version: 4.5.0-0.nightly-2020-04-10-040610
Server Version: 4.5.0-0.nightly-2020-04-10-040610
Kubernetes Version: v1.18.0-rc.1
[rmarasch@loaner ~]$

@ricardomaraschini
Copy link

Seems like the failure has changed now for e2e-aws-operator.

Copy link

@ricardomaraschini ricardomaraschini left a comment

Choose a reason for hiding this comment

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

@gabemontero I have created PR(gabemontero#2) against your branch that removes a check for the existence of a secret on openshift namespace, IIUC the logic to create that secret has been removed.

@gabemontero
Copy link
Contributor Author

updated this PR with @ricardomaraschini 's gabemontero#2

thanks @ricardomaraschini

@gabemontero
Copy link
Contributor Author

terraform / aws flake with latest e2e-aws-operator

/test e2e-aws-operator

@gabemontero
Copy link
Contributor Author

failed to acquire lease e2e upgrade

/test e2e-aws-upgrade

@gabemontero
Copy link
Contributor Author

failed to acquire lease image eco

/test e2e-aws-image-ecosystem

@gabemontero
Copy link
Contributor Author

/retest

2 similar comments
@gabemontero
Copy link
Contributor Author

/retest

@gabemontero
Copy link
Contributor Author

/retest

@gabemontero
Copy link
Contributor Author

ok @dmage @ricardomaraschini we got imagestream imports in the openshift namespace working via the latest image-eco and operator e2e's !!

based on how CI health is on Friday, we'll see about getting this in

@gabemontero gabemontero changed the title WIP: remove pull secret copy, defer to image registry remove pull secret copy, defer to image registry Apr 17, 2020
@openshift-ci-robot openshift-ci-robot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 17, 2020
@gabemontero
Copy link
Contributor Author

/test e2e-aws-upgrade

@gabemontero
Copy link
Contributor Author

/test e2e-aws

@gabemontero gabemontero added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@openshift-merge-robot openshift-merge-robot merged commit ed3d147 into openshift:master Apr 17, 2020
@gabemontero gabemontero deleted the del-install-secret-copy branch April 17, 2020 22:18
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants