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

Don't enforce quota in registry by default #9400

Conversation

miminar
Copy link

@miminar miminar commented Jun 17, 2016

The quota enforcement has been off by default for existing deployments.
Let's keep it off by default for the new ones as well.

Resolves #9393

@miminar
Copy link
Author

miminar commented Jun 17, 2016

Flake #9402. And two errors in extended tests:

[Fail] [images] openshift limit range admission [It] should deny a push of built image exceeding openshift.io/Image limit 
/data/src/github.com/openshift/origin/test/extended/images/limitrange_admission.go:56

[Fail] [images] openshift limit range admission [It] should deny a push of built image exceeding limit on openshift.io/images resource 
/data/src/github.com/openshift/origin/test/extended/images/limitrange_admission.go:87

I need to investigate them. They may be test flakes or an error in server's admission plugin. Either way, this change didn't introduce the problem.

[test]

@smarterclayton
Copy link
Contributor

This isn't enough (wont fix the issue) - the absence of the ENV var must default to "off" in the registry middleware - today the quotaEnforcing code is still added even when quota is off, which causes limit range checks, which breaks backwards compatibility.

The middleware needs to default to not using the quotaBlobStore if the env var isn't set to true

@smarterclayton
Copy link
Contributor

When we add backward compat things, we can't default new features to on if those features also require changes to the master for several releases.

@miminar
Copy link
Author

miminar commented Jun 17, 2016

@smarterclayton but we default to not enforcing quota by default right now.
See config file and middleware's default.

So you'd have to have either a custom config file or a new deployment config created with current defaults (the enforce env var set to true) to have it enabled.

@miminar
Copy link
Author

miminar commented Jun 17, 2016

@smarterclayton oh I see now. There's a bug. Will update.

The quota enforcement has been off by default for existing deployments.
Let's keep it off by default for the new ones as well.

Signed-off-by: Michal Minar <miminar@redhat.com>
@miminar miminar force-pushed the dont-enforce-registry-limits-by-default-9393 branch from 7f52d6f to f303562 Compare June 17, 2016 14:52
@@ -168,7 +168,7 @@ func (r *repository) Blobs(ctx context.Context) distribution.BlobStore {

bs := r.Repository.Blobs(ctx)

if quotaEnforcing != nil {
if !quotaEnforcing.enforcementDisabled {
Copy link
Author

Choose a reason for hiding this comment

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

@smarterclayton this was the culprit - well, my accomplice.

Copy link
Contributor

Choose a reason for hiding this comment

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

:). After Alexey lands his changes to distribution I have a refactor to the middleware initialization path that prevents us from reloading the clients multiple times (which was not intentional) that will affect some of this code. But the quick fix here is good.

@openshift-bot
Copy link
Contributor

Evaluated for origin testextended up to f303562

@smarterclayton
Copy link
Contributor

Waiting for extended but looks good to me.

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/218/) (Extended Tests: core(admission))

@miminar
Copy link
Author

miminar commented Jun 17, 2016

So the extended tests actually fail because of issue #9343 (tl;dr; we fail to parse output of docker 1.10). The builder reports success for failure. The images are actually refused as reported by registry:

time="2016-06-17T17:40:53.920732704Z" level=error msg="response completed with error" err.code=DENIED err.message="requested access to the resource is denied"

@miminar
Copy link
Author

miminar commented Jun 17, 2016

The other was flake #9129. [test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to f303562

@openshift-bot
Copy link
Contributor

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

@smarterclayton
Copy link
Contributor

Lgtm [merge]

Will be part of 1.3.0.alpha.2

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 18, 2016

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

@smarterclayton
Copy link
Contributor

[merge]


=== BEGIN TEST CASE ===
test/cmd/basicresources.sh:238: executing 'oc create -f
test/testdata/external-service.yaml' expecting success
FAILURE after 0.618s: test/cmd/basicresources.sh:238: executing 'oc
create -f test/testdata/external-service.yaml' expecting success: the
command returned the wrong error code
There was no output from the command.
Standard error from the command:
The Service "external" is invalid.
spec.clusterIP: Invalid value: "172.30.0.100": provided IP is already allocated
=== END TEST CASE ===

On Sat, Jun 18, 2016 at 1:45 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/merge FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5082/)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9400 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_pxKSuy8KRKMijwtmQdIVasS-S8Kfks5qNC6rgaJpZM4I4NSR
.

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to f303562

@openshift-bot openshift-bot merged commit 94c3b96 into openshift:master Jun 18, 2016
@miminar miminar deleted the dont-enforce-registry-limits-by-default-9393 branch August 8, 2016 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants