Skip to content

WIP: Support changing servertypes #57

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.

Sign up for GitHub

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

Draft
RonjaPonja wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

WIP: Support changing servertypes #57

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e46133
query_committer: Support changing servertypes
Jan 10, 2019
29cd3ae
Format fixes
Jan 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 106 additions & 13 deletions serveradmin/serverdb/query_committer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,6 @@ def commit_query(created=[], changed=[], deleted=[], app=None, user=None):
if not user:
user = app.owner

pre_commit.send_robust(
commit_query, created=created, changed=changed, deleted=deleted
)

entities = []
if not user.is_superuser:
entities.append(
Expand All @@ -81,9 +77,14 @@ def commit_query(created=[], changed=[], deleted=[], app=None, user=None):
in list(attribute_lookup.values()) + list(Attribute.specials.values())
}

_resolve_servertype_changes(changed, joined_attributes)
pre_commit.send_robust(
commit_query, created=created, changed=changed, deleted=deleted
)

with transaction.atomic():
change_commit = ChangeCommit.objects.create(app=app, user=user)
changed_servers = _fetch_servers(set(c['object_id'] for c in changed))
changed_servers = _fetch_servers({c['object_id'] for c in changed})
changed_objects = _materialize(changed_servers, joined_attributes)

deleted_servers = _fetch_servers(deleted)
Expand Down Expand Up @@ -116,7 +117,8 @@ def commit_query(created=[], changed=[], deleted=[], app=None, user=None):


def _validate(attribute_lookup, changed, changed_objects):
servertype_attributes = _get_servertype_attributes(changed_objects)
servertype_ids = {s['servertype'] for s in changed_objects.values()}
servertype_attributes = _get_servertype_attributes(servertype_ids)

# Attributes must be always validated
violations_attribs = _validate_attributes(
Expand Down Expand Up @@ -359,6 +361,12 @@ def _log_changes(commit, changed, created_objects, deleted_objects):


def _fetch_servers(object_ids):
"""Fetch servers with row lock

Returns a dict from object_id to an instance of the Server model. Raise
an error if an object in question (no longer) exists. Lock the row of all
instances in question.
"""
servers = {
s.server_id: s
for s
Expand All @@ -373,15 +381,21 @@ def _fetch_servers(object_ids):


def _materialize(servers, joined_attributes):
"""Get a dict from object_id to DatasetObject

QueryMaterializer will get the current values of joined_attributes and
resolve joins recursively.
"""
return {
o['object_id']: o
for o in QueryMaterializer(list(servers.values()), joined_attributes)
}


def _get_servertype_attributes(servers):
def _get_servertype_attributes(servertype_ids):
"""Get a dict from servertype names to dicts of attributes"""
servertype_attributes = dict()
for servertype_id in {s['servertype'] for s in servers.values()}:
for servertype_id in set(servertype_ids):
servertype_attributes[servertype_id] = dict()
for sa in Servertype.objects.get(pk=servertype_id).attributes.all():
servertype_attributes[servertype_id][sa.attribute_id] = sa
Expand All @@ -397,11 +411,6 @@ def _validate_attributes(changes, servers, servertype_attributes):
attributes = servertype_attributes[server['servertype']]

for attribute_id, change in attribute_changes.items():
# If servertype is attempted to be changed, we immediately
# error out.
if attribute_id == 'servertype':
raise CommitValidationFailed('Cannot change servertype', [])

# We have no more checks for the special attributes.
if attribute_id in Attribute.specials:
continue
Expand Down Expand Up @@ -617,6 +626,90 @@ def _validate_real_attributes(servertype, real_attributes): # NOQA: C901
)


def _resolve_servertype_changes(changed, joined_attributes):
"""Expand servertype changes

Changing servertypes likely entails deleting attributes not available on
the target servertype.
"""

# Filter for servertype changes
servertype_changes = [
change for change in changed
if 'servertype' in change.keys()
]

# No servertype changes, we're done here
if not servertype_changes:
return

# Forbid deleting the servertype attribute
for change in servertype_changes:
if (
change['servertype']['action'] != 'update' or
'new' not in change['servertype'] or
not change['servertype']['new']
):
CommitError('Cannot delete servertype')

# Get attributes of the new and old servertypes to check what changes
involved_servertype_ids = chain(*[
[change['servertype']['old'], change['servertype']['new']]
for change in servertype_changes
])
Copy link
Contributor

@hasegeli hasegeli Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to *[].

Copy link
Contributor Author

@RonjaPonja RonjaPonja Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this doesn't seem to get flattened without *[] 🤔

Traceback (most recent call last):
  File "/Users/space/.local/share/virtualenvs/serveradmin-m3Kn4UAw/lib/python3.5/site-packages/django/core/handlers/exception.py", line 41, in inner
    response = get_response(request)
  File "/Users/space/.local/share/virtualenvs/serveradmin-m3Kn4UAw/lib/python3.5/site-packages/django/core/handlers/base.py", line 249, in _legacy_get_response
    response = self._get_response(request)
  File "/Users/space/.local/share/virtualenvs/serveradmin-m3Kn4UAw/lib/python3.5/site-packages/django/core/handlers/base.py", line 187, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/Users/space/.local/share/virtualenvs/serveradmin-m3Kn4UAw/lib/python3.5/site-packages/django/core/handlers/base.py", line 185, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/Users/space/.local/share/virtualenvs/serveradmin-m3Kn4UAw/lib/python3.5/site-packages/django/contrib/auth/decorators.py", line 23, in _wrapped_view
    return view_func(request, *args, **kwargs)
  File "./serveradmin/servershell/views.py", line 305, in commit
    commit_query(changed=changed, deleted=deleted, user=user)
  File "./serveradmin/serverdb/query_committer.py", line 80, in commit_query
    _resolve_servertype_changes(changed, joined_attributes)
  File "./serveradmin/serverdb/query_committer.py", line 663, in _resolve_servertype_changes
    involved_servertype_ids
  File "./serveradmin/serverdb/query_committer.py", line 398, in _get_servertype_attributes
    for servertype_id in set(servertype_ids):
TypeError: unhashable type: 'list'

Copy link
Contributor

@hasegeli hasegeli Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I see, chain() doesn't seem to be the right thing though. Some use sum(([...] for x in y), []) to concatenate items, but a simple for to append items is more efficient and more readable.


try:
servertype_attributes = _get_servertype_attributes(
involved_servertype_ids
)
except Servertype.DoesNotExist:
raise CommitError('Unknown servertype')

# The fetching and materializing of servers here is duplicating queries
# done in the commit_query transaction. We need to do this to resolve all
# the real attribute changes, resulting from a servertype change, before
# sending the pre_commit signal. Alternatively we could send the
# pre_commit signal inside the commit_query transaction but sending the
# signal can take many seconds if a receiver misbehaves.
Copy link
Contributor

@hasegeli hasegeli Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get this.

Copy link
Contributor Author

@RonjaPonja RonjaPonja Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out commit_query(). There we have the real transaction committing changes to the database. You chose to put the signals before and after the transactions as the signals are blocking until all receivers are done. To expand what attributes I have to delete I first have to materialize the object though. I need to expand the changes first as otherwise the pre_commit and post_commit hook would have different values for 'changed'. That would suck.

Copy link
Contributor Author

@RonjaPonja RonjaPonja Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So before my change the behavior was:

  • pre_commit hook
  • transaction
    • materialize the objects
    • commit the objects
  • post_commit hook

Now the behavior is:

  • transaction
    • materialize objects with changed servertypes
  • expand servertype changes to real attribute changes
  • pre_commit hook
  • transaction
    • materialize all objects
    • commit the objects
  • post_commit hook

Copy link
Contributor

@hasegeli hasegeli Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise the pre_commit and post_commit hook would have different values for 'changed'. That would suck.

I think it is fair that they are different. Complicating the flow in here may have more downsides.

with transaction.atomic():
Copy link
Contributor

@hasegeli hasegeli Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not okay to start another transaction in the same request. If things fail after committing this one, you would end up persisting part of the commit. Whatever critical need to go the .atomic() block started after calling this function.

Copy link
Contributor

@hasegeli hasegeli Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything that write to the database is "critical". There are more of them later on this function.

Copy link
Contributor Author

@RonjaPonja RonjaPonja Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this function doesn't write to the database at all, except for the _fetch_servers row lock, which is why I added this transaction in the first place. I want this function to only expand the changes and then put the changes through the normal commit process. Am I missing something here?

changed_servers = _fetch_servers(
{c['object_id'] for c in servertype_changes}
)
changed_objects = _materialize(changed_servers, joined_attributes)


# Mark all attributes, not available on the new servertype, for deletion.
for change in changed:
if change not in servertype_changes:
continue

old = set(servertype_attributes[change['servertype']['old']].keys())
new = set(servertype_attributes[change['servertype']['new']].keys())
for del_attr in old - new:
if del_attr in change:
raise CommitError(
'Attribute value of {} needs to be deleted for servertype '
'change, but was modified in this commit.'.format(del_attr)
)

changed_object = changed_objects[change['object_id']]
if changed_object[del_attr]:
change[del_attr] = {
'action': 'update',
'old': changed_object[del_attr],
'new': None
}

# TODO: Handle attributes beeing required on the new servertype that are
# not available on the old servertype. Changing servertypes means the
# available attributes change. Adminapi and servershell are not equipped
# for this. Hence we can not set values for these attributes in the same
# commit for now. While this is not super clean, the user will be forced
# to set a correct value once he opens an edit dialog for this object in
# servershell. I think that's good enough for now as changing servertypes
# is an edgecase anyway.
Copy link
Contributor

@hasegeli hasegeli Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convince it would be good enough. Especially not having the defaults on the changed objects may cause real trouble.

Copy link
Contributor Author

@RonjaPonja RonjaPonja Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I loop over all servertype_attributes of the new servertype and if it has a default value and the object does not have that attribute set, I set the default. Would that be enough?

Copy link
Contributor

@hasegeli hasegeli Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess. See get_default_attribute_values().



def _insert_server(hostname, intern_ip, servertype, attributes):

if Server.objects.filter(hostname=hostname).exists():
Expand Down