Create an overall Ready to indicate whether the SBR reconcile is completed successfully

[cdlliuy]

According to the discuss on last call for #565 (comment) , I would like to get more comments on the proposal listed by @DhritiShikhar

Any objection to go ahead with this table for the ready status under different condition?

[isutton]

@cdlliuy

To confirm, the idea would be re-introduce the Ready condition, and a True status might be set in the following circumstances:

• CollectionReady is True and InjectionReady is False with the EmptyApplicationSelector reason, indicating the user has not declared an application; or
• CollectionReady is True and InjectionReady is also True.

Does this match your expectations?

[cdlliuy]

yes, @isutton , exactly.

[cdlliuy]

@isutton , if we already get agreement on this, maybe we can put effort to get start to add ready ? Let us decide this on Thursday's call?

[DhritiShikhar]

@cdlliuy Sure

[DhritiShikhar]

@pedjak

Based on the discussion in the last community call,
there are two happy paths:

[1] CollectionReady -> True + InjectionReady -> False
Intermediate secret is populated and ready to be used.
This is generic binding -> #350

[2] CollectionReady -> True + InjectionReady -> True

Another Ready condition was proposed because as far as I understand, @cdlliuy 's team needs something to represent these two scenario. Refer this -> #565 (comment)

[pedjak]

By looking at the condition table above, it appears to me that they are actually not independent from each other, i.e. some combinations are not possible. Hence, we can simplify the model a lot: we can keep just Ready type and incorporate other two as reasons:

Type	Status	Reason	Message
Ready	True	AppBound	Application Foo bound to service(s)
Ready	False	EmptyServiceSelector	Empty service selector
Ready	False	ServiceNotFound	Service Bar not found
Ready	False	EmptyApplicationSelector	Empty application selector
Ready	False	ApplicationNotFound	Application Foo not found

[cdlliuy]

@isutton @DhritiShikhar @sbose78 , can you help to share you comment on this?

[isutton]

I'm still not convinced that coalescing the conditions into a single one is the way to go; this will move us back to a single observation regarding a process that contains at least two independent workflows (1, 2).

1. Support creation of managed secret without associating it to a workload #350
2. Split BindingReady into CollectionReady and InjectionReady #565
3. Generic binding : BindingFail status is reported  #381

[pedjak]

that contains at least two independent workflows

The first workflow is contained into the other one. Creating binding secret without associating it to an app is just a special case - when app does not yet exist. However, from user point of view, service binding is still not performed (i.e. not ready). It becomes ready only when we inject binding into the app. All other states could be reported on the way via condition's reason (and having ready=true). CollectionReady and InjectionReady are not really independent from each-other.

[isutton]

They're not contained into each other, one is an optional post-collection step that might or not be executed.

What I think we're missing is that the .spec contents indicate what the user wants to achieve through the usage of optional fields.

Let's consider the following service binding resources:

spec:
  services: [...]

and

spec:
  application: {...}
  services: [...]

My interpretation is that in the latter, the user is explicitly indicating an application and services--IOW there's the expectation from the user's point of view that the system will do whatever is fine for this configuration--whereas in the former the user has explicitly (intentional or unintentionally) ignored the application field.

I believe my usage of independent was way too vague and for that I apologize: they're not workflows contained one into another, but a workflow of two steps where the second is optional.

The first step is responsible for a) collecting values from resources and b) materializing a secret to be referred later on, and the second (optional because the service binding resource's .spec.application is optional) which is responsible for modifying the application resources accordingly.

CollectionReady	InjectionReady	Ready	Reason
F	F	F	If collection can't be performed, injection can't as well
T	F	F	If collection could be performed, but the given application couldn't be modified, then the service binding can't be considered ready
T	T	T	If collection could be performed and the given application could be modified, then the service binding can be considered ready
T	F	T	If collection could be performed and no application has been informed, then the service binding can be considered ready

The last two rows in the table above indicate that the service binding could be observed ready--as in there's nothing else to do and in the next reconciliation no changes should be made--both with InjectionReady having a False status.

Of course, in the case we're really interested into coalescing both Conditions into a single Ready one, we'd still need to consider the other cases where the observations are False: values can be collected but some other issue could prevent the service binding secret to be created in the first place; applications could not be patched; and likely other reasons which will make understanding different aspects of operations quite hard to distinguish.

[pedjak]

they're not workflows contained one into another, but a workflow of two steps where the second is optional.

First step could be considered as separate sub workflow, this is what I meant by saying "that a workflow is contained in another one".

However, observing this single workflow, we still have just one condition to report: namely a user wants to know if the workflow has completed, i.e. Ready = True. If not true, then through "Reason" we can find out where we got stuck. Even if application is optional, we can encode this as

Ready = true, Reason = EmptyAppSelector

At the given workflow, Reason will give us at what stage we are.

[cdlliuy]

@isutton , in above table, why the row

By looking at the condition table above, it appears to me that they are actually not independent from each other, i.e. some combinations are not possible. Hence, we can simplify the model a lot: we can keep just Ready type and incorporate other two as reasons:

Type Status Reason Message
Ready True AppBound Application Foo bound to service(s)
Ready False EmptyServiceSelector Empty service selector
Ready False ServiceNotFound Service Bar not found
Ready False EmptyApplicationSelector Empty application selector
Ready False ApplicationNotFound Application Foo not found

@pedjak should you update the 4th row of above table to be "Ready True EmptyApplicationSelector" ?

[pedjak]

@cdlliuy @isutton I am withdrawing my proposal - it was focused too much on the current implementation. Let's keep then the existing conditions and introduce Ready. In any case, we do not need to treat Ready as the consequence of other conditions in the system, i.e. we do not need to keep the table. Ready is either true indicating that service binding got performed, or false.

[cdlliuy]

@isutton @DhritiShikhar @pedjak as discussed on the meeting, @qibobo will help to start the effort to add Ready back while still keeping the fine-gained collectionReady and injectionReady.

Just follow the table in #665 (comment).

Igor will help to review and provide support when we make the change :-) Thanks @isutton !

[Avni-Sharma]

I'm still not convinced that coalescing the conditions into a single one is the way to go; this will move us back to a single observation regarding a process that contains at least two independent workflows (1, 2).

1. Support creation of managed secret without associating it to a workload #350
2. Split BindingReady into CollectionReady and InjectionReady #565
3. Generic binding : BindingFail status is reported  #381

I agree with @isutton on this
I'm not convinced too, that coalescing the conditions into a single one is the way to go.

[Avni-Sharma]

I think that having Ready might be Redundant

We can have it also , but is it a necessity-, then perhaps no

[Avni-Sharma]

According to #565 (comment) the UX can depend on InjectReady itself 🤔 Why to have an extra condition and then depend on that. I am a bit confused here 🙈

[Avni-Sharma]

@cdlliuy @isutton I am withdrawing my proposal - it was focused too much on the current implementation. Let's keep then the existing conditions and introduce Ready. In any case, we do not need to treat Ready as the consequence of other conditions in the system, i.e. we do not need to keep the table. Ready is either true indicating that service binding got performed, or false.

@pedjak when you say service binding got performed, does that mean collectionReady True or collectionReafy and InjectionReady true ?

[cdlliuy]

According to #565 (comment) the UX can depend on InjectReady itself 🤔 Why to have an extra condition and then depend on that. I am a bit confused here 🙈

@Avni-Sharma , since the name of InjectReady may not a stable in the future, and then UX need to prepare for breaking changes again and again. Providing a stable Ready is the purpose of this issue.

[Avni-Sharma]

PR merged , closing this issue