Container Admin Functions (DNA/instance management) #840

lucksus · 2019-01-09T23:13:11Z

Everything in grey is part of this PR:

Added container API functions

admin/dna/install_from_file({id, path})
admin/dna/uninstall({id})
admin/dna/list()
admin/instance/add({id, dna_id, agent_id})
admin/instance/remove({id})
admin/instance/start({id})
admin/instance/stop({id})
admin/instance/list()
admin/instance/running()

Which get added to an interface if admin = true.

Prework / Refactoring

Had to make the container live in a static place so the interface threads can get a mutable reference for calling admin functions. Note though that I left this lazy_static! CONTAINER reference as something optional to use so tests and holochain_nodejs can own a Container instance and don't run into problems as @maackle pointed out..
Interfaces can now be shutdown which happens now in Container::shutdown() but not (yet !) between admin calls. We will need that to have changes (like new installed DNA) be reflected in the registered JSON RPC functions but I've left this as a follow-up since this PR starts to get big..

Follow-ups

The big thing that is missing from this is updating interfaces after changes to the config. I almost added that to this PR already but want to think about a good solution first though. Easy catch would be to completely reload the config after any change but I think we can be more intelligent than that. I currently favor adding functions for handling interfaces (which we need anyway - like adding/removing interfaces and adding/removing instances from/to interfaces) and include a restart/update function there. Note that this would transfer responsibility for deciding when to update which interface over to the UI, i.e. HCadmin. (@zo-el, @JettTech)
File/storage management. Currently the path to the DNA file to be installed is left as it is and added to the config. I would like to have admin/dna/install_from_file copy the the DNA file into a managed directory (~/.holochain/dnas/, or that is /dnas). Next to that we could then also configure instances to have their storage in ~/.holochain/instances//storage (or /instance//storage) - currently the storage is hard-coded to memory (!).

…m interface threads

maackle · 2019-01-10T21:09:25Z

I'm concerned about introducing a singleton for this. Singletons and tests do not mix well. As long as we can keep this singleton completely out of tests, it should be OK. But the moment we want two Containers in one process, we are screwed.

I think the way to do this without a singleton would be to build the IoHandler and the Container at the same time. ContainerApiBuilder might become ContainerBuilder. The ContainerBuilder would build the Container and return it as an Arc<Mutex<Container>>, then clone that Arc and move each IoHandler closure that needs it, to construct the API. Then the Container becomes tightly coupled to its API, and you can only interact with the Container through the mutex, but you can have multiple containers in multiple mutexes.

willemolding · 2019-01-10T23:56:38Z

Making this visible, from Lucksus:

"admin/dna/install_from_file should work and I tried to confirm by actually using hc-web-client to connect to an admin interface (used container/example-configs/basic.toml for that) but I get "Method not found" for the admin function and also for info/instances which is strange. Tried to debug a bit but couldn't find the reason yet."

willemolding · 2019-01-11T00:30:24Z

In response to the above I can call info/instances and admin/dna/install_from_file using cURL with no problems. To recreate

holochain_container -c ./example-config/empty-container.toml

and

curl -X POST -H "Content-Type: application/json" -d '{"jsonrpc":"2.0","id":"0","method":"info/instances"}' http://localhost:4000

curl -X POST -H "Content-Type: application/json" -d '{"jsonrpc":"2.0","id":"0","method":"admin/dna/install_from_file"}' http://localhost:4000

…_dna_from_file to response

willemolding · 2019-01-11T13:17:30Z

admin/dna/install is now working great. I started on admin/instance/start but I've come up against an issue. It is possible to add a new instance to the config and get the container to start the instance but it also needs to rebuild its rpc interface to allow calls to the new instance. I am not sure of the best way to go about that yet.

lucksus · 2019-01-11T13:18:46Z

@willemolding, yeah, after the config was changed, the container actually needs to reload the config in a proper way.. I was anticipating that.

…ain/holochain-rust into container-admin-functions # Conflicts: # container_api/src/container.rs # container_api/src/container_admin.rs # container_api/src/interface.rs

…in container/main.rs

…erface

willemolding · 2019-01-15T07:34:28Z

container_api/src/interface.rs

+    /// To be consistent with holochain function calls the expected output of the admin calls
+    /// is stringified JSON. This performs that stringification and then wraps it in the format expected
+    /// by the jsonrpc add_method closure
+    fn format_response(response: &serde_json::Value) -> jsonrpc_core::Result<serde_json::Value> {


Added this to produce the json string format that is required

willemolding · 2019-01-15T07:35:48Z

container_api/src/interface.rs

+                let id = Self::get_as_string("id", &params_map)?;
+                let path = Self::get_as_string("path", &params_map)?;
+                container_call!(|c| c.install_dna_from_file(PathBuf::from(path), id.to_string()))?;
+                Self::format_response(&json!({"success": true}))


This produces a result as expected by hc-web-client/hc-redux-middleware and consistent with the other container functions.

zippy

This all looks very good as an important initial step. I approve of it, but we need to keep track of the followups that come from it. The big one is the singleton issue. The small one is a quick decision on using https://docs.rs/directories/1.0.2/directories/struct.ProjectDirs.html for getting our project directory, and then implementing it sooner rather than later in a way that all parts of the code that want to put things there can make use of. I will put this in the tre...

zippy · 2019-01-15T18:02:56Z

container/example-config/basic.toml

 file = "example-config/app_spec.hcpkg"
 hash = "Qm328wyq38924y"
+id = "app spec rust"


it's not a big point but I think the name "app spec rust" is probably a little confusing here in the container crate, and we should just use something like "test-dna-instance-1" or something like that. Or even, as I read the example more, "instance-for-http-testing" and "intance-for-websocket-testing" because it looks like they are hooked up to different interfaces, and that would be semantically clearer.

zippy · 2019-01-15T18:07:25Z

container_api/src/config.rs

+                    .collect();
+                interface
+            })
+            .collect();


nice. I like retain!

container_api/src/container.rs

zippy · 2019-01-15T18:22:20Z

container_api/src/container.rs

@@ -73,10 +103,15 @@ impl Container {
    /// Creates a new instance with the default DnaLoader that actually loads files.
    pub fn from_config(config: Configuration) -> Self {
        let rules = config.logger.rules.clone();
+        let config_path = dirs::home_dir()
+            .expect("No home dir defined. Don't know where to store config file")
+            .join(std::path::PathBuf::from(".holochain/container-config.toml"));


I think we probably should start using the directories crate project dir for storing these things. But I'll do that in a separate PR.

zippy · 2019-01-15T18:44:39Z

container_api/src/container.rs

@@ -638,6 +708,29 @@ pub mod tests {
        container.stop_all_instances().unwrap();
    }

+    //#[test]
+    // Default config path ~/.holochain/container-config.toml won't work in CI


but it will if we use the directories crate...

zippy · 2019-01-15T18:49:56Z

container_api/src/container_admin.rs

+
+[logger]
+type = ""
+[[logger.rules.rules]]


hmm. I don't like that the default logger rules is being populated here when the default type isn't "debug" because the rules don't even apply if the type isn't "debug".. but this is a problem with the logger, not with this code...

This needs to be here because the defaults get added when saving the config. So we need to add it to the fixture. I will post my 2nd PR on ContainerAdmin in a few minutes that includes a DRY out of these fixtures..

@zippy @lucksus In my scenario-tests branch I make "debug" the default logger_type because I assume that's what we want, would you agree?

Yeah probably but this indicates that we need to think through different default conditions. For running scenario tests, and end user probably wants the default logging just to show debug/dna messages, and maybe err/* For the container, the default is likely no debug messages. For us as holochain core devs, the default is what it currently is: All messages.

zippy · 2019-01-15T18:57:31Z

container_api/src/interface.rs

@@ -212,4 +409,24 @@ pub mod tests {
        assert!(result.contains(r#""happ-store/greeter/public/hello""#));
        assert!(!result.contains(r#""test-instance-1//test/test""#));
    }
+
+    /// The below test cannot be extented to test the other RPC methods due to the singleton design of the container


Hmm this speaks to the singleton problem, in that it's also an issue in our own container test code, not just the nodejs scenario tests....

# Conflicts: # container_api/src/error.rs

…into container-admin-functions

… with other admin functions

willemolding · 2019-01-16T05:11:24Z

Format for all admin calls is not unstringified json and all holochain calls return just a string. I am happy now and will update hc-web-client/hc-redux-middleware

…s to its module

lucksus · 2019-01-16T12:37:01Z

@maackle I've implemented the refactoring we talked about. Visibility of Container structs is now restricted to a sub-module:

pub struct Container {
    instances: InstanceMap,
    pub(in crate::container) instances: InstanceMap,
    config: Configuration,
    pub(in crate::container) config: Configuration,
    interface_threads: HashMap<String, InterfaceThreadHandle>,
    pub(in crate::container) config_path: PathBuf,
    dna_loader: DnaLoader,
    interface_threads: HashMap<String, Sender<()>>,
    pub(in crate::container) dna_loader: DnaLoader,

Please go ahead and merge if you approve :)

maackle · 2019-01-16T16:42:28Z

That's kind of a cool pattern, module-level pub. As long as the module doesn't get huge.

StaticallyTypedAnxiety · 2019-01-16T17:08:11Z

container_api/src/container/base.rs

+    pub fn shutdown(&mut self) {
+        let _ = self
+            .stop_all_instances()
+            .map_err(|error| notify(format!("Error during shutdown: {}", error)));


Why did we change the this from returning a result? stop_all_instances() seems to be mapping an error

Because if one instance errors during shutdown this function would exit early and not reset self.instances.

Logically, shutdown should always work. If an instance complains when being shutdown, it should only show in the log.

StaticallyTypedAnxiety · 2019-01-16T17:09:17Z

container_api/src/container/base.rs

@@ -246,8 +300,7 @@ impl Container {
        }

        let config = self.config.clone();
-        self.shutdown().map_err(|e| e.to_string())?;
-        self.instances = HashMap::new();
+        self.shutdown();


shutdown could propagate the error up here

Container Admin Functions (DNA/instance management)

WIP admin/dna/install

ff241d2

lucksus added the review label Jan 9, 2019

willemolding and others added 6 commits January 10, 2019 14:58

adds build with dummy with_admin_dna_functions

b5da46f

adds serializing of config to default dir

705399f

ContainerAdmin::install_dna_from_file()

7df66aa

Make Container a mutable singleton so we can call admin functions fro…

d941619

…m interface threads

Fix public address in basic.toml container config example

7fdcc84

Fix integer overflow

289c7fc

admin = true in container config example basic.toml

93ec241

adds ability to create empty containers with a config

0d68802

willemolding and others added 8 commits January 11, 2019 12:01

fixes bug where lock could not be obtained. Passes error from install…

122ecda

…_dna_from_file to response

fixes bug converting parameters to strings. Use as_str not to_string

880e057

add console output on install

48de609

adds checking config consistency before mutation in admin functions

be80f13

renames add_instance_and_start and refactors tests

b7b69d4

no message

19aadc3

adds partial test for add_instance_and_start

1328b6c

rustfmt

83c55d7

lucksus added 7 commits January 11, 2019 14:25

Merge branch 'container-admin-functions' of https://github.com/holoch…

08e4144

…ain/holochain-rust into container-admin-functions # Conflicts: # container_api/src/container.rs # container_api/src/container_admin.rs # container_api/src/interface.rs

Fix broken merge

ff53386

clean up

1a2ddc5

rustfmt

65acfaa

Add line that got lost: actual saving of config during DNA install.

de03cea

Set Container::config_path to actual file the config was loaded from …

58480e1

…in container/main.rs

Harmonize example-config/basic.toml with serde and add http admin int…

5430253

…erface

rustfmt

2a7c3ab

willemolding reviewed Jan 15, 2019

View reviewed changes

Merge branch 'develop' into container-admin-functions

9db64df

zippy approved these changes Jan 15, 2019

View reviewed changes

Merge branch 'develop' into container-admin-functions

a1d1414

# Conflicts: # container_api/src/error.rs

lucksus mentioned this pull request Jan 15, 2019

Container Admin functions for interface/agent/bridge CRUD #865

Merged

willemolding added 3 commits January 16, 2019 15:46

Merge branch 'develop' of https://github.com/holochain/holochain-rust …

c5acec0

…into container-admin-functions

modifies info/instances to return unstringified json to be consistent…

764dbee

… with other admin functions

rustfmt

287f7dd

willemolding approved these changes Jan 16, 2019

View reviewed changes

lucksus added 3 commits January 16, 2019 13:27

Refactor container modules and restrict visibility to Container field…

d05ec2b

…s to its module

rustfmt

f2a0ed9

Clarify code comment

7f90169

maackle approved these changes Jan 16, 2019

View reviewed changes

StaticallyTypedAnxiety reviewed Jan 16, 2019

View reviewed changes

Nicolas Luck added 2 commits January 16, 2019 18:14

Merge branch 'develop' into container-admin-functions

efe6387

Merge branch 'develop' into container-admin-functions

587b06c

willemolding mentioned this pull request Jan 17, 2019

Container manage dir #876

Merged

Connoropolous and others added 2 commits January 16, 2019 22:50

Merge branch 'develop' into container-admin-functions

3443337

Merge branch 'develop' into container-admin-functions

5e64766

lucksus merged commit 8a6644f into develop Jan 17, 2019

lucksus removed the review label Jan 17, 2019

maackle pushed a commit that referenced this pull request Jan 17, 2019

Merge pull request #840 from holochain/container-admin-functions

56444ee

Container Admin Functions (DNA/instance management)

maackle mentioned this pull request Jan 19, 2019

Update ContainerApiBuilder to enforce assumptions #777

Open

zippy deleted the container-admin-functions branch July 4, 2019 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container Admin Functions (DNA/instance management) #840

Container Admin Functions (DNA/instance management) #840

lucksus commented Jan 9, 2019 •

edited

Loading

maackle commented Jan 10, 2019 •

edited

Loading

willemolding commented Jan 10, 2019

willemolding commented Jan 11, 2019

willemolding commented Jan 11, 2019 •

edited

Loading

lucksus commented Jan 11, 2019

willemolding Jan 15, 2019

willemolding Jan 15, 2019

zippy left a comment

zippy Jan 15, 2019

lucksus Jan 15, 2019

zippy Jan 15, 2019

zippy Jan 15, 2019

zippy Jan 15, 2019

zippy Jan 15, 2019

lucksus Jan 15, 2019

maackle Jan 16, 2019

zippy Jan 16, 2019

zippy Jan 15, 2019

willemolding commented Jan 16, 2019

lucksus commented Jan 16, 2019

maackle commented Jan 16, 2019

StaticallyTypedAnxiety Jan 16, 2019

lucksus Jan 16, 2019

StaticallyTypedAnxiety Jan 16, 2019

Container Admin Functions (DNA/instance management) #840

Container Admin Functions (DNA/instance management) #840

Conversation

lucksus commented Jan 9, 2019 • edited Loading

Added container API functions

Prework / Refactoring

Follow-ups

maackle commented Jan 10, 2019 • edited Loading

willemolding commented Jan 10, 2019

willemolding commented Jan 11, 2019

willemolding commented Jan 11, 2019 • edited Loading

lucksus commented Jan 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zippy left a 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

willemolding commented Jan 16, 2019

lucksus commented Jan 16, 2019

maackle commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucksus commented Jan 9, 2019 •

edited

Loading

maackle commented Jan 10, 2019 •

edited

Loading

willemolding commented Jan 11, 2019 •

edited

Loading