-
Notifications
You must be signed in to change notification settings - Fork 346
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
More comments and docs #14
Comments
@YJDoc2 |
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. |
@YJDoc2 |
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 😅 |
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. |
@YJDoc2
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 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.
That's for sure. I will try to be more careful when looking at PR, including mine.
|
Hey, what is the meaning of Cond and why is it named so? with respect to cond.rs |
It is means |
My suggestions would be :
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 Is uds in setup_uds in process/child.rs short for unified diagnostic service or something else? |
@YJDoc2
This number is not significant. It is set to an appropriate timeout value.
If there is a timeout, the code will fail here, so this code will never actually be executed. So
No, it isn't. I was using this as an abbreviation for unix domain socket. This may have been a little confusing.
|
Ok, I'll leave them as they are in my current PR then.
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 : |
Sorry, this code is unnecessary now.
|
Ok, I have removed that call in the PR #64 . Can you tell me for what it was originally intended for? |
Nice work!
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.
|
I was documenting linux.rs, and in |
@YJDoc2 It works fine that both arguments of pivot_root are the same directory. This is because mount points are stacked.
If you still have any questions, you can try eliminating umout and other things to see what happens. |
I know you said it's outdated, but I converted your sequence diagram into a PlantUML script that we can embed into the README.
|
@PeterYordanov Thanks for the great suggestion! The image is easier to understand. |
Perhaps this doesn't need to generate an image for each change? |
I'm not sure what your concern is. 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
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 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:
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
However, considering the benefits of using images to add color and clarity, I think it's a good policy to adopt. |
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. |
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 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 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: 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
|
@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 :
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 😅 Sorry again for the inactivity, I will try to get the documentation done as soon as I can :) |
Hey in capabilites.rs : drop_privileges,
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
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 |
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
|
In the logger module, we set the default log level as :
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 |
@YJDoc2 I forgot I was going to do your suggestion. Could you create a PR? |
I add a PR for some trivial fix on doc: #220 One thing I'm not sure about.
Should be:
? |
Another idea is, we should give two separate documents for those who just want to use |
The following PR comment sums up why a two-step fork is necessary. This is a Japanese blog, but the diagram in this article is very clear. Youki follows almost the same process. |
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. |
This issue has served its purpose well. |
Now, This project has too few comments and documentation.
I would like to improve the development experience by improving them.
The text was updated successfully, but these errors were encountered: