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 Molecule to Docker role #5129

Merged
merged 2 commits into from
Apr 16, 2020

Conversation

aharrisson
Copy link
Contributor

@aharrisson aharrisson commented Aug 28, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This adds Molecule configuration for one role:

  • container-runtime/docker

Adding Molecule would simplify the testability of the roles individually, which could potentially make the verification of changes less time-consuming.
https://molecule.readthedocs.io/en/stable/

The functionality can easily be extended to cover more roles.

The Molecule configuration uses delegated driver configured for kubevirt and could be run both locally using a nodeport or through CI.
The commit is non-intrusive and only adds Molecule files that does not affect the actual role implementation.

Which issue(s) this PR fixes:

Special notes for your reviewer:
The initial PR contained Molecule for multiple roles and multiple scenarios for each role. I have slimmed it down to just one role and added CI support. The two tip commits are just there for test purposes and removes all CI jobs except the molecule test.

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot
Copy link
Contributor

Welcome @aharrisson!

It looks like this is your first PR to kubernetes-sigs/kubespray 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubespray has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 28, 2019
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 28, 2019
@aharrisson
Copy link
Contributor Author

/assign @riverzhang

@riverzhang
Copy link
Contributor

@aharrisson Can you add some documentation? It's hard to understand how it works.

@riverzhang
Copy link
Contributor

/assign @mattymo @woopstar

@mattymo
Copy link
Contributor

mattymo commented Sep 17, 2019

I don't see much value with this PR as it is. The way it's written, there are 60 new nearly identical molecule.yml files. That's just outrageous and hard to maintain. If they're identical, they should just be located in a central place, such as tests/molecule. What would be even better is to template them out with the role/OS and run them in a systematic fashion. Storing multiple copies of the same file is going to make any future maintainer of this code quite unhappy.

Second, if you want to add such test capability, can you enable this with the docker molecule provider and actually add these tests to a job in our gitlab CI pipeline? We don't have a convenient route to run virtualbox, so the currently provider config is not very useful.

@mattymo
Copy link
Contributor

mattymo commented Sep 17, 2019

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2019
@aharrisson
Copy link
Contributor Author

Thank you for your comments.

The code (or rather configuration) duplication in this PR is intentional, though of course arguable. Molecule is meant to run tests on roles individually, thus storing them centrally would go against that goal.

Regarding the provider configuration, I fully agree that it should conform to the CI setup. I am unfamiliar with your CI environment and also not entirely sure how well Kubernetes would run on Docker.

My point with this PR was to emphasize that treating the roles as individual components rather than parts of a monolith would be beneficial in many aspects. Also, running Molecule on the roles shows that there are some problems that could need some attention.

@MarkusTeufelberger
Copy link
Contributor

I wrote molecule tests for the bootstrap-os role a while ago... the thing I liked was that it makes it relatively easy to actually test all supported distributions, but it took quite some effort and will take even more effort to get onto kubevirt (which the CI system of kubespray is using) instead of virtualbox controlled by vagrant (which molecule is using by default).

There can be a middle ground between having tests for each role and duplicating files, but in general only very few roles in kubespray are standalone (or even designed to be used standalone) to begin with.

@aharrisson
Copy link
Contributor Author

Thank you for the comment, @MarkusTeufelberger. We discussed your molecule tests for bootstrap-os on slack a while back.
The fact that very few roles are standalone should not be a reason against Molecule IMO. Rather the other way around.

My reasons for submitting this PR is not to necessarily start to use Molecule (though I think the project would benefit from it). It is rather to solve issues that Molecule finds. I started configuring Molecule for my own sake, partly to get my bearings in Kubespray and understand the flow of the play(s), but also to catch issues that I could submit fixes for. But just trying to run Molecule on a few roles made me realize that there are not only minor lint issues, but also more fundamental issues with dependencies and idempotency.

I'd be happy to discuss (on slack?) how we could incorporate Molecule in the project to fit the CI environment and to benefit the development of the project.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 4, 2019
.gitlab-ci/molecule.yml Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated
- .gitlab-ci/terraform.yml
- .gitlab-ci/packet.yml
- .gitlab-ci/molecule.yml
# - .gitlab-ci/lint.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to reviewers: this (and following 4 lines) have to be removed before merging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for highlighting that. I edited the PR description and added information that the two tip commits are for testing purposes only. They should not be merged.

.gitlab-ci/molecule.yml Outdated Show resolved Hide resolved
@Miouge1
Copy link
Contributor

Miouge1 commented Dec 12, 2019

I love the idea, of using molecule. I'm happy to get help out to get things moving to get that in 2.13.

If you need, reach out on Kubernetes' Slack #kubespray-dev

Btw, you need to rebase on latest master to resolve merge conflicts.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2019
@Miouge1
Copy link
Contributor

Miouge1 commented Mar 27, 2020

I've been doing some tests with Ubuntu 20.04 (#5835) and @champtar on Centos 8 (#5842), it looks like Ansible 2.8 might become a requirement for those newer OSs and that would also open the door to this PR.

@andersharrisson-ess
Copy link

@Miouge1 , Thank you for the update.
I am not sure if everyone is in agreement regarding the Molecule driver to use? This PR runs the tests on kubevirt, which in this implementation requires Ansible 2.8. If we prefer Vagrant, that would not be an issue, but then we would have to reconsider the CI setup.

@Miouge1
Copy link
Contributor

Miouge1 commented Mar 27, 2020

@andersharrisson-ess, indeed I would have preferred to go with the Vagrant driver to make it possible to run locally, but I don't have the bandwidth to make that happen, so I'm OK with the kubevirt option.

Moreover some roles might not need a full blown VM, and could do nested with the docker driver (adduser, bootstrap-os, bastion, ...).

I think we need to drop the "only changes" part since that works only with GitLab merge request from what I understand in the doc.

I am happy to get this moving, I think we need to start experimenting with molecule and iterate.

@Miouge1
Copy link
Contributor

Miouge1 commented Mar 27, 2020

To give more info the Vagrant stuff... I did a PoC to run Vagrant in Docker with auto scaling GitLab runners provisioned through docker-machine with the Packet driver.

The thing is that docker-machine is not deprecated but not actively maintained either (AFAIK), therefore I'm not convinced that it's the right way to go at this stage, and back to square one on Vagrant CI.

@MarkusTeufelberger
Copy link
Contributor

For bootstrap-os at least there are lots of distros that don't offer base images which look like a typical installation of that distro, so VMs is nearly the only way to test them.

@Miouge1
Copy link
Contributor

Miouge1 commented Mar 27, 2020

@MarkusTeufelberger yes that is definitely a problem in os-bootstrap. I've created a PR with a PoC to use Vagrant in auto scaler mode (as described above) in #5845. If docker-machine ever gets deprecated we can maybe do a static server instead.

@Miouge1
Copy link
Contributor

Miouge1 commented Apr 2, 2020

Now that we have #5845 is merged, I think we can move forward with this PR with a few changes:

  • Switch to vagrant+libvirt driver
  • Drop the kubevirt driver for molecule

Any objections?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2020
@aharrisson
Copy link
Contributor Author

Thank you, @Miouge1
I am a bit preoccupied atm, but will try to fix this PR accordingly next week.
Do we have any plan to fix the issue with only: changes: in the pipeline? i.e. running CI on the same place as the SCM (github vs gitlab)?

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 6, 2020
@aharrisson
Copy link
Contributor Author

aharrisson commented Apr 7, 2020

Refactored to use Vagrant (again). Aligned with the implementation in #5845

Possibly prepare.yml could be placed in a generic location to be reused by some other roles: However, since Kubespray doesn't use role dependencies correctly, we would not be able to reuse it for all roles. I added the preparation in the role, since that would make it more explicit what steps are needed for that individual role.

@aharrisson aharrisson changed the title Add Molecule to roles Add Molecule to Docker role Apr 7, 2020
@Miouge1
Copy link
Contributor

Miouge1 commented Apr 7, 2020

I have tested this locally, it works nicely. Also the pipeline includes the tests, so I'm OK with this.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aharrisson, Miouge1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2020
@Miouge1
Copy link
Contributor

Miouge1 commented Apr 7, 2020

@mattymo can you /hold cancel ?

@mattymo
Copy link
Contributor

mattymo commented Apr 15, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2020
@Miouge1
Copy link
Contributor

Miouge1 commented Apr 16, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 16, 2020
@k8s-ci-robot k8s-ci-robot merged commit b634128 into kubernetes-sigs:master Apr 16, 2020
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Apr 18, 2020
* Add Molecule for container-engine/docker

* Add bootstrap-os to Molecule prepare stage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants