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
Bug 2002368: acccount for image api returning invalid on imagestream create based on allowed/blocked registry settings #394
Conversation
@gabemontero: This pull request references Bugzilla bug 2002368, which is invalid:
Comment In response to this:
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. |
/bugzilla refresh |
@gabemontero: This pull request references Bugzilla bug 2002368, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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. |
/hold I can get a bit more sophisticated / robust with this. |
/retest |
aws flake /test e2e-aws-upgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
yeah I'm going to pivot from our prior conversations @dperaza4dustbit I'm going to watch allowed/blocked registries, and if we are blocked, I'm going to bootstrap as removed, similar to what we do with ipv6/proxy/disconnected .... a "separate cluster config" situation makes samples install untenable. I'll add a new reason code when we are removed, and I believe the existing alert we have there should be sufficient. With that, I don't think there would be any future work for you team to do wrt new metrics and alerts. This will require a doc update with @rolfedh (which I just mentioned in scrum with him). We'll need to work with him here in this PR, perhaps with a Jira on his board, to track. /assign @rolfedh |
pkg/stub/imagestreams.go
Outdated
@@ -175,7 +175,7 @@ func (h *Handler) upsertImageStream(imagestreamInOperatorImage, imagestreamInClu | |||
// return the error so the caller can decide what to do | |||
return err | |||
} | |||
if IsRetryableAPIError(err) || kerrors.IsConflict(err) { | |||
if ShouldNotGoDegraded(err) { | |||
logrus.Printf("CRDUPDATE: retryable error %s with imagestream update %s", err.Error(), imagestreamInCluster.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pre-existing problem but this line causes panics, imagestreamInCluster is guaranteed to be nil (line 170 checks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for heads up @stbenjam ... I've added a commit in this PR for now, but have also pinged @dperaza4dustbit about this
we'll either address here with that commit , or he can do a separate PR, based on his taking ownership of samples (we are in the middle of the transition this week).
/hold cancel |
/hold the new global image config check needs some work |
updated I did make are in separate commits for now @coreydaley .... I'll squash after you sign off on those, plus we sort out remaining questions also, ideally we do not merge until I hear again from @dperaza4dustbit on these more recent changes ... he lgtmed an earlier version |
don't know why I couldn't find "errors" before ... switch will be up momentarily @coreydaley |
round 2 updates up @coreydaley |
known unit flake with okd version .. all other okd's passed /skip /retest |
/retest |
/lgtm |
…reation - on initial startup, as part of tbr check, bootstrap as removed - if the samples registry has been overridden, mark config invalid with new reason code
…n payload imagestreams
ade9947
to
de7179f
Compare
great thanks @coreydaley @dperaza4dustbit I've squashed the review commits from @coreydaley 's reviews if I could get one more lgtm from one of you, we'll go from there about to ping @rolfedh on slack now to get the doc updates going |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: coreydaley, dperaza4dustbit, 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 |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest-required Please review the full test history for this PR and help us cut down flakes. |
unrelated flakes in okd build test failure; sig-builds passed /skip |
unrelated sig-instrumentation flake image eco /test e2e-aws-image-ecosystem |
@gabemontero: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
@gabemontero: All pull requests linked via external trackers have merged: Bugzilla bug 2002368 has been moved to the MODIFIED state. In response to this:
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. |
when it is time to cherrypick, the 4.9 and other z streams will need both this PR and #397 |
/assign @jitendar-singh ideally you apply the qe approved label to this PR /assign @rolfedh ideally you apply the docs approved label to this PR, where the input I gave you at https://docs.google.com/document/d/1fN_-2_ZPC6DQBIYJkrlqL_0i9GSVbqJ8YyhB8hRns-o/edit is the starting point you need |
/label qe-approved |
Fixes #385