cluster quota to namespace mapping controller #9558

deads2k · 2016-06-24T20:27:00Z

Third commit: Needs to be moved and lacks any tests, but I'm interested in whether anyone has concerns about intent.

@derekwaynecarr This is like a doubly-linked project authz cache that cannot rely resource versions since quota tracks status.

Also added a create clusterquota command that I'll probably split into another pull. This should match create quota if we ever make one.

derekwaynecarr · 2016-06-27T15:34:44Z

pkg/cmd/cli/cmd/create/clusterquota.go

+	ClusterQuotaRecommendedName = "clusterquota"
+
+	clusterQuotaLong = `
+Create a deployment config that uses a given image.


just a reminder to fix all this up.

derekwaynecarr · 2016-06-27T15:51:03Z

@deads2k - I won't have too much time today to dive deep into the cache side of things, but if i recall the complicated part of project cache was cluster acl changes requiring a full rebuild. I don't know if there is a similar style use case in your work here, but have not reasoned too deeply around it.

deads2k · 2016-06-27T15:51:56Z

@Kargakis alright, the algorithm is ready for review, but I've got a really cool race.

Add, Update, Delete a namespace really fast, before any of those are observed.
Add or Update a quota that matches the Add or Update namespace
The cache had the "bad" state for the namespace for some period of time. This makes the quota update the cache indicating a match.
The ns Delete is compressed out and never delivered, so the improper match is never cleared.

Neat, right?

deads2k · 2016-06-27T18:38:18Z

This is ready for review. I brought in 27341 because I was hitting races in my fuzzing unit test. I did not make our code do the work in the lock because we deadlocked I think I'd rather spend the time reworking onto shared informers instead of trying to fix the problem another way.

deads2k · 2016-06-27T18:38:53Z

[test]

deads2k · 2016-06-27T19:54:05Z

#9512

deads2k · 2016-06-27T19:56:59Z

@stevekuznetsov I got

Stderr from the command:

pkg/quota/controller/clusterquotamapping/clusterquotamapping.go:366: declaration of err shadows declaration at pkg/quota/controller/clusterquotamapping/clusterquotamapping.go:358: 
pkg/quota/controller/clusterquotamapping/clusterquotamapping.go:506: declaration of ok shadows declaration at pkg/quota/controller/clusterquotamapping/clusterquotamapping.go:504: 
pkg/quota/controller/clusterquotamapping/clusterquotamapping.go:548: declaration of ok shadows declaration at pkg/quota/controller/clusterquotamapping/clusterquotamapping.go:546:

I'm pretty sure that go recommends anti-patterns like that. How do I make govet less strict?

stevekuznetsov · 2016-06-27T20:23:55Z

pkg/quota/controller/clusterquotamapping/clusterquotamapping.go

-		return err
+	allQuotas, err1 := c.quotaLister.List(kapi.ListOptions{})
+	if err1 != nil {
+		return err1


This does seem silly, but your method is ~35 lines long and has indentation five levels deep. Maybe make your for{} block a named function?

stevekuznetsov · 2016-06-27T20:28:49Z

@deads2k left some comments on code refactors that might help, but mainly I have two things to say:

if "the Go way" actively introduces shadowing ... that sucks and maybe we should be OK with naming our oks and errs uniquely. I really don't think that the "negative" of having better named vars like that outweighs the ability for us to use static analysis to entirely remove shadowed variables from our codebase
you could always add your new package to the blacklist so go vet doesn't run shadow checks on you

0xmichalis · 2016-06-28T08:58:35Z

pkg/client/cache/namespace.go

+	quotaapi "github.com/openshift/origin/pkg/quota/api"
+)
+
+type IndexerToNamespaceLister struct {


Shouldn't this lister be upstreamed?

Shouldn't this lister be upstreamed?

Can I get a TODO? I think we need to move the entire InformerFactory interface.

Yes, that's fine.

0xmichalis · 2016-06-28T09:41:22Z

pkg/quota/controller/clusterquotamapping/clusterquotamapping.go

+// If the mapping cache is wrong and a previously deleted quota or namespace is created, this controller
+// correctly adds the items back to the list and clears out all previous mappings.
+//
+// In addition to those constraints, the timing threshold for actaully hitting this problem is really tight.  It's


deads2k · 2016-06-28T12:22:34Z

comments addressed.

0xmichalis · 2016-06-28T19:46:12Z

pkg/quota/controller/clusterquotamapping/clusterquotamapping.go

+	c.enqueueNamespace(cur)
+}
+func (c *ClusterQuotaMappingController) updateNamespace(old, cur interface{}) {
+	c.enqueueNamespace(cur)


Do you need to enqueue in case there are no label changes for the namespace? Same question about quota without selector changes in its own update handler.

Do you need to enqueue in case there are no label changes for the namespace? Same question about quota without selector changes in its own update handler.

The enqueue method is the spot that does that check since it is common to both the add and update paths.

missed. cool

0xmichalis · 2016-06-28T19:54:50Z

LGTM

deads2k · 2016-06-28T20:00:22Z

[merge]

openshift-bot · 2016-06-28T20:00:58Z

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

openshift-bot · 2016-06-28T20:00:59Z

Evaluated for origin merge up to 92f7e72

0xmichalis · 2016-06-29T08:03:10Z

[merge]

deads2k · 2016-06-29T11:46:11Z

re[test]

openshift-bot · 2016-06-29T11:50:37Z

Evaluated for origin test up to 92f7e72

openshift-bot · 2016-06-29T13:10:57Z

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

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2016

deads2k force-pushed the cq-03-reverse-index branch from 9782f29 to 4015e85 Compare June 27, 2016 12:12

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2016

deads2k force-pushed the cq-03-reverse-index branch from 4015e85 to a63bde1 Compare June 27, 2016 14:51

derekwaynecarr reviewed Jun 27, 2016
View reviewed changes

deads2k force-pushed the cq-03-reverse-index branch 2 times, most recently from 72afb8b to b0449ad Compare June 27, 2016 15:48

deads2k force-pushed the cq-03-reverse-index branch 2 times, most recently from 168c6ea to 8276f5f Compare June 27, 2016 17:51

deads2k added 2 commits June 27, 2016 14:35

UPSTREAM: 27341: Fix race in informer

dfadcac

react to 27341: this does not fix races in our code

43366d6

deads2k force-pushed the cq-03-reverse-index branch from 8276f5f to a24ce97 Compare June 27, 2016 18:35

deads2k changed the title ~~[WIP] cluster quota to namespace mapping controller~~ cluster quota to namespace mapping controller Jun 27, 2016

deads2k assigned 0xmichalis Jun 27, 2016

stevekuznetsov reviewed Jun 27, 2016
View reviewed changes

0xmichalis reviewed Jun 28, 2016
View reviewed changes

deads2k force-pushed the cq-03-reverse-index branch from 054da9b to eb4bf26 Compare June 28, 2016 12:22

deads2k force-pushed the cq-03-reverse-index branch from eb4bf26 to a180754 Compare June 28, 2016 12:22

deads2k mentioned this pull request Jun 28, 2016

project clusterresourcequota into applicable namespaces #9609

Merged

add clusterresourcequota/namespace reverse index

92f7e72

deads2k force-pushed the cq-03-reverse-index branch from a180754 to 92f7e72 Compare June 28, 2016 17:27

0xmichalis reviewed Jun 28, 2016
View reviewed changes

openshift-bot merged commit e345c2c into openshift:master Jun 29, 2016

deads2k deleted the cq-03-reverse-index branch September 6, 2016 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster quota to namespace mapping controller #9558

cluster quota to namespace mapping controller #9558

deads2k commented Jun 24, 2016

derekwaynecarr Jun 27, 2016

derekwaynecarr commented Jun 27, 2016

deads2k commented Jun 27, 2016

deads2k commented Jun 27, 2016

deads2k commented Jun 27, 2016

deads2k commented Jun 27, 2016

deads2k commented Jun 27, 2016

stevekuznetsov Jun 27, 2016

stevekuznetsov commented Jun 27, 2016

0xmichalis Jun 28, 2016

deads2k Jun 28, 2016

0xmichalis Jun 28, 2016

0xmichalis Jun 28, 2016

deads2k commented Jun 28, 2016

0xmichalis Jun 28, 2016

deads2k Jun 28, 2016

0xmichalis Jun 28, 2016

0xmichalis commented Jun 28, 2016

deads2k commented Jun 28, 2016

openshift-bot commented Jun 28, 2016 •

edited

Loading

openshift-bot commented Jun 28, 2016

0xmichalis commented Jun 29, 2016

deads2k commented Jun 29, 2016

openshift-bot commented Jun 29, 2016

openshift-bot commented Jun 29, 2016

cluster quota to namespace mapping controller #9558

cluster quota to namespace mapping controller #9558

Conversation

deads2k commented Jun 24, 2016

Choose a reason for hiding this comment

derekwaynecarr commented Jun 27, 2016

deads2k commented Jun 27, 2016

deads2k commented Jun 27, 2016

deads2k commented Jun 27, 2016

deads2k commented Jun 27, 2016

deads2k commented Jun 27, 2016

Choose a reason for hiding this comment

stevekuznetsov commented Jun 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jun 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Jun 28, 2016

deads2k commented Jun 28, 2016

openshift-bot commented Jun 28, 2016 • edited Loading

openshift-bot commented Jun 28, 2016

0xmichalis commented Jun 29, 2016

deads2k commented Jun 29, 2016

openshift-bot commented Jun 29, 2016

openshift-bot commented Jun 29, 2016

openshift-bot commented Jun 28, 2016 •

edited

Loading