states/docker_container: pass temp_container_name, not the dict, to _replace#68968
Open
SAY-5 wants to merge 1 commit intosaltstack:3007.xfrom
Open
states/docker_container: pass temp_container_name, not the dict, to _replace#68968SAY-5 wants to merge 1 commit intosaltstack:3007.xfrom
SAY-5 wants to merge 1 commit intosaltstack:3007.xfrom
Conversation
…replace running() builds a temp_container dict from docker.create and derives temp_container_name from it. The detected-config-diff path already calls _replace(name, temp_container_name). The skip_comparison / force=True path (reached via mod_watch and when the caller forces a re-create) passed the dict itself instead. _replace immediately calls docker.rm(name) on the original container and then docker.rename(new, orig). docker.rename forwards its first argument to inspect_container, which expects a string name/ID - handing it a dict raises, after the original container has already been removed. The state returns result: False but the minion is left with the original container destroyed and the temp container stranded under its generated name (saltstack#68959). Pass temp_container_name on this call site too so both replace paths are consistent. Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com> Fixes saltstack#68959
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes a wrong-argument bug in
salt/states/docker_container.py'srunning()state.running()creates a temp container fromdocker.createand captures two related values:temp_container— the full dict returned bydocker.createtemp_container_name—temp_container["Name"], a stringThe detected-config-diff path correctly passes the string to the local
_replace(orig, new)helper:The
skip_comparison/force=Truepath (also reached frommod_watch) passed the dict instead:_replaceimmediately callsdocker.rm(name, stop=True)on the original container and thendocker.rename(new, orig).docker.rename's first argument is forwarded toinspect_container, which expects a string name/ID. Handing it a dict raises aCommandExecutionError— and by then the original container has already been removed. The state returnsresult: False, but the minion is left with the original container destroyed and the temp container stranded under its generated temp name.This PR passes
temp_container_nameon the second call site too, matching the first path.What issues does this PR fix or reference?
Fixes #68959
Previous Behavior
docker_container.runningwithforce=True(or triggered viamod_watch) on an existing container whose spec matches the desired spec would:docker.rm.docker.renameraises on the dict argument).result: Falsewith the original container destroyed and the temp container present but renamed to something likeSalt_Temp_XXXX.New Behavior
_replace(name, temp_container_name)is called with the string container name on both paths. The force-replace path now correctly swaps the temp container into place, matching the non-force path's behaviour.Merge requirements satisfied?
changelog/68959.fixed.md_replace(name, temp_container_name)call site at line ~1890. Happy to add a targeted regression test againstdocker.renameif maintainers prefer.Commits signed with GPG?
Commits are DCO-signed off (
Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>) per the repository's contribution flow.