More comments and docs
utam0k opened this issue · 44 comments
Now, This project has too few comments and documentation.
I would like to improve the development experience by improving them.
Yes sure, I'd be happy to work on this 👍 I am a bit busy this week, so probably will start working on this weekend or next week.
Currently my plan is to start with main.rs, comment that and recursively work on the imports. I am also thinking to make a rough md file to describe the control flow as the program is run, so someone new can get a quick high level overview of things. Let me know if you have/get any suggestions or ideas. :)
I assigned this issue to you. Your plan looks great and I am looking forward to it!
I prefer to have them proceed slowly at their own pace. Can I ask you to send out PRs in small sections so I can see the progress if possible?
I am a bit busy this week, so probably will start working on this weekend or next week.
This is a diagram I made to explain the process flow of youki to others. It is a wrong diagram because the process is different from now, but it may be helpful to see the big flow.
@YJDoc2
I'd like to invite you to join this repository in a little while, with you in charge of maintaining the documentation. Maybe I'll ask for other ranges as well.
If you're up to the challenge, I'd love to work with you.
Hey, I think it'd be better if you see more of my commits/PRs and then decide, I have made only 1 PR till now 😅
But I would like to work on this for sure. Thank for giving the chance , I am getting to learn a lot about containers and Linux by doing this :)
I had a question about the code, in load_console_socket, what is exactly the console part? I understood the socket for the tty from given socket path; but in second half of function, fcntl open call is given only "console", what is it meant to open? I tried it in C, but it always returned -1.
If possible can you comment and give outerview for the spec file? I tried to go through it and understand, but it is a bit complex, so rather than commenting every struct, can you create a file-level comment describing what it is meant for and what various structs in it are meant to represent?
Also for the new files and PRs, it would be great if authors themselves comment it in PR, so that it'll best explain author's intent, and only original files will need to be commented, otherwise, the commenting work will keep on going as more and more files/changes are added.
Thank you!
@YJDoc2
Yes, I'd like to see a little more of your PR, but I feel your understanding and PR is very good, so I'd like to think in that direction.
Hey, I think it'd be better if you see more of my commits/PRs and then decide, I have made only 1 PR till now 😅
To be honest, the "tty" part was done through a lot of trial and error, and I think it still needs some work. I don't understand some parts of it either. If you'd like, I'd like you to change the code at hand and delete or edit any unneeded processing, would you be interested?
The tty is a very important part of the dokcer integration. If you're not interested, I'll do some weekend research on my end.
I had a question about the code, in load_console_socket, what is exactly the console part? I understood the socket for the tty from given socket path; but in second half of function, fcntl open call is given only "console", what is it meant to open? I tried it in C, but it always returned -1.
The spec.rs follow the oci-runtime specification, so it's better to refer to that one. If it's too difficult, I can write a comment on files.
https://github.com/opencontainers/runtime-spec
If possible can you comment and give outerview for the spec file? I tried to go through it and understand, but it is a bit complex, so rather than commenting every struct, can you create a file-level comment describing what it is meant for and what various structs in it are meant to represent?
That's for sure. I will try to be more careful when looking at PR, including mine.
Also for the new files and PRs, it would be great if authors themselves comment it in PR, so that it'll best explain author's intent, and only original files will need to be commented, otherwise, the commenting work will keep on going as more and more files/changes are added.
Thank you!
Hey, what is the meaning of Cond and why is it named so? with respect to cond.rs
It is means conductivity. However, I will try to rename it more clearly. If you are ok, you will try to rename it.
My suggestions would be :
- Pipe, which will be most clear to understand, but can cause confusion with mio::unix::pipe
- Channel, but I think there are std::sync::mpsc::channel, which can potentially cause confusion
- Connector , should be acceptable
- Tunnel, but I don't think it is particularly good
Let me know if you agree with any of these, I'll change it in my next PR
Also in parent.rs,
https://github.com/containers/youki/blob/main/src/process/parent.rs#L26-L27
Is there any specific significance of 128 and 5 seconds, or they are 'sensible' constants? If so is it ok if I extract them in constant variables ? same for
https://github.com/containers/youki/blob/main/src/process/child.rs#L66-L67
In
https://github.com/containers/youki/blob/main/src/process/parent.rs#L44
should it be a panic!("No message from child process within timelimit"); instead of bail? as reaching there indicates that there was no event received from polling, so child has not yet ready after 5 seconds. same for process/child.rs
https://github.com/containers/youki/blob/main/src/process/child.rs#L80
Is uds in setup_uds in process/child.rs short for unified diagnostic service or something else?
Thanks
@YJDoc2
I agree with it. In terms of Pipe, I don't think it will be that big of a problem since they both refer to the same thing. I'm going to do a major refactoring of cond and notify_socket.
Pipe, which will be most clear to understand, but can cause confusion with mio::unix::pipe
This number is not significant. It is set to an appropriate timeout value.
Is there any specific significance of 128 and 5 seconds, or they are 'sensible' constants? If so is it ok if I extract them in constant variables ?
If there is a timeout, the code will fail here, so this code will never actually be executed. So unreachable!() might be more accurate.
Line 27 in cbcc4a3
In
https://github.com/containers/youki/blob/main/src/process/parent.rs#L44
should it be a panic!("No message from child process within timelimit"); instead of bail? as reaching there indicates that there was no event received from polling, so child has not yet ready after 5 seconds. same for process/child.rs
https://github.com/containers/youki/blob/main/src/process/child.rs#L80
No, it isn't. I was using this as an abbreviation for unix domain socket. This may have been a little confusing.
Is uds in setup_uds in process/child.rs short for unified diagnostic service or something else?
Ok, I'll leave them as they are in my current PR then.
I agree with it. In terms of Pipe, I don't think it will be that big of a problem since they both refer to the same thing. I'm going to do a major refactoring of cond and notify_socket.
can you tell me what is util::set_name is meant to do, as it is currently unimplemented. It is called only in fork.rs :
https://github.com/containers/youki/blob/main/src/process/fork.rs#L36
also what is rc-user supposed to signify?
Thanks.
Sorry, this code is unnecessary now.
can you tell me what is util::set_name is meant to do, as it is currently unimplemented. It is called only in fork.rs :
https://github.com/containers/youki/blob/main/src/process/fork.rs#L36
also what is rc-user supposed to signify?
Ok, I have removed that call in the PR #64 . Can you tell me for what it was originally intended for?
Nice work!
Ok, I have removed that call in the PR #64 .
The original purpose was supposed to give names to init and child processes. This is a reference to railcar, but it shouldn't be necessary since it's not particularly necessary and the container name is set elsewhere.
Can you tell me for what it was originally intended for?
I was documenting linux.rs, and in
https://github.com/containers/youki/blob/main/src/command/linux.rs#L36
I am not sure how this works and passes test, as in the manpage for pivot_root
https://man7.org/linux/man-pages/man2/pivot_root.2.html
it is stated that put_old must be underneath new_root, but here we are giving same path for both.
@YJDoc2
This code is very relative to prepare_rootfs.
Lines 33 to 42 in d72ffa5
And,
It works fine that both arguments of pivot_root are the same directory. This is because mount points are stacked.
See the NOTES section of this page for more details.
https://manpages.ubuntu.com/manpages/hirsute/man2/pivot_root.2.html
pivot_root(".", ".")
new_root and put_old may be the same directory. In particular, the following sequence
allows a pivot-root operation without needing to create and remove a temporary directory:
chdir(new_root);
pivot_root(".", ".");
umount2(".", MNT_DETACH);
This sequence succeeds because the pivot_root() call stacks the old root mount point on
top of the new root mount point at /. At that point, the calling process's root directory
and current working directory refer to the new root mount point (new_root). During the
subsequent umount() call, resolution of "." starts with new_root and then moves up the
list of mounts stacked at /, with the result that old root mount point is unmounted.
If you still have any questions, you can try eliminating umout and other things to see what happens.
This part of the programming is not normally done, so more detailed comments would be appreciated.
If you don't understand, please ask.
I know you said it's outdated, but I converted your sequence diagram into a PlantUML script that we can embed into the README.
If you want to have this in the README as code that we can update as the project evolves, let me know so that I can create a PR:
@startuml
box User Space #LightBlue
actor User as youki_user
participant Docker as docker
end box
box Container Runtime #Coral
participant "Youki (Parent Process)" as parent_process
participant "Youki (Child Process)" as child_process
participant "Youki (Init Process)" as init_process
end box
alt Create container
youki_user -> docker: $ docker run -rm -it --runtime youki $image
docker -> parent_process: $ youki create $container
parent_process -> child_process: fork(2)
child_process -> child_process: Create new namespaces
child_process -> init_process: fork(2)
init_process -> init_process: mount the devices
parent_process <-- init_process: send a ready message(unix domain socket)
docker <- parent_process: $ exit $code
end
alt Start container
docker -> parent_process: $youki start $container_id
parent_process --> init_process: send a start message(unix domain socket)
init_process -> init_process: run the cmd in Dockerfile
docker -> docker: monitor the pid written in the pid file
youki_user <- docker: exit $code
end
@enduml
The end result should look like this:
@PeterYordanov Thanks for the great suggestion! The image is easier to understand.
I think with images there is less flexibility for changes than with text-based. Also, documentation is one of those areas that people tend to forget to change. I don't think it should be too difficult to change.
If you can address this issue, I'd love to use it.
I would also like to hear @YJDoc2's opinion.
Perhaps this doesn't need to generate an image for each change?
I'm not sure what your concern is.
Initially, I thought of using Github's own diagrams, but they were too basic and didn't offer enough. PlantUML covers whatever we need for sequence diagrams and more.
To generate the image above, I personally used PlantUML Editor, but Planttext and other online editors do the job well enough.
The purpose was to have centralized way of modifying these diagrams. Otherwise, we would have to pass around images that are hard, if not impossible to modify reliably. With PlantUML and other diagram components, in the future we can still have the luxury of putting an advanced diagram in the form of an image in the README, while also keeping track of the script that was used to generate it in a doc/ folder, so that the next person that modifies it can also contribute to the documentation of the project.
@PeterYordanov
In general, I think that representing UML with images makes it easier to understand. However, it is a bit more complicated to edit than a text-only representation. I think this is something to be concerned about.
I think there is a trade-off here between clarity and ease of editing.
For example, if I could only just write the following markdown to display UML images, I'd be happy to adopt that. However, I think it raises the hurdle of changing the documentation a bit if the images cannot be generated without running a script here.
@startuml
box User Space #LightBlue
actor User as youki_user
participant Docker as docker
end box
@enduml
Nevertheless, if the script can generate images, the tediousness of editing them goes up compared to text alone, but I think the readability of this diagram more than makes up for that shortcoming.
I'd also like to hear @YJDoc2's opinion and make a decision.
I don't think you understand where I'm getting at. This is not a text-only representation. The tediousness doesn't go up, it's 5 steps forward and 1 step back.
I think it would be best explained with a hypothetical scenario, consider this:
- A new contributor comes in, wants to make a change to the diagram
- In the CONTRIBUTING.md, we can have separate sections for code and documentation contributions and steps for what needs to be used to perform certain steps
- Our new contributor wants to change the documentation diagrams
- Sees the instructions in CONTRIBUTING.md:
takes the script
puts it in an online PlantUML editor
changes the script
then updates it in the docs/ folder
then creates an image from the online editor through that script and updates the image as well in the docs folder
Now, not only can the contributor effectively and efficiently make changes to the script, but also visualize it and extract it as an image (from the online editors) and put it in the README.
The only annoyance comes from having to maintain 2 files: one for the script and one for the image, but given that it makes modifications to the diagrams a whole lot easier (and actually possible), I think it's a reasonable sacrifice. In the end, it's not a text-only representation, it's text + image, where the text can be used to generate the image. If we only have images, we'd have to become Photoshop experts every time we want to update the diagrams.
@PeterYordanov
Thanks for the clear explanation!
I am concerned that the following two steps will raise the hassle of editing the image. If it's just text, this step shouldn't be necessary. I'm a little concerned that this hassle will prevent the images from being updated.
puts it in an online PlantUML editor
then updates it in the docs/ folder then creates an image from the online editor through that script and updates the image as well in the docs folder
However, considering the benefits of using images to add color and clarity, I think it's a good policy to adopt.
For example, how about preparing the UML script as a single file and using CI to automatically update the image when there is a change in it?
I found this to be very useful in this case. It's a good way to create a PR instead of a commit at the end.
https://github.com/cloudbees/plantuml-github-action
https://github.com/marketplace/actions/create-pull-request
Yeah, agreed, the process should be more automated, so that it eliminates the only flaw right now. I haven't done the research on how, but I'm glad you did. I can take this as a task if everyone else is busy on other things.
Hey, @PeterYordanov , thanks for making the UML diagram, I agree with you that the complex operations can be understood quite effectively with diagrams so the extra steps that might be needed are worth the hassle. However I also agree with @utam0k 's point that it would be slightly tedious to make the updates require change the text as well as image representation, and it would be better to have some kind of action script to generate diagrams.
That said, initially while making the doc-draft.md, I used mermaid js, which in md file directly generates various diagrams form their text representation, without need of requiring any kind of generation step. Thus, the text under the control flow diagram section in https://github.com/containers/youki/blob/main/docs/doc-draft.md should be displayed as
(This is using vs code plugin which lets you see md file rendered.)
For some reason it is not rendered on github, but I am trying to find the reason.
Meanwhile if possible, can you check https://mermaid-js.github.io/mermaid/#/ , and see if your diagram could be recreated in mermaid?
(There seem to be some issues while using mermaid diagram on github)
If we could produce same quality diagram using mermaid, we would not need the extra step of generating the image file, and will need to maintain only text representation.
EDIT : it seems that even though this is much requested feature, it is not yet integrated in github md file rendering. There is a way around using this which uses base64 url to directly provide the image, but it again becomes 2 step process 😞 😢 For example, check test branch in my fork : https://github.com/YJDoc2/youki/blob/test/README.md (also take a look at the source of readme.md)
@YJDoc2 @utam0k
My personal projects are on Gitlab, where I've used mermaid extensively to document and didn't know they provided sequence diagrams too.
Now, I'm no Markdown expert, but maybe someone can step in and fill in missing/incorrect information.
What if we integrate HTML into the README, where we can then import mermaid.js, like this:
https://mermaid-js.github.io/mermaid/#/n00b-gettingStarted?id=_4-adding-mermaid-as-a-dependency
That way we get the best of both worlds: It's text-based, but will (theoretically) generate the graph using the mermaid.js library, but also anyone can contribute, and we don't have to move around trying to synchronize several files.
And mermaid is also feature-rich (and looks better as well), which can prove quite useful for further system design documentations.
@PeterYordanov
I think it would be nice if it could be done. I don't know if this is possible, as I also am not a Markdown expert.
If you try and succeed, I would be happy to adopt it.
What if we integrate HTML into the README, where we can then import mermaid.js, like this:
https://mermaid-js.github.io/mermaid/#/n00b-gettingStarted?id=_4-adding-mermaid-as-a-dependency
@PeterYordanov @utam0k I tried to check that, but it seems github does not allow running script tag in md files, which seems sensible as it can enable third party attacks, so I don't think it will work in plain html-js way to embed mermaid in md files on github
mermaid js officially indicates that it can be integrated with github (https://mermaid-js.github.io/mermaid/#/./integrations) using either :
- https://github.com/SimonKenyonShepard/mermaidjs-github-svg-generator
- https://github.com/neenjaw/compile-mermaid-markdown-action
The first one will required developers to have node installed on their system, as well as run the command every time with changes
Second is a github action which compiles the mermaid js, but I do not have much knowledge of actions, so someone will have to check it for viability.
Edit : we are currently thinking that our documentation will live on the md files on github only, but we can also think of the route of making an md-book thing or a simple website, which can live on the github pages for this repo, in which we can embed the mermaid js script easily.
Well, we can technically have both.
A simple website that displays a very general, high-level overview explaining what the project is about and simple tutorials/user guide, how to set it up etc.. Then an md book to supplement it with more detailed explanations on how the runtime works on a lower level.
You might think that having both isn't needed, as there's already README, but we're putting way too much information there as it is, something has to be extracted, even if there's not a static website, at least an md book should be considered.
@YJDoc2 Hello. Are you busy these days? I'm wondering if you are having any problems.
Hey, sorry for no activity, I was a bit busy last few days 😅
Now only few files in the src are remaining for commenting, files under src/*/ are mostly commented (such as container/builder etc). I will get working again on commenting on these files , so we can be done with the remaining ones. In the mean time can you suggest/decide which way should we create an overall developer documentation? Should we use github pages for now, or something else? This will serve to explain the overall structure and flow of the Youki, and will expand from doc-draft.md, which will then be removed.
Sorry again for the inactivity, I will try to get the documentation done as soon as I can :)
Hey in capabilites.rs : drop_privileges,
let all = caps::all();
log::debug!("dropping bounding capabilities to {:?}", cs.bounding);
for c in all.difference(&to_set(&cs.bounding)) {
match c {
Capability::CAP_PERFMON | Capability::CAP_CHECKPOINT_RESTORE | Capability::CAP_BPF => {
log::warn!("{:?} is not supported.", c);
continue;
}
_ => caps::drop(None, CapSet::Bounding, *c)?,
}
}
This is done, as caps cannot set bounding capabilities, so any extra than in spec are dropped. But I think this should be moved to the implementation of set_capabilities in linux.rs, and the code here should be as
syscall.set_capability(CapSet::Bounding, &to_set(&cs.bounding))?;
syscall.set_capability(CapSet::Effective, &to_set(&cs.effective))?;
syscall.set_capability(CapSet::Permitted, &to_set(&cs.permitted))?;
syscall.set_capability(CapSet::Inheritable, &to_set(&cs.inheritable))?;
My reason behind this is that setting capabilites is abstracted in Syscall structure, and separating the bounding capabilities in drop_privileges defeats the purpose of that abstraction.
Let me know if I should refactor the above code along with commenting capabilities.rs
Hey, sorry for no activity, I was a bit busy last few days 😅
Now only few files in the src are remaining for commenting, files under src/*/ are mostly commented (such as container/builder etc). I will get working again on commenting on these files , so we can be done with the remaining ones. In the mean time can you suggest/decide which way should we create an overall developer documentation? Should we use github pages for now, or something else? This will serve to explain the overall structure and flow of the Youki, and will expand from doc-draft.md, which will then be removed.Sorry again for the inactivity, I will try to get the documentation done as soon as I can :)
I'm not trying to rush you. I was just wondering if there was any trouble. I can't help it if you're busy, so it's enough if you proceed at your own pace. I would be relieved if you could write the issue that you will be busy and inactive if you can afford it.
@YJDoc2 This is a nice idea. Of course, you can have it refactored
My reason behind this is that setting capabilites is abstracted in Syscall structure, and separating the bounding capabilities in drop_privileges defeats the purpose of that abstraction.
Let me know if I should refactor the above code along with commenting capabilities.rs
In the logger module, we set the default log level as :
let level_filter = if let Ok(log_level_str) = env::var("YOUKI_LOG_LEVEL") {
LevelFilter::from_str(&log_level_str).unwrap_or(LevelFilter::Warn)
} else {
LevelFilter::Warn
};
Should we change the Warn level to Debug level instead if we are running in debug mode using cfg assertion? As when running debug mode, or running tests, we will always get all the logs possible, and we don't have to explicitly set the level evn var
@utam0k ?
I add a PR for some trivial fix on doc: #220
I'm also not a native English speaker, but sometimes shorter is easier for reading.
One thing I'm not sure about.
Otherwise, if using fork, Youki would need to fork two processes,
the first to enter into usernamespace, and a second time to enter into pid namespace correctly.
Should be:
... and the second one to enter into pid namespace correctly.
?
Another idea is, we should give two separate documents for those who just want to use Youki as a runtime and those who want to develop or know the design and details of Youki? I think it will be more clearer.
I add a PR for some trivial fix on doc: #220
I'm also not a native English speaker, but sometimes shorter is easier for reading.One thing I'm not sure about.
Otherwise, if using fork, Youki would need to fork two processes, the first to enter into usernamespace, and a second time to enter into pid namespace correctly.Should be:
... and the second one to enter into pid namespace correctly.?
The following PR comment sums up why a two-step fork is necessary.
#217 (comment)
This is a Japanese blog, but the diagram in this article is very clear. Youki follows almost the same process.
https://kurobato.hateblo.jp/entry/2021/05/02/164218
Another idea is, we should give two separate documents for those who just want to use
Youkias a runtime and those who want to develop or know the design and details ofYouki? I think it will be more clearer.
I think it's a good idea to divide youki into different documents for different audiences, and I think doc-draft is currently where the internal details of youki stand. On the other hand, I'd like to write a README for those who want to use youki, but it's not enough yet.