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

Extend CDROM provider to support any common type of disk/partition with ISO format #3280

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@errordeveloper
Copy link
Contributor

errordeveloper commented Jan 30, 2019

No description provided.

@errordeveloper errordeveloper changed the title WIP: Extend CDROM provider to support any disk/partition with ISO format Extend CDROM provider to support any disk/partition with ISO format Jan 30, 2019

@errordeveloper

This comment has been minimized.

Copy link
Contributor Author

errordeveloper commented Jan 30, 2019

I've tested it with linuxkit run -disk file=metadata-test.iso and it worked. If anyone can suggests other tests, I'm happy to look into it.

@rn

This comment has been minimized.

Copy link
Member

rn commented Jan 30, 2019

The diff in GH and git does not look right. Can you do a git mv provider_cdrom.go provider_iso.go and then make the changes. That way one actually sees the differences. Or even split this up into two commits. One which does the rename and one which adds the new functionality? That way one can see the actual changes.

It would also be good if the commit message explains more of the rationale for this change.

@ijc

This comment has been minimized.

Copy link
Collaborator

ijc commented Jan 30, 2019

git mv is no different to git rm && git add in terms of what is stored in the repo, there is no specific metadata recorded about such things. The "move" and "copy" display is entirely a presentation layer thing. IOW git mv wouldn't help here -- it's just that the old and new files differ enough that GH didn't guess there was a move involved while rendering the diff.

For the CLI you can control the heuristics a bit with the -M and -C options to git show, I suppose that isn't possible in the GH webui though.

Making it two commits would indeed be one way to avoid the problem though. and +1 to better commit messages...

@errordeveloper

This comment has been minimized.

Copy link
Contributor Author

errordeveloper commented Jan 30, 2019

@errordeveloper errordeveloper force-pushed the errordeveloper:metadata-iso-disk branch from 3027a60 to 144e5c7 Jan 30, 2019

@GordonTheTurtle

This comment has been minimized.

Copy link
Collaborator

GordonTheTurtle commented Jan 30, 2019

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "metadata-iso-disk" git@github.com:errordeveloper/linuxkit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353890016
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@errordeveloper errordeveloper force-pushed the errordeveloper:metadata-iso-disk branch from 144e5c7 to 9bf7c99 Jan 30, 2019

@GordonTheTurtle GordonTheTurtle removed the dco/no label Jan 30, 2019

@errordeveloper

This comment has been minimized.

Copy link
Contributor Author

errordeveloper commented Jan 30, 2019

I've split the commits as advised.

@errordeveloper errordeveloper changed the title Extend CDROM provider to support any disk/partition with ISO format Extend CDROM provider to support any common type of disk/partition with ISO format Jan 30, 2019

@rn

This comment has been minimized.

Copy link
Member

rn commented Feb 1, 2019

Thanks for the update. the diff is not much easier to read.

I'm not sure I like this approach. This seems to mount/unmount all disk type devices instead of stopping when one suitable device is found.

@errordeveloper

This comment has been minimized.

Copy link
Contributor Author

errordeveloper commented Feb 3, 2019

the diff is not much easier to read.

I thought 5c3bde1 fairly trivial...

I'm not sure I like this approach. This seems to mount/unmount all disk type devices instead of stopping when one suitable device is found.

That's just what it did before, only limited to all /dev/sr* devices. If we believe that behaviour should change, I'm more than happy to implement that in this PR, or perhaps separate PR would make sense.
Perhaps the current behaviour was somewhat satisfactory, considering that systems with many optical drives aren't very common. I agree that trying to mount each and every drive is not a good idea, perhaps pre-filtering on filesystem type would be a good idea? We could even match on a well-known ISO volume ID, e.g. something like 'LinuxKit metadata'.

@rn

This comment has been minimized.

Copy link
Member

rn commented Feb 3, 2019

Sorry, I meant that the diff is now much easier to read.

But the code is mounting every disk in the system. And only later, during probe, it goes through the providers and stops on the first suitable on. This doesn't look nice. Why not mount only during probe and stop mounting once a suitable device is found. I dn't think we want to make it more complicated with filesystem probing in whatnot, just stop mounting once you found a suitable device.

@errordeveloper

This comment has been minimized.

Copy link
Contributor Author

errordeveloper commented Feb 3, 2019

Sure, that makes sense. Happy to adjust the behaviour that way.

@errordeveloper errordeveloper force-pushed the errordeveloper:metadata-iso-disk branch from 9bf7c99 to dad8789 Feb 9, 2019

errordeveloper added some commits Jan 30, 2019

Generalise CDROM provider to support common block devices
CDROM maybe convenient for virtual machines, yet with bare-meta
it actually implies that machine has to have an optical dirve.
It's much more covenient to use USB flash drives instead, this
change allows for any SCSI disk devices and MMC block devices,
as well as adds support for SCSI CDROMs with new prefix.
This change simply expands the range of supported block devices,
the provider implementation continues to rely on ISO filesystem.

Simply put - you can now put metadata.iso onto a separate partition
of a USB drive or SD card, and it will be recognised by the
metadata package.

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
Rename provider_cdrom.go -> provider_iso.go
Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

@errordeveloper errordeveloper force-pushed the errordeveloper:metadata-iso-disk branch from 59bbb46 to 247bdaf Feb 11, 2019

@GordonTheTurtle GordonTheTurtle removed the dco/no label Feb 11, 2019

@errordeveloper

This comment has been minimized.

Copy link
Contributor Author

errordeveloper commented Feb 11, 2019

@rn please take a look at the latest commit.

@errordeveloper errordeveloper force-pushed the errordeveloper:metadata-iso-disk branch from 247bdaf to 13fc50b Feb 11, 2019

Improve ISO provider
- mount one devices at a time
- stop after first suitable device is found
- unmount and remove mount point properly
- prioritize MMC devices over any SCSI disks

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

@errordeveloper errordeveloper force-pushed the errordeveloper:metadata-iso-disk branch from 13fc50b to f3a012c Feb 12, 2019

@errordeveloper

This comment has been minimized.

Copy link
Contributor Author

errordeveloper commented Feb 13, 2019

There is a challenge with USB devices, I ended up having to do this:

onboot:
  - name: modprobe-usb-storage
    image: linuxkit/modprobe:v0.6
    command: ["modprobe", "usb-storage"]
  - name: wait-usb-storage
    image: linuxkit/alpine:3683c9a66cd4da40bd7d6c7da599b2dcd738b559
    binds:
      - /dev:/dev
    command:
      - sh
      - -c
      - until test -b /dev/sdc ; do sleep 1 ; done
  - name: metadata
    image: errordeveloper/metadata:9250ccfc8a4ba879989281b5f782bae2551521b9

I might find a better way, but this is what I have to do at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment