tliron / puccini

Cloud topology management and deployment tools based on TOSCA
https://puccini.cloud
Apache License 2.0
88 stars 20 forks source link

Keywords TARGET and SOURCE are not supported in output mapping #121

Closed Shishqa closed 1 year ago

Shishqa commented 1 year ago

Example

Let's consider an example, where I want to change some attributes through a relationship:

tosca_definitions_version: tosca_simple_yaml_1_3

relationship_types:

  CustomDependsOn:
    derived_from: tosca.relationships.DependsOn
    interfaces:
      Configure:
        operations:
          configure:
            implementation: playbook.yaml
            outputs:
              var: [ TARGET, tosca_id ]

topology_template:

  node_templates:

    node_2:
      type: tosca:Root
      requirements:
        - dependency:
            node: node_1
            relationship:
              type: CustomDependsOn

    node_1:
      type: tosca:Root

The puccini gives a parsing error:

PROBLEMS (1)
  file:/home/shishqa/dev/tmp/puccini/example.yaml
    @13,22 relationship_types["CustomDependsOn"].interfaces["Configure"].operations["configure"].outputs["var"][0]: unknown node template: TARGET

Puccini was built from latest commit f8c4d215ac1c75eb0645771425af60ff711909ad

Expected

As TARGET and SOURCE are known when parsing the template, puccini can set a nodeTemplateName to the exact node bound to the relationship

Details

For now, puccini can parse such an example:

tosca_definitions_version: tosca_simple_yaml_1_3

topology_template:

  node_templates:

    node_2:
      type: tosca:Compute
      interfaces:
        Standard:
          operations:
            configure:
              implementation: playbook.yaml
              outputs:
                var: [ node_1, tosca_id ]

    node_1:
      type: tosca:Compute

And produces:

outputs:
    var:
      nodeTemplateName: node_1
      targetType: attribute
      target: tosca_id

This is done with the following code: https://github.com/tliron/puccini/blob/f8c4d215ac1c75eb0645771425af60ff711909ad/tosca/grammars/tosca_v2_0/output-mapping.go#L55-L74

And the current implementation of mapping for the relationship almost duplicates the implementation for node: https://github.com/tliron/puccini/blob/f8c4d215ac1c75eb0645771425af60ff711909ad/tosca/grammars/tosca_v2_0/output-mapping.go#L76-L94

It is not intuitive for me, how to get the requirement's nodeTemplateName or source node's name in this function context. Is this possible?

tliron commented 1 year ago

Sorry, this was my omission. The spec isn't clean on outputs in notification and operation definitions, but you are right that there is an additional section [3.6.15] (not linked from the previous definitions) that does specify that SOURCE and TARGET can be used.

It is not possible to implement this in this area of the code, because the requirement resolution phase has not happened yet and there is neither target nor source. That information is only available in Clout and created by the resolution JavaScript code.

Let's think about this a bit. One solution could be to just pass through these special names as is to Clout and let it be handled in JavaScript, perhaps the resolution JavaScript code can specifically look for these mappings.

tliron commented 1 year ago

Actually, section [3.6.15] says more things -- that the mapping supports nested attributes, ... more work will be necessary here.

tliron commented 1 year ago

The new commit has a solution. I would appreciate your feedback! Note that you have to --coerce to get the final TARGET name.

From the commit description:

output mappings for operations and notifications now fully support TOSCA 1.3 section 3.6.15, with modelable entity names (SELF, SOURCE, TARGET) and nested attributes. This works by making outputs into values, specifically a list of strings, a TOSCA path. If the entity is SOURCE or TARGET then the first element becomes a function call to the new internal _get_modelable_entity which will return the correct node template name.

Shishqa commented 1 year ago

Thank you for fast response!

I've checked your commit, but for me it still doesn't seem to be working:

tosca_definitions_version: tosca_simple_yaml_1_3

relationship_types:

  CustomDependsOn:
    derived_from: tosca.relationships.DependsOn
    valid_target_types:
      - tosca:Compute
    interfaces:
      Configure:
        operations:
          pre_configure_target:
            implementation: playbook.yaml
            outputs:
              ip: [ TARGET, public_address ]

topology_template:

  node_templates:

    server_2:
      type: tosca:Compute
      requirements:
        - dependency:
            node: server_1
            relationship:
              type: CustomDependsOn

    server_1:
      type: tosca:Compute

Here I want to set the target's ip address, for example. And puccini gives:

> puccini-tosca parse example.yaml 
PROBLEMS (1)
  file:/home/shishqa/dev/tmp/puccini/example.yaml
    @15,29 relationship_types["CustomDependsOn"].interfaces["Configure"].operations["pre_configure_target"].outputs["ip"][1]: unknown attribute reference in relationship "relationship": public_address

I assume, this happens because self.setRelationship still looks for attributes inside the relationship: https://github.com/tliron/puccini/blob/f73035b66939a4b0a095df8e483963d2e7978e00/tosca/grammars/tosca_v2_0/output-mapping.go#L126-L134

tliron commented 1 year ago

You are correct, I shall fix!

tliron commented 1 year ago

OK, another fix. I think I figured it all out this time. :)

From the commit:

We can now validate attributes against the relevant node and relationship types. Note, however, that we cannot validate the TARGET attributes, because the target node type is not known until the target node template is resolved. We at best know the base type only.

Shishqa commented 1 year ago

Thank you for the fixes! Now it works good for me.

As far as I understand, the comment https://github.com/tliron/puccini/blob/01a008b170190c3e94dbc8f3f91916a160edb0bb/tosca/grammars/tosca_v2_0/output-mapping.go#L22-L24 should be extended as well :)

tliron commented 1 year ago

Actually ... there is another issue I just noticed with validation. If the attribute is inside a capability it will error right now. One more fix!

tliron commented 1 year ago

For reference, you can see this bug with the following example:

tosca_definitions_version: tosca_simple_yaml_1_3

relationship_types:

  CustomDependsOn:
    derived_from: tosca.relationships.DependsOn
    valid_target_types:
      - tosca:Compute
    interfaces:
      Configure:
        operations:
          pre_configure_target:
            implementation: playbook.yaml
            outputs:
              ip: [ SOURCE, endpoint, ip_address ]

topology_template:

  node_templates:

    server_2:
      type: tosca:Compute
      requirements:
        - dependency:
            node: server_1
            relationship:
              type: CustomDependsOn

    server_1:
      type: tosca:Compute
tliron commented 1 year ago

@Shishqa If you can help validate this final commit, maybe we can close the issue.

Shishqa commented 1 year ago

@tliron I've created a PR with a test case, that covers your example and other enhancements. Don't sure if it's not bloated, maybe it's better to add more output mappings to existing examples. I'd be happy to hear your opinion.

tliron commented 1 year ago

Many thanks for this! As you've probably noticed, Puccini does not have a real test suite. Running through these examples is just a smoke test, to see that nothing is really broken, but that doesn't test that features work or how they work. (If they emit no Clout and no error, the tests would still pass!)

The purpose of these examples is more about teaching the basics of TOSCA, an overview of all the features that Puccini supports.

Would you consider reworking your PR to enhance the existing interfaces example? It does have an output mapping (for a notification) but it's indeed quite anemic and doesn't have the richness of your full example in showing how outputs can be used.

Shishqa commented 1 year ago

Yes, I understand that this is just an example and not a real test. But this is still some kind of validation, as go test fails without commits related to #121. I've reworked PR to enhance interfaces example. I am now concerned if the example with DHCP is correct, but at least it shows all the related features :)

Shishqa commented 1 year ago

@tliron, is the issue still unresolved? Or should I change the example (e.g. change the domain)?

tliron commented 1 year ago

I simply have not had time to look at it... Soon I hope. :)

tliron commented 1 year ago

Thank you for the PR, and my deep apologies for the long wait to merge it!