make RampingVersionPercentageChangedTime visible
#8089
Conversation
carlydf
left a comment
carlydf
left a comment
There was a problem hiding this comment.
looks good -- does this need a replay test?
| d.State.RoutingConfig.RampingDeploymentVersion = worker_versioning.ExternalWorkerDeploymentVersionFromStringV31(newRampingVersion) | ||
| d.State.RoutingConfig.RampingVersionPercentage = args.Percentage | ||
| d.State.RoutingConfig.RampingVersionChangedTime = rampingVersionUpdateTime | ||
| d.State.RoutingConfig.RampingVersionPercentageChangedTime = routingUpdateTime |
There was a problem hiding this comment.
@dnr - pinging ya since I wanted to confirm my hunch/understanding of what will/won't break NDE;
question: Will adding this extra field to the struct RoutingConfig make our entity workflows NDE?
context: d.State.RoutingConfig is currently being stored as part of the workflow's memo via UpsertMemo calls. In other words, we are storing the state present in the routingConfig as a history event.
My initial assumption is no, this wont' cause an NDE since changing/adding inputs does not cause NDE (unless they literally trigger the addition or removal of history events and this does not)
There was a problem hiding this comment.
afaik, changing the contents of a memo update does not count as NDE.
potential caveat: do you ever read the memo from workflow.GetInfo().Memo for this workflow directly? it looks like no. if you did, things could get confused since the memo you read would be different from the memo you set. but if you only set here then I think it's fine.
There was a problem hiding this comment.
I also double checked some of the SDK code to see if inputs were being compared and don't think that's the case;
do you ever read the memo from workflow.GetInfo().Memo for this workflow directly
nope, the memo is only being used forListRPC calls and we are fetching the information required in those requests from here
the only other caveat I can think about it the fact that this struct, RoutingConfig, is passed as input for every CAN run. I wonder if that is a problem but I don't think so?
|
merging this for now since I am fairly confident this won't cause NDE's in our CICD env where we have these wfs running; can also quickly revert if I do notice things might break if @dnr answers to my replied caveat by saying there is a potential 🔥 |
- Made `RampingVersionPercentageChangedTime` visible to a client (be it the worker-controller or an operator carrying out a `DescribeDeployment` call) from the `RoutingConfig`. - Safe deploys correctness. Also, this is a big bug. I'm very sorry for skipping over this. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) - None, improves the feature. Tested my changes out as well.
What changed?
RampingVersionPercentageChangedTimevisible to a client (be it the worker-controller or an operator carrying out aDescribeDeploymentcall) from theRoutingConfig.Why?
How did you test it?
Potential risks