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

[xenial] Deprecate Vagrantfile in favor of Molecule scenarios #3208

Closed
conorsch opened this issue Mar 29, 2018 · 7 comments · Fixed by #5960
Closed

[xenial] Deprecate Vagrantfile in favor of Molecule scenarios #3208

conorsch opened this issue Mar 29, 2018 · 7 comments · Fixed by #5960

Comments

@conorsch
Copy link
Contributor

conorsch commented Mar 29, 2018

Feature request

Description

We've started to use Molecule for testing various aspects of the SD development environment, including most recently the potential introduction of local upgrade testing (#3075). Let's continue in that direction and adopt Molecule as a wrapper for all Vagrant calls, as we did with the build VM in #2160.

Doing so will allow improvements such as:

  • Evaluating Xenial as a potential migration path ([xenial] Add staging VMs for Ubuntu 16.04 #3206)
  • Ditch custom tesitnfra/test.py script (Add a make target for running testinfra tests #2437)
  • Tighter workflow integration, such as molecule test -s staging performing essentially:
    • vagrant up /staging/ --no-provision
    • vagrant provision /staging/
    • vagrant provision /staging/ # idempotence check
    • flake8 testinfra/
    • ansible-lint install_files/ansible-base/securedrop-staging.yml
    • ./testinfra/test.py app-staging
    • ./testinfra/test.py mon-stagng
    • vagrant destroy -f /staging/

For more information, see the Molecule docs on scenarios. Note that we're already using Molecule for the CI staging scenario (#2218); we should rename that scenario to staging-aws for clarity, IMO.

One potential shortcoming is that Molecule v2 requires one provider, e.g. VirtualBox or KVM/libvirt, whereas raw Vagrantfiles support multiple. If that turns out to be a problem we can always provide a Vagrantfile template to Molecule and customize at will.

User Stories

As a maintainer, I want to lean on community-maintained test harnesses where possible.

As a developer, I want to deal with a minimum of fuss to provision and test dev environments for a complicated project like SecureDrop.

@eloquence eloquence changed the title Deprecate Vagrantfile in favor of Molecule scenarios [xenial] Deprecate Vagrantfile in favor of Molecule scenarios Apr 5, 2018
@eloquence eloquence added this to the 0.8 milestone Apr 6, 2018
@redshiftzero redshiftzero modified the milestones: 0.8, 0.9 Jun 6, 2018
@eloquence
Copy link
Member

For the 7/11-7/25 sprint, we've committed to the creation of a molecule scenario that installs SecureDrop against Trusty (not Xenial); our goal is to get a working baseline from which to make further improvements such as supporting configuration testing and adding support for all virtualization providers. Once that is all working, we can add a Xenial target.

@conorsch
Copy link
Contributor Author

Working on porting the testinfra tests now. The major customization we have via test.py (and conftest.py) is that we import "vars" to keep the test suite DRY. There's a lot of wrapper logic in play, e.g. helper scripts in the AWS Molecule scenario, all intended to populate an env var that is used in the pytest bootstrapping logic.

It will be difficult to cull much of that env-var-based wrapper logic. It's worth noting that we're not currently testing the prod VM config anywhere in CI, so much of that logic is essentially a dead code path. If we can expose the hostname targeted by testinfra within the test suite, then we're golden on reducing the wrapper code.

If we cannot expose the hostname targeted by testinfra (we could not back when we wrote the wrapper logic in #1616, although a lot has changed upstream since then), then we should consider dropping the vars import logic altogether. We can fall back to re-using the side-effect strategy mentioned above if unforeseen blockers crop up.

@conorsch
Copy link
Contributor Author

conorsch commented Aug 1, 2018

If we cannot expose the hostname targeted by testinfra (we could not back when we wrote the wrapper logic in #1616, although a lot has changed upstream since then), then we should consider dropping the vars import logic altogether.

Decided to simplify the vars import logic by concatenating the staging vars into a single YAML file, which can be imported in the context of both app-staging and mon-staging. That's working quite well so far. Currently debugging the test_journalist_mail.py test suite, which is about all that's failing. Once I have that sorted, will open a WIP PR to port the changes to the CI setup.

@conorsch
Copy link
Contributor Author

conorsch commented Aug 1, 2018

The test_journalist_mail.py failures are caused by some hardcoded testinfra backend calls that will need to be updated. Optimistic we can get that whole suite passing again with a minimum of fuss.

Other than that, the only other tests I've seen fail are related to OSSEC inter-VM communication. Will need to take a closer look at those on subsequent builds to evaluate whether they're flakes, or OSSEC genuinely isn't connecting between the machines.

@conorsch
Copy link
Contributor Author

conorsch commented Aug 2, 2018

Down to two failing tests:

  • TestJournalistMail.test_process_submissions_today
  • TestJournalistMail.test_journalist_mail_notification

Everything else is green across the board. Will drill down on those two tests today to understand why they're not passing locally.

@conorsch
Copy link
Contributor Author

conorsch commented Aug 2, 2018

Upon further investigation, it appears that the problematic tests in TestJournalistMail are not currently running in CI—they're skipped every time. Dug into this because it didn't make any sense that I was having trouble with them only locally. After inspecting the logic a bit more closely, there are a few race conditions that would explain the errors I've been seeing.

I'll open a separate issue to follow up on that particular test suite; I'm sufficiently unblocked now to continue with the porting as planned.

@eloquence
Copy link
Member

eloquence commented Sep 24, 2018

So, there's a bit of work remaining before we can close this; capturing here for the record so we can start scoping and parceling it out:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants