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

Deprecate relationship annotations #596

Merged

Conversation

isutton
Copy link
Contributor

@isutton isutton commented Aug 3, 2020

This PR deprecates the usage of annotations to relate Service Binding Requests and its services and applications in favor of a programmatic lookup.

Closes #458

Motivation

The main reason for this change is to reduce the amount of writes in the API server during the SBR reconciliation: those writes can trigger other reconciliation processes leading to unexpected race conditions; additionally, those annotations also seem to not be necessary to find a direct relationship between a Service Binding Request and a related resource, being it a service, application or the intermediate secret as changes in this PR can show.

Changes

This PR does not stamp the SBR information to related resources using annotations during the reconciliation process, and performs a lookup up to identify the SBR associated with an incoming change, if any.

Testing

Unit tests have been modified to a test table, and no e2e tests have been changed.

@openshift-ci-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@isutton
Copy link
Contributor Author

isutton commented Aug 3, 2020

/test all

@isutton
Copy link
Contributor Author

isutton commented Aug 3, 2020

/test all

@isutton isutton marked this pull request as ready for review August 4, 2020 13:33
@isutton
Copy link
Contributor Author

isutton commented Aug 4, 2020

/test all

@isutton
Copy link
Contributor Author

isutton commented Aug 4, 2020

/retest

@isutton
Copy link
Contributor Author

isutton commented Aug 5, 2020

/retest

@isutton
Copy link
Contributor Author

isutton commented Aug 6, 2020

/retest

1 similar comment
@isutton
Copy link
Contributor Author

isutton commented Aug 6, 2020

/retest

@DhritiShikhar
Copy link
Contributor

DhritiShikhar commented Aug 7, 2020

additionally, those annotations also seem to not be necessary to find a direct relationship between a Service Binding Request and a related resource, being it a service, application or the intermediate secret as changes in this PR can show.

Hmm.. To conclude, there are no side effects of not stamping annotations to SBR's related resource.

There is only unidirectional tracing now. We can trace relationship from SBR to application but not from application to SBR.

@DhritiShikhar
Copy link
Contributor

The changes seem good to be merged.

Waiting for Igor to address this -> #596 (comment)

@DhritiShikhar
Copy link
Contributor

/lgtm
/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DhritiShikhar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@isutton
Copy link
Contributor Author

isutton commented Aug 7, 2020

additionally, those annotations also seem to not be necessary to find a direct relationship between a Service Binding Request and a related resource, being it a service, application or the intermediate secret as changes in this PR can show.

Hmm.. To conclude, there are no side effects of not stamping annotations to SBR's related resource.

Not that I'm aware of... Perhaps the only one I can think of with is related to "nested" bindings, where for example a SBR might include a CR storing relevant data in a Secret: the current implementation won't match this Secret with a SBR but it can definitely be expanded to match resources related to the SBR at hand, in addition of checking whether the resource might appear as an application, service and intermediate/intermediary secret.

There is only unidirectional tracing now. We can trace relationship from SBR to application but not from application to SBR.

Yes, and to be more explicit: you do like you would with a database: filter the SBRs that have declared an arbitrary resource as either application, service and/or intermediate/intermediary secret.

@Avni-Sharma
Copy link
Contributor

/test 4.5-e2e

@Avni-Sharma
Copy link
Contributor

/test 4.5-acceptance

@pratikjagrut
Copy link
Contributor

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only the last SBR is recorded in backingservice annotation.
6 participants