Handle InProgressProperties for retries properly #1903
Conversation
k8s-ci-robot
added
size/L
| var planExternalID string | ||
| var userInfo *v1beta1.UserInfo | ||
|
|
||
| if instance.Status.CurrentOperation == "" { |
There was a problem hiding this comment.
@kibbles-n-bytes after making this change, I feel like it would be better to always have a CurrentOperation set before preparing an OSB request. In other words, instead of explicitly mitigating a potential issue with retries here, we just reorder logic in controller:
- First record the intent in the status
- Then use the recorded status as a source for OSB requests.
What do you think?
There was a problem hiding this comment.
Making this change is hard though, given that inProgressProperties value depends on event type (Add/Update/Delete). We could probably merge this quick fix as it is, and discuss the follow-up refactoring.
|
/retest |
| if instance.Status.InProgressProperties == nil { | ||
| return nil, fmt.Errorf("InProgressProperties must not be nil if the CurrentOperation is set") | ||
| } | ||
| specParameters = instance.Status.InProgressProperties.InlineParameters |
There was a problem hiding this comment.
This field will be empty if the operation has started prior to upgrading to the version of Service Catalog with InlineParameters field.
Could check for nil there and copy value from the spec if it's not set, but then it could break a fix: what if parameters were empty when we started operation, and were set afterwards?
nilebox
changed the title
|
@nilebox can you elaborate on why you need the inlinedParameters? And, why the same info could not be retrieved from the Parameters if we skip the redacted ones? |
|
@duglin I need
How do you skip them? Ignore strings containing |
|
I'm confused, I thought Status.ExternalProperties were for that purpose. |
|
@duglin the way svc-cat currently works is:
The problem is that if you fail after the step 1, you have recorded This PR fixes that - once you recorded |
| @@ -710,6 +710,11 @@ type ServiceInstancePropertiesState struct { | |||
| // a secret, its value will be "<redacted>" in this blob. | |||
There was a problem hiding this comment.
Should we clarify that this is only secrets now?
| @@ -1002,7 +1002,9 @@ func (c *controller) prepareBindRequest( | |||
| } | |||
| } | |||
|
|
|||
| parameters, parametersChecksum, rawParametersWithRedaction, err := prepareInProgressPropertyParameters( | |||
| // TODO nilebox: If the current operation is already set, we need to ignore the spec and read parameters from the status | |||
There was a problem hiding this comment.
Are you going to handle this TODO here?
| @@ -35,18 +35,19 @@ import ( | |||
| // The second return value is a map of parameters with secret values redacted, | |||
| // replaced with "<redacted>". | |||
| // The third return value is any error that caused the function to fail. | |||
There was a problem hiding this comment.
Can you update the godoc here
| @@ -154,34 +156,47 @@ func generateChecksumOfParameters(params map[string]interface{}) (string, error) | |||
| // 2 - a checksum for the map of parameters. This checksum is used to determine if parameters have changed. | |||
| if instance.Status.InProgressProperties == nil { | ||
| return nil, fmt.Errorf("InProgressProperties must not be nil if the CurrentOperation is set") | ||
| } | ||
| specParameters = instance.Status.InProgressProperties.InlineParameters |
|
The alternative fix is to always read parameters from the spec, but also update Then, to fix the update conflict issue @kibbles-n-bytes wrote about, we could do This seems like an easier fix that doesn't require adding |
|
Closing, as #1916 seems to be simpler and good enough. |
Addressing the issue blocking unlocking (no pun intended) discussed in #1872 (comment)