-
-
Notifications
You must be signed in to change notification settings - Fork 462
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
[Feature] support mapped btrfs devices on Linux #48
Conversation
Let me know if #46 will land first, and I will rebase this to use that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice feature, that certainly a bunch of people will need, thanks for that 👍
Just added some minor comments.
Hey there, PR #46 just got merged 👍 |
First of all. Thank you so much @jaredallard for sharing your findings and filed nice bug report, also for working on the PR. I am sure the facts collected and experiences described here will help many k3s / k3d users. To be honest, I am ambivalent about this patch. On one hand, it is very helpful to folks who need /dev/mapper access for their cluster to work -- The error message they got will not be very helpful in pointing them toward the root cause nor solution. Having k3d built it in helps with the out of box experience. On the other hand. It feels that k3d will be doing too much. It suppose to be a thin layer over k3s and we already have the -v option that can be applied when needed. Another concerned I have is that this option is always added regardless the cluster needs it or not -- this can be a security concern since we are unnecessarily increasing the attack surface. There is no way to turn it off. The third concern I have is that this option gets added behind user's back -- It can be a surprise to many users unless they know k3d very well. May be reasonable path forward for this is to document this, say in k3d FAQ? Another possible option is to add a "--helpful" option to k3d, telling user that k3d will automatically add a few options for the user, and this mapping can be part of the "--helpful" option. This option leaves default k3d bare bone. If we still want to go head with this patch, I'd suggest that we add an option to not apply this mapping. WDYT? |
I already added a |
On a side-note: I'm using LUKS in LVM as well and I didn't have any problems with |
I think the problem actually lies with how EDIT: I'd be very intrested to see what the output of |
@andyz-dev I think that it should be included by default, but perhaps we might want to consider how that volume should be presented in the container further to reduce the attack surface. If the outcome is to have a I can set But, again, I can see the argument for withholding it as well, so I think I'm OK with whatever is agreed upon by everyone here. |
@jaredallard there you go:
|
I think we should only include the option by default, when we figured out, what exactly the issue is and only if we can verify that the executing system has this problem, we should adjust for it in |
Interesting:
These two should be causing problems, since the Source attribute is a mapper device that, presumably, doesn't exist in the container. Interesting. I'll boot up a Ubuntu VM later tonight to see if I can get some stuff to repro (Japan Time). EDIT: Yeah I agree with that, I'll look at this more. Thanks for the active feedback everyone. I've pushed code that adjusts the params a bit to remove |
I see what you mean... thanks for your efforts in investigating what's the problem here 👍 |
OK! Done investigating, the issue is the combination of |
@jaredallard nice progress! I am not an expert in device map. So the following are really just my curiosity. Do we have other volume types may need this besides brtfs? Soft RAID and encrypted volumes came to mind, but I don't them well enough. Do we have a way to determine if the /dev/mapper is being used? May be we can use /dev/mapper being used as an indicator rather than detecting volume types? Wonder if this will be a more reliable approach in the long run. For the patch, I wonder if we should back off if user maps /dev/mapper to some other path: -v /my-vm-imaeg/dev/mapper:/dev/mapper With that, a user can block the hidden bind mount by: -v /tmp:/dev/mapper At any rate, when adding the hidden mount, we should probably generate a console message to tell user about it. |
@andyz-dev So, my guess right now is that this is actually not an issue with mapped devices at all, but with how the special btrfs code in cAdvisor is written. That was written to handle subvolumes and the like, my guess is that it stats the I'll attempt to scope this down further later today, as well as potentially dig through cadvisor's fs code to see if there is anything that can be done to not require that.
|
Update: This isn't occurring in Ubuntu without mapped devices, but an important note is that the disk provided by So, recap, at the moment this will only occur on devices using mapped + btrfs. Will continue by digging into cadvisor. |
So, located in cadvisor/fs/fs.go we can see that, here: https://github.com/google/cadvisor/blob/master/fs/fs.go#L740 a fix is made to resolve partition ids being reported by I can't see a way to fix that since it was added to resolve that issue. I think we should go ahead with this fix since it resolves this issue, and including mapped devices is no more vulnerable, in my opinion, than the currently existing |
As far as I understand, the issue is with systems where the FileSystem is btrfs and the But still, I see the issue can be solved by following the README and the PR doesn't do anything else. Can you summarize the PR and how is it different from following the README ? |
There was nothing in the README until this PR was made and I discovered the root cause of this. Yes that is the correct, that is the issue, again though that all came out of this thread if you back read it. |
Hi again @jaredallard, thanks a lot for investigating the specific cases in which this issue might occur. UPDATE: if we're going to include this in the code as a default setting, we should ensure that it's only being applied in the cases where it's really needed and then inform the user about it. |
@@ -28,6 +28,8 @@ const ( | |||
defaultServerCount = 1 | |||
) | |||
|
|||
var defaultBindMounts = []string{"/dev/mapper:/dev/mapper"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this shouldn't be in defaultBindMounts
, since it's not default, but only added in specific cases, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sortof, it checks if it exists so it's a defaultBindMount but only applied when it exists. Could change that naming, but not sure what that'd get us since it's still sorta a default.
I'm impartial still on which to do. If I always have to I'd prefer to have this go in, but I can see the reasons not too. At this point I'd say this is ready to go since it's being only applied when |
I understand your point of view, that it should be included to ease usage on systems with btrfs mapped volumes. But to me, checking only for the existence of Why would you need to check for |
Yeah I agree with that. The idea behind having to check Thanks everyone for being involved in this, now massive, thread! 🚀 |
Hoping to get some work on this sometime this week. 🙀 |
Based on schu/kubedee#2, the solution for btrfs would be to pass in the block device underneath Perhaps https://github.com/ashald/docker-volume-loopback could be of use here? Example below works regardless of underlying filesystem:
This example obviously fails on a multi-node cluster due to sharing state not meant to be shared, but it's something to be pushed down to per-container volume creation when starting a cluster. Although, the "quickest" solution might be passing through |
@jaredallard @iwilltry42 thoughts on my comment? |
Hi @zer0def , on a first glance, this looks quite cool to me, especially if it's platform/FS agnostic. |
@iwilltry42 with the introduction of #116 one can dodge this limitation by doing something like this:
Granted, it's a little clunky when compared to just providing a volume driver for Docker to consume from, but this at least get the job sufficiently done. This also means that this PR can probably end up being closed. |
Closing this due to the existing workarounds and inactivity. Feel free to reopen if there's need for it. |
found this thread after struggling to get k3d working due to disk pressure taints on all nodes,.
Got it working after reading this thread. does seem to complicate k3d just working out of the box. |
@diepes , this will be handled OOB soon as per #629 and it's also mentioned in the FAQ: https://k3d.io/faq/faq/#issues-with-btrfs 👍 |
What this PR does: Adds support for
LUKS and LVMbtrfs mapped device users on linux. This is related to k3s-io/k3s#471.Notes for my reviewer: This might not be the best way to do this, since this will likely fail on pure windows machines in the future (one day). I've tested this on machines w/ luks+lvm and machines w/o it.