Static provisioning requires nonsensical managed resources #28
Comments
|
We can fix this pretty easily by deprecating func IsManagedKind(k ManagedKind, ot runtime.ObjectTyper) PredicateFn {
return func(obj runtime.Object) bool {
// We probably want a GetKind variant of MustGetKind that returns
// an error instead of panicing.
gvk, err := runtime.GetKind(obj, ot)
if err != nil {
return false
}
return gvk == schema.GroupVersionKind(k)
}
} |
|
I'd like to reopen this issue in the spirit of integrating early. I've raised crossplane/crossplane#860 to update the developer guide to match the change. @hasheddan did you happen to test whether this fix works as expected per the reproduction steps in this issue? P.S. I noticed during the doc update that |
|
@negz it appears that the claim stays in the Managed Resource: Binding Phase: Unbound
Conditions:
Last Transition Time: 2019-10-01T19:41:05Z
Reason: Managed resource is available for use
Status: True
Type: Ready
Last Transition Time: 2019-10-01T19:38:05Z
Reason: Successfully reconciled managed resource
Status: True
Type: SyncedClaim: Conditions:
Last Transition Time: 2019-10-01T19:38:02Z
Reason: Managed claim is waiting for managed resource to become bindable
Status: False
Type: Ready
Last Transition Time: 2019-10-01T19:38:02Z
Reason: Successfully reconciled managed resource
Status: True
Type: Synced |
|
@hasheddan Thanks for testing that out! There are three scenarios we need to make sure the claim reconciler handles:
I'd like to be sure scenarios 1 and 2 still work as expected, because I think they're more common and important than case 3. As a lower priority, we should figure out why case 3 isn't working and fix that. |
|
FWIW - based only on following the reconciler code and not actually testing it - I'd expect scenario three to result in a nil pointer exception in the claim reconciler:
My theory is that instead of using |
|
Another possible issue with static provisioning is that as far as I can tell we never actually set the managed resource's claim reference during the static provisioning codepath (we only set it when we dynamically provision new managed resources). The |
|
It sounds like there's a chance static provisioning is currently broken in master - I'm assigning this to myself to investigate and fix before we ship 0.4. |
|
Confirming that static provisioning seems broken. When I submit the following (at the same time): ---
apiVersion: database.gcp.crossplane.io/v1beta1
kind: CloudsqlInstance
metadata:
name: example
spec:
writeConnectionSecretToRef:
namespace: crossplane-system
name: cloudsqlinstance-example
forProvider:
databaseVersion: POSTGRES_9_6
region: us-west2
settings:
tier: db-custom-1-3840
dataDiskType: PD_SSD
dataDiskSizeGb: 10
providerRef:
name: example
reclaimPolicy: Delete
---
apiVersion: database.crossplane.io/v1alpha1
kind: PostgreSQLInstance
metadata:
name: app-postgresql
spec:
resourceRef:
apiVersion: database.gcp.crossplane.io/v1beta1
kind: CloudsqlInstance
name: example
writeConnectionSecretToRef:
name: postgresqlconn
engineVersion: "9.6"I eventually see the managed resource become available for binding (i.e. binding phase Meanwhile the PostgreSQLInstance says: |
|
However, static binding does work if you:
In this case, with the exact same managed resource, I see the following on the claim: |
Yuuup. If I submit the same resource claim before the managed resource exists I see: |
What happened?
Changes were made to our resource claim controller watch predicates recently in #18 and #24. The latter refactored the watch predicates used by resource claim controllers with the intention of supporting both statically and dynamically provisioned managed resources. It does this by accepting resources that satisfy any of three watch predicates, applied in the following order:
resource.HasManagedResourceReferenceKindaccepts resource claims with a.spec.resourceRefto the kind of managed resource the reconciler is concerned with.resource.HasDirectClassReferenceKindaccepts managed resources with a.spec.classRefto the kind of non-portable class the reconciler is concerned with.resource.HasIndirectClassReferenceKindaccepts resource claims with a.spec.classRefto a portable class with a.classRefto the kind of non-portable class the reconciler is concerned with.In retrospect
resource.HasDirectClassReferenceKindonly makes sense in a dynamic provisioning context, because statically provisioned managed resources don't use a resource class, and thus typically don't set their.spec.classRef. Based on my read of the current predicate logic (I have not confirmed this) I expect our resource claim reconcilers would never notice changes to statically provisioned managed resources.This is not too big an issue given the resource claim reconciler only watches managed resources to wait for them to become bindable, which they typically do before the resource claim is created in a static provisioning scenario. I'm guessing that's why I didn't encounter this case when I tested static provisioning per #24 (comment).
How can we reproduce it?
My guess is that the claim will never bind.
There are two workarounds for this:
.status.bindingPhaseto becomeUnboundbefore creating a resource claim that references it..spec.classRef.kindand.spec.classRef.apiVersionto its corresponding non-portable resource class kind.What environment did it happen in?
crossplane-runtime version: 26a458d
The text was updated successfully, but these errors were encountered: