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

Introduce label selector based class scheduling #48

Merged
merged 7 commits into from
Oct 23, 2019

Conversation

negz
Copy link
Member

@negz negz commented Oct 10, 2019

Description of your changes

This enables label based class matching per #926. Changes will also be required to crossplane, and all stacks.

Checklist

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Ensured this PR contains a neat, self documenting set of commits.
  • Updated any relevant documentation, examples, or release notes.
  • Updated the RBAC permissions in clusterrole.yaml to include any new types.

@negz negz force-pushed the notsoclassy branch 5 times, most recently from 8a53de7 to 933c6de Compare October 16, 2019 07:26
negz added a commit to negz/crossplane-tools that referenced this pull request Oct 17, 2019
negz added a commit to negz/crossplane-tools that referenced this pull request Oct 17, 2019
negz added a commit to negz/crossplane-tools that referenced this pull request Oct 17, 2019
negz added a commit to negz/crossplane-tools that referenced this pull request Oct 17, 2019
Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

This looks great! Cross-checked with the stated design and feels like it adheres. None of my comments are blocking so I will go ahead and approve, but also will be looking for the first stack implementation to review. Awesome work :)

apis/core/v1alpha1/resource.go Outdated Show resolved Hide resolved
pkg/resource/claim_defaulting_reconciler.go Show resolved Hide resolved
pkg/resource/claim_scheduling_reconciler.go Show resolved Hide resolved
// garbage collection, to determine whether to delete a managed resource
// when its claim is deleted per https://github.com/crossplaneio/crossplane/issues/550

// TODO(negz): Avoid setting this owner reference? Kubernetes specifies that
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what happens if we try and set this owner reference?

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't looked at this for some time, but as far as I can tell it works. Currently we almost always dynamically provision a managed resource in a separate namespace from the resource claim, yet deleting the resource claim automatically deletes the managed resource (via owner reference based garbage collection).

What's interesting is that I have no idea how this works - I'm not sure how the garbage collector figures out what to delete without knowing the owner's namespace.

Copy link
Member

Choose a reason for hiding this comment

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

I am going to dive into this a little bit more, but I don't think my investigation needs to block here :)

@negz negz force-pushed the notsoclassy branch 4 times, most recently from 25a5cfa to 6bd3d20 Compare October 20, 2019 07:34
negz added a commit to negz/provider-azure that referenced this pull request Oct 21, 2019
negz added a commit to negz/provider-azure that referenced this pull request Oct 21, 2019
negz added a commit to negz/provider-azure that referenced this pull request Oct 21, 2019
@negz negz marked this pull request as ready for review October 21, 2019 07:35
negz added a commit to negz/provider-azure that referenced this pull request Oct 21, 2019
@negz
Copy link
Member Author

negz commented Oct 22, 2019

Here's an example of simple resource class selection in action:

$ kubectl get redisclass -l classy="true"
NAME                     PROVIDER-REF   RECLAIM-POLICY   AGE
azure-redis-labelled-a   example        Delete           51m
azure-redis-labelled-b   example        Delete           51m
azure-redis-labelled-c   example        Delete           51m
azure-redis-labelled-d   example        Delete           51m
azure-redis-labelled-e   example        Delete           51m

$ kubectl get cloudmemorystoreinstanceclass -l classy="true"
NAME                   PROVIDER-REF   RECLAIM-POLICY   AGE
gcp-redis-labelled-a   example        Delete           50m
gcp-redis-labelled-b   example        Delete           49m
gcp-redis-labelled-c   example        Delete           49m
gcp-redis-labelled-d   example        Delete           49m
gcp-redis-labelled-e   example        Delete           49m

$ kubectl get rediscluster
NAME          STATUS   CLASS-KIND                      CLASS-NAME               RESOURCE-KIND              RESOURCE-NAME               AGE
app-redis-0            RedisClass                      azure-redis-labelled-b   Redis                      default-app-redis-0-8rhkr   4m31s
app-redis-1            RedisClass                      azure-redis-labelled-b   Redis                      default-app-redis-1-tbn7g   4m11s
app-redis-2            CloudMemorystoreInstanceClass   gcp-redis-labelled-a     CloudMemorystoreInstance   default-app-redis-2-mrqpm   3m52s
app-redis-3            RedisClass                      azure-redis-labelled-c   Redis                      default-app-redis-3-glr4s   3m36s
app-redis-4            RedisClass                      azure-redis-labelled-b   Redis                      default-app-redis-4-vdzc9   3m27s
app-redis-5            CloudMemorystoreInstanceClass   gcp-redis-labelled-b     CloudMemorystoreInstance   default-app-redis-5-2qt5r   3m20s
app-redis-6            RedisClass                      azure-redis-labelled-a   Redis                      default-app-redis-6-t7g59   3m11s
app-redis-7            CloudMemorystoreInstanceClass   gcp-redis-labelled-a     CloudMemorystoreInstance   default-app-redis-7-cwmpl   2m25s
app-redis-8            CloudMemorystoreInstanceClass   gcp-redis-labelled-b     CloudMemorystoreInstance   default-app-redis-8-9dcm4   2m25s
app-redis-9            CloudMemorystoreInstanceClass   gcp-redis-labelled-b     CloudMemorystoreInstance   default-app-redis-9-fdxqf   2m25s

The claims are all variants of:

---
apiVersion: cache.crossplane.io/v1alpha1
kind: RedisCluster
metadata:
  name: app-redis-0
spec:
  classSelector:
    matchLabels:
      classy: "true"
  writeConnectionSecretToRef:
    name: app-redis-0

This commit renames "non portable resource class" back to "resource class", and
requires that resource claims reference a (non portable) resource class in any
namespace.

Signed-off-by: Nic Cope <[email protected]>
This allows us to require that name (and namespace where appropriate) are set at
the CRD level. In the case of cluster scoped resources that reference secrets
this is less surprising than defaulting to the `default` namespace when the
namespace is omitted.

Signed-off-by: Nic Cope <[email protected]>
For resource claim controllers, now that we're unconcerned with indirect
resource classes.

Signed-off-by: Nic Cope <[email protected]>
200ms seems low enough that GCP consistently beats Azure when scheduling
RedisCluster claims in my experiments.

Signed-off-by: Nic Cope <[email protected]>
negz added a commit to negz/crossplane-tools that referenced this pull request Oct 23, 2019
negz added a commit to negz/provider-azure that referenced this pull request Oct 23, 2019
negz added a commit to negz/provider-azure that referenced this pull request Oct 23, 2019
@negz negz merged commit 0f37bea into crossplane:master Oct 23, 2019
@negz negz deleted the notsoclassy branch October 23, 2019 21:56
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.

2 participants