Adds spec cli command #55

ferrell-code · 2021-06-02T02:47:25Z

thought it would be useful to be able to generate the config.json natively like runc :)

Also might be useful for unit tests

utam0k · 2021-06-02T07:02:22Z

@ferrell-code
Thanks for your PR. I think this is a good addition. For example, I felt it would be nice to use this in the tutorials in the README.
If you'd like, you can put a simple test on it.

ferrell-code · 2021-06-07T04:21:00Z

I wanted to get Spec to fully serialize and de-serialize, unfortunately I'm not sure how to manually impl it for the case of LinuxCapabilityType

youki/oci_spec/src/lib.rs

Lines 94 to 105 in adb4310

    
           #[derive(Debug, Clone)] 
        
           pub struct LinuxCapabilityType { 
        
               pub cap: Capability, 
        
           } 
        
           impl<'de> Deserialize<'de> for LinuxCapabilityType { 
        
               fn deserialize<D>(desirializer: D) -> Result<Self, D::Error> 
        
               where 
        
                   D: serde::de::Deserializer<'de>, 
        
               { 
        
                   let r: serde_json::Value = serde::Deserialize::deserialize(desirializer)?; 
        
                   match r {

ferrell-code · 2021-06-07T04:21:24Z

other than that let me know what you think :) @utam0k

utam0k · 2021-06-07T14:35:52Z

@ferrell-code
This deserializing code could be written more concisely by using rename_all. Like this.

youki/oci_spec/src/lib.rs

Line 73 in adb4310

#[serde(rename_all = "SCREAMING_SNAKE_CASE")]

Wouldn't that also make serialization easier?

Also, you can reference the railcar's code.
https://github.com/oracle/railcar/blob/ef5918e21e7ad9ffd25c1a507df96458ec4e0c24/oci/src/lib.rs#L86-L127

ferrell-code · 2021-06-08T04:21:54Z

I changed LinuxCapabilityType to enum. Thanks for the railcar reference, I think that it is the most sensible solution, unless there is a reason it was a struct I'm unaware of

Furisto · 2021-06-08T08:32:49Z

oci_spec/src/caps_migrations.rs

+        LinuxCapabilityType::CAP_MAC_ADMIN => Capability::CAP_MAC_ADMIN,
+        LinuxCapabilityType::CAP_WAKE_ALARM => Capability::CAP_WAKE_ALARM,
+        LinuxCapabilityType::CAP_BLOCK_SUSPEND => Capability::CAP_BLOCK_SUSPEND,
+        _ => panic!("not a Linux Capability Type"),


You can use unreachable! here instead of panic! to communicate the intent a bit better. I am wondering though, is this even necessary? If you exhaustively check for all variants of the enum, a wildcard pattern should not be needed.

Altered it to be fully explicit 👍 . the caps Capability has the field __Nonexaustive though

Furisto · 2021-06-08T08:33:44Z

oci_spec/src/caps_migrations.rs

+        Capability::CAP_MAC_ADMIN => LinuxCapabilityType::CAP_MAC_ADMIN,
+        Capability::CAP_WAKE_ALARM => LinuxCapabilityType::CAP_WAKE_ALARM,
+        Capability::CAP_BLOCK_SUSPEND => LinuxCapabilityType::CAP_BLOCK_SUSPEND,
+        _ => panic!("not a Linux Capability type"),


utam0k · 2021-06-08T08:47:34Z

oci_spec/src/caps_migrations.rs

+use crate::LinuxCapabilityType;
+
+// utility function to switch between the types LinuxCapabilityType and Capabality
+pub fn to_caps(local_cap: &LinuxCapabilityType) -> Capability {


It is better to use the Into trait.
https://doc.rust-lang.org/std/convert/trait.Into.html

I agree but we would need to be able to impl the From trait for caps::Capability which cannot be done with an external crate.

See idea below to fix this :)

I must of been sleepy when I looked at it last night you can totally impl From for an external crate! I was thinking of the orphan rule but it does not apply here.

updated 👍

ferrell-code · 2021-06-08T20:19:16Z

I think it would be better to fork the caps dependency and host it in its own crate in youki to avoid the migrations back and forth and modify it as we see fit.

Perhaps that should be done in a different PR

utam0k · 2021-06-09T11:41:47Z

@ferrell-code
Ok, do you want to try it?

Perhaps that should be done in a different PR

ferrell-code · 2021-06-10T06:00:00Z

I'd be happy to add the caps crate if you think it is necessary, but looking at it again that might be overkill. Let me know what you think of the current PR when you get the chance :)

utam0k · 2021-06-10T06:27:03Z

@ferrell-code
I also feel a little overwhelmed. I think it's not too late to deal with it when it becomes more necessary.
I'm sorry I can't provide the opportunity. If you are up for some challenge, please look at the cgroups v2 support and so on.

I'm going to do a careful review again when the Draft is off, but for now, I think it's good overall. However, it would be nice to comment on why Default is needed (perhaps to be used in the spec command).
Also, although it doesn't have to be in this PR, lib.rs is too big, and It is better to split it.

ferrell-code · 2021-06-11T01:16:49Z

I think its a great project for us to learn more about containers and linux internals! Take as much time as you need, I know its not easy to have time for opensource work. We appreciate your effort!

My thought process behind Default was to have a basic boilerplate for Spec, but if you think it is more idiomatic for Spec to be initialized with some fn init_spec() instead of the Default trait i can easily copy paste the Default into that.

I will split the lib.rs file too but probably after this PR would be easier 🍶

utam0k · 2021-06-11T11:53:26Z

@ferrell-code
It encourages me to hear you say that. If you want, you can continue to contribute to the project.

I think its a great project for us to learn more about containers and linux internals! Take as much time as you need, I know its not easy to have time for opensource work. We appreciate your effort!

I think your idea is a good one.
In any case, in my opinion, it would be better to have some comments on the background.

My thought process behind Default was to have a basic boilerplate for Spec, but if you think it is more idiomatic for Spec to be initialized with some fn init_spec() instead of the Default trait i can easily copy paste the Default into that.

👍

I will split the lib.rs file too but probably after this PR would be easier 🍶

ferrell-code · 2021-06-23T02:34:21Z

Ok I added a lot of documentation to it, let me know what you think

ferrell-code · 2021-06-23T02:47:06Z

oci_spec/src/lib.rs

+// Device types
+#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq)]
 #[serde(rename_all = "lowercase")]
 pub enum LinuxDeviceType {
+    // block (buffered)
    B,
+    // character (unbuffered)
    C,
+    // character (unbufferd)
    U,
+    // FIFO
    P,
+    // ??
    A,
 }


Do you know what A is for? maybe any?

Sorry It's my mistake. Could you like to remove it?
https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#devices

Furisto · 2021-07-17T17:25:24Z

@utam0k @ferrell-code This looks fine to me. What is us preventing from merging this? (apart from the minor merge conflicts that have accumulated in the mean time)

utam0k · 2021-07-18T02:09:20Z

@ferrell-code Do you have any implementation left? Other than conflicts, I think this is fine.

ferrell-code · 2021-07-18T05:30:14Z

Ah yes sorry for leaving this hanging. It is all good to go, I resolved the conflicts and split lib.rs into multiple files. Unfortunately right now it breaks something related to LinuxCapabilities.

Edit: Just needed to add some serde defaults, should of been there anyways :). It is good on my end, although I only did basic stuff to test it locally

Furisto · 2021-07-18T08:35:58Z

@ferrell-code Can you squash some of the commits? I am talking about the commits from June 10 to June 14 which are basically all "Comments & Formatting" and from June 23 to today which are mostly "Resolve conflicts & compile"

utam0k · 2021-07-18T08:40:46Z

Could you remove the WIP in the title of this PR if you're ready to merge?

ferrell-code · 2021-07-19T03:44:42Z

I squashed it all to one commit, because rebase would make me resolve all the conflicts (as you can see my 5 force push attempts). I think this is the most reasonable solution.

In any future prs I will keep in mind to squash commits before pushing 👍

Furisto · 2021-07-19T20:24:23Z

Looks very good to me. If @utam0k also agrees we can do the merge.

utam0k

Perfect!

utam0k · 2021-07-20T12:38:13Z

@Furisto thanks for your review

ferrell-code marked this pull request as draft June 2, 2021 02:53

ferrell-code marked this pull request as ready for review June 7, 2021 04:12

Furisto reviewed Jun 8, 2021

View reviewed changes

utam0k reviewed Jun 8, 2021

View reviewed changes

ferrell-code commented Jun 23, 2021

View reviewed changes

ferrell-code force-pushed the add-spec-arg branch 5 times, most recently from ada394d to 9d4ed4e Compare July 18, 2021 22:08

add spec arg to cli

e4b0d33

ferrell-code force-pushed the add-spec-arg branch from 35cac7e to e4b0d33 Compare July 19, 2021 03:41

ferrell-code changed the title ~~[WIP] Adds spec cli command~~ Adds spec cli command Jul 19, 2021

Furisto approved these changes Jul 19, 2021

View reviewed changes

utam0k approved these changes Jul 20, 2021

View reviewed changes

utam0k merged commit 4076f16 into containers:main Jul 20, 2021

ferrell-code deleted the add-spec-arg branch July 20, 2021 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds spec cli command #55

Adds spec cli command #55

ferrell-code commented Jun 2, 2021 •

edited

Loading

utam0k commented Jun 2, 2021

ferrell-code commented Jun 7, 2021

ferrell-code commented Jun 7, 2021 •

edited

Loading

utam0k commented Jun 7, 2021

ferrell-code commented Jun 8, 2021 •

edited

Loading

Furisto Jun 8, 2021

ferrell-code Jun 8, 2021 •

edited

Loading

Furisto Jun 8, 2021

utam0k Jun 8, 2021

ferrell-code Jun 8, 2021

ferrell-code Jun 10, 2021 •

edited

Loading

ferrell-code commented Jun 8, 2021 •

edited

Loading

utam0k commented Jun 9, 2021 •

edited

Loading

ferrell-code commented Jun 10, 2021 •

edited

Loading

utam0k commented Jun 10, 2021 •

edited

Loading

ferrell-code commented Jun 11, 2021 •

edited

Loading

utam0k commented Jun 11, 2021

ferrell-code commented Jun 23, 2021

ferrell-code Jun 23, 2021 •

edited

Loading

utam0k Jun 24, 2021

Furisto commented Jul 17, 2021

utam0k commented Jul 18, 2021

ferrell-code commented Jul 18, 2021 •

edited

Loading

Furisto commented Jul 18, 2021

utam0k commented Jul 18, 2021

ferrell-code commented Jul 19, 2021 •

edited

Loading

Furisto commented Jul 19, 2021

utam0k left a comment

utam0k commented Jul 20, 2021

Adds spec cli command #55

Adds spec cli command #55

Conversation

ferrell-code commented Jun 2, 2021 • edited Loading

utam0k commented Jun 2, 2021

ferrell-code commented Jun 7, 2021

ferrell-code commented Jun 7, 2021 • edited Loading

utam0k commented Jun 7, 2021

ferrell-code commented Jun 8, 2021 • edited Loading

Furisto Jun 8, 2021

Choose a reason for hiding this comment

ferrell-code Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Furisto Jun 8, 2021

Choose a reason for hiding this comment

utam0k Jun 8, 2021

Choose a reason for hiding this comment

ferrell-code Jun 8, 2021

Choose a reason for hiding this comment

ferrell-code Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

ferrell-code commented Jun 8, 2021 • edited Loading

utam0k commented Jun 9, 2021 • edited Loading

ferrell-code commented Jun 10, 2021 • edited Loading

utam0k commented Jun 10, 2021 • edited Loading

ferrell-code commented Jun 11, 2021 • edited Loading

utam0k commented Jun 11, 2021

ferrell-code commented Jun 23, 2021

ferrell-code Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

utam0k Jun 24, 2021

Choose a reason for hiding this comment

Furisto commented Jul 17, 2021

utam0k commented Jul 18, 2021

ferrell-code commented Jul 18, 2021 • edited Loading

Furisto commented Jul 18, 2021

utam0k commented Jul 18, 2021

ferrell-code commented Jul 19, 2021 • edited Loading

Furisto commented Jul 19, 2021

utam0k left a comment

Choose a reason for hiding this comment

utam0k commented Jul 20, 2021

ferrell-code commented Jun 2, 2021 •

edited

Loading

ferrell-code commented Jun 7, 2021 •

edited

Loading

ferrell-code commented Jun 8, 2021 •

edited

Loading

ferrell-code Jun 8, 2021 •

edited

Loading

ferrell-code Jun 10, 2021 •

edited

Loading

ferrell-code commented Jun 8, 2021 •

edited

Loading

utam0k commented Jun 9, 2021 •

edited

Loading

ferrell-code commented Jun 10, 2021 •

edited

Loading

utam0k commented Jun 10, 2021 •

edited

Loading

ferrell-code commented Jun 11, 2021 •

edited

Loading

ferrell-code Jun 23, 2021 •

edited

Loading

ferrell-code commented Jul 18, 2021 •

edited

Loading

ferrell-code commented Jul 19, 2021 •

edited

Loading