Skip to content
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.

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

Orchestration interfaces #233

Merged
merged 14 commits into from
Jun 4, 2019
Merged

Conversation

kyllingstad
Copy link
Member

@kyllingstad kyllingstad commented Apr 3, 2019

I started looking into issue #165 and tried to think of how it can be put into a larger orchestration context, and here are some ideas that I'd like to discuss.

Here, I take "orchestration" to include one or more of the following tasks:

  1. Getting information about available models (⊇ FMUs).
  2. Starting up slaves based on specific models, possibly remotely.
  3. Connecting to already-up-and-running, possibly remote, slaves so they can be included in a simulation.

Premises:

  • Given the definitions above, and given the fact that we will support both local simulations (with local, on-disk FMUs) and distributed simulations, some level of orchestration will be needed in CSE.
  • We probably won't make the one orchestration framework to rule them all, as that will be highly platform specific. For example, simulations run locally on a single machine, on DNV GL's OSP Foundation service, or on SINTEF's SIMA cloud, will all be handled entirely differently.

What I propose here is therefore not to make the actual distributed orchestration system, but rather to try to define some interfaces and utility classes that are simple enough, and general enough, that they can support any orchestration system we can think of. We can supply some generally useful implementations, to support things like local slaves and on-disk FMUs, but leave the more advanced stuff to the platforms themselves.

The basic idea is to use URIs to refer to models and slaves. A local FMU URI could for example be file:///models/my_model.fmu, while an FMU-proxy remote FMU URI could look like fmu-proxy://sim.example.com:6345?url=http://example.com/my_model.fmu. A slave URI for an existing DCP slave could be dcp-slave://10.0.0.52:9054.

Based on my definition of "orchestration" further up, we can divide the classes into the same three levels:

  1. Obtaining an overview of available models: model_directory
  2. Accessing and using specific models: model, model_uri_sub_resolver, model_uri_resolver
  3. Accessing slaves that are already up and running: slave_uri_sub_resolver, slave_uri_resolver

We may or may not want/need all levels; that is part of what I'd like to discuss. Should we decide to go further with this, I think the concrete implementations we'd add to CSE Core would include:

  • fmi::fmu_directory, implementing model_directory to track on-disk FMUs.
  • fmi::fmu (already exists) would implement model.
  • file_uri_resolver, implementing model_uri_sub_resolver to support file:// URIs.
  • fmuproxy::remote_fmu (introduced in PR Feature/160 fmuproxy integration #162) would implement model. This is where Generic FMU interface #165 is actually addressed in the present PR. ;)
  • fmuproxy::uri_resolver, implementing model_uri_sub_resolver to support e.g. fmu-proxy:// URIs.
  • dcp::slave_uri_resolver, implementing slave_uri_sub_resolver to support e.g. dcp-slave:// URIs for already-instantiated DCP slaves.

Please destroy. :)

@kyllingstad kyllingstad added enhancement New feature or request discussion needed Let's have a discussion about this labels Apr 3, 2019
@kyllingstad kyllingstad self-assigned this Apr 3, 2019
@kyllingstad
Copy link
Member Author

We added some implementation code to this PR during a mob session yesterday, but for the sake of the discussion here, let's keep focus on the contents of cse/orchestration.hpp. I'll remove the "discussion needed" label and change the PR title when the rest is ready for a full review.

@markaren
Copy link
Contributor

markaren commented May 7, 2019

In order to not pollute this PR, I've made a separate branch where fmu-proxy makes use of this.
https://github.com/open-simulation-platform/cse-core/tree/feature/165-generic-fmu-interface-fmuproxy

PR: #239

@markaren
Copy link
Contributor

markaren commented May 8, 2019

Btw, I have implemented the uri-resolver for fmu-proxy now. Supports urls, files and pre-loaded guids.

src/cpp/ssp_parser.cpp Outdated Show resolved Hide resolved
@markaren
Copy link
Contributor

markaren commented May 8, 2019

I like this PR.

But, somehow we must be able to support SSP sources that is just a relative path (to comply with the standard this is actually the ONLY uri that have to be supported. Everything else is optional)

So in the SSP, we should be able to specify as the source:

relative/path/to/fmu.fmu <---- required by the SSP standard
file://C:/absolute/path/to/fmu.fmu
file://relative/path/to/fmu.fmu
http://example.com/path/to/fmu.fmu
fmu-proxy://host:port?<file/url/guid>=...
++

Right?

@kyllingstad
Copy link
Member Author

Absolutely. See my comment regarding URI resolution above. This is exactly what I mean. It's just like how the base URI for an HTML file is its own URL, and inside it you can use both relative paths and absolute URLs for links, images, etc.

We have no need for model_directory and slave_uri_resolver at the moment.
It is better to re-introduce them at a later time if needed.
@markaren
Copy link
Contributor

markaren commented May 9, 2019

For FMU-proxy to support relative file paths, the source uri must be modified with the baseUri.

e.g. fmu-proxy://localhost:9090?file=Demo.fmu must be changed to ...?file=C:/demo/Demo.fmu

To support this I changed resolvein generic_uri_resolver to :

std::string _uri;
auto npos = std::string_view::npos;
if ((uri.find(':') == npos) || uri[0] == '.')
{
    _uri = std::string(baseUri) + "/" + std::string(uri); 
 } else if (uri.find("fmu-proxy") != npos && uri.find("?file=") != npos) {
    _uri = std::string(uri).insert(uri.find("=")+1, std::string(baseUri.substr(8)) + "/");
} else {
     _uri = std::string(uri);
 }

However, this inject knowledge about fmu-proxy into cse-core.. So any thoughts?

@kyllingstad
Copy link
Member Author

So any thoughts?

In the update to this PR that I will push soon, I have replaced the poor-man's URI handling that you see here (which is completely wrong) with RFC 3986 compliant URI handling code. Part of that is a relative-to-base resolution function like the resolve_relative_uri() example I gave earlier. The idea was for all client code to use this, and to drop the baseUri argument from model_uri_resolver::lookup_model().

However, there is no standards-compliant way to do what you want here. The algorithm for resolving a URI reference relative to a base URI is given by the RFC, and it doesn't work like this.

A way to support what you want to do would be to push the relative reference resolution all the way down to the model_uri_sub_resolver implementations, i.e., add a baseUri parameter to model_uri_sub_resolver::lookup_model(). Then it would be up to the sub-resolvers whether they want to use the standards-compliant method or not.

How does that sound?

@markaren
Copy link
Contributor

markaren commented May 9, 2019

Sounds good to me!

@kyllingstad kyllingstad changed the title Orchestration interfaces (for discussion; not ready to merge) Orchestration interfaces May 10, 2019
@kyllingstad kyllingstad removed the discussion needed Let's have a discussion about this label May 10, 2019
@markaren
Copy link
Contributor

Sorry for the spam. I've found out that the query returned from resolve_reference is correct.
However, the uri passed to the sub_uri resolvers is incorrect. The query then includes the ?..

src/cpp/ssp_parser.cpp Outdated Show resolved Hide resolved
@markaren
Copy link
Contributor

markaren commented May 10, 2019

With uri=file:///C:/Demo/Controller.fmu
Is it correct that uri.path() == "/C:/Demo/Controller.fmu" ?

@kyllingstad
Copy link
Member Author

kyllingstad commented May 10, 2019

With uri=file:///C:/Demo/Controller.fmu
Is it correct that uri.path() == "/C:/Demo/Controller.fmu" ?

Yes. The uri class is supposed to be scheme agnostic, and then the leading slash is counted as part of the "path" component. It's only for the file scheme, and then only on Windows, that the leading slash should (sometimes) be removed.

I'll add a function to convert a file URI to a filesystem path, i.e. the reverse of make_file_uri(). Does that sound OK?

@markaren
Copy link
Contributor

I'll add a function to convert a file URI to a filesystem path, i.e. the reverse of make_file_uri(). Does that sound OK?

I guess :P

Btw, should I add a url resolver to this PR? I can re-use the logic used by fmi4cpp . However, this will add a dependency to cURL. Do we want that?

@kyllingstad
Copy link
Member Author

Btw, should I add a url resolver to this PR?

What would it do? For http addresses, you mean?

At any rate, I think this PR is probably too big already. So let's make a new issue/PR to discuss that.

@kyllingstad
Copy link
Member Author

kyllingstad commented May 13, 2019

It's alive!

Man, it's annoying when Windows decides to do things differently from all other OSes, so you have to switch back and forth between them when developing. :-/

@markaren
Copy link
Contributor

Btw, should I add a url resolver to this PR?

What would it do? For http addresses, you mean?

At any rate, I think this PR is probably too big already. So let's make a new issue/PR to discuss that.

Yes. But we can make a future issue on it as you say.

@markaren
Copy link
Contributor

markaren commented May 14, 2019

Mhh.. I notice that absolute file paths in SSP is not supported by this PR.
It cannot handle e.g paths like C:/my_fmu.fmu due to the :. Thus you have to add the file:// part, but then authors needs to use file:// on linux and file:/// on windows. Somewhat confusing.

Also the unzipper can't find the FMU, probably related to the baseUri resolving to a file and not a directory.
Should absolute paths (in SSP) be supported though? It's not an requirement from the SPP standard.

@markaren markaren self-requested a review May 14, 2019 07:41
@kyllingstad
Copy link
Member Author

file:// with only two slashes is never valid on any platform, it's always three. The cases supported by this PR are:

  • Absolute file URI on POSIX: file:///foo/bar.fmu
  • Absolute file URI on Windows: file:///C:/foo/bar.fmu
  • Relative URI on any platform: foo/bar.fmu

The URI standard doesn't treat filesystem paths as a special case, so your example C:/my_fmu.fmu will be interpreted as an uri of the scheme C with a path /my_fmu.fmu.

Does SSP specifically allow the use of plain (non-URI) absolute paths on Windows?

@markaren
Copy link
Contributor

markaren commented May 14, 2019

Then I was wrong about the slashes. I just noticed in code that there was some special handling of these and thought it would map to something like the above.

But anyway, absolute paths won't work regardless of the format in the SSP right now. But the question remains, do we want or need to support it?

@kyllingstad
Copy link
Member Author

Also the unzipper can't find the FMU, probably related to the baseUri resolving to a file and not a directory.

In which case? All the tests pass for this PR now, including the SSP parser tests.

That said, I just realised that I forgot to change the base URI for SSP like we agreed. I'll be AFK for the remainder of this week, so if anyone else wants to take a stab at it, feel free. It is only a matter of a change in ssp_parser.cpp. Remember to ensure that there is a slash at the end of the URI. This may help: https://stackoverflow.com/a/33387566

@markaren
Copy link
Contributor

markaren commented May 14, 2019

Damn, sorry. I must have pasted to wrong directory path into the .ssd. Good thing you are here to correct me.

Perhaps we should print the path of the FMU we are trying to import when it fails?

@kyllingstad
Copy link
Member Author

But anyway, absolute paths won't work regardless of the format in the SSP right now. But the question remains, do we want or need to support it?

It looks to me like the SSP standard specifically requires URIs, and I don't think we should violate that.

@markaren
Copy link
Contributor

You are absolutely right.

@kyllingstad kyllingstad force-pushed the feature/165-generic-fmu-interface branch from 8dc2f34 to 5b62ec8 Compare May 20, 2019 06:21
@kyllingstad
Copy link
Member Author

That said, I just realised that I forgot to change the base URI for SSP like we agreed.

I made the change, but then I undid it again. On closer inspection, it turns out that the SSP standard actually specifies that the base URI for an SSD is the URI of the SSD file itself (as opposed to its containing directory). I suppose this may be to allow distribution of standalone SSD files.

@kyllingstad kyllingstad force-pushed the feature/165-generic-fmu-interface branch from f0763b5 to 20e24d6 Compare May 27, 2019 14:35
src/cpp/ssp_parser.cpp Outdated Show resolved Hide resolved
@kyllingstad kyllingstad merged commit 1c6084d into master Jun 4, 2019
@kyllingstad kyllingstad mentioned this pull request Jun 4, 2019
@kyllingstad kyllingstad deleted the feature/165-generic-fmu-interface branch June 5, 2019 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants