-
Notifications
You must be signed in to change notification settings - Fork 20
docs: Add initial docs on how to use node local storage #51
Conversation
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.
Added suggestions. The overall document seems all right.
9faf488
to
c428f16
Compare
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.
Few more found.
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.
Few nits and suggestions. The document is helpful. But the last section can go into other doc or be totally removed IMO. With so many links and lot of text it is just confusing than of help to users.
docs/advanced/node-local-storage.md
Outdated
[1]: https://github.com/kubernetes-sigs/sig-storage-local-static-provisioner/blob/master/docs/operations.md | ||
[2]: https://github.com/kubernetes-sigs/sig-storage-local-static-provisioner/blob/master/docs/faqs.md | ||
|
||
### Useful links |
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.
I am not sure if rest of this document should be here, rather than helping user it is just confusing user to follow lot of other links.
I would rather let this document be only something we support and have tried, tested and trusted.
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.
Okay... But I think that is useful and I would prefer to have this somewhere, where do you suggest? Or you suggest to just drop this?
bb76b2d
to
9fd22f9
Compare
@surajssd PTAL. I've addressed the all comments, except the ones I asked you something, and adapted to use |
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.
This is what I have reviewed for now, will continue another time.
docs/advanced/node-local-storage.md
Outdated
``` | ||
|
||
You can also connect to the pod and write to the volume which is mounted on | ||
`/usr/test-pod`, you will see in the host on `/mnt/node-local-storage`. |
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.
No you won't see it on the host, although if you delete the pod and a new one gets created, it will still have the same contents of the old pod.
I did a setup using this PR, but whatever I wrote to the pod on /usr/test-pod
, did not show up on the host's /mnt/node-local-storage
😕 . So, I decided to try with a different directory. I created a /mnt/data
directory on the host and it had the following permissions by default.
core@kosy-worker-worker-0 /mnt $ ls -l
total 12
drwxr-xr-x. 2 root root 4096 Sep 23 09:59 data
drwxr-xr-x. 3 root root 4096 Sep 23 07:33 node-local-storage
After creating the pv to use /mnt/data
instead of node-local-storage
, I wrote to the pod and saw the new file also in /mnt/data
and noticed that the permissions on /mnt/data/
changed by itself to:
core@kosy-worker-worker-0 /mnt $ ls -ll
total 12
drwxrwsr-x. 2 root bin 4096 Sep 23 10:15 data
drwxr-xr-x. 3 root root 4096 Sep 23 07:33 node-local-storage
Node local storage was moved back to |
@rata do you have time to take care of this PR? |
@invidian not really, and is kind of outdated now. I'll try to keep an eye on this during next week, but no promises :( |
9fd22f9
to
898f696
Compare
I'm taking a look into it to get it merged. |
Dismissing my own review for others to review.
docs/advanced/node-local-storage.md
Outdated
|
||
1. Lokomotive Kubernetes currently only exposes `/mnt/` to the kubelet, so only that | ||
directory can be used for node local storage. You can **manually** mount different | ||
devices on `/mnt/sda` or paths inside `/mnt` at your convenience. |
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.
This was not the case before self-hosted kubelet, as the pod didn't saw mounts inside mnt (see #73) Not sure if it is now possible with self-hosted kubelet
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.
If' say remove the "You can manually mount different devices on /mnt/sda
or paths inside /mnt
at your convenience" but I'm failing to create a github suggestion for this, not sure why 🤦♂️
IMHO, it looks fine besides the two open comments (the one I just posted and suraj comment from long ago). Maybe we can oing and wait for suraj to comment again? |
docs/advanced/node-local-storage.md
Outdated
|
||
1. Lokomotive Kubernetes currently only exposes `/mnt/` to the kubelet, so only that | ||
directory can be used for node local storage. You can **manually** mount different | ||
devices on `/mnt/sda` or paths inside `/mnt` at your convenience. |
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.
If' say remove the "You can manually mount different devices on /mnt/sda
or paths inside /mnt
at your convenience" but I'm failing to create a github suggestion for this, not sure why 🤦♂️
docs/advanced/node-local-storage.md
Outdated
|
||
1. You can share the volume in different PVs if you create a sub-directory per | ||
PV, bind mount each of them and only expose the subdirectories. It is explained | ||
in detail [here][1]. But take into account that it needs to be done manually. |
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.
For the same issue (#73) that was discovered later, I think this might not be accurate either. I'd remove this, probably. What do you think?
The link (the [1]
), is also shared in the section below, so we still keep a reference for that useful doc :)
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.
I guess it would work, if we enable mount propagation for this path, which we don't at the moment. I can remove it then.
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.
Ok, removed now.
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.
This looks ok.
@surajssd do you think it looks good to approve the PR? Can you approve it, then? :) @invidian can you approve too? (not sure, it's weird, you addressed the last comments at least. But I can't approve as it is my PR :)) @pothos or maybe can you take a look and approve if it seems okay? :) PS: I'd merge with squash and merge, if we are all ok with this (don't see value in the split commits) |
This adds basic documentation on how to use node local storage using local persistent volumes in filesystem mode. Other sections are mentioned, although empty for now, on other ways to use it that we may want to expand later.
23852c4
to
f235454
Compare
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.
Approving on behalf of @rata
Merging on behalf of @invidian ? 😂 |
@invidian thanks a lot for driving this forward! :) |
This adds basic documentation on how to use node local storage using
local persistent volumes in filesystem mode.
Other sections are mentioned, although empty for now, on other ways to
use it that we may want to expand later.