Merge lp:~makyo/juju-deployer/machines-and-placement into lp:juju-deployer
| Status: | Merged |
|---|---|
| Merged at revision: | 144 |
| Proposed branch: | lp:~makyo/juju-deployer/machines-and-placement |
| Merge into: | lp:juju-deployer |
| Diff against target: |
1612 lines (+1109/-73) 24 files modified
README (+5/-1) deployer/action/importer.py (+74/-4) deployer/config.py (+5/-3) deployer/deployment.py (+48/-6) deployer/env/base.py (+1/-1) deployer/env/go.py (+27/-3) deployer/service.py (+249/-33) deployer/tests/base.py (+8/-2) deployer/tests/test_config.py (+2/-2) deployer/tests/test_data/v4/container-new.yaml (+43/-0) deployer/tests/test_data/v4/container.yaml (+43/-0) deployer/tests/test_data/v4/fill_placement.yaml (+17/-0) deployer/tests/test_data/v4/hulk-smash-nounits-nomachines.yaml (+38/-0) deployer/tests/test_data/v4/hulk-smash-nounits.yaml (+43/-0) deployer/tests/test_data/v4/hulk-smash.yaml (+43/-0) deployer/tests/test_data/v4/placement-invalid-subordinate.yaml (+13/-0) deployer/tests/test_data/v4/placement.yaml (+45/-0) deployer/tests/test_data/v4/series.yaml (+21/-0) deployer/tests/test_data/v4/simple.yaml (+1/-0) deployer/tests/test_data/v4/validate.yaml (+16/-0) deployer/tests/test_deployment.py (+302/-14) deployer/tests/test_goenv.py (+27/-4) deployer/tests/test_importer.py (+19/-0) deployer/utils.py (+19/-0) |
| To merge this branch: | bzr merge lp:~makyo/juju-deployer/machines-and-placement |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| David Britton | 2015-03-05 | Approve on 2015-03-25 | |
| 🤖 Landscape Builder (community) | Approve on 2015-03-25 | ||
|
Review via email:
|
|||
Description of the Change
Add v4 support for machine spec per the bundles spec.
QA:
Ensure that each of the bundles in deployer/
PYTHONPATH=. python deployer/cli.py -c deployer/
All tests should pass.
| Brad Crittenden (bac) wrote : | # |
| Francesco Banconi (frankban) wrote : | # |
Hi Madison, this is an impressive branch.
Thank you for sorting out the intricacies of the new units placement logic.
Did not review the tests yet, but I have some suggestions and questions below: please let me know what do you think.
I am not sure about repeated placement parsing logic across the modules in this branch (see below).
I'll take another and deeper look after your replies.
Thank you!
| David Britton (davidpbritton) wrote : | # |
Hi -- Other than examining the code, where can I see differences between the v4 and v3 spec? Could a README change be introduced with perhaps an example v4 file with annotated differences? Also, rather than duplicating the entire v3 class, could things be refactored to be shared, and then tested together? Like a common set of validations, and then just the v4 additions (same for 'get()').
I have a number of comments inline, mostly around test coverage. I found one coding error just by inspection, making me a bit worried about the lack of unit tests for the change. Relying on just the more functional style of tests (of parsing a complete bundle) for covering this new code seems like a risky approach.
Some of the more complex functions (ex: validate()) could also use from refactoring and breaking apart as they are multiple pages in length (with clear duplication in the v4 addition).
| Madison Scott-Clary (makyo) wrote : | # |
Checkpoint for pairing.
| Brad Crittenden (bac) wrote : | # |
Looks good Madison with a few comments.
| Madison Scott-Clary (makyo) wrote : | # |
For live environment testing, assuming an LXC env named local,
TEST_ENDPOINT=
| Madison Scott-Clary (makyo) wrote : | # |
> For live environment testing, assuming an LXC env named local,
>
> TEST_ENDPOINT=
> deployer.
Also, ensure that that happens on a single line. LP broke it up
| Kapil Thangavelu (hazmat) wrote : | # |
What happens when i use a v4 spec with multiple machines, remove a unit that was placed to one of those machines, and then try to run it again? the code while it looks nice seems to have some gaps around consideration of running it multiple times, which sort of defines an ideal production usage (run again to apply delta from vcs).
| Francesco Banconi (frankban) wrote : | # |
Thanks for the fixes Madison, nicely done!
Also thanks for improving the pre-existing code docstrings and test coverage.
I have some minor questions and suggestions, please see below.
In the case Kapil described, the current behavior seems to be that we get the last unit placements.
In theory, we should really diff and retrieve the missing placement.
For the time being, a safe tradeoff could be to always create new machines for missing units if cur_units > 0.
| Madison Scott-Clary (makyo) wrote : | # |
> What happens when i use a v4 spec with multiple machines, remove a unit that
> was placed to one of those machines, and then try to run it again? the code
> while it looks nice seems to have some gaps around consideration of running it
> multiple times, which sort of defines an ideal production usage (run again to
> apply delta from vcs).
We talked about this with the team this morning (and there was additional discussion online last night), noting that this is something that the new bundle specification does not currently have in place. As part of this branch, I will change the logic that is used to to decide what a v4 bundle is (that is, only one with a machine spec), and all other bundles will fall back to the v3 format, which can be run multiple times.
| David Britton (davidpbritton) wrote : | # |
Thanks Makyo -- I have added a number of diff comments. Getting much better, thanks for the improvements and additional tests, though it could still use more. I will keep this as needs fixing, and I'll also test it out with a couple v3 bundles live while I wait to hear your response.
I'd still also like to see an addtion to the README pointing to or explaining the v4 spec, and an annotated file that highlights differences.
Thanks!
| Madison Scott-Clary (makyo) wrote : | # |
Committing most fixes now; tests will be in the next commit.
- 153. By Madison Scott-Clary on 2015-03-18
-
Merge trunk; update tests for subordinate placements.
- 154. By Madison Scott-Clary on 2015-03-24
-
Add invalid subordinate placement v4 bundle.
- 155. By Madison Scott-Clary on 2015-03-24
-
Add series to machine spec.
Thanks, test coverage much better, follow-on branch will address concerns about attempts being idempotent. Deployed both v3 and v4 bundles with placement.
| David Britton (davidpbritton) wrote : | # |
Thanks, test coverage much better, follow-on branch will address concerns about attempts being idempotent. Deployed both v3 and v4 bundles with placement.

QA instructions with PYTHONPATH won't work if juju-deployer has previously been pip installed.
Still doing QA.