Nvme o tcp poc #1884
Nvme o tcp poc #1884
Conversation
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
ogayot
force-pushed
the
nvme-o-tcp-poc
branch
4 times, most recently
from
9c173b5
to
b8c2431
Compare
| @@ -704,12 +707,21 @@ class Dasd: | |||
| preserve: bool = False | |||
|
|
|||
|
|
|||
| @fsobj("nvme_controller") | |||
| class NVMeController: | |||
| transport: str | |||
There was a problem hiding this comment.
This is a pretty finite list, correct? An Enum would be nice.
There was a problem hiding this comment.
I guess we should consider what happens if there is a NVMe-oF devices of some other kind on the system.
There was a problem hiding this comment.
Yes, I'm not really sure what the other values could be. "rdma" would probably be one of them but I haven't tested it.
Curtin's storage config will not complain if it encounters a value that it does not know about (it only knows about "tcp" really, not even "pcie"). Instead of raising a ValueError if the value for transport is unknown, I'd rather attempt the install. wdyt?
There was a problem hiding this comment.
I think I agree with your plan here
subiquity/ui/mount.py
Outdated
| @@ -79,6 +79,16 @@ def enable_common_mountpoints(self): | |||
| if isinstance(opt.value, str): | |||
| opt.enabled = opt.value in common_mountpoints | |||
|
|
|||
| def disable_all_mountpoints_but_home(self): | |||
| """Currently, filesystems of non-local disks can only be mounted at | |||
| /home.""" | |||
There was a problem hiding this comment.
The comment isn't quite right as users can type whatever they like into the "other" box, including things like / and /boot. Is it worth displaying a warning like we do when the user mounts a partition at say / without reformatting it?
There was a problem hiding this comment.
Good point, updated the comment and added a warning message:
| if device: | ||
| ofstype = device.original_fstype() | ||
| if ofstype: | ||
| self.existing_fs_type = ofstype | ||
| initial_path = initial.get("mount") | ||
| if not initial_path and remote_storage: | ||
| initial["mount"] = "/home" |
There was a problem hiding this comment.
Probably shouldn't do this if there is already a mount for /home?
There was a problem hiding this comment.
Yeah, it's not ideal. But since we show an error message in the form when a mount already exists for the mountpoint, I think it's not that bad.
| @@ -117,6 +117,8 @@ def desc(device): | |||
| def _desc_disk(disk): | |||
| if disk.multipath: | |||
| return _("multipath device") | |||
| if disk.on_remote_storage(): | |||
| return _("NVMe/TCP drive") | |||
There was a problem hiding this comment.
This looks a bit like an easy thing to forget if we add iscsi or nvme-oF or whatever; I think you should poke under the covers a bit more here.
There was a problem hiding this comment.
Updated with a default value of "remote drive". Currently no scenario should lead to this value being displayed since only NVMe over TCP drives are considered remote.
ogayot
force-pushed
the
nvme-o-tcp-poc
branch
from
7e4eb41
to
827ab4f
Compare
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
When trying to delete a partition using the answers-based mechanism, subiquity tries to call .done() on the ConfirmDeletesStretchy overlay. However, this method does not exist. The .confirm() method is what we should use instead. Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
ogayot
force-pushed
the
nvme-o-tcp-poc
branch
from
827ab4f
to
c1105df
Compare
|
The curtin bits are merged now. @mwhudson it's just missing your approval (since you requested changes earlier) before I can merge. Thanks! |
This PR adds basic support for NVMe over TCP, with the following limits:
/homecurrently) can be placed on remote storageThis PR depends on https://code.launchpad.net/~ogayot/curtin/+git/curtin/+merge/458446 for proper NVMe-over-TCP detection.
For testing, you can
s/simple/nvme-over-tcp/in the makefile and then usemake dryrunSome things to address before merging:
I also wanted to ensure that API endpoints would enforce restrictions on remote storage, but since we are going to change the restrictions in the near future, I don't think it is worth the effort here. Desktop is also not the main use-case for NVMe over TCP AFAICT.