Merge pull request #3842 from ryanpetrello/instance-group-order

enforce a stable list order when attaching/detaching instance groups

Reviewed-by: https://github.com/softwarefactory-project-zuul[bot]
This commit is contained in:
softwarefactory-project-zuul[bot] 2019-05-09 21:04:29 +00:00 committed by GitHub
commit 8725d3e539
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 537 additions and 39 deletions

View File

@ -11,6 +11,7 @@ from jinja2 import Environment, StrictUndefined
from jinja2.exceptions import UndefinedError, TemplateSyntaxError
# Django
import django
from django.core import exceptions as django_exceptions
from django.db.models.signals import (
post_save,
@ -24,8 +25,10 @@ from django.db.models.fields.related_descriptors import (
ForwardManyToOneDescriptor,
ManyToManyDescriptor,
ReverseManyToOneDescriptor,
create_forward_many_to_many_manager
)
from django.utils.encoding import smart_text
from django.utils.functional import cached_property
from django.utils.translation import ugettext_lazy as _
# jsonschema
@ -52,8 +55,8 @@ from awx.main import utils
__all__ = ['AutoOneToOneField', 'ImplicitRoleField', 'JSONField',
'SmartFilterField', 'update_role_parentage_for_instance',
'is_implicit_parent']
'SmartFilterField', 'OrderedManyToManyField',
'update_role_parentage_for_instance', 'is_implicit_parent']
# Provide a (better) custom error message for enum jsonschema validation
@ -987,3 +990,115 @@ class OAuth2ClientSecretField(models.CharField):
if value and value.startswith('$encrypted$'):
return decrypt_value(get_encryption_key('value', pk=None), value)
return value
class OrderedManyToManyDescriptor(ManyToManyDescriptor):
"""
Django doesn't seem to support:
class Meta:
ordering = [...]
...on custom through= relations for ManyToMany fields.
Meaning, queries made _through_ the intermediary table will _not_ apply an
ORDER_BY clause based on the `Meta.ordering` of the intermediary M2M class
(which is the behavior we want for "ordered" many to many relations):
https://github.com/django/django/blob/stable/1.11.x/django/db/models/fields/related_descriptors.py#L593
This descriptor automatically sorts all queries through this relation
using the `position` column on the M2M table.
"""
@cached_property
def related_manager_cls(self):
model = self.rel.related_model if self.reverse else self.rel.model
def add_custom_queryset_to_many_related_manager(many_related_manage_cls):
class OrderedManyRelatedManager(many_related_manage_cls):
def get_queryset(self):
return super(OrderedManyRelatedManager, self).get_queryset().order_by(
'%s__position' % self.through._meta.model_name
)
def add(self, *objs):
# Django < 2 doesn't support this method on
# ManyToManyFields w/ an intermediary model
# We should be able to remove this code snippet when we
# upgrade Django.
# see: https://github.com/django/django/blob/stable/1.11.x/django/db/models/fields/related_descriptors.py#L926
if not django.__version__.startswith('1.'):
raise RuntimeError(
'This method is no longer necessary in Django>=2'
)
try:
self.through._meta.auto_created = True
super(OrderedManyRelatedManager, self).add(*objs)
finally:
self.through._meta.auto_created = False
def remove(self, *objs):
# Django < 2 doesn't support this method on
# ManyToManyFields w/ an intermediary model
# We should be able to remove this code snippet when we
# upgrade Django.
# see: https://github.com/django/django/blob/stable/1.11.x/django/db/models/fields/related_descriptors.py#L944
if not django.__version__.startswith('1.'):
raise RuntimeError(
'This method is no longer necessary in Django>=2'
)
try:
self.through._meta.auto_created = True
super(OrderedManyRelatedManager, self).remove(*objs)
finally:
self.through._meta.auto_created = False
return OrderedManyRelatedManager
return add_custom_queryset_to_many_related_manager(
create_forward_many_to_many_manager(
model._default_manager.__class__,
self.rel,
reverse=self.reverse,
)
)
class OrderedManyToManyField(models.ManyToManyField):
"""
A ManyToManyField that automatically sorts all querysets
by a special `position` column on the M2M table
"""
def _update_m2m_position(self, sender, **kwargs):
if kwargs.get('action') in ('post_add', 'post_remove'):
order_with_respect_to = None
for field in sender._meta.local_fields:
if (
isinstance(field, models.ForeignKey) and
isinstance(kwargs['instance'], field.related_model)
):
order_with_respect_to = field.name
for i, ig in enumerate(sender.objects.filter(**{
order_with_respect_to: kwargs['instance'].pk}
)):
if ig.position != i:
ig.position = i
ig.save()
def contribute_to_class(self, cls, name, **kwargs):
super(OrderedManyToManyField, self).contribute_to_class(cls, name, **kwargs)
setattr(
cls, name,
OrderedManyToManyDescriptor(self.remote_field, reverse=False)
)
through = getattr(cls, name).through
if isinstance(through, str) and "." not in through:
# support lazy loading of string model names
through = '.'.join([cls._meta.app_label, through])
m2m_changed.connect(
self._update_m2m_position,
sender=through
)

View File

@ -0,0 +1,43 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.20 on 2019-05-06 13:48
from __future__ import unicode_literals
from django.db import migrations, models
import django.db.models.deletion
class Migration(migrations.Migration):
dependencies = [
('main', '0072_v350_deprecate_fields'),
]
operations = [
migrations.CreateModel(
name='InventoryInstanceGroupMembership',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('position', models.PositiveIntegerField(default=None, null=True, db_index=True)),
('instancegroup', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='main.InstanceGroup')),
('inventory', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='main.Inventory')),
],
),
migrations.CreateModel(
name='OrganizationInstanceGroupMembership',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('position', models.PositiveIntegerField(default=None, null=True, db_index=True)),
('instancegroup', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='main.InstanceGroup')),
('organization', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='main.Organization')),
],
),
migrations.CreateModel(
name='UnifiedJobTemplateInstanceGroupMembership',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('position', models.PositiveIntegerField(default=None, null=True, db_index=True)),
('instancegroup', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='main.InstanceGroup')),
('unifiedjobtemplate', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='main.UnifiedJobTemplate')),
],
),
]

View File

@ -0,0 +1,42 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.20 on 2019-05-06 13:49
from __future__ import unicode_literals
from django.db import migrations
def create_through_relations(apps, schema_editor):
relations = [
[
'UnifiedJobTemplate',
apps.get_model('main', 'UnifiedJobTemplateInstanceGroupMembership'),
],
[
'Organization',
apps.get_model('main', 'OrganizationInstanceGroupMembership'),
],
[
'Inventory',
apps.get_model('main', 'InventoryInstanceGroupMembership'),
],
]
for cls, Membership in relations:
Target = apps.get_model('main', cls)
for x in Target.objects.iterator():
for i, instance_group in enumerate(x.instance_groups.all()):
Membership(
instancegroup=instance_group,
position=i,
**{'%s' % cls.lower(): x}
).save()
class Migration(migrations.Migration):
dependencies = [
('main', '0073_v360_create_instance_group_m2m'),
]
operations = [
migrations.RunPython(create_through_relations)
]

View File

@ -0,0 +1,27 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.20 on 2019-05-06 15:15
from __future__ import unicode_literals
from django.db import migrations
class Migration(migrations.Migration):
dependencies = [
('main', '0074_v360_migrate_instance_group_relations'),
]
operations = [
migrations.RemoveField(
model_name='inventory',
name='instance_groups',
),
migrations.RemoveField(
model_name='organization',
name='instance_groups',
),
migrations.RemoveField(
model_name='unifiedjobtemplate',
name='instance_groups',
),
]

View File

@ -0,0 +1,31 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.20 on 2019-05-06 15:20
from __future__ import unicode_literals
from django.db import migrations, models
from awx.main.fields import OrderedManyToManyField
class Migration(migrations.Migration):
dependencies = [
('main', '0075_v360_remove_old_instance_group_relations'),
]
operations = [
migrations.AddField(
model_name='inventory',
name='instance_groups',
field=OrderedManyToManyField(blank=True, through='main.InventoryInstanceGroupMembership', to='main.InstanceGroup'),
),
migrations.AddField(
model_name='organization',
name='instance_groups',
field=OrderedManyToManyField(blank=True, through='main.OrganizationInstanceGroupMembership', to='main.InstanceGroup'),
),
migrations.AddField(
model_name='unifiedjobtemplate',
name='instance_groups',
field=OrderedManyToManyField(blank=True, through='main.UnifiedJobTemplateInstanceGroupMembership', to='main.InstanceGroup'),
),
]

View File

@ -332,3 +332,54 @@ def on_job_create(sender, instance, created=False, raw=False, **kwargs):
instance=Instance.objects.me(),
unified_job=instance,
)
class UnifiedJobTemplateInstanceGroupMembership(models.Model):
unifiedjobtemplate = models.ForeignKey(
'UnifiedJobTemplate',
on_delete=models.CASCADE
)
instancegroup = models.ForeignKey(
'InstanceGroup',
on_delete=models.CASCADE
)
position = models.PositiveIntegerField(
null=True,
default=None,
db_index=True,
)
class OrganizationInstanceGroupMembership(models.Model):
organization = models.ForeignKey(
'Organization',
on_delete=models.CASCADE
)
instancegroup = models.ForeignKey(
'InstanceGroup',
on_delete=models.CASCADE
)
position = models.PositiveIntegerField(
null=True,
default=None,
db_index=True,
)
class InventoryInstanceGroupMembership(models.Model):
inventory = models.ForeignKey(
'Inventory',
on_delete=models.CASCADE
)
instancegroup = models.ForeignKey(
'InstanceGroup',
on_delete=models.CASCADE
)
position = models.PositiveIntegerField(
null=True,
default=None,
db_index=True,
)

View File

@ -37,6 +37,7 @@ from awx.main.fields import (
ImplicitRoleField,
JSONBField,
SmartFilterField,
OrderedManyToManyField,
)
from awx.main.managers import HostManager
from awx.main.models.base import (
@ -156,9 +157,10 @@ class Inventory(CommonModelNameNotUnique, ResourceMixin, RelatedJobsMixin):
default=None,
help_text=_('Filter that will be applied to the hosts of this inventory.'),
)
instance_groups = models.ManyToManyField(
instance_groups = OrderedManyToManyField(
'InstanceGroup',
blank=True,
through='InventoryInstanceGroupMembership',
)
admin_role = ImplicitRoleField(
parent_role='organization.inventory_admin_role',

View File

@ -15,7 +15,9 @@ from django.utils.translation import ugettext_lazy as _
# AWX
from awx.api.versioning import reverse
from awx.main.fields import AutoOneToOneField, ImplicitRoleField
from awx.main.fields import (
AutoOneToOneField, ImplicitRoleField, OrderedManyToManyField
)
from awx.main.models.base import (
BaseModel, CommonModel, CommonModelNameNotUnique, CreatedModifiedModel,
NotificationFieldsModel
@ -39,9 +41,10 @@ class Organization(CommonModel, NotificationFieldsModel, ResourceMixin, CustomVi
app_label = 'main'
ordering = ('name',)
instance_groups = models.ManyToManyField(
instance_groups = OrderedManyToManyField(
'InstanceGroup',
blank=True,
through='OrganizationInstanceGroupMembership'
)
max_hosts = models.PositiveIntegerField(
blank=True,

View File

@ -48,7 +48,7 @@ from awx.main.utils import polymorphic, schedule_task_manager
from awx.main.constants import ACTIVE_STATES, CAN_CANCEL
from awx.main.redact import UriCleaner, REPLACE_STR
from awx.main.consumers import emit_channel_notification
from awx.main.fields import JSONField, AskForField
from awx.main.fields import JSONField, AskForField, OrderedManyToManyField
__all__ = ['UnifiedJobTemplate', 'UnifiedJob', 'StdoutMaxBytesExceeded']
@ -164,9 +164,10 @@ class UnifiedJobTemplate(PolymorphicModel, CommonModelNameNotUnique, Notificatio
blank=True,
related_name='%(class)s_labels'
)
instance_groups = models.ManyToManyField(
instance_groups = OrderedManyToManyField(
'InstanceGroup',
blank=True,
through='UnifiedJobTemplateInstanceGroupMembership'
)
def get_absolute_url(self, request=None):

View File

@ -1,3 +1,5 @@
import random
import pytest
from awx.api.versioning import reverse
@ -6,6 +8,7 @@ from awx.main.models import (
InstanceGroup,
ProjectUpdate,
)
from awx.main.utils import camelcase_to_underscore
@pytest.fixture
@ -78,6 +81,11 @@ def instance_group_jobs_successful(instance_group, create_job_factory, create_pr
return jobs_successful + project_updates_successful
@pytest.fixture(scope='function')
def source_model(request):
return request.getfixturevalue(request.param)
@pytest.mark.django_db
def test_instance_group_is_controller(instance_group, isolated_instance_group, non_iso_instance):
assert not isolated_instance_group.is_controller
@ -196,3 +204,39 @@ def test_prevent_non_isolated_instance_added_to_isolated_instance_group_via_poli
resp = patch(url, {'policy_instance_list': [non_iso_instance.hostname]}, user=admin, expect=400)
assert [u"Isolated instance group membership may not be managed via the API."] == resp.data['policy_instance_list']
assert isolated_instance_group.policy_instance_list == []
@pytest.mark.django_db
@pytest.mark.parametrize(
'source_model', ['job_template', 'inventory', 'organization'], indirect=True
)
def test_instance_group_order_persistence(get, post, admin, source_model):
# create several instance groups in random order
total = 5
pks = list(range(total))
random.shuffle(pks)
instances = [InstanceGroup.objects.create(name='iso-%d' % i) for i in pks]
view_name = camelcase_to_underscore(source_model.__class__.__name__)
url = reverse(
'api:{}_instance_groups_list'.format(view_name),
kwargs={'pk': source_model.pk}
)
# associate them all
for instance in instances:
post(url, {'associate': True, 'id': instance.id}, admin, expect=204)
for _ in range(10):
# remove them all
for instance in instances:
post(url, {'disassociate': True, 'id': instance.id}, admin, expect=204)
resp = get(url, admin)
assert resp.data['count'] == 0
# add them all in random order
before = sorted(instances, key=lambda x: random.random())
for instance in before:
post(url, {'associate': True, 'id': instance.id}, admin, expect=204)
resp = get(url, admin)
assert resp.data['count'] == total
assert [ig['name'] for ig in resp.data['results']] == [ig.name for ig in before]

View File

@ -0,0 +1,102 @@
import pytest
from awx.main.models import InstanceGroup
@pytest.fixture(scope='function')
def source_model(request):
return request.getfixturevalue(request.param)
@pytest.mark.django_db
@pytest.mark.parametrize(
'source_model', ['job_template', 'inventory', 'organization'], indirect=True
)
def test_instance_group_ordering(source_model):
groups = [
InstanceGroup.objects.create(name='host-%d' % i)
for i in range(5)
]
groups.reverse()
for group in groups:
source_model.instance_groups.add(group)
assert [g.name for g in source_model.instance_groups.all()] == [
'host-4', 'host-3', 'host-2', 'host-1', 'host-0'
]
assert [
(row.position, row.instancegroup.name)
for row in source_model.instance_groups.through.objects.all()
] == [
(0, 'host-4'),
(1, 'host-3'),
(2, 'host-2'),
(3, 'host-1'),
(4, 'host-0'),
]
source_model.instance_groups.remove(groups[0])
assert [g.name for g in source_model.instance_groups.all()] == [
'host-3', 'host-2', 'host-1', 'host-0'
]
assert [
(row.position, row.instancegroup.name)
for row in source_model.instance_groups.through.objects.all()
] == [
(0, 'host-3'),
(1, 'host-2'),
(2, 'host-1'),
(3, 'host-0'),
]
source_model.instance_groups.clear()
assert source_model.instance_groups.through.objects.count() == 0
@pytest.mark.django_db
@pytest.mark.parametrize(
'source_model', ['job_template', 'inventory', 'organization'], indirect=True
)
def test_instance_group_middle_deletion(source_model):
groups = [
InstanceGroup.objects.create(name='host-%d' % i)
for i in range(5)
]
groups.reverse()
for group in groups:
source_model.instance_groups.add(group)
source_model.instance_groups.remove(groups[2])
assert [g.name for g in source_model.instance_groups.all()] == [
'host-4', 'host-3', 'host-1', 'host-0'
]
assert [
(row.position, row.instancegroup.name)
for row in source_model.instance_groups.through.objects.all()
] == [
(0, 'host-4'),
(1, 'host-3'),
(2, 'host-1'),
(3, 'host-0'),
]
@pytest.mark.django_db
@pytest.mark.parametrize(
'source_model', ['job_template', 'inventory', 'organization'], indirect=True
)
def test_explicit_ordering(source_model):
groups = [
InstanceGroup.objects.create(name='host-%d' % i)
for i in range(5)
]
groups.reverse()
for group in groups:
source_model.instance_groups.add(group)
assert [g.name for g in source_model.instance_groups.all()] == [
'host-4', 'host-3', 'host-2', 'host-1', 'host-0'
]
assert [g.name for g in source_model.instance_groups.order_by('name').all()] == [
'host-0', 'host-1', 'host-2', 'host-3', 'host-4'
]

View File

@ -1,5 +1,5 @@
export default
['Rest', function(Rest) {
['$q', 'Rest', function($q, Rest) {
return {
addInstanceGroups: function(url, instance_groups) {
let groups = (instance_groups || []);
@ -9,33 +9,70 @@ export default
},
editInstanceGroups: function(url, instance_groups) {
Rest.setUrl(url);
let currentGroups = Rest.get()
.then(({data}) => {
return data.results.map((i) => i.id);
return Rest.get()
.then(res => {
const { data: { results = [] } } = res;
const updatedGroupIds = (instance_groups || []).map(({ id }) => id);
const currentGroupIds = results.map(({ id }) => id);
const groupIdsToAssociate = [];
const groupIdsToDisassociate = [];
// loop over the array of currently saved instance group ids - if we encounter
// a difference between the current array and the updated array at a particular
// position, mark it and all remaining currentIds in the array for disassociation.
let disassociateRemainingIds = false;
currentGroupIds.forEach((currentId, position) => {
if (!disassociateRemainingIds && updatedGroupIds[position] !== currentId) {
disassociateRemainingIds = true;
}
if (disassociateRemainingIds) {
groupIdsToDisassociate.push(currentId);
}
});
updatedGroupIds.forEach(updatedId => {
if (groupIdsToDisassociate.includes(updatedId)) {
// we get here if the id was marked for disassociation due to being
// out of order - we'll need to re-associate it.
groupIdsToAssociate.push(updatedId);
} else if (!currentGroupIds.includes(updatedId)) {
// we get here if the id is a new association
groupIdsToAssociate.push(updatedId);
}
});
// convert the id arrays into request data
const groupsToAssociate = groupIdsToAssociate.map(id => ({ id, associate: true}));
const groupsToDisassociate = groupIdsToDisassociate.map(id => ({ id, disassociate: true }));
// make the disassociate request sequence - we need to do these requests
// sequentially to make sure they get processed in the right order so we
// build a promise chain here instead of using .all()
let disassociationPromise = $q.resolve();
groupsToDisassociate.forEach(data => {
disassociationPromise = disassociationPromise.then(() => {
Rest.setUrl(url);
return Rest.post(data);
});
});
// make the disassociate-then-associate request sequence
return disassociationPromise
.then(() => {
// we need to do these requests sequentially to make sure they get
// processed in the right order so we build a promise chain here
// instead of using .all()
let associationPromise = $q.resolve();
groupsToAssociate.forEach(data => {
associationPromise = associationPromise.then(() => {
Rest.setUrl(url);
return Rest.post(data);
});
});
return associationPromise;
});
});
return currentGroups.then(function(current) {
let groupsToAdd = (instance_groups || [])
.map(val => val.id);
let groupsToDisassociate = current
.filter(val => groupsToAdd
.indexOf(val) === -1)
.map(val => ({id: val, disassociate: true}));
let groupsToAssociate = groupsToAdd
.filter(val => current
.indexOf(val) === -1)
.map(val => ({id: val, associate: true}));
let pass = groupsToDisassociate
.concat(groupsToAssociate);
Rest.setUrl(url);
let defers = pass.map((group) => Rest.post(group));
Promise.resolve(defers);
});
}
};
}];
}
};
}];

View File

@ -532,7 +532,7 @@ export default
var credDefer = MultiCredentialService
.saveRelated(jobTemplateData, $scope.multiCredential.selectedCredentials);
InstanceGroupsService.editInstanceGroups(instance_group_url, $scope.instance_groups)
const instanceGroupDefer = InstanceGroupsService.editInstanceGroups(instance_group_url, $scope.instance_groups)
.catch(({data, status}) => {
ProcessErrors($scope, data, status, form, {
hdr: 'Error!',
@ -609,7 +609,7 @@ export default
Rest.setUrl(data.related.labels);
var defers = [credDefer];
var defers = [credDefer, instanceGroupDefer];
for (var i = 0; i < toPost.length; i++) {
defers.push(Rest.post(toPost[i]));
}