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

[ansible][docker.py]Replace docker shell with connection plugin #1269

Merged

Conversation

msosyak
Copy link
Contributor

@msosyak msosyak commented Dec 9, 2019

Description of PR

We are using a custom docker shell plugin to run tasks inside the container on the remote hosts. But shell plugins ware not made for this purpose. So I would recommend deprecate docker shell plugin and use native docker connection plugin.

To be able to use docker connection plugin to run tasks inside containers on the remote host we just should to:

  • Have installed docker-ce-cli on ansible host
  • Have generated ssh-key as docker cli is not able to establish ssh connection by user/password

Summary:
Replace docker shell plugin by native ansible docker connection plugin.
Fixes #1268

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • [] Test case(new/improvement)

Approach

Dynamically add needed containers to inventory and use delegate_to to run tasks inside containers.

How did you do it?

How did you verify/test it?

  1. Install docker cli and generate shh-keys:
sudo apt-get update
sudo apt-get install \
    apt-transport-https \
    ca-certificates \
    curl \
    gnupg-agent \
    software-properties-common
curl -fsSL https://download.docker.com/linux/ubuntu/gpg | sudo apt-key add -
sudo apt-key fingerprint 0EBFCD88
sudo add-apt-repository \
   "deb [arch=amd64] https://download.docker.com/linux/ubuntu \
   $(lsb_release -cs) \
   stable"
sudo apt-get update
sudo apt-get install  docker-ce-cli

ssh-keygen -q -t rsa -f $HOME/.ssh/id_rsa   -C "" -N ""
  1. Run ecn_wred CT

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

There are some articles found during the investigation:
https://medium.com/better-programming/docker-tips-access-the-docker-daemon-via-ssh-97cd6b44a53
https://docs.ansible.com/ansible/latest/user_guide/intro_inventory.html#non-ssh-connection-types
https://www.serverlab.ca/tutorials/containers/docker/how-to-access-remote-docker-daemon-using-ssh/
https://docs.ansible.com/ansible/latest/modules/authorized_key_module.html

@msosyak msosyak closed this Dec 9, 2019
@msosyak msosyak reopened this Dec 9, 2019
@msosyak msosyak marked this pull request as ready for review December 9, 2019 16:53
@msosyak msosyak changed the title [ansible][docker.py]Replace docker shell with connection in ecn_wred [ansible][docker.py]Replace docker shell with connection plugin Dec 9, 2019
@lguohan
Copy link
Contributor

lguohan commented Dec 10, 2019

i think this is a more generic approach. @qiluo-msft , what do you think?

@lguohan
Copy link
Contributor

lguohan commented Dec 10, 2019

@msosyak , can you add the docker-ce-cli installation in the sonic-mgmt docker so that we will have docker command in the sonic-mgmt docker. https://github.com/Azure/sonic-buildimage/blob/master/dockers/docker-sonic-mgmt/Dockerfile.j2

@lguohan
Copy link
Contributor

lguohan commented Dec 10, 2019

@msosyak, can you resolve the conflict?

@msosyak msosyak force-pushed the migrate_from_docker_shell_to_connection branch from a8b139b to a327d81 Compare December 10, 2019 17:27
@msosyak
Copy link
Contributor Author

msosyak commented Dec 10, 2019

@lguohan Conflict resolved.
docker-ce-cli added to sonic-mgmt: sonic-net/sonic-buildimage#3868

lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Dec 11, 2019
In the scope of migration from docker shell plugin to docker connection plugin, we need to have docker-ce-cli installed in docker-sonic-mgmt. sonic-net/sonic-mgmt#1269

Added docker-ce-cli package to docker-sonic-mgmt.
authorized_key:
user: "{{ ansible_ssh_user }}"
state: present
key: "{{ lookup('file', lookup('env','HOME') + '/.ssh/id_rsa.pub') }}"
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key [](start = 4, length = 3)

You can get key from previous openssh_keypair return values. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your refinement! I feel public_key is just more straightforward.


In reply to: 356376796 [](ancestors = 356376796)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not choose the name of the public key file.

"Name of the files containing the public and private key. The file containing the public key will have the extension .pub. "
https://docs.ansible.com/ansible/latest/modules/openssh_keypair_module.html#parameters

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there is some miscommunication. I suggest

    key: out.public_key

Could you help explorer this one? Seems it is designed just for this purpose.


In reply to: 356776386 [](ancestors = 356776386,356376796)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, I missed this option. Changed.

with_items:
- "lldp"
- "syncd"

Copy link
Contributor

@qiluo-msft qiluo-msft Dec 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this part into add_container_to_inventory, add with_items for all containers? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can, but I do not see the reason for that. There are only two places where we add more than one container.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean if we move every include_task: add_container_to_inventory into inside that file with all the known container names, we can keep other files clean, only include_tasks once.


In reply to: 356377776 [](ancestors = 356377776)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not use it in all test cases, so I do not see the reason to implicitly add all containers to inventory.

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As comments

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One remaining minor issue: #1269 (review). Otherwise it looks good to me.

@qiluo-msft qiluo-msft merged commit 3e3a14c into sonic-net:master Dec 12, 2019
rlhui pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jul 7, 2020
In the scope of migration from docker shell plugin to docker connection plugin, we need to have docker-ce-cli installed in docker-sonic-mgmt. sonic-net/sonic-mgmt#1269

Added docker-ce-cli package to docker-sonic-mgmt.
praveen-li pushed a commit to praveen-li/sonic-buildimage that referenced this pull request May 28, 2021
…-net#3868)

In the scope of migration from docker shell plugin to docker connection plugin, we need to have docker-ce-cli installed in docker-sonic-mgmt. sonic-net/sonic-mgmt#1269

Added docker-ce-cli package to docker-sonic-mgmt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker shell module failed in the latest sonic-mgmt for some tasks
3 participants