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

add docker proxy minion #47716

Merged
merged 7 commits into from Jun 6, 2018

Conversation

Projects
None yet
4 participants
@gtmanfred
Copy link
Contributor

commented May 18, 2018

What does this PR do?

This proxy minion + executor combo allows for using the docker.call module to run salt modules inside of docker containers.

This also adds the ability to run a highstate on the docker container.

Tests written?

Yes

Commits signed with GPG?

Yes

@gtmanfred gtmanfred requested a review from saltstack/team-core as a code owner May 18, 2018

@gtmanfred gtmanfred force-pushed the gtmanfred:docker branch from 0c58d28 to 6f5f826 May 18, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse May 18, 2018

@isbm

isbm approved these changes May 18, 2018

Apply states! This function will call highstate or state.sls based on the
arguments passed in, state.apply is intended to be the main gateway for
all state executions.

This comment has been minimized.

Copy link
@terminalmage

terminalmage May 18, 2018

Contributor

Should the wording here be modified to reference the docker module instead of state?

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred May 18, 2018

Author Contributor

I am just going to remove the word state because this will actually be referenced as state.apply when called through the docker proxy minion.

This comment has been minimized.

Copy link
@terminalmage

terminalmage May 18, 2018

Contributor

OK, cool 👍

@gtmanfred gtmanfred force-pushed the gtmanfred:docker branch from 6f5f826 to 965f12c May 18, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented May 18, 2018

@gtmanfred Can you write some tests for this, please?

EDIT: Oh and this should be added to the fluorine release notes, too.

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

Will do, yeah i need to write at least a test for the highstate, and then will see what I can do about adding a test for the proxy minion tests.

@gtmanfred gtmanfred force-pushed the gtmanfred:docker branch from 965f12c to 1ef021c May 18, 2018

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

re-run py

@gtmanfred gtmanfred force-pushed the gtmanfred:docker branch 4 times, most recently from e35c4c8 to 4b2c73b May 18, 2018

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

Tests should be good now.

@gtmanfred gtmanfred force-pushed the gtmanfred:docker branch from 9769647 to 17e97f2 May 20, 2018

@rallytime rallytime requested a review from terminalmage May 22, 2018

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

re-run py

@rallytime

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

@gtmanfred Looks like some dockermod tests are failing on this:

  • integration.modules.test_dockermod.DockerCallTestCase.test_docker_call
  • integration.modules.test_dockermod.DockerCallTestCase.test_docker_highstate
  • integration.modules.test_dockermod.DockerCallTestCase.test_docker_sls

Can you take a look?

Might be worth rebasing as well - there are some old test failures on this PR that have since been fixed on the branch.

EDIT: Those tests look to be failing on PY3 only

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2018

re-run py

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2018

@gtmanfred Those 3 tests are still failing after the latest updates.

gtmanfred added some commits Dec 8, 2017

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2018

FInally was able to replicate this.

Looking now.

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2018

Aha! i know exactly the problem.

@gtmanfred gtmanfred force-pushed the gtmanfred:docker branch from 86e98b7 to 5fe51d2 Jun 4, 2018

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2018

Ok, this has been failing for a while.

It was just not caught, because self.assertTrue was seeing the return of test.ping as true, even though it was just returning a string about not being able to import yaml.

It looks like it is being caused by the changes that were made to making the thing tarball for salt-ssh, by @isbm

       ('ERROR: Traceback (most recent call last):\n'
        '  File "/tmp/salt.docker.2694e1/salt-call", line 27, in <module>\n'
        '    salt_call()\n'
        '  File "/tmp/salt.docker.2694e1/pyall/salt/scripts.py", line 395, in '
        'salt_call\n'
        '    import salt.cli.call\n'
        '  File "/tmp/salt.docker.2694e1/pyall/salt/cli/call.py", line 5, in '
        '<module>\n'
        '    import salt.utils.parsers\n'
        '  File "/tmp/salt.docker.2694e1/pyall/salt/utils/parsers.py", line 27, in '
        '<module>\n'
        '    import salt.config as config\n'
        '  File "/tmp/salt.docker.2694e1/pyall/salt/config/__init__.py", line 24, '
        'in <module>\n'
        '    import salt.utils.data\n'
        '  File "/tmp/salt.docker.2694e1/pyall/salt/utils/data.py", line 17, in '
        '<module>\n'
        '    import salt.utils.dictupdate\n'
        '  File "/tmp/salt.docker.2694e1/pyall/salt/utils/dictupdate.py", line 15, '
        'in <module>\n'
        '    from salt.serializers.yamlex import merge_recursive as '
        '_yamlex_merge_recursive\n'
        '  File "/tmp/salt.docker.2694e1/pyall/salt/serializers/yamlex.py", line '
        '118, in <module>\n'
        '    import yaml\n'
        'ImportError: No module named yaml')

Bo, do you have time to look at this error? I am not sure what the changes were made for using pyall and why it isn't finding the yaml import on python3 versions using docker.call.

Thanks,
Daniel

@gtmanfred gtmanfred force-pushed the gtmanfred:docker branch from 5fe51d2 to b12f807 Jun 4, 2018

@isbm

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

@gtmanfred oh, interesting. Any hints how to reproduce this? Definitely I am looking into this immediately.

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2018

@isbm if you run it with the py3 tests, it just fails, i only had to install the test suite and docker + docker python module.

if you want an automated way, setup kitchen and run bundle exec kitchen test py3-centos-7, but that is probably over kill.

https://kitchen.saltstack.com/docs/file/docs/gettingstarted.md

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2018

re-run py3

@rallytime rallytime merged commit d5954e0 into saltstack:develop Jun 6, 2018

4 of 10 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5511 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10483 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19560 — ABORTED
Details
codeclimate 3 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23437 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25709 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17770 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22401 — SUCCESS
Details

@gtmanfred gtmanfred deleted the gtmanfred:docker branch Jun 6, 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.