[on ice] add a clean option to the admin/instance/remove method

[jamesray1]

PR summary

Closes #1303 (or at least aims to), going with the first option to add a clean option to the "admin/instance/remove" method, and if this option is true, remove the instance's storage.

testing/benchmarking notes

( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here ) Adds a test test_remove_instance_clean_true, and modifies test_remove_instance_clean_false

Note that there are a lot of commits because my laptop freezes or is very slow when trying to test locally, so I'm using the CI for that. I'm intending to get mobile broadband set up so that I can use my desktop at home. I also used it for testing building, but I've found that I can do that locally.

followups

~~#1817~~

changelog

Please check one of the following, relating to the CHANGELOG-UNRELEASED.md

• [x] this is a code change that effects some consumer (e.g. zome developers) of holochain core so it is added to the CHANGELOG-UNRELEASED.md (linked above), with the format - summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)
• [ ] this is not a code change, or doesn't effect anyone outside holochain core development

[jamesray1]

~~Not sure how to get the storage path of the instance correctly when passing it to remove_dir_all. So the remove_instance_ tests are failing. There are also many "Tried to use dropped state" errors from https://github.com/holochain/holochain-rust/pull/1569.~~

[jamesray1]

~~I can't see whether there is a clear way to get the storage configuration path from an instance. If the StorageConfiguration type is memory, there won't be a persistent file path, while if it it File or Pickle then there will be a path. Perhaps I am missing something, but I think there needs to be a new function to get a Some of this storage configuration path, if it exists, or return None if not?~~

[jamesray1]

After looking through the code and API I found this article: https://developer.holochain.org/docs/guide/state_actions/, through a search in the codebase. So to get a better understanding of Holochain, I'm going to finish reading the guidebook, as it looks like I missed all of the completed articles when I got up to the (E) Going Live with Holochain Apps section.

[jamesray1]

~~I'm still struggling to figure out how to best get the storage path from an instance. instance.state().agent().chain_store().content_storage() won't work because I don't know what the address is for the storage path. I'll try to figure out how to get the InstanceConfiguration from an Instance. Once I can read the InstanceConfiguration I can then access the StorageConfiguration, then use a get_path method, if applicable.~~

Nevermind, the config is accessible in the remove_instance method already.

[jamesray1]

~~One of the stress tests is failing (details here, although I'm not sure why.~~ TODO: modify test to confirm storage is deleted.

[jamesray1]

Just need to figure out a way to test that the storage of the instance has been removed without using what I've added.

[jamesray1]

For future extensibility with testing it seems like it would be helpful to generate more customizable tomls, by passing fields or using defaults. However, that would be best be done in a separate PR, perhaps before continuing with this one. The scenario I have is here, is I can:

• create a pub fn instance1_with_file_storage(), which is like instance1, just with type = 'file' and path = 'TBD'. Not very elegant
• create a toml from scratch. Again, pretty verbose.
• create a instance_custom with optional arguments for each field and defaults.

By extension, this could be extended to other more complex toml block creators that have multiple fields in a block.

[jamesray1]

More details:

create_test_conductor is used in test_remove_instance_clean_true. create_test_conductor uses test_toml, which in turn uses instance1(). And, as explained above, instance1() uses the memory variant of storage, without a persistent path. So in order to create a proper test_remove_instance_clean_true, that will actually be able to check whether the storage directory is removed, I need a different toml. To do that, it seems like it would be best to be able to pass a different instnace block to what are currently created with test_toml and instance1. For just this PR, the easiest way seems like to just add an option to select whether to use a memory type, with no path, or a file type, with some path relative to the config path, and then adjust wherever instance1() is called.

pub fn test_toml(test_name: &str, port: u32) -> String {
        let mut toml = header_block(test_name);

        toml = add_block(toml, agent1());
        toml = add_block(toml, agent2());
        toml = add_block(toml, dna());
        toml = add_block(toml, instance1());
        toml = add_block(toml, instance2());
        toml = add_block(toml, interface(port));
        toml = add_block(toml, logger());
        toml
    }

    pub fn instance1() -> String {
        r#"[[instances]]
agent = 'test-agent-1'
dna = 'test-dna'
id = 'test-instance-1'

[instances.storage]
type = 'memory'"#
            .to_string()
    }

[jamesray1]

Hmm, wasn't expecting this fail:

thread 'conductor::admin::tests::test_remove_instance_clean_true' panicked at 'The storage directory for the instance doesn't exist after creating the test conductor!', crates/conductor_lib/src/conductor/admin.rs:1267:9

[jamesray1]

I'm not sure why the fmt test is required, but it looks like a bunch of diffs related to the changes I've made. Would people like to review? @AshantiMutinta, @maackle, @thedavidmeister, @zippy, @ddd-mtl, @lucksus. Edit: just read through the README at https://github.com/rust-lang/rustfmt, and I will look into fixing the formatting.

[jamesray1]

Okay, all checks are passing, feel free to review!

[jamesray1]

Need to write app spec tests.

[thedavidmeister]

i think i missed some of the original context around this, @maackle ?

[maackle]

So, for context: this is based on a pretty old issue #1303 which I wrote as an idea for fixing the problem one might encounter as a DNA developer, where you modify your DNA but try to load it in your conductor with the same instance ID as before, and if you're using file storage, now you've got a collision with your old storage because the directory name is based on the instance ID.

It is now 6 months later. @jamesray1, this is good work, but I think a good first step would have been to have a discussion in the issue about how to approach this. Since the issue was created, this problem was tackled in another direction by https://github.com/holochain/holochain-rust/pull/1335, which instead of clearing out the stale storage, makes it impossible to load DNA from a storage that does not match the intended hash.

Unfortunately we have been pretty bad about using GH issues for visibility of what we're working on. I feel personally responsible for this one since I created the issue and even marked it as a good first issue, so, sorry for this being misleading. We have some leveling up to do around making it easier for others to contribute!

I think this could still be a useful feature, but it might look a little different now, so I wouldn't want to merge it in right now. We are pretty focused on other priorities right now in preparation for Holo closed alpha launch, so we don't even have the bandwidth to come back to discuss this issue right now -- so this PR will have to sit for a while. I appreciate you jumping in to work on this, though, and doing a thorough job of it. Beginning of next year we will be able to broaden our focus, and maybe even have a better practice about doing triage on issues for contributors to take on.

[jamesray1]

Sorry that I didn't try to get more clarity and have a discussion about what would be the right approach. Please feel free to work with me and let me know if there is anything I can do to help with the closed alpha launch.

[thedavidmeister]

@jamesray1 @maackle i think a really good thing to do would be to triage issues and open these discussions with the goal to either:

• close the issue if not relevant any more
• keep the issue open as a low priority, but add information that can help us meaningfully progress without re-treading the same discussions/investigations on the next pass
• write some code now that resolves an issue

just sort the issue queue by oldest and find things to knock off :)

i used to do the same thing with the old drupal issue queues and ended up triaging literally thousands of issues that were 6-9+ years old at the time!

i've found that code is often the last thing that needs doing on an active open source issue queue :sweat_smile:

[jamesray1]

I'm happy to work with you all on clearing out stale issues and resolving ones that are still relevant, but as @maackle said, that is not high on the list of priorities right now, with the Holo closed alpha launch, so I don't want to distract you from those efforts. I did start looking for a job yesterday, as I got the impression based on @maackle's comment above that there was lack of capacity to onboard new developers at the moment. Note that I don't have the ability to manage issues, e.g. label, assign, or close.

For now, it seems like it may be best change approach to working on recent issues that seem important, rather than older ones that are more likely to be outdated or not as important, or just harder to resolve. Obviously that's not the best long-term but it may be better to contribute this way. :S

[jamesray1]

I can have a look into this again and see how it can be updated after looking at #1335, while trying to work on it independently, because as mentioned, bandwidth to discuss it is limited right now. Then again, there may be other issues that have a higher priority. On the other hand, the longer this sits on ice, the harder it will be to update to the current codebase. Still, it seems like it's best to let this one sit for a while as suggested.