Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix out of order docker Env comparison #50272

Merged
merged 6 commits into from Nov 7, 2018

Conversation

Projects
None yet
4 participants
@jyurdal
Copy link
Contributor

commented Oct 29, 2018

What does this PR do?

I have experienced bug on windows only minions, regarding docker_container.running, where container is replaced on each state.apply whether it has changes or not.

If there are many "environment" variables specified, the environment list is having random order each time state is applied. As a result existing container and the temporarily created container are seen to have different environment variables ( although only order is different) and docker container is replaced each time salt is doing state.apply.

Argument reorder happens only on windows and it happens in salt/utils/docker/init.py function translate_input) . (Maybe it would make sense to see why it happens in translate_input. As for my system I just compare sorted list instead.) It has been introduced between 2017.7 and 2018.3.0

Previous Behavior

docker container placed on each state.apply despite no configuration changes.

New Behavior

docker container modified only on changes.

Tests written?

No

Commits signed with GPG?

No

Joanna Yurdal and others added some commits Oct 29, 2018

Joanna Yurdal
@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Oct 31, 2018

Joanna Yurdal and others added some commits Oct 31, 2018

@cachedout cachedout requested a review from terminalmage Nov 1, 2018

terminalmage added some commits Nov 6, 2018

Remove unnecessary u prefix from string literals
These are redundant with unicode_literals being used
@terminalmage
Copy link
Contributor

left a comment

First of all, thanks for the pull request! This is a good catch.

I would however like to clear some things up. We're not actually changing any order when we translate input. We are placing the env vars into a dictionary to be passed to docker-py for container creation. Under the hood, it is likely dictionary iteration order which decides which order the environment variables are set, because the container's metadata contains the env vars as a list of KEY=value pairs.

I took the liberty of adding a unit test for this and pushed it to this PR.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

@rallytime this will need a backport to 2018.3.

@cachedout cachedout merged commit e54e09a into saltstack:develop Nov 7, 2018

7 of 11 checks passed

jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has failed
Details
continuous-integration/jenkins/pr-merge This commit is being built
Details
jenkins/pr/py2-ubuntu-1604 running py2-ubuntu-1604...
Details
WIP Ready for review
Details
codeclimate Approved by Mike Place.
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details

@jyurdal jyurdal deleted the jyurdal:fix-docker-env-order/develop branch Nov 7, 2018

rallytime added a commit that referenced this pull request Nov 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.