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

Expand LXD driver functionality #1436

Merged
merged 14 commits into from Sep 15, 2018
Merged

Expand LXD driver functionality #1436

merged 14 commits into from Sep 15, 2018

Conversation

wilmardo
Copy link
Contributor

@wilmardo wilmardo commented Aug 13, 2018

As the title states I expanded the LXD driver. The default Docker driver doesn't cut it for me because of the missing systemd. Took a try with gdraheim/docker-systemctl-replacement but it comes short sometimes, not suitable for stable testing.

What has changed:

  • Mainly more mappings of the lxd_container module to allow more flexible usage
  • The lxd_container option profiles defaults to the default profile when not provided(source) therefore now standard omitted
  • The prepare playbook needed work since Python is not always installed in LXC containers. Also installation of some basic packages (based on the Dockerfile in the repo). Which for Gentoo took some bash magic for the Ansible required gentoolkit.
  • Moved the create, destroy and prepare playbooks to the provisioner/ansible/playbooks directory
  • Fixed a little unrelated typo in molecule init template docs

Testing done:

  • Ran tox -e py27-ansible26-functional -- -k lxd -v which passed successfully
  • Created a test repo and did a travis build against all the aliases on the official LXC repo see the travis build. Only UbuntuCore and Arch failed. UbuntuCore is special so that is fine, Arch fails because of this issue.

Two points for discussion:

  • Do I need to update the LXD driver tests to reflect these new changes? I thought about it but in my opinion it doesn't add much since it only implements the lxd_container module
  • The lxd_container option source is a dict in the lxd_container module but not in molecule. I thought about changing this but it would break backwards compatibility since alias moves into this dict

@@ -1,11 +0,0 @@
---
Copy link
Contributor

@retr0h retr0h Aug 14, 2018

Choose a reason for hiding this comment

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

Why was this removed? Seemed like a good file to help users bootstrap the default image lxd was using. IIRC python didn't ship with that image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is replaced by the expanded molecule/provisioner/ansible/playbooks/lxd/prepare.yml as seen a bit further below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see I had missed that. The prepare you created is similar to our Dockerfile setup where we ensure the docker container has the necessary dependencies. 👍

server: https://images.linuxcontainers.org
alias: ubuntu/xenial/amd64
profiles:
- default
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you have implemented this API for molecule.yml we need to validate the schema. Can you update the v2 schema similar to what we did with dockder and vagrant.

https://github.com/metacloud/molecule/blob/master/molecule/model/schema_v2.py#L596
https://github.com/metacloud/molecule/blob/master/molecule/model/schema_v2.py#L862

And we will need to update tests with working and failing tests:

https://github.com/metacloud/molecule/blob/master/test/unit/model/v2/test_platforms_section.py

@wilmardo
Copy link
Contributor Author

wilmardo commented Aug 15, 2018

Do I need to update the LXD driver tests to reflect these new changes? I thought about it but in my opinion it doesn't add much since it only implements the lxd_container module

Looked into this with help of your comment, I think I implemented it all. Most if it was similar to the vagrant|docker implementation, only the devices argument which has a nested dict where the keys can differ was difficult. See the lxd API docs and the lxd devices docs

The lxd_container option source is a dict in the lxd_container module but not in molecule. I thought about changing this but it would break backwards compatibility since alias moves into this dict

Had some inspiration and moved the source options to a dict with some help of Jinja2. This is the same as the lxd_container module and keeps things more easy for the end user. The part about breaking backward compatibility was not true since there was no variables before (used a custom create.yml myself, that got me confused).

@retr0h Ready for review again!

},
'protocol': {
'type': 'string',
'allowed': [
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 made the schema as specific as possible, these are the only values valid for the LXD api. There are now allowed values (source).

},
'architecture': {
'type': 'string',
'allowed': [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here (source)

'i686',
],
},
'devices': {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please take a good look at the devices schema, that is the one I struggled with since I am not familiar with the markup. Might be not 100% correct

@wilmardo
Copy link
Contributor Author

Is there a reason why there is a test/resources/playbooks/lxd? Couldn't we just point to the molecule/provisioner/ansible/playbooks/lxd in molecule.yml for testing? This would dedup these playbooks.

@wilmardo
Copy link
Contributor Author

@retr0h Do you know when you have time to review the latest changes? In no means I want to rush you, just curious if everything is alright now :)

@retr0h
Copy link
Contributor

retr0h commented Aug 31, 2018

@wilmardo the model unit tests are failing.

@retr0h
Copy link
Contributor

retr0h commented Aug 31, 2018

Also, run tox -e format to reformat the code with yapf.

@retr0h
Copy link
Contributor

retr0h commented Sep 5, 2018

@wilmardo v2 model unit tests still failing

test/unit/model/v2/test_platforms_section.py::test_platforms_lxd[_model_platforms_lxd_section_...] FAILED                                       [ 62%]
test/unit/model/v2/test_platforms_section.py::test_platforms_lxd_has_errors[_model_platforms_docker_error...] FAILED                            [ 64%]

@wilmardo
Copy link
Contributor Author

wilmardo commented Sep 5, 2018

@retr0h Yeah sadly I know, I struggled with it yesterday but there seems to be an issue on my own laptop since the master branch is failing too. Hopefully this evening I have time to retry on my desktop on which I know it worked before.
Any tips on debugging the tox errors? They seem very non descriptive to me (first time using tox).
Also are there any useful tox commands I (or any other contributor) need to know? I might expand the Testing section of the docs some more, for example only yesterday I found out the schema is Cerberus. Also the LXD test commands are not longer valid (v1 legacy?).

@retr0h
Copy link
Contributor

retr0h commented Sep 5, 2018

I suggest just running py.test.

$ py.test test/unit/model/v2/ -vvv

You can then see which test failed.

@wilmardo
Copy link
Contributor Author

wilmardo commented Sep 5, 2018

Fixed the unit tests! Pytest helped a lot, I will try to create a PR for some more documentation about testing.
The most of the issue was that in test_platforms_lxd_has_errors the items in the source variable are alphabetically sorted and source itself becomes a dict in a list for some reason.

Thanks for the pointers!

decentral1se added a commit to decentral1se/molecule that referenced this pull request Sep 9, 2018
As a result of direction in the following comment:

> ansible#1458 (comment)

And the good work and lead of ansible#1436 in:

> ansible#1436
@retr0h
Copy link
Contributor

retr0h commented Sep 15, 2018

Merging. @wilmardo is our resident LXD driver community member.

@retr0h retr0h merged commit 6fabd37 into ansible:master Sep 15, 2018
@wilmardo wilmardo deleted the lxd branch September 17, 2018 11:34
@wilmardo wilmardo mentioned this pull request Oct 3, 2018
@ssbarnea ssbarnea added this to the 2.18 milestone Apr 2, 2019
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.

None yet

3 participants