Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Allow retries for instances with Failed condition after spec changes #1751

Merged
merged 2 commits into from
Mar 6, 2018
Merged

Allow retries for instances with Failed condition after spec changes #1751

merged 2 commits into from
Mar 6, 2018

Conversation

nilebox
Copy link
Contributor

@nilebox nilebox commented Feb 21, 2018

Implementation of item number 2. from the checklist #1715 (comment)

After this change we should be able to retry after receiving 400 Bad Request from broker as soon as we change the spec.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 21, 2018
@@ -110,7 +110,7 @@ func withConfigGetFreshApiserverAndClient(

if err := server.RunServer(options, stopCh); err != nil {
close(serverFailed)
t.Fatalf("Error in bringing up the server: %v", err)
t.Logf("Error in bringing up the server: %v", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see golang/go#15976
The integration test was failing with data race

Copy link
Contributor

@kibbles-n-bytes kibbles-n-bytes left a comment

Choose a reason for hiding this comment

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

We should also remove the Failed condition when we're starting the new operation.

@staebler
Copy link
Contributor

This needs to wait on #1748, right? Otherwise, if the user tries to update an instance that failed to provision, then the controller will attempt to send an update request to the broker, instead of a provision request.

@nilebox
Copy link
Contributor Author

nilebox commented Feb 28, 2018

We should also remove the Failed condition when we're starting the new operation.

@kibbles-n-bytes yes, you're right, I do this in #1748

@nilebox
Copy link
Contributor Author

nilebox commented Feb 28, 2018

@staebler yes, I think both this PR and #1765 depend on #1748.
Will add a do-not-merge label.

@nilebox
Copy link
Contributor Author

nilebox commented Mar 5, 2018

The only issue with allowing retries for instances with Failed:True after updating the spec is that if we exhausted retries for orphan mitigation, it will lead to provisioning without retrying the orphan mitigation first.

See #1771 (comment)

@n3wscott
Copy link
Contributor

n3wscott commented Mar 6, 2018

LGTM

@n3wscott n3wscott added the LGTM1 label Mar 6, 2018
@staebler staebler added the LGTM2 label Mar 6, 2018
@staebler staebler merged commit bddb9a7 into kubernetes-retired:master Mar 6, 2018
@nilebox nilebox deleted the retry-after-spec-updated branch March 6, 2018 22:09
jeremyrickard pushed a commit to jeremyrickard/service-catalog that referenced this pull request Mar 13, 2018
…ubernetes-retired#1751)

* Allow retries for instances with Failed condition after spec changes

* Remove Fatalf from server startup goroutine to fix data race
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants