From 00101968d8304fd992a0856c4515340fbebb80ee Mon Sep 17 00:00:00 2001 From: Connor Osborn Date: Sat, 3 Mar 2018 01:16:30 +0000 Subject: [PATCH 1/6] Fix image scripts not being included in user deploy In this commit acb6729544449b352401a307c8300b1ab2cd6d8b, we removed the task which ran image scripts against an instance. There is a lot confusing terminology that needs to be cleaned up. --- service/deploy.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/service/deploy.py b/service/deploy.py index 2982c7c5d..4de29bb52 100644 --- a/service/deploy.py +++ b/service/deploy.py @@ -269,9 +269,9 @@ def user_deploy(instance_ip, username, instance_id, first_deploy=True, **runner_ # Example 'user only' strategy: # user_keys = [k.pub_key for k in get_user_ssh_keys(username)] instance = Instance.objects.get(provider_alias=instance_id) - scripts = instance.scripts.all() - if not first_deploy: - scripts = scripts.filter(run_every_deploy=True) + image_scripts = instance.source.providermachine.application_version.boot_scripts.all() + instance_scripts = instance.scripts.all() + scripts = image_scripts.union(instance_scripts) # Example 'all members' strategy: if not instance.project: @@ -280,10 +280,20 @@ def user_deploy(instance_ip, username, instance_id, first_deploy=True, **runner_ group_ssh_keys = SSHKey.keys_for_group(group) user_keys = [k.pub_key for k in group_ssh_keys] + async_scripts, deploy_scripts = [], [] + for script in scripts: + if not (first_deploy or script.run_every_deploy): + continue + if script.wait_for_deploy: + deploy_scripts.append(script) + else: + async_scripts.append(script) + + format_script = lambda s: {"name": s.get_title_slug(), "text": s.get_text()} extra_vars = { "USERSSHKEYS": user_keys, - "ASYNC_SCRIPTS": [{"name": s.get_title_slug(), "text": s.get_text()} for s in scripts.filter(wait_for_deploy=False)], - "DEPLOY_SCRIPTS": [{"name": s.get_title_slug(), "text": s.get_text()} for s in scripts.filter(wait_for_deploy=True)], + "ASYNC_SCRIPTS": map(format_script, async_scripts), + "DEPLOY_SCRIPTS": map(format_script, deploy_scripts) } playbook_runner = ansible_deployment( instance_ip, username, instance_id, playbooks_dir, From 7b95d9a7c259b1b47a4795b65419fbe759da1099 Mon Sep 17 00:00:00 2001 From: Connor Osborn Date: Tue, 6 Mar 2018 04:24:47 +0000 Subject: [PATCH 2/6] Remove dead code --- core/factories/__init__.py | 8 ---- core/factories/account.py | 17 -------- core/factories/group.py | 16 -------- core/factories/identity.py | 22 ----------- core/factories/provider.py | 81 -------------------------------------- core/factories/user.py | 24 ----------- 6 files changed, 168 deletions(-) delete mode 100644 core/factories/__init__.py delete mode 100644 core/factories/account.py delete mode 100644 core/factories/group.py delete mode 100644 core/factories/identity.py delete mode 100644 core/factories/provider.py delete mode 100644 core/factories/user.py diff --git a/core/factories/__init__.py b/core/factories/__init__.py deleted file mode 100644 index 6e91ea66a..000000000 --- a/core/factories/__init__.py +++ /dev/null @@ -1,8 +0,0 @@ -""" -""" -from core.factories.account import AccountProviderFactory -from core.factories.user import AtmosphereUserFactory -from core.factories.group import GroupFactory, GroupMembershipFactory -from core.factories.identity import IdentityFactory -from core.factories.provider import PlatformTypeFactory,\ - ProviderTypeFactory, ProviderFactory diff --git a/core/factories/account.py b/core/factories/account.py deleted file mode 100644 index e2e649b6a..000000000 --- a/core/factories/account.py +++ /dev/null @@ -1,17 +0,0 @@ -""" -""" -import factory -from factory.django import DjangoModelFactory - -from core import models -from core.factories.identity import IdentityFactory -from core.factories.provider import ProviderFactory - - -class AccountProviderFactory(DjangoModelFactory): - - class Meta: - model = models.AccountProvider - - provider = factory.SubFactory(ProviderFactory) - identity = factory.SubFactory(IdentityFactory) diff --git a/core/factories/group.py b/core/factories/group.py deleted file mode 100644 index a28c16751..000000000 --- a/core/factories/group.py +++ /dev/null @@ -1,16 +0,0 @@ -import factory -from factory.django import DjangoModelFactory - -from core.models import group as models - - -class GroupFactory(DjangoModelFactory): - - class Meta: - model = models.Group - name = "New Test Group" - -class GroupMembershipFactory(DjangoModelFactory): - - class Meta: - model = models.GroupMembership diff --git a/core/factories/identity.py b/core/factories/identity.py deleted file mode 100644 index bcd84ae45..000000000 --- a/core/factories/identity.py +++ /dev/null @@ -1,22 +0,0 @@ -""" -""" -from uuid import uuid4 - -import factory -from factory.django import DjangoModelFactory - -from core import models -from core.factories.user import AtmosphereUserFactory -from core.factories.provider import ProviderFactory - - -class IdentityFactory(DjangoModelFactory): - - class Meta: - model = models.Identity - - uuid = uuid4() - - # forward dependencies - created_by = factory.SubFactory(AtmosphereUserFactory) - provider = factory.SubFactory(ProviderFactory) diff --git a/core/factories/provider.py b/core/factories/provider.py deleted file mode 100644 index 704689d80..000000000 --- a/core/factories/provider.py +++ /dev/null @@ -1,81 +0,0 @@ -""" - Factory to create Service Provider models in testing -""" -from datetime import datetime -from uuid import uuid4 -import pytz - -import factory -from factory.django import DjangoModelFactory - -from core.models import provider as models -from core.models.instance_action import InstanceAction - - -class PlatformTypeFactory(DjangoModelFactory): - - class Meta: - model = models.PlatformType - - name = "test-platform" - - -class ProviderTypeFactory(DjangoModelFactory): - - class Meta: - model = models.ProviderType - - name = factory.Sequence(lambda n: "Provider Type %s" % n) - start_date = datetime(2015, 1, 1, tzinfo=pytz.UTC) - end_date = datetime(2015, 1, 3, tzinfo=pytz.UTC) - - -class ProviderFactory(DjangoModelFactory): - - class Meta: - model = models.Provider - - uuid = uuid4() - location = "Tucson, AZ" - description = "Provider used in testing" - active = True - public = True - start_date = datetime(2015, 1, 1, tzinfo=pytz.UTC) - end_date = None - - # forward dependencies - type = factory.SubFactory(ProviderTypeFactory) - virtualization = factory.SubFactory(PlatformTypeFactory) - - -class DNSFactory(DjangoModelFactory): - - class Meta: - model = models.ProviderDNSServerIP - - ip_address = factory.Sequence(lambda n: "%s.%s.%s.%s" % n) - order = factory.Sequence(lambda n: n) - - # forward dependencies - provider = factory.SubFactory(ProviderFactory) - -# Placed here in order to use "ProviderInstanceAction" -# Move to "Instance" factories when enough classes exist. - - -class InstanceActionFactory(DjangoModelFactory): - - class Meta: - model = InstanceAction - - name = factory.Sequence(lambda n: "action-%s" % n) - description = factory.Sequence(lambda n: "Description for action %s" % n) - - -class ProviderInstanceActionFactory(DjangoModelFactory): - - class Meta: - model = models.ProviderInstanceAction - enabled = True - instance_action = factory.SubFactory(InstanceActionFactory) - provider = factory.SubFactory(ProviderFactory) diff --git a/core/factories/user.py b/core/factories/user.py deleted file mode 100644 index befc21562..000000000 --- a/core/factories/user.py +++ /dev/null @@ -1,24 +0,0 @@ -""" -""" -import factory -from factory.django import DjangoModelFactory - -from core import models - - -class AtmosphereUserFactory(DjangoModelFactory): - - class Meta: - model = models.AtmosphereUser - - email = "test@test.com" - username = "test" - password = factory.PostGenerationMethodCall('set_password', - 'test') - is_active = True - is_staff = True - is_superuser = True - - # forward dependencies - selected_identity = factory.SubFactory( - "core.factories.identity.IdentityFactory") From 3947f07810cc778faa99b6fbb2c3bcc3214fa25d Mon Sep 17 00:00:00 2001 From: Connor Osborn Date: Tue, 6 Mar 2018 04:27:31 +0000 Subject: [PATCH 3/6] Skip broken test This test will start failing when it has built up results in the redis cache. Need to revisit it, but travis doesn't catch it because it starts with a clean cache. --- api/tests/v2/test_image_metrics.py | 1 + 1 file changed, 1 insertion(+) diff --git a/api/tests/v2/test_image_metrics.py b/api/tests/v2/test_image_metrics.py index a048d98ab..d03b9e628 100644 --- a/api/tests/v2/test_image_metrics.py +++ b/api/tests/v2/test_image_metrics.py @@ -172,6 +172,7 @@ def test_user_sees_no_statistics(self): response = client.get(url) self.assertEquals(response.status_code, 404) + @skip("Test is non deterministic and yields different results based on its redis cache") def test_staff_sees_accurate_application_statistics(self): """Given the setUp above, 1/4 instances are active.""" client = APIClient() From 7c6691489f03e46144aaeed74db3e66b490a1d2f Mon Sep 17 00:00:00 2001 From: Connor Osborn Date: Tue, 6 Mar 2018 06:36:28 +0000 Subject: [PATCH 4/6] Prefer testing fewer things This test case broke when the instance_factory began returning a project. This change makes the test only compare values it is actively trying to test, namely the assignment of an allocation source. --- .../instance.features/edit_instance.feature | 44 +++++-------------- 1 file changed, 10 insertions(+), 34 deletions(-) diff --git a/features/instance.features/edit_instance.feature b/features/instance.features/edit_instance.feature index 61b129dde..1f1558648 100644 --- a/features/instance.features/edit_instance.feature +++ b/features/instance.features/edit_instance.feature @@ -53,21 +53,11 @@ Feature: Launching & editing of an instance And the API response contains """ { - "usage": -1, - "start_date": "2017-02-16T07:00:00Z", - "status": "active", - "shell": false, - "vnc": false, - "end_date": null, - "scripts": [], - "ip_address": null, - "project": null, "name": "Instance in active", - "allocation_source": null, - "activity": "" + "start_date": "2017-02-16T07:00:00Z", + "allocation_source": null } """ - # Assign the allocation source to the instance Given a current time of '2017-02-16T07:01:00Z' with tick = False When I assign allocation source "user407" to active instance Then the API response code is 201 @@ -76,28 +66,14 @@ Feature: Launching & editing of an instance And the API response contains """ { - "usage": 0.0, - "start_date": "2017-02-16T07:00:00Z", - "status": "active", - "shell": false, - "vnc": false, - "end_date": null, - "scripts": [], - "ip_address": null, - "project": null, - "name": "Instance in active", - "activity": "" - } - """ - And I set "response_data" to attribute "data" of "response" - And I set "allocation_source" to key "allocation_source" of "response_data" - Then "allocation_source" contains - """ - { - "name": "user407", - "renewal_strategy": "default", - "compute_allowed": 168, - "start_date": "2017-02-16T06:00:00Z" + "name": "Instance in active", + "start_date": "2017-02-16T07:00:00Z", + "allocation_source": { + "compute_allowed": 168, + "name": "user407", + "renewal_strategy": "default", + "start_date": "2017-02-16T06:00:00Z" + } } """ And I set "instance01" to another variable "active_instance" From 87fd0612955ec7bdde82416a20fad11d7e944382 Mon Sep 17 00:00:00 2001 From: Connor Osborn Date: Tue, 6 Mar 2018 06:36:28 +0000 Subject: [PATCH 5/6] Prefer testing fewer things This test case broke when the instance_factory began returning a project. This change makes the test only compare values it is actively trying to test, namely instance name. --- .../instance.features/edit_instance.feature | 41 +------------------ 1 file changed, 2 insertions(+), 39 deletions(-) diff --git a/features/instance.features/edit_instance.feature b/features/instance.features/edit_instance.feature index 1f1558648..a29521a40 100644 --- a/features/instance.features/edit_instance.feature +++ b/features/instance.features/edit_instance.feature @@ -121,18 +121,8 @@ Feature: Launching & editing of an instance And the API response contains """ { - "usage": -1, "start_date": "2017-02-16T07:00:00Z", - "status": "active", - "shell": false, - "vnc": false, - "end_date": null, - "scripts": [], - "ip_address": null, - "project": null, - "name": "Instance in active", - "allocation_source": null, - "activity": "" + "name": "Instance in active" } """ When I change the name of the active instance to "My New Instance Name" @@ -141,34 +131,7 @@ Feature: Launching & editing of an instance And the API response contains """ { - "usage": -1, - "start_date": "2017-02-16T07:00:00Z", - "status": "active", - "shell": false, - "vnc": false, - "end_date": null, - "scripts": [], - "ip_address": null, - "project": null, - "name": "My New Instance Name", - "activity": "" - } - """ - When we get the details for the active instance via the API - Then the API response code is 200 - And the API response contains - """ - { - "usage": -1, "start_date": "2017-02-16T07:00:00Z", - "status": "active", - "shell": false, - "vnc": false, - "end_date": null, - "scripts": [], - "ip_address": null, - "project": null, - "name": "My New Instance Name", - "activity": "" + "name": "My New Instance Name" } """ From 46666edc72696955f7ef68b77d879f65f954c0af Mon Sep 17 00:00:00 2001 From: Connor Osborn Date: Tue, 6 Mar 2018 04:29:46 +0000 Subject: [PATCH 6/6] Add test to verify ansible is called with image and instance scripts --- api/tests/factories/__init__.py | 4 ++- api/tests/factories/boot_script_factory.py | 21 +++++++++++ api/tests/factories/group_factory.py | 3 ++ api/tests/factories/identity_factory.py | 4 +++ api/tests/factories/image_factory.py | 3 ++ api/tests/factories/instance_factory.py | 16 +++++++++ .../factories/instance_source_factory.py | 12 +++++++ api/tests/factories/project_factory.py | 10 ++++-- .../factories/provider_machine_factory.py | 13 ++++--- api/tests/factories/version_factory.py | 5 +++ service/tests/test_deploy.py | 36 +++++++++++++++++++ 11 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 api/tests/factories/boot_script_factory.py create mode 100644 api/tests/factories/instance_source_factory.py create mode 100644 service/tests/test_deploy.py diff --git a/api/tests/factories/__init__.py b/api/tests/factories/__init__.py index 10b220d9b..d202d115a 100644 --- a/api/tests/factories/__init__.py +++ b/api/tests/factories/__init__.py @@ -13,7 +13,9 @@ from .quota_factory import QuotaFactory from .allocation_factory import AllocationFactory from .provider_type_factory import ProviderTypeFactory -from .provider_machine_factory import ProviderMachineFactory, InstanceSourceFactory +from .provider_machine_factory import ProviderMachineFactory from .platform_type_factory import PlatformTypeFactory from .size_factory import SizeFactory from .allocation_source_factory import AllocationSourceFactory, UserAllocationSourceFactory +from .boot_script_factory import BootScriptRawTextFactory, BootScriptURLFactory +from .instance_source_factory import InstanceSourceFactory diff --git a/api/tests/factories/boot_script_factory.py b/api/tests/factories/boot_script_factory.py new file mode 100644 index 000000000..3357d9bc3 --- /dev/null +++ b/api/tests/factories/boot_script_factory.py @@ -0,0 +1,21 @@ +import factory +from factory import fuzzy +from api.tests.factories import UserFactory, InstanceFactory +from core.models import BootScript, ScriptType + + +class BootScriptFactory(factory.DjangoModelFactory): + + class Meta: + model = BootScript + + title = fuzzy.FuzzyText(prefix="bootscript-title-") + created_by = factory.SubFactory(UserFactory) + + +class BootScriptRawTextFactory(BootScriptFactory): + script_type = factory.LazyAttribute(lambda _: ScriptType.objects.get_or_create(name='Raw Text')[0]) + + +class BootScriptURLFactory(BootScriptFactory): + script_type = factory.LazyAttribute(lambda _: ScriptType.objects.get_or_create(name='URL')[0]) diff --git a/api/tests/factories/group_factory.py b/api/tests/factories/group_factory.py index f692d634b..b407678d0 100644 --- a/api/tests/factories/group_factory.py +++ b/api/tests/factories/group_factory.py @@ -1,4 +1,5 @@ import factory +from factory import fuzzy from core.models import Group @@ -6,3 +7,5 @@ class GroupFactory(factory.DjangoModelFactory): class Meta: model = Group + + name = fuzzy.FuzzyText(prefix="name-") diff --git a/api/tests/factories/identity_factory.py b/api/tests/factories/identity_factory.py index 25eef0022..c19ab53ba 100644 --- a/api/tests/factories/identity_factory.py +++ b/api/tests/factories/identity_factory.py @@ -37,3 +37,7 @@ def create_identity(created_by, group=None, provider=None, quota=None): class Meta: model = Identity + + created_by = factory.SubFactory(UserFactory) + provider = factory.SubFactory(ProviderFactory) + quota = factory.SubFactory(QuotaFactory) diff --git a/api/tests/factories/image_factory.py b/api/tests/factories/image_factory.py index e31b62fab..ed53aa99f 100644 --- a/api/tests/factories/image_factory.py +++ b/api/tests/factories/image_factory.py @@ -1,8 +1,11 @@ import factory from core.models import Application as Image +from .user_factory import UserFactory class ImageFactory(factory.DjangoModelFactory): class Meta: model = Image + + created_by = factory.SubFactory(UserFactory) diff --git a/api/tests/factories/instance_factory.py b/api/tests/factories/instance_factory.py index 7917c3e30..2ce388310 100644 --- a/api/tests/factories/instance_factory.py +++ b/api/tests/factories/instance_factory.py @@ -1,8 +1,24 @@ import factory from core.models import Instance +from django.utils import timezone +from .user_factory import UserFactory +from .project_factory import ProjectFactory +from .instance_source_factory import InstanceSourceFactory +from .provider_machine_factory import ProviderMachineFactory +from .identity_factory import IdentityFactory + class InstanceFactory(factory.DjangoModelFactory): class Meta: model = Instance + exclude = ('provider_machine',) + + provider_machine = factory.SubFactory(ProviderMachineFactory) + start_date = factory.LazyFunction(timezone.now) + project = factory.SubFactory(ProjectFactory) + source = factory.SelfAttribute('provider_machine.instance_source') + created_by = factory.SubFactory(UserFactory) + created_by_identity = factory.LazyAttribute( + lambda model: IdentityFactory(created_by=model.created_by)) diff --git a/api/tests/factories/instance_source_factory.py b/api/tests/factories/instance_source_factory.py new file mode 100644 index 000000000..7989f822e --- /dev/null +++ b/api/tests/factories/instance_source_factory.py @@ -0,0 +1,12 @@ +import factory +import uuid +from core.models import ProviderMachine, InstanceSource +from .provider_factory import ProviderFactory + +class InstanceSourceFactory(factory.DjangoModelFactory): + + class Meta: + model = InstanceSource + + identifier = factory.Sequence(lambda n: uuid.uuid4()) + provider = factory.SubFactory(ProviderFactory) diff --git a/api/tests/factories/project_factory.py b/api/tests/factories/project_factory.py index 6c2310690..141ee70f5 100644 --- a/api/tests/factories/project_factory.py +++ b/api/tests/factories/project_factory.py @@ -1,11 +1,17 @@ import factory +from factory import fuzzy from core.models import Project +from .group_factory import GroupFactory +from .user_factory import UserFactory + class ProjectFactory(factory.DjangoModelFactory): class Meta: model = Project - name = 'project name' - description = 'project description' + name = fuzzy.FuzzyText(prefix="name-") + description = fuzzy.FuzzyText(prefix="description-") + created_by = factory.SubFactory(UserFactory) + owner = factory.SubFactory(GroupFactory) diff --git a/api/tests/factories/provider_machine_factory.py b/api/tests/factories/provider_machine_factory.py index c8c8bcc46..4c84b296c 100644 --- a/api/tests/factories/provider_machine_factory.py +++ b/api/tests/factories/provider_machine_factory.py @@ -3,6 +3,9 @@ from core.models import ProviderMachine, InstanceSource from .version_factory import ApplicationVersionFactory from .image_factory import ImageFactory +from .provider_factory import ProviderFactory +from .instance_source_factory import InstanceSourceFactory +from .version_factory import ApplicationVersionFactory class ProviderMachineFactory(factory.DjangoModelFactory): @@ -32,10 +35,6 @@ def create_provider_machine(user, identity, application=None, version=None): class Meta: model = ProviderMachine - -class InstanceSourceFactory(factory.DjangoModelFactory): - - class Meta: - model = InstanceSource - - identifier = factory.Sequence(lambda n: uuid.uuid4()) + # Field occurs on super class of ProviderMachine + instance_source = factory.SubFactory(InstanceSourceFactory) + application_version = factory.SubFactory(ApplicationVersionFactory) diff --git a/api/tests/factories/version_factory.py b/api/tests/factories/version_factory.py index a467233a7..10b5fcd3d 100644 --- a/api/tests/factories/version_factory.py +++ b/api/tests/factories/version_factory.py @@ -1,6 +1,8 @@ import factory from core.models import ApplicationVersion from .image_factory import ImageFactory +from .user_factory import UserFactory +from .identity_factory import IdentityFactory class ApplicationVersionFactory(factory.DjangoModelFactory): @@ -21,3 +23,6 @@ class Meta: model = ApplicationVersion application = factory.SubFactory(ImageFactory) + created_by = factory.SubFactory(UserFactory) #factory.SelfAttribute("created_by_identity.created_by") + created_by_identity = factory.LazyAttribute( + lambda model: IdentityFactory(created_by=model.created_by)) diff --git a/service/tests/test_deploy.py b/service/tests/test_deploy.py new file mode 100644 index 000000000..81bf8e78f --- /dev/null +++ b/service/tests/test_deploy.py @@ -0,0 +1,36 @@ +from django.test import TestCase +import mock + +from api.tests.factories import BootScriptRawTextFactory, InstanceFactory, UserFactory + + +class UserDeployTests(TestCase): + def test_image_and_instance_scripts_are_included(self): + user = UserFactory.create() + instance = InstanceFactory.create(created_by=user) + + # Create/add instance script + instance_script = BootScriptRawTextFactory.create( + created_by=user, wait_for_deploy=True) + instance.scripts.add(instance_script) + + # Create/add image script + image_script = BootScriptRawTextFactory.create( + created_by=user, wait_for_deploy=True) + application_version = instance.source.providermachine.application_version + application_version.boot_scripts.add(image_script) + + # Mock out ansible_deployment to verify its called with both image and + # instance scripts + with mock.patch( + 'service.deploy.ansible_deployment') as ansible_deployment: + from service.deploy import user_deploy + user_deploy(instance.ip_address, user.username, + instance.provider_alias) + kwargs = ansible_deployment.call_args[1] + script_titles = { + s['name'] + for s in kwargs['extra_vars']['DEPLOY_SCRIPTS'] + } + self.assertIn(instance_script.get_title_slug(), script_titles) + self.assertIn(image_script.get_title_slug(), script_titles)