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

Claim portability improvements #703

Closed
eonpatapon opened this issue Aug 23, 2019 · 7 comments
Closed

Claim portability improvements #703

eonpatapon opened this issue Aug 23, 2019 · 7 comments
Labels
enhancement New feature or request

Comments

@eonpatapon
Copy link

Current situation

Currently claims are not really portable if they are not using the default class configured with ressource class policy.

To target a specific class which is not the default a user needs to know the specific class kind and api as well as its name and namespace using the classRef attribute:

apiVersion: database.crossplane.io/v1alpha1
kind: MySQLInstance
metadata:
  name: mysql-instance
spec:
  classRef:
    kind: RDSInstanceClass
    apiVersion: database.aws.crossplane.io/v1alpha1
    name: mysql-standard
    namespace: crossplane-system

Proposal

Instead of targeting a specific class the user could define a set of wanted properties for the claim. At creation crossplane would automatically choose the ressource class that matches the user claim.

For example in the case of a database instance the user could define the minimum size of the DB:

apiVersion: database.crossplane.io/v1alpha1
kind: MySQLInstance
metadata:
  name: mysql-instance
spec:
  classRequirements:
    size: 40

In this case crossplane will try to find a concrete class that has a size at least of 40. If not a proper error should be returned to the user.

To be able to find the available classes for an abstract type the relation between policy and classes could be done the other way.

Currenly with a MySQLInstancePolicy resource it is possible to define the default resource class to use:

apiVersion: storage.crossplane.io/v1alpha1
kind: MySQLInstancePolicy
metadata:
  name: mysql-policy
  namespace: demo
defaultClassRef:
  kind: RDSInstanceClass
  apiVersion: database.aws.crossplane.io/v1alpha1
  name: standard-mysql
  namespace: crossplane-system

Instead of defining a ref MySQLInstancePolicy - <defaultClassRef> -> RDSInstanceClass it is maybe possible to reverse it to reference multiple concrete class in the policy: (RDSInstanceClass|CloudsqlInstanceClass|...) - <policyRef (default: true|false)> -> MySQLInstancePolicy:

apiVersion: database.aws.crossplane.io/v1alpha1
kind: RDSInstanceClass
metadata:
  name: standard-mysql
  namespace: crossplane-system
specTemplate:
  size: 20
  providerRef: ...
  policyRef: 
    kind: MySQLInstancePolicy
    apiVersion: storage.crossplane.io/v1alpha1
    default: true
---
apiVersion: database.gcp.crossplane.io/v1alpha1
kind: CloudsqlInstanceClass
metadata:
  name: big-mysql
  namespace: crossplane-system
specTemplate:
  databaseVersion: MYSQL_5_7
  storageGB: 100
  providerRef: ...
  policyRef: 
    kind: MySQLInstancePolicy
    apiVersion: storage.crossplane.io/v1alpha1

Now, from the MySQLInstancePolicy it would be possible to find concrete classes to choose from following back references.

One of theses references can be set as the default one.

@eonpatapon eonpatapon added the enhancement New feature or request label Aug 23, 2019
@negz
Copy link
Member

negz commented Aug 24, 2019

Thanks for the valuable suggestions @eonpatapon! We're definitely interested in increasing claim portability, and I wouldn't be surprised if we didn't end up with something like your suggestions above.

@negz
Copy link
Member

negz commented Sep 25, 2019

@eonpatapon We made some changes to how class references work as part of 0.3. We tracked a lot of that work in #723. How do you feel about the new pattern?

@eonpatapon
Copy link
Author

@negz I had a look to design/one-pager-default-resource-class.md and indeed it would solve the portability problem I was mentioning in this issue.

About the implementation if I may criticize it a bit I can see some drawbacks ;).

Principaly the increased number of resources to be created by the administrator. Basically it seems to me that for each concrete type you'll need to create an associated abstract type that can be referenced in claims.
Moreover if I understand correctly they may need to be created in different namespaces when apps are deployed in different namespaces.

@negz
Copy link
Member

negz commented Oct 2, 2019

@eonpatapon Thanks again for the feedback! The number of Kubernetes resources required to support portable, dynamic provisioning has been a concern for us too. Do you have any ideas about how we could simplify things without sacrificing portability?

I should note that the "property matching" feature you initially proposed in this issue is still a direction we'd like to investigate for Crossplane. It could be that if we solve that well we could remove some of the resources that are required today (e.g. portable class).

@eonpatapon
Copy link
Author

@negz Thanks for considering my proposal. Again I'm not sure it is doable since I don't know much how k8s API works internally but my main idea was to reference concrete classes the other way around.

Let's say we have 3 flavors of mysql, curently we need:

  • 3 concretes resources:
    • RDSInstanceClass A
    • RDSInstanceClass B
    • CloudsqlInstanceClass C
  • at least 3 classes with references:
    • MySQLInstanceClass A -> ref -> RDSInstanceClass A
    • MySQLInstanceClass B -> ref -> RDSInstanceClass B
    • MySQLInstanceClass C -> ref -> RDSInstanceClass C
  • then claims can reference any class:
    • MySQLInstance X -> ref -> MySQLInstanceClass B

So to remove some redundancy maybe it is possible to define references like that:

  • 3 concretes resources:
    • RDSInstanceClass A -> ref <label: default> -> MySQLInstanceClass
    • RDSInstanceClass B -> ref -> MySQLInstanceClass
    • CloudsqlInstanceClass C -> ref -> MySQLInstanceClass
  • then claims can reference any class:
    • MySQLInstance X -> ref -> MySQLInstanceClass B

MySQLInstanceClass would be some kind of singleton resource (automatically provisionned by crossplace) which make possible to find back references to concrete classes without knowing their exact type, just using their name or automatically choosing the default one.

When a claim is created (eg: MySQLInstance), the controller fetch MysqlInstanceClass resource and from there it has references to concrete classes. If a class name is asked in the claim it would need to match the appropriate concrete class. If nothing is specified in the claim it would just take the default one.

Then I think that a property matching feature could be easily implemented on top of that afterwards.

@negz
Copy link
Member

negz commented Nov 4, 2019

Hi @eonpatapon. We ended up going with a slightly different (but still hopefully much simpler) direction as described in #926. In short:

  • Portable resource classes are gone.
  • All managed resources are now cluster scoped
  • Resource claims can match a resource class annotated as default
  • Resource claims can match a resource class using a label selector

The last part is a little like what you proposed in this issuem, in that it lets you describe requirements. For example:

apiVersion: database.crossplane.io/v1alpha1
kind: MySQLInstance
metadata:
  name: mysql-instance
spec:
  classRequirements:
    size: 40

Would instead be:

apiVersion: database.crossplane.io/v1alpha1
kind: MySQLInstance
metadata:
  name: mysql-instance
spec:
  classSelector:
    matchLabels:
      size: "40"

This is not quite as powerful as what you were thinking - it requires the resource class to be labelled (rather than inspecting its spec) and supports only exact matches, not "at least" or "at most". #91 tracks adding more powerful support for matching on spec fields.

I hope the spirit of this issue is addressed, so I'm going to mark this issue closed, but feel free to let me know if you think there's more we need to do here and I'll reopen it.

@negz negz closed this as completed Nov 4, 2019
@eonpatapon
Copy link
Author

thanks for the heads up @negz, I think the solution you proposed is a step in the right way. Good for me!

luebken pushed a commit to luebken/crossplane that referenced this issue Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants