Added instance command type #156

nesitor · 2023-04-25T17:32:36Z

No description provided.

odesenfans

First review. We need to implement support for instance messages on CCNs before merging this.

src/aleph_client/asynchronous.py

odesenfans · 2023-04-26T16:56:41Z

src/aleph_client/asynchronous.py

+    Post a (create) INSTANCE message.
+
+    :param account: Account to use to sign the message
+    :param rootfs: Root filesystem to use


What is it exactly? A message item hash? A CID?

Is a persistent volume, but with the parent option required, because this persistent volume should be the main hard disk of the instance.

src/aleph_client/asynchronous.py

odesenfans · 2023-04-26T17:01:13Z

src/aleph_client/asynchronous.py

+    :param rootfs_name: Name of root filesystem
+    :param environment_variables: Environment variables to pass to the program
+    :param storage_engine: Storage engine to use (Default: "storage")
+    :param channel: Channel to use (Default: "TEST")


Unrelated to your PR, but why the hell is "TEST" our default channel? Makes no sense. Maybe make it default to None? Channels are useless the way they are implemented anyway.

Yes, I agree with that, we can use None by default, but anyway I don't understand exactly what's the use case to use channels.

Moshe planned to use channels to split the network in smaller parts for performance reasons. This has never been implemented and may not be the way we want to implement it.

I find it kind of weird TBH, if we can leave Channel None. When having None as a valid input, I would expect a drastically different behavior on these messages than on others. Meaning that:

They would never be filtered, if a filter would be applied

They could never be filtered, as otherwise you'd have to pass in a List, mixed with str and NoneType into the node software

Could only appear if no channel filter is applied at all

When thinking about scaling, this makes me uncomfortable. What would be the primary method of sharding messages then? Sender addresses?

odesenfans · 2023-04-26T17:02:35Z

src/aleph_client/asynchronous.py

+                "seconds": timeout_seconds,
+            },
+            "rootfs": {
+                "parent": rootfs,


parent is pretty vague. I guess this is related to the device mapping you implemented on the CRNs. This is an implementation detail and should not appear as clearly. Suggestions:

rootfs_image

image

disk_image

Well, I don't agree, because we can use the parent option to extend different persistent volumes, not only the main rootfs. I think that parent is a good name for the field, but we can discuss it, obviously.

odesenfans · 2023-04-26T17:02:56Z

src/aleph_client/asynchronous.py

+    )
+
+    # Ensure that the version of aleph-message used supports the field.
+    assert content.on.persistent == persistent


odesenfans · 2023-04-26T17:06:16Z

src/aleph_client/commands/instance.py

+
+
+@app.command()
+def unpersist(


Not really applicable to instances. That's a whole other issue, but we should improve the communication between CCNs and CRNs now that CCNs are more reliable. Having a P2P pubsub channel or a websocket to retrieve operations on programs and instances would make it possible to delete a VM once the instance message is forgotten, for example.

In the meantime: remove this command as it does not make sense to "unpersist" a VM instance.

@hoh do we want a delete command that just forgets the instance instead?

I have used the same existing method on the VMs, but we can change it to delete, that also for me have better sense.

odesenfans · 2023-04-26T17:07:01Z

src/aleph_client/conf.py

        "bd79839bf96e595a06da5ac0b6ba51dea6f7e2591bb913deccded04d831d29f4"
    )
+    DEFAULT_ROOTFS_ID: str = (
+        "549ec451d9b099cad112d4aaa2c00ac40fb6729a92ff252ff22eef0b5c3cb613"


Could you add a comment describing what this rootfs is based on? Is it Ubuntu, Debian, something else?

On the client's help, it is specified: https://github.com/aleph-im/aleph-client/pull/156/files/67eeb697af6230858c593a5a93e0f88491c6bf8a#diff-0cf3f4fcc940f01848a5832101eb2274d6f19932363e17a25ff32b62d5a59f84R60

MHHukiewitz · 2023-09-27T13:38:32Z

I am going to migrate the SDK part to https://github.com/aleph-im/aleph-sdk-python, while keeping all CLI functionality mergeable.

hoh

Some comments I forgot to submit.

src/aleph_client/asynchronous.py

hoh · 2023-05-11T13:09:52Z

src/aleph_client/asynchronous.py

+    :param rootfs_name: Name of root filesystem
+    :param environment_variables: Environment variables to pass to the program
+    :param storage_engine: Storage engine to use (Default: "storage")
+    :param channel: Channel to use (Default: "TEST")


Moshe planned to use channels to split the network in smaller parts for performance reasons. This has never been implemented and may not be the way we want to implement it.

hoh · 2023-05-11T13:20:35Z

src/aleph_client/commands/instance.py

+        print_instance_message: bool = typer.Option(False),
+        rootfs: str = typer.Option(
+            None,
+            help="Hash of the rootfs to use for your instance. Defaults to aleph debian with Python3.8 and node. You can also create your own rootfs and pin it",


Suggested change

help="Hash of the rootfs to use for your instance. Defaults to aleph debian with Python3.8 and node. You can also create your own rootfs and pin it",

help="Hash of the rootfs to use for your instance. Defaults to aleph debian with Python3.9 and node. You can also create your own rootfs and pin it",

MHHukiewitz · 2023-11-25T14:35:47Z

Being superseded by #178

Added instance command type.

aa4d5b7

nesitor requested review from aliel, hoh and odesenfans April 25, 2023 17:32

nesitor self-assigned this Apr 25, 2023

nesitor added 2 commits April 25, 2023 19:43

Added needed changes on the environment to pass the tests

92020f7

Fixed missing resource to import.

67eeb69

odesenfans requested changes Apr 26, 2023

View reviewed changes

nesitor requested a review from odesenfans May 11, 2023 09:40

hoh reviewed Sep 27, 2023

View reviewed changes

MHHukiewitz mentioned this pull request Nov 14, 2023

Add create_instance method aleph-im/aleph-sdk-python#78

Merged

MHHukiewitz closed this Nov 25, 2023

	help="Hash of the rootfs to use for your instance. Defaults to aleph debian with Python3.8 and node. You can also create your own rootfs and pin it",
	help="Hash of the rootfs to use for your instance. Defaults to aleph debian with Python3.9 and node. You can also create your own rootfs and pin it",



		@app.command()
		def unpersist(

Added instance command type #156

Added instance command type #156

Uh oh!

Conversation

nesitor commented Apr 25, 2023

Uh oh!

odesenfans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MHHukiewitz commented Sep 27, 2023

Uh oh!

hoh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MHHukiewitz commented Nov 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants