2019-06-27 19:52:16

by Michał Lowas-Rzechonek

[permalink] [raw]
Subject: Re: Was: mesh: Added ImportLocalNode call with its API --> Multiple Methods?

Hi everyone,

On 06/27, Gix, Brian wrote:
> I am starting to think we may need multiple methods here to deal with
> the desired use cases:

I don't like this. Back in the day we discussed that we'd like to avoid
D-Bus API bloat...

> * ImportNodeProvData()
> (...)
> The daemon method would perform a GetManagedObject of the calling
> application, to create a node.json with all of the Composition data,
> elements, models, features, etc.

While the initial idea was indeed about importing the whole node,
including configuration, models etc., during implementation we figured
that the reason to do it is to eventually Attach() to such a node - it
doesn't make sense to import something you wouldn't use.

If so, then you need to have an appropriate application anyway, which
the daemon can simply query for all the needed info, as it does at the
moment during Join() and CreateNetwork() calls. This nicely fits into
REQUEST_TYPE processing in get_managed_objects_cb().

This results in smaller API and *significantly* simpler code. As you
mentioned, doing a full migration is complicated.

> * MigrateNode()
> This method would be what Inga and I had originally envisioned for the
> ImportLocalNode() call... It would contain *all* the information that
> a pre-existing node had... including preconfigured pub/sub, features,
> Sequence value that reflected that this node already existed elsewhere
> on the mesh, but was simply being migrated to this device, etc.

The application can do all the configuration using loopback Config
Server messages, so I don't think we need a Migrate() call at all. The
application already receives current node configuration when
Attach()ing, so it can determine if something needs to be reconfigured.

And again, you would need an Attach()able application anyway, so all the
information would need to be duplicated in application's D-Bus
interfaces and in the JSON passed to Migrate() call. This seems like a
violation of DRY principle.

regards
--
Michał Lowas-Rzechonek <[email protected]>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND


2019-06-28 14:30:21

by Gix, Brian

[permalink] [raw]
Subject: Re: Was: mesh: Added ImportLocalNode call with its API --> Multiple Methods?

Hi All,

On Thu, 2019-06-27 at 21:51 +0200, [email protected] wrote:
> Hi everyone,
>
> On 06/27, Gix, Brian wrote:
> > I am starting to think we may need multiple methods here to deal with
> > the desired use cases:
>
> I don't like this. Back in the day we discussed that we'd like to avoid
> D-Bus API bloat...
>
> > * ImportNodeProvData()
> > (...)
> > The daemon method would perform a GetManagedObject of the calling
> > application, to create a node.json with all of the Composition data,
> > elements, models, features, etc.
>
> While the initial idea was indeed about importing the whole node,
> including configuration, models etc., during implementation we figured
> that the reason to do it is to eventually Attach() to such a node - it
> doesn't make sense to import something you wouldn't use.
>
> If so, then you need to have an appropriate application anyway, which
> the daemon can simply query for all the needed info, as it does at the
> moment during Join() and CreateNetwork() calls. This nicely fits into
> REQUEST_TYPE processing in get_managed_objects_cb().
>
> This results in smaller API and *significantly* simpler code. As you
> mentioned, doing a full migration is complicated.
>
> > * MigrateNode()
> > This method would be what Inga and I had originally envisioned for the
> > ImportLocalNode() call... It would contain *all* the information that
> > a pre-existing node had... including preconfigured pub/sub, features,
> > Sequence value that reflected that this node already existed elsewhere
> > on the mesh, but was simply being migrated to this device, etc.
>
> The application can do all the configuration using loopback Config
> Server messages, so I don't think we need a Migrate() call at all. The
> application already receives current node configuration when
> Attach()ing, so it can determine if something needs to be reconfigured.


I don't see this actually working very well. Firstly, these "Loop-back" Config Server messages do not all
actually work unless the node is a Provisioner, which will *not* be required of most nodes.

And However, even if the App went through all of the restoration of all the settings under the control of the
Provisioner via this Config Server loop-back method, there is still the critical issue of Sequence numbers.

A node that is being migrated needs to pick up where it was prior to the migration, assuming that it has
already had conversations with other nodes, and has entries in their Replay Protection List.


If we want a single method (avoid API bloat) I don't think we have any choice but to use a well developed
structure (like JSON or XML) that faithfully sets up the node in the state it was in prior to Migration. We
can perhaps "Overload" this functionality by allowing a minimal JSON with only Prov Data parts, if we are
looking for a Provisioning shortcut, and always requiring the ObjectManager calls fetch the Composition (if the
JSON was minimal) and to Sanity check the Composition (if the JSON contains a fully developed/configured
Migrated node).

>
> And again, you would need an Attach()able application anyway, so all the
> information would need to be duplicated in application's D-Bus
> interfaces and in the JSON passed to Migrate() call. This seems like a
> violation of DRY principle.
>
> regards

2019-07-01 09:23:59

by Michał Lowas-Rzechonek

[permalink] [raw]
Subject: Re: Was: mesh: Added ImportLocalNode call with its API --> Multiple Methods?

Hi,

On 06/28, Gix, Brian wrote:
> > The application can do all the configuration using loopback Config
> > Server messages, so I don't think we need a Migrate() call at all. The
> > application already receives current node configuration when
> > Attach()ing, so it can determine if something needs to be reconfigured.
>
> I don't see this actually working very well. Firstly, these
> "Loop-back" Config Server messages do not all actually work unless the
> node is a Provisioner, which will *not* be required of most nodes.

I don't think this is true. Application can send messages from the
application to its node using straightforward Send() call with key index
0x7fff, and they are processed correctly. While this might be considered
a "hack", you've seen that I am currently working on DevKeySend() API.

As far as I can tell, a node always has its own Device Key in the
keyring, so loopback messaging via DevKeySend() is going to work even on
non-provisioner nodes.

> And However, even if the App went through all of the restoration of
> all the settings under the control of the Provisioner via this Config
> Server loop-back method, there is still the critical issue of Sequence
> numbers.

Indeed. I would propose simply adding starting sequence number to the
dictionary passed to ImportLocalNode() call. It can be made optional,
i.e. when the application doesn't provide it, the imported node starts
counting from 0.

> If we want a single method (avoid API bloat) I don't think we have any
> choice but to use a well developed structure (like JSON or XML) that
> faithfully sets up the node in the state it was in prior to Migration.
>
> We can perhaps "Overload" this functionality by allowing a minimal
> JSON with only Prov Data parts, if we are looking for a Provisioning
> shortcut, and always requiring the ObjectManager calls fetch the
> Composition (if the JSON was minimal) and to Sanity check the
> Composition (if the JSON contains a fully developed/configured
> Migrated node).

Ok, that sounds better. We could start by implementing the "Provisoining
shortcut" variant, and add full-blown migration when it's needed.

Would that be OK from your POV?

regards
--
Michał Lowas-Rzechonek <[email protected]>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

2019-07-01 16:39:45

by Gix, Brian

[permalink] [raw]
Subject: RE: Was: mesh: Added ImportLocalNode call with its API --> Multiple Methods?

Hi Michal.

> > We can perhaps "Overload" this functionality by allowing a minimal
> > JSON with only Prov Data parts, if we are looking for a Provisioning
> > shortcut, and always requiring the ObjectManager calls fetch the
> > Composition (if the JSON was minimal) and to Sanity check the
> > Composition (if the JSON contains a fully developed/configured
> > Migrated node).
>
> Ok, that sounds better. We could start by implementing the "Provisoining
> shortcut" variant, and add full-blown migration when it's needed.
>
> Would that be OK from your POV?

This would be OK for me. How about Inga?

BR,
Brian

2019-07-02 05:45:07

by Stotland, Inga

[permalink] [raw]
Subject: Re: Was: mesh: Added ImportLocalNode call with its API --> Multiple Methods?

Hi Brian, Michal, Jakub,

On Mon, 2019-07-01 at 08:57 -0700, Gix, Brian wrote:
> Hi Michal.
>
> > > We can perhaps "Overload" this functionality by allowing a
> > > minimal
> > > JSON with only Prov Data parts, if we are looking for a
> > > Provisioning
> > > shortcut, and always requiring the ObjectManager calls fetch the
> > > Composition (if the JSON was minimal) and to Sanity check the
> > > Composition (if the JSON contains a fully developed/configured
> > > Migrated node).
> >
> > Ok, that sounds better. We could start by implementing the
> > "Provisoining
> > shortcut" variant, and add full-blown migration when it's needed.
> >
> > Would that be OK from your POV?
>
> This would be OK for me. How about Inga?
>
> BR,
> Brian


So what is the final versionof the ImportLocalNode() will look like?

uint64 token ImportLocalNode(object app_path, array{byte} uuid, string
config_data)

or

uint64 token ImportLocalNode(object app_path, array{byte} uuid, byte
config_data_type, string config_data)

where config_data_type indicates the format of config_data (json, xml,
etc)

A bit clunky, but, if we want to keep everything wrapped in one method
call, I don't see a way around this.

Also, my feeling is that app_path = NULL shuold be allowed. In this
case node/app configuration is incurred solely from config_data, daemon
will check for the presence of all the mandatory settings.
In case both app_path and fully fleshed config_data are provided, a
series of checks will need to be performed to validate the coherncy of
the configuration.

Best regards,
Inga



Attachments:
smime.p7s (3.19 kB)