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

Add usbip mount option #3895

Closed
wants to merge 3 commits into from
Closed

Conversation

irakhlin
Copy link

@irakhlin irakhlin commented Sep 25, 2022

Proposed change

This proposed change resolves feature requested tracked here: #3861
A new boolean option (usbip) is added which can be used to mount required '/sys' mount points inside addon container.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to the supervisor)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes Supervisor addon functionality lacks ability to allow usbip bind with home assistant os 9.0rc2 #3861
  • This PR is related to issue:
  • Link to documentation pull request:
  • Link to cli pull request:
  • In addition to added tests I was able to perform a 'integration test' using a physical amd64 nuc running home assistant OS 9. This can be reproduced by deploying the supervisor branch, adding https://github.com/irakhlin/hassio-addons to the supervisor addon repository and install the "SSH" addon (NOT usbip). This is simply the ssh addon from the community repo with the new option "usbip: true" added and kernel_modules enabled. In the configuration for the addon add the following packages: kmod, hwids-usb and linux-tools-usbip. After connecting to ssh run
sudo modprobe vhci-hcd
usbip list -r {address of remote usbip server with exported device} 
usbip attach -r {address} -b {selected device from previous command}

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast supervisor tests)
  • Tests have been added to verify that the new code works.

If API endpoints of add-on configuration are added/changed:

- Add optional boolean parameter 'usbip' which will add the following mount points for the container all with 'rw' mode: /sys/devices/platform, /sys/bus/platform/drivers, /sys/module

- Logic to check if usbip & gpio are both enabled to prevent mounting on top of an already mounted path (/sys/devices/platform/soc_

- Test and fixture
Fix mounting issue
@axemann
Copy link

axemann commented Sep 25, 2022

Hi @irakhlin, I would recommend also adding the 'hwids-usb' package to aid in identifying remote USB devices. I thought you had mentioned that package before in another thread regarding this, but I was unable to find it to add a link. :-)

@irakhlin
Copy link
Author

irakhlin commented Sep 26, 2022

@axemann Yes you are right this is a good package to have in the addon when testing this. I think the goal is to have these or a version of these changes added to supervisor to be able to create a cleaner addon with simple configurations to managing the mounting of the devices. That addon will definitely benefit from hwids-usb, I am still tinkering with the best way to implement that in an addon but if you have any ideas feel free to ping me.

edit: I have a basic working version of a very simple add-on using a slimmed down image with only needed packages with support for the standard add-on configuration system https://github.com/irakhlin/hassio-usbip-mounter it should be useful for anyone testing this

@pvizeli
Copy link
Member

pvizeli commented Sep 26, 2022

Hi @irakhlin, thank you for your contribution.

I'm discussing that today with our team. It seems to be pretty heavy from the access/security perspective. But on the other hand, it would be lame if everyone implemented their solution for that. We should solve that more central.

Our idea is to create a plugin called: connector (?). They would support usbip + uart/serial over IP (socat) and generate devices on /dev that other add-ons/core can access it (at least on usbip, that is the default anyway).

Do you want work with us to archive that? We have to create follow:

  • A new plugin on supervisor and define config format and API that can be used by CLI/UI. I would split that by type kinda /conector/info show usbip & uart and API endpoint for each type to connect to an endpoint or disconnect
  • Creating a new plugin that consume that information in a first step

@irakhlin
Copy link
Author

irakhlin commented Sep 26, 2022

Hi @irakhlin, thank you for your contribution.

I'm discussing that today with our team. It seems to be pretty heavy from the access/security perspective. But on the other hand, it would be lame if everyone implemented their solution for that. We should solve that more central.

Our idea is to create a plugin called: connector (?). They would support usbip + uart/serial over IP (socat) and generate devices on /dev that other add-ons/core can access it (at least on usbip, that is the default anyway).

Do you want work with us to archive that? We have to create follow:

* A new plugin on supervisor and define config format and API that can be used by CLI/UI. I would split that by type kinda `/conector/info` show `usbip` & `uart` and API endpoint for each type to connect to an endpoint or disconnect

* Creating a new plugin that consume that information in a first step

@pvizeli I totally agree that it is indeed pretty heavy on access exposure. When initially sitting down to do the work I had hoped the exposure would be scoped to the the vhci_hcd (usbip device hub) ex ( /sys/devices/platform/vhci_hcd.0 ) but this was not possible as the driver has to be loaded for that mount point to exist .. which had to be done during the start of the add-on and the mount points had to be configured before the start of the add-on. Big circular mess.

I think the idea of using a plugin makes sense. Its not something I thought of as I was not too familiar with supervisor code before looking into this. From my understanding currently the only plugin is the Observer plugin? edit: i now see the other available plugins. This definitely in my mind would be better suited in a plugin and not an add-on. I do like the idea and would be happy to work with you guys or help however I can. I imagine the first steps would be drafting up some proposed architecture to start defining API ends points. Feel free to reach email me if we want to continue this discussion outside of this pr.

@pvizeli
Copy link
Member

pvizeli commented Sep 27, 2022

Correct, you can reach me out on discord. I think there are 2 ways but we can discuss that later 👍

@pvizeli pvizeli closed this Sep 27, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supervisor addon functionality lacks ability to allow usbip bind with home assistant os 9.0rc2
4 participants