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

[BUG] Broken concurrency: one cannot run few molecule processes in parallel #1573

Closed
webknjaz opened this issue Nov 8, 2018 · 10 comments
Closed

Comments

@webknjaz
Copy link
Member

webknjaz commented Nov 8, 2018

Issue Type

  • Bug report

Molecule and Ansible details

devel

Molecule installation method (one of):

N/A

Ansible installation method (one of):

N/A

Detail any linters or test runners used:

docker driver

Desired Behavior

Molecule should create resources (test envs) with names unique to test run.
I want to be able to run concurrently the test of Molecule itself.

Actual Behaviour

Molecule's default docker image instance name defaults to the driver name, that's it. So when you run multiple processes they mess with the same resource breaking each other in unpredictable ways.

@decentral1se
Copy link
Contributor

It looks like a work-around for this is in a0627fd, right?

@ragingpastry
Copy link
Contributor

Ran into this a while back like lwm pointed out. There is probably a much better way to solve it though.

@webknjaz
Copy link
Member Author

webknjaz commented Jan 24, 2019

@lwm yea, kinda.
Also, see a prototype of the possible fix: webknjaz@87ccd81

@webknjaz webknjaz changed the title Broken concurrency: one cannot run few molecule processes in parallel [BUG] Broken concurrency: one cannot run few molecule processes in parallel Jan 24, 2019
@decentral1se
Copy link
Contributor

Fix looks legit @webknjaz! How do you want to proceed? The CI should be able to verify this as working or we need to do some manual testing? Just to mark, I'll need to update #1683 documentation when we have this working differently.

@webknjaz
Copy link
Member Author

webknjaz commented Feb 1, 2019

@lwm thanks, feel free to fork from that branch (master...webknjaz:feature/molecule-parallelism) or cherry-pick it if you need this done fast because I'm not going to have time for it during the next few weeks. It seems fine but I don't like that it's a bit sketchy (and I'd normally add docstrings before merging such thing).
I think CI would probably be flaky with xdist enabled if the issue still exists but yes, if checked manually I'd be more confident.

In general, it's more about seeking the design feedback: is it fine to add such random thing to identifier names? what else would this affect? I checked with docker but what about other providers? do existing users rely on having the same instance name across subsequent re-runs (current patch would break this expectation because the random thing would be different every time)? what else did I miss? any UX problems?

@decentral1se
Copy link
Contributor

Related: #1716 + #1704.

@hugoShaka
Copy link

It is interesting, and way cleaner that I've done on hugoShaka@b335b09 which was provider-specific 👏

I don't know what to think about the UUID thing. I guess it's the best solution if you need to run the same scenario in parallel on the same machine/cluster (I'm not sure about the use-case, maybe if you add some random and want to do stats). If you don't need to run the same scenario in parallel maybe using the scenario name as a prefix makes more sense for debugging purposes.

@decentral1se
Copy link
Contributor

In general, it's more about seeking the design feedback: is it fine to add such random thing to identifier names? what else would this affect? I checked with docker but what about other providers? do existing users rely on having the same instance name across subsequent re-runs (current patch would break this expectation because the random thing would be different every time)? what else did I miss? any UX problems?

Right, good points. I think molecule users are expecting their instances to be named the way they see them in the molecule.yml, so maybe we're going down the wrong path. See, for example, #1731 and also the CLI of molecule login --host {instance-name}.

@rockandska
Copy link

Just my 2cts
Regarding the solutions in this issue after having take a quick look about the possible fixes, IMO the best one is the one provided by @hugoShaka + 1 addition.

name: "{{ molecule_yml.scenario.name }}_{{ item.name | d(uuid_function)  }}"

This way:

  • if you don't care about the instance name, a unique one will be generated and will reduce the options to provide in molecule platform description
  • if you need to referring to an instance by it's name and want to fix one, you could fix the name and referring it by "{{ scenario_name }}_{{ name}}"
  • race condition will be not a problem since the scenario is part of the instance name

Regards,

@webknjaz
Copy link
Member Author

Summary update:

I think that using a state file suggested by @decentral1se is a way to go: #1702 (comment)

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

No branches or pull requests

5 participants