ARIA-92 Automatic operation task configuration #95

tliron · 2017-04-04T23:29:15Z

Main changes:

Removed runs_on field from task model and API
Two new instantiations phase, one to find the host nodes, and the second to configure operation instances for the execution plugin
Parser fills in configuration field in OperationTemplate using dependencies in TOSCA syntax
Fixes to reqs-and-caps bugs

asfbot · 2017-04-04T23:29:16Z

Can one of the admins verify this patch?

ran-z · 2017-04-05T07:08:53Z

aria/modeling/orchestration.py

    """

    __tablename__ = 'task'

    __private_fields__ = ['node_fk',
                          'relationship_fk',
                          'plugin_fk',
-                          'execution_fk',


The diff algorithm borked here, if you look at the file it makes sense.

ran-z · 2017-04-05T07:09:59Z

aria/modeling/orchestration.py

    retry_interval = Column(Float, default=0)
    ignore_failure = Column(Boolean, default=False)
+    due_at = Column(DateTime, nullable=False, index=True, default=datetime.utcnow())


if you already do the separation to "state" fields and other, this should probably be under state.

Does due_at ever change after creation? I guess you're implying that it does...

ran-z · 2017-04-05T10:14:56Z

aria/orchestrator/workflows/api/task.py

        super(OperationTask, self).__init__()

        self.actor = actor
+        self.actor_type = actor_type


why are any changes needed here (besides runs_on)?

Because we decided together that the logic that was here should not be in operation creation, but in service instance creation.

ran-z · 2017-04-05T10:41:19Z

extensions/aria_extension_tosca/profiles/tosca-simple-1.0/interfaces.yaml

@@ -63,24 +63,48 @@ interface_types:
    pre_configure_source:
      description: >-
        Operation to pre-configure the source endpoint.
+      _extensions:
+        default_dependencies:
+          - edge > source


as we mentioned, these should not be a part of the dependencies. it should sit elsewhere.

furthermore, for the time being, it's not "default", but rather the only place where one can determine where the operation shall run.

I thought you said that if there is no cost to allowing the user to override it, we could.

ran-z · 2017-04-05T10:44:19Z

tests/modeling/test_models.py

@@ -583,48 +583,49 @@ def test_node_model_creation(self, node_template_storage, is_valid, name, runtim
                   node_template_storage.service.list()[0]


-class TestNodeIP(object):
+class TestNodeHostAddress(object):


why change this everywhere?
should really talk about massive renames first, seems like a waste to have to revert everything if that'll be needed

That whole part of the code (finding the "ip") had to be refactored because it was essentially broken. As I was changing it anyway it made sense to change the name. It's really not hard to change back.

ran-z · 2017-04-05T10:45:29Z

tests/resources/service-templates/tosca-simple-1.0/node-cellar/node-cellar.yaml

+                    implementation:
+                      primary: changed.sh
+                      dependencies:
+                        - edge > target


ran-z · 2017-04-05T10:46:16Z

aria/parser/consumption/modeling.py

@@ -145,6 +145,24 @@ def consume(self):
        self.context.modeling.instance.validate_capabilities()


+class FindHosts(Consumer):


why is this implemented as another consumer?

This is just how phases work right now in "the parser". We keep talking about all of this moving into a separate "instantiation" module, but this is what we have for now.

ran-z · 2017-04-05T10:48:27Z

aria/orchestrator/execution_plugin/instantiation.py

+from ...parser.consumption import ConsumptionContext
+
+
+def configure_operation(operation):


almost everything in this module needs to go. this method is okay, the rest should be transparent to the execution plugin

OK, where do you suggest moving them?

ran-z · 2017-04-05T10:50:33Z

aria/modeling/service_instance.py

@@ -1407,6 +1441,19 @@ def operation_template_fk(cls):

    # endregion

+    def configure(self):


this definitely cant be here.

OK, so where? We don't have these separate module yet and I did not think I should be creating it as part of this PR.

ran-z · 2017-04-05T10:55:49Z

aria/modeling/service_instance.py

@@ -180,6 +181,16 @@ def validate_capabilities(self):
                satisfied = False
        return satisfied

+    def find_hosts(self):


more methods on the models which we'll want to remove in a short while :(
i mean, this method isn't relevant for an instantiated service - it's only used for its (and its dependencies) creation. why would it sit as part of the instance?

That's not true: this phase will need be run every time there is a change in the topology. This is part of the reason I don't think instantiation is the right name for this module we talk about, since it would actually be used in many cases after instantiation.

ran-z · 2017-04-05T10:58:23Z

aria/modeling/service_instance.py

+                        return host
+            return None
+
+        self.host = _find_host(self)


why is this dynamic? are we expecting this to change for an existing node?

I don't know what you mean by "dynamic" here. These functions must be called during the initial instantiation phase, and then will need to be called again whenever there is a change in the topology.

mxmrlv · 2017-04-05T12:21:05Z

aria/modeling/service_instance.py

+                    if host is not None:
+                        return host
+            for the_relationship in node.inbound_relationships:
+                if (the_relationship.target_capability is not None) and \


this seems odd, target_capability is basicaly self.capabilities, isn't it? why would i need to traverse all the inbound relationships an test each ones target_capability?

This is a bit hard to explain. We tend to think of the host relationship as "contained in", but TOSCA also allows for "hosts a" kind of relationship, for example for features. See here: to find out the loadbalancer node's host, we need to look inbound that it is a feature of Nginx.

So, bottom line is that this function has to be bi-recursive, both inbound and outbound. Yay. :)

mxmrlv · 2017-04-05T12:22:40Z

aria/modeling/service_instance.py

@@ -529,6 +524,32 @@ def validate_capabilities(self):
                satisfied = False
        return satisfied

+    def find_host(self):
+        def _find_host(node):


why would i need to define a helper method, and not just call the_relationship.target_node.find_host() for example?

If we do that then we'll be calling find_host again on again on the same nodes that are targets of different relationships. So, to do that efficiently we would need some extra flag (found_host?) to signify that we don't have to do that again for that node. It made more sense to me to have it set once and only once per node.

im still a bit confused by this myself - caching the host is definitely a good idea, but it looks here like the internal method _find_host is called recursively repeatedly.
I'm not sure this is exactly the same as what Maxim meant, but I think I'd expect each node's host finding method to run once at most, so that would mean either "host" is a self-initializing property, or a field initialized at its constructor, or you could use the "find_host" method, but basically what i'm missing here is the check for whether the value has been cached already or not.

i'd expect it to be more like:

def find_host(self): def _find_host(node): .... host = the_relationship.target_node.find_host() .... if not self.host: # or indeed use some extra flag self.host = _find_host(node) return self.host

mxmrlv · 2017-04-05T12:24:50Z

aria/modeling/service_template.py

+                plugin = self.plugin_specification.find_plugin(workflow_context.model.plugin.list())
+            except context.exceptions.ContextException:
+                pass
+                # TODO


Yes, we need to talk about this. :) I don't know how else to find other models. Is there a current context for SQLAlchemy?

mxmrlv · 2017-04-05T13:20:26Z

tests/modeling/test_models.py

@@ -583,48 +583,49 @@ def test_node_model_creation(self, node_template_storage, is_valid, name, runtim
                   node_template_storage.service.list()[0]


-class TestNodeIP(object):
+class TestNodeHostAddress(object):


no new tests were written for the new feature.

ran-z · 2017-04-12T13:20:44Z

aria/modeling/service_instance.py

-        if host_ip_property:
-            return host_ip_property.value
+    def host_address(self):
+        if self.host:


ran-z · 2017-04-12T13:21:39Z

aria/modeling/service_instance.py

+    def host_address(self):
+        if self.host:
+            if self.host.runtime_properties:
+                return self.host.runtime_properties.get('host_address')


while I understand your reasoning to using "host_address" instead of "ip" everywhere, the actual runtime property that gets set by plugins remains to be "ip". this can't change at this stage.

ran-z · 2017-04-12T13:37:22Z

aria/modeling/service_instance.py

-    def plugin_specification_fk(cls):
-        """For Operation one-to-one to PluginSpecification"""
-        return relationship.foreign_key('plugin_specification', nullable=True)
+    def plugin_fk(cls):


does it make sense for operation no longer to have an fk for plugin specification at all?
is that going to remain for operation template?

ran-z · 2017-04-12T13:38:35Z

aria/modeling/service_instance.py

    dependencies = Column(modeling_types.StrictList(item_cls=basestring))
    executor = Column(Text)
    max_retries = Column(Integer)
    retry_interval = Column(Integer)

+    def configure(self):
+        from . import models
+        if (self.implementation is None) or (self.interface is None):


how can interface be None here?

It will be None if the operation is attached to Service as a workflow :) I know you suggested a different model entirely for workflows ... that would be a separate JIRA perhaps.

oh, right. Please add a tiny comment next to it explaining that this case is only relevant for "Workflow operations" because I'm afraid I'll wonder about it the next time I see it as well 😅 thanks!

ran-z · 2017-04-12T13:39:45Z

aria/modeling/service_instance.py

+            return
+
+        if self.plugin is None:
+            arguments = configure_operation(self)


please use execution_plugin.instantiation.configure_operation or so, there are enough configure_operation calls in this module as it is and it's easily confusing :)

ran-z · 2017-04-12T13:47:21Z

tests/resources/service-templates/tosca-simple-1.0/node-cellar/node-cellar.yaml

@@ -98,8 +98,8 @@ topology_template:
              #token: { get_property: [ HOST, flavor_name ] }
      interfaces:
        Maintenance:
-          enable: juju > charm.maintenance_on
-          disable: juju > charm.maintenance_off
+          enable: juju \> charm.maintenance_on


wait so what's this symbol about again? :)
Why are we not using the same symbol as we do for dependencies?

I've put the backslash to escape it and disable it for now. :/ Since the Juju plugin is not installed, the tests that use this would fail otherwise.

I'm changing this back. :) Will explain the new behavior on Slack.

ran-z · 2017-04-12T14:05:50Z

extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py

@@ -78,6 +78,11 @@ def get_inherited_capability_definitions(context, presentation, for_presentation
                #capability_definitions[capability_name] = capability_definition
            else:
                capability_definition = our_capability_definition._clone(for_presentation)
+                if isinstance(capability_definition._raw, basestring):
+                    # # Make sure we have a dict


double comment

ran-z · 2017-04-12T14:14:10Z

aria/orchestrator/workflows/builtin/utils.py

        )
    if target_operation_name:
        operations.append(
            OperationTask.for_relationship(relationship=relationship,
                                           interface_name=interface_name,
-                                           operation_name=target_operation_name,
-                                           runs_on='target')
+                                           operation_name=target_operation_name,)


👍 (merge leftover -- though it's totally legal in Python and in fact is a tuple!)

yes i know, I just figured this is not what you wanted to have there 😅 anyway i see its fixed, thanks

ran-z · 2017-04-12T14:24:37Z

extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py

+                    level=Issue.BETWEEN_TYPES)
+
+        relationship_edge = operation._get_extensions(context).get('relationship_edge')
+        if relationship_edge is not None:


what if it is None? where do we set the default? I think either we don't or I've missed it :)

None is the default -- it means that nowhere did the user set the edge explicitly set one way or the other (the column is nullable).

Don't you think it'd be better to have it be set to source by default?
I assume when it's None we should treat it as source, so I think it'd be better if the data model reflects this design decision, no?

We agreed that it's better to keep this "edge was not set" information.

ran-z · 2017-04-12T14:26:10Z

extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py

+            model.plugin_specification = service_template.plugin_specifications.get(plugin_name)
+            if model.plugin_specification is None:
+                context.validation.report(
+                    'unknown plugin "%s" specified in operation implementation: %s'


is this message clear enough about the fact that the plugin is not properly declared as a policy in the service template (rather than the question of whether it exists or not in the plugin repository)?

ran-z · 2017-04-12T14:26:57Z

extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py

-            model.plugin_specification, model.implementation = \
-                parse_implementation_string(context, service_template, prop.value)
+            plugin_name, model.implementation = split_prefix(prop.value)
+            if plugin_name is not None:


duplicate code, why not refactor this?

ran-z · 2017-04-12T14:30:08Z

extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py


-    implementation = implementation[index+1:].strip()
-    return plugin_specification, implementation
+def set_nested(the_dict, keys, value):


could you add a few comments here, possibly an example of input --> output, just so it's clearer? thanks

ran-z · 2017-04-12T14:39:07Z

aria/modeling/service_template.py

+            # moved to. There, we will probably have a context with a storage manager. Until then,
+            # this is the only potentially available context, which of course will only be available
+            # if we're in a workflow.
+            plugin = None


ran-z · 2017-04-12T14:41:37Z

aria/modeling/service_template.py

@@ -1848,11 +1856,30 @@ def as_raw(self):

    def instantiate(self, container):
        from . import models
+        from ..orchestrator import context
+        plugin = None
+        if self.plugin_specification is not None:


I'm not sure I understand this section.
Operations are instantiated at service creation time. No workflow context should be available at this time.
If a storage model object is required, it should be passed into the instantiate module directly (the current interface is problematic as is, and I've already broken it on the CLI branch in order to pass inputs to service template's instantiate), not taken off some thread local context.

You are correct, I had a long conversation with Maxim about it. Eventually when we have an instantiation module, it is very likely that the module will have a context. See comment at line 1861.

I understand and I have seen the comment, but I'm saying the changes I've mentioned need to happen now, not after we have the instantiation module.
More so, I don't understand how this is expected to work anyway, as no execution context is expect to be present at the time this method is called.

ran-z · 2017-04-12T15:31:17Z

aria/orchestrator/workflows/api/task.py

+        if interface is not None:
+            operation = interface.operations.get(operation_name)
+
+        if operation is None:


(FYI this section is refactored elsewhere in the CLI branch, so it will get overridden here too)

ran-z · 2017-04-12T15:34:54Z

aria/orchestrator/workflows/api/task.py

+                'Could not find operation "{0}" on interface "{1}" for {2} "{3}"'
+                .format(operation_name, interface_name, actor_type, actor.name))
+
+        if operation.implementation is None:


Why should an empty operation raise an error (rather than do nothing)?
if a node template has its type as root, and implements create with a proper implementation but does not use configure, would it not appear as an empty implementation operation?

What is the meaning of an empty implementation operation? The operation would fail in the orchestrator, seems much better to catch that here and not create that operation.

We agreed to keep this behavior for now.

ran-z · 2017-04-12T15:54:02Z

aria/orchestrator/execution_plugin/instantiation.py

+    """
+    Remote SSH operation via Fabric.
+    """
+    default_user = 'admin'


why these defaults? wouldnt it be better to fail?

Oops, leftover from devs. 👍

ran-z · 2017-04-12T15:54:47Z

aria/orchestrator/execution_plugin/instantiation.py

+    arguments['fabric_env']['password'] = ssh.get('password')
+    arguments['fabric_env']['key'] = ssh.get('key')
+    arguments['fabric_env']['key_filename'] = ssh.get('key_filename')
+    if 'address' in ssh:


i assume you came up with this one?

Well, I read our code and figured these out. :) They were not well documented anywhere.

ran-z · 2017-04-12T15:55:00Z

aria/orchestrator/execution_plugin/instantiation.py

+        ssh['user'] = default_user
+    if ('password' not in ssh) and ('key' not in ssh) and ('key_filename' not in ssh):
+        ssh['password'] = default_password
+    arguments['use_sudo'] = ssh.get('use_sudo')


why not use copy of some sort?

Due to the various validations. The structure is also a little bit different. In the service template, the user doesn't have to write "fabric_env" (or really shouldn't care about our implementation detail at all), the just write "ssh.password".

ran-z · 2017-04-12T15:57:59Z

aria/orchestrator/execution_plugin/instantiation.py

+                                  .format(name, full_type_name(the_type)),
+                                  level=validation.Issue.BETWEEN_TYPES)
+
+def _str_to_bool(value, name):


this and the next method need to sit elsewhere. on CLI branch there's a type module under utils, and the dict_to_list should go to where there's already similar methods.

the validation thats called within these methods can be called before or after them

ran-z · 2017-04-12T16:00:29Z

aria/orchestrator/execution_plugin/instantiation.py

+        return {}
+    _validate_type(value, dict, 'ssh')
+    for k, v in value.iteritems():
+        if k == 'use_sudo':


i think it'd probably have been neater to have a dict mapping field-name to type and then validating it rather than having to add here manually every new field that might get introduced

I thought it would be more straightforward to do this than have a generic mechanism. Since only this plugin does this (for all the rest the configuration params get merged into inputs) it seemed unnecessary for me to create a generic mechanism.

ran-z · 2017-04-13T11:34:57Z

extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py

+        model.plugin_specification = service_template.plugin_specifications.get(plugin_name)
+        if model.plugin_specification is None:
+            context.validation.report(
+                'no policy for plugin "%s" specified in operation implementation: %s'


use string format please 😅

ran-z · 2017-04-14T18:29:58Z

aria/modeling/service_template.py

+        if self.plugin_specification and self.plugin_specification.enabled:
+            plugin = self.plugin_specification.plugin
+            implementation = self.implementation if plugin is not None else None
+            # "plugin" would be none if a match was not found. In that case, a validation error


im not sure i understood this comment.
i think you're trying to say that plugin can't be None here since the test for enabled has already been performed earlier?
the first line of the comment makes it sound like plugin can be None when it cant

ran-z · 2017-04-14T18:31:43Z

aria/modeling/service_template.py


    def coerce_values(self, container, report_issues):
        pass

    def instantiate(self, container):
-        from . import models


wait, so instantiate is now empty? 😕

ran-z reviewed Apr 5, 2017

View reviewed changes

ran-z suggested changes Apr 5, 2017

View reviewed changes

mxmrlv reviewed Apr 5, 2017

View reviewed changes

asfgit force-pushed the ARIA-92-plugin-in-implementation-string branch 3 times, most recently from fb80405 to 3bb7e4c Compare April 7, 2017 18:50

ran-z reviewed Apr 12, 2017

View reviewed changes

aria/modeling/service_instance.py Outdated

if host_ip_property:

return host_ip_property.value

def host_address(self):

if self.host:

Copy link

ran-z Apr 12, 2017

Choose a reason for hiding this comment

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

&&? :P

ran-z reviewed Apr 12, 2017

View reviewed changes

asfgit force-pushed the ARIA-92-plugin-in-implementation-string branch from 3bb7e4c to a062d12 Compare April 12, 2017 17:09

ran-z reviewed Apr 13, 2017

View reviewed changes

asfgit force-pushed the ARIA-92-plugin-in-implementation-string branch 2 times, most recently from 6e807d9 to fe5e964 Compare April 14, 2017 17:16

ran-z reviewed Apr 14, 2017

View reviewed changes

asfgit force-pushed the ARIA-92-plugin-in-implementation-string branch from fe5e964 to b5a373a Compare April 14, 2017 18:53

ARIA-92 Automatic operation task configuration

a7e7826

asfgit force-pushed the ARIA-92-plugin-in-implementation-string branch from b5a373a to a7e7826 Compare April 14, 2017 19:05

asfgit merged commit a7e7826 into master Apr 14, 2017

hubot deleted the ARIA-92-plugin-in-implementation-string branch April 28, 2017 17:09

hubot restored the ARIA-92-plugin-in-implementation-string branch April 28, 2017 22:05

asfgit deleted the ARIA-92-plugin-in-implementation-string branch June 7, 2017 19:51

		@@ -145,6 +145,24 @@ def consume(self):
		self.context.modeling.instance.validate_capabilities()


		class FindHosts(Consumer):

		from ...parser.consumption import ConsumptionContext


		def configure_operation(operation):

		@@ -1407,6 +1441,19 @@ def operation_template_fk(cls):

		# endregion

		def configure(self):

ARIA-92 Automatic operation task configuration #95

ARIA-92 Automatic operation task configuration #95

Conversation

tliron commented Apr 4, 2017

asfbot commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ran-z Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ran-z Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tliron Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ran-z Apr 5, 2017 •

edited

Loading

ran-z Apr 12, 2017 •

edited

Loading

tliron Apr 12, 2017 •

edited

Loading