ARIA-321 Provide Clearwater IMS example #191

tliron · 2017-08-15T22:00:16Z

Related fixes included in this commit:

Allows capabilities, interfaces, and properties to override parent
definition types only if the new type is a descendant of the overridden
type
Fix bugs in model instrumentation (to allow ctx access to capability
properties and complex values)
Fix to get_property intrinsic function
Fix the "required" field for parameters (it wasn't working)
Don't let scalar values be negative
Doc fixes related to ARIA-277

asfgit · 2017-08-15T22:00:17Z

Can one of the admins verify this patch?

ran-z · 2017-09-03T09:59:27Z

examples/clearwater/clearwater-live-test-existing.yaml

+  template_name: clearwater-live-test-existing
+  template_author: ARIA
+  template_version: '1.0'
+  aria_version: '0.1.2'


the version should probably be 0.2.0

ran-z · 2017-09-03T10:01:20Z

examples/clearwater/clearwater-live-test-existing.yaml

+  inputs:
+    hosts.ssh.user:
+      type: string
+    hosts.ssh.password:


why is one "hosts.X.Y" and the other "existing_host.Z.W"? It makes it seem like the user/password and the public-address are for two different hosts, doesn't it?

I was thinking forward here to the future template that will have multiple hosts. For consistency, I thought to call "hosts." so it would apply to all hosts. This is a simpler example with only one host.

ran-z · 2017-09-03T10:03:50Z

examples/clearwater/clearwater-single-existing.yaml

+  template_name: clearwater-single-existing
+  template_author: ARIA
+  template_version: '1.0'
+  aria_version: '0.1.2'


ran-z · 2017-09-03T10:04:10Z

examples/clearwater/clearwater-single-existing.yaml

+      description: >-
+        Existing SSH password.
+      type: string
+    existing_host.public_address:


same comment about hosts vs existing_host..?

ran-z · 2017-09-03T10:19:40Z

examples/clearwater/scripts/bono/delete.sh

+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.


Why have these empty files?
Why not simply omit such operation implementations?

I'm leaving that as TODOs -- we do need a way to uininstall, too, no?

Ok - it wasn't clear that this was a TODO

+1 added TODO

ran-z · 2017-09-03T10:55:03Z

examples/clearwater/scripts/ralf/create.sh

@@ -0,0 +1,15 @@
+#!/bin/bash


why does dime's create operation install ralf? I'm not sure i understand the modeling. Is Ralf "contained in" Dime conceptually?
(even if this is the case, why have this file at all?)

In terms of software installation, installing Dime will install Ralf+Homestead. However, those components run entirely differently in terms of configuration, logging, networking, etc. I imagine that in the multi-node Clearwater there will be things in this script to configure networking (open ports) for Ralf specifically. So, again, this is left here as a TODO.

ran-z · 2017-09-03T11:21:55Z

extensions/aria_extension_tosca/simple_v1_0/modeling/data_types.py

@@ -159,10 +159,6 @@ def get_data_type(context, presentation, field_name, allow_none=False):
        else:
            return str

-    # Make sure not derived from self
-    if type_name == presentation._name:


what is this about?

The previous code was broken and threw an exception when reaching those lines. I found the bug by accident while working on this so just decided to do a spot fix.

ran-z · 2017-09-03T12:51:13Z

aria/modeling/service_instance.py

+
+        return None
+
+    def satisfy_requirements(self):


im not sure i understand this addition. who's calling this, and why is this a part of this PR?

The get_node_by_type and get_policy_by_type are utility functions for use by the ctx proxy. You can see them being used in clearwater/scripts/hosts/configure.sh. I see them as being very useful for users.

I agree, those indeed seem useful - But I was actually referring to the satisfy_requirements and other methods, which I now see are the result of a merge issue - Note that they have been moved to topology.py, and should no longer be in service_instance.py.

ran-z · 2017-09-03T13:00:07Z

aria/modeling/service_instance.py

+                return True
+        return False
+
+    def _is_node_a_target(self, source_node, target_node):


Pretty confused by this.

Who's using this?

Why not check the node's incoming_relationships?

Why is the private helper function recursive? The main public function already goes over all nodes and checks each one for a source node.

This is a leftover utility function from a previous experiment of mine (for solving an SMTP config challenge). It's not needed anymore, so I will just remove it.

ran-z · 2017-09-11T08:03:08Z

.travis.yml

  - tox --version
-  - PYTEST_PROCESSES=1 tox -e $TOX_ENV


why not keep this instead of having it on each env line?

While working on this file it seemed odd to me that this one env variable is not in the .travis.yml "env" section. It makes sense to group it here, too, because in the future we might find that some tox envs run fine with multiprocess. (The Travis VMs/containers all have 2 cores, actually. I still don't know why are our tests fail when we switch to 2 processes.)

ran-z · 2017-09-11T08:03:21Z

.travis.yml

  - pip install --upgrade pip
  - pip install --upgrade setuptools
  - pip install tox
-
-script:
-  - pip --version


why remove this? it can be useful for debugging

While working on fixing the tests, I actually added a whole bunch of stuff here to assist debugging. And then it occurred to me, why test pip version specifically? There is so much other stuff here that might be useful. (Also, we explicitly upgrade pip to the latest version when the test runs.) I think it's more confusing to have this here. If someone needs to debug something specific with the test mechanism on Travis, they can add whatever they want here (like I did) that is relevant.

ran-z · 2017-09-11T08:08:46Z

.travis.yml

+
+before_install:
+  # Create SSH keys for SSH tests
+  - ssh-keygen -f $HOME/.ssh/id_rsa -t rsa -N ''


I don't really see the benefit of replacing password with keypair usage, and so IMO it's better to avoid this extra work + hardcoding file systme path in the actual SSH test - though obviously the test is aimed specifically at Travis env and so it doesn't matter that much I guess.

As I mentioned in Slack, I added the use of keys during trying to fix the SSH tests, but when I managed to get them passing finally I decided to keep this change because I feel it tests more things: specifically the parts of Python cryptography support. Since using keys instead of passwords is by far more common in cloud platforms (it should be required, really!) I thought it would more sense to keep this test as is.

Also not that our SSH tests are specifically designed for Travis, anyway, so I don't see a problem with the .travis.yml file corresponding to it.

Related fixes included in this commit: * Allows capabilities, interfaces, and properties to override parent definition types only if the new type is a descendant of the overridden type * Fix to get_property intrinsic function * Fix the "required" field for parameters (it wasn't working) * Don't let scalar values be negative * Doc fixes related to ARIA-277 * Fix SSH tests

asfgit force-pushed the ARIA-321-clearwater branch 2 times, most recently from 5f1001d to 7679bc1 Compare August 15, 2017 22:46

ran-z suggested changes Sep 3, 2017

View reviewed changes

asfgit force-pushed the ARIA-321-clearwater branch 26 times, most recently from 5d9b24a to b5d8ab4 Compare September 7, 2017 18:59

asfgit force-pushed the ARIA-321-clearwater branch 9 times, most recently from 40f2c8a to a03cf98 Compare September 7, 2017 20:48

ran-z suggested changes Sep 11, 2017

View reviewed changes

asfgit force-pushed the ARIA-321-clearwater branch from a03cf98 to 126d4e8 Compare September 11, 2017 15:21

asfgit merged commit 126d4e8 into master Sep 11, 2017

asfgit deleted the ARIA-321-clearwater branch September 11, 2017 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARIA-321 Provide Clearwater IMS example #191

ARIA-321 Provide Clearwater IMS example #191

tliron commented Aug 15, 2017

asfgit commented Aug 15, 2017

ran-z Sep 3, 2017

tliron Sep 5, 2017

ran-z Sep 3, 2017

tliron Sep 5, 2017

ran-z Sep 6, 2017

ran-z Sep 3, 2017

tliron Sep 5, 2017

ran-z Sep 3, 2017

ran-z Sep 3, 2017

tliron Sep 5, 2017

ran-z Sep 6, 2017

tliron Sep 6, 2017

ran-z Sep 3, 2017

tliron Sep 5, 2017

ran-z Sep 3, 2017

tliron Sep 5, 2017

ran-z Sep 3, 2017

tliron Sep 5, 2017

ran-z Sep 6, 2017

ran-z Sep 3, 2017

tliron Sep 5, 2017

ran-z Sep 11, 2017

tliron Sep 11, 2017

ran-z Sep 11, 2017

tliron Sep 11, 2017

ran-z Sep 11, 2017

tliron Sep 11, 2017

ARIA-321 Provide Clearwater IMS example #191

ARIA-321 Provide Clearwater IMS example #191

Conversation

tliron commented Aug 15, 2017

asfgit commented Aug 15, 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

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