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.

Sign up for GitHub

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

Jump to bottom

BZ 1356030 record quota errors during import #10183

Merged
merged 1 commit into from
Aug 15, 2016
Merged

BZ 1356030 record quota errors during import #10183

merged 1 commit into from
Aug 15, 2016

Conversation

pweil-
Copy link
Contributor

@pweil- pweil- commented Aug 3, 2016 •
edited
Loading

Adds event recording for failed image imports

updated to record conditions during import if they are quota errors

@miminar @mfojtik @soltysh

@pweil-
Copy link
Contributor Author

pweil- commented Aug 3, 2016

Output:

25s        31s         6         mybox                      ImageStream                                           Warning   FailedImport              {image-import-controller }                 Import stream default/mybox partial=true error: ImageStream "mybox" is invalid: imageStream: Forbidden: ImageStream "mybox" is forbidden: requested usage of openshift.io/images exceeds the maximum limit per openshift.io/ImageStream (2 > 1)

miminar
miminar reviewed Aug 5, 2016
View reviewed changes
pkg/image/controller/factory.go
@@ -86,12 +89,16 @@ type scheduled struct {
}

// newScheduled initializes a scheduled import object and sets its scheduler. Limiter is optional.
func newScheduled(enabled bool, client client.ImageStreamsNamespacer, buckets int, bucketLimiter, importLimiter flowcontrol.RateLimiter) *scheduled {
func (f *ImportControllerFactory) newScheduled(enabled bool, client client.ImageStreamsNamespacer, buckets int, bucketLimiter, importLimiter flowcontrol.RateLimiter) *scheduled {
Copy link

Choose a reason for hiding this comment

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

It seems like client and importLimiter can be fetched from f.

@miminar
Copy link

miminar commented Aug 5, 2016

One nit, LGTM otherwise.

@soltysh
Copy link
Contributor

soltysh commented Aug 5, 2016

One issue, otherwise LGTM.

[test]

@miminar
Copy link

miminar commented Aug 5, 2016

I'm not sure if it really fixes the referenced bug though. The reporter would probably like to see the message either as an output of oc tag or oc describe is/<isname>.

@pweil- pweil- force-pushed the image-import-events branch 2 times, most recently from 40dc283 to dc88bd3 Compare August 5, 2016 17:02
@pweil- pweil- changed the title BZ 1356030 record image import errors as events BZ 1356030 record quota errors during import Aug 5, 2016
pweil-
pweil- reviewed Aug 5, 2016
View reviewed changes
pkg/image/registry/imagestreamimport/rest.go
return nil, err
}
isi.Status.Import = obj.(*api.ImageStream)
return isi, nil
}

// recordQuotaExceededStatus adds the err to any tag that does not already have conditions set.
func recordQuotaExceededStatus(originalStream *api.ImageStream, newStream *api.ImageStream, err error, now unversioned.Time, nextGeneration int64) {
for tag, _ := range newStream.Status.Tags {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miminar PTAL - is there a better way to just update the newest tags?

@pweil-
Copy link
Contributor Author

pweil- commented Aug 5, 2016

updated to record the quota error as a condition on the tag so it shows during the describe. Removed the events.

@pweil-
Copy link
Contributor Author

pweil- commented Aug 5, 2016

example output:

[pweil@localhost origin]$ oc describe is mybox
Name:           mybox
Namespace:      default
Created:        About a minute ago
Labels:         <none>
Annotations:        <none>
Docker Pull Spec:   <none>

Tag Spec                Created         PullSpec                        Image
v5  busybox             3 seconds ago                                   import failed: ImageStream "mybox" is invalid: imageStream: Forbidd...
v4  busybox             57 seconds ago                                  import failed: ImageStream "mybox" is invalid: imageStream: Forbidd...
v3  busybox             About a minute ago                              import failed: ImageStream "mybox" is invalid: imageStream: Forbidd...
v2  busybox             About a minute ago                              import failed: ImageStream "mybox" is invalid: imageStream: Forbidd...
v1  busybox             About a minute ago                              import failed: ImageStream "mybox" is invalid: imageStream: Forbidd...
hello   openshift/hello-openshift   About a minute ago  openshift/hello-openshift@sha256:e6aaedfdf3a0c0...  <same>

@pweil- pweil- force-pushed the image-import-events branch 2 times, most recently from 0106a4a to efe0ffc Compare August 5, 2016 20:14
miminar
miminar reviewed Aug 8, 2016
View reviewed changes
pkg/image/registry/imagestreamimport/rest.go
return nil, err
}
isi.Status.Import = obj.(*api.ImageStream)
return isi, nil
}

// recordQuotaExceededStatus adds the quota err to any new tag.
func recordQuotaExceededStatus(originalStream *api.ImageStream, newStream *api.ImageStream, err error, now unversioned.Time, nextGeneration int64) {
for tag := range newStream.Status.Tags {
Copy link

Choose a reason for hiding this comment

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

Could you set condition only on tags affected by the import?

@pweil-
Copy link
Contributor Author

pweil- commented Aug 15, 2016

@miminar please see the feedback commit based on our IRC discussion

@pweil-
Copy link
Contributor Author

pweil- commented Aug 15, 2016

[test]

@miminar
Copy link

miminar commented Aug 15, 2016

LGTM, thanks!

@pweil-
Copy link
Contributor Author

pweil- commented Aug 15, 2016

LGTM, thanks!

cool, squashed. Will merge on green

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to baef250

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7923/)

@pweil-
Copy link
Contributor Author

pweil- commented Aug 15, 2016

[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 15, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7923/) (Image: devenv-rhel7_4831)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to baef250

@openshift-bot openshift-bot merged commit 0244e3c into openshift:master Aug 15, 2016
@miminar miminar mentioned this pull request Aug 17, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pweil- @miminar @soltysh @openshift-bot