[Local] Allow for graceful termination #3823
Conversation
pkg/platform/local/platform.go
Outdated
| "functionName", createFunctionOptions.FunctionConfig.Meta.Name, | ||
| "containerID", container.ID) | ||
| if err := p.dockerClient.StopContainer(container.ID); err != nil { | ||
| if err := p.dockerClient.StopContainer(container.ID, int(p.GetConfig().Local.FunctionContainersGracefulTerminationTimeout.Seconds())); err != nil { |
There was a problem hiding this comment.
I suggest to add a handler function- p.GetConfig().GetFunctionContainersGracefulTerminationTimeout (or shorter name) to verifies backward compitibilty if this value is missing, like this for example
There was a problem hiding this comment.
@weilerN I believe those are different scenarios. I don't see any BC issues here, we always enrich if it's empty in enrich platform step.
pkg/platform/local/platform.go
Outdated
| if err := p.dockerClient.StopContainer(container.ID, int(p.GetConfig().Local.FunctionContainersGracefulTerminationTimeout.Seconds())); err != nil { | ||
| return 0, errors.Wrap(err, "Failed to stop a container") |
There was a problem hiding this comment.
If 1 container fails but there are still others left, we'll skip them don't try to stop them.
I know that was the previous implementation, but maybe we can continue before failing
There was a problem hiding this comment.
agree, will change that
…o graceful-termnination-local
…lways delete a function afterwards
pkg/platform/local/platform.go
Outdated
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() |
| if errPtr := errDuringStop.Load(); errPtr != nil { | ||
| return *errPtr | ||
| } | ||
| return nil |
There was a problem hiding this comment.
| return nil | |
| p.Logger.InfoWithCtx(ctx, "Successfully deleted function", | |
| "name", functionName) | |
| return nil |
| // no need to fail the entire operation if stopping failed | ||
| // we will retry to stop with SIGKILL below | ||
| if err = p.dockerClient.StopContainer(containerInfo.ID, int(p.GetConfig().Local.FunctionContainersGracefulTerminationTimeout.Seconds())); err != nil { | ||
| errDuringStop.CompareAndSwap(nil, &err) |
There was a problem hiding this comment.
If multiple containers fail to stop, only the first error is returned. I’d suggest collecting all errors instead of keeping just the first one.
There was a problem hiding this comment.
I think it's fine to return only the first one as we anyway logged the others
📝 Description
Bug was found during writing tests in #3821
We did
docker rm -finstead ofdocker stop, which didn’t allow to run termination callback properlyDocker stop docs: docker container stop
docker rm docs: docker container rm
✅ Checklist
🧪 Testing
tested as part of #3821
🔗 References
🚨 Breaking Changes?