Update nix to 0.28.0 #2728

omprakaash · 2024-03-16T03:42:29Z

Updates nix to 0.28.0. Changed the deprecated functions to new recommended ones. Another noticeable change was the write() ~~and close()~~ functions in nix now take in OwnedFd instead of RawFd. Included changes for them as well.

codecov-commenter · 2024-03-16T07:27:52Z

Codecov Report

Merging #2728 (1edc090) into main (18f3e0b) will decrease coverage by 0.02%.
Report is 18 commits behind head on main.
The diff coverage is 18.75%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2728      +/-   ##
==========================================
- Coverage   65.21%   65.20%   -0.02%     
==========================================
  Files         133      133              
  Lines       16981    16981              
==========================================
- Hits        11074    11072       -2     
- Misses       5907     5909       +2

utam0k · 2024-03-16T12:09:39Z

Another noticeable change was the write() and close() functions in nix now take in OwnedFd instead of RawFd. Included changes for them as well.

Where can I find this change? I couldn't find the change you mentioned.
https://github.com/nix-rust/nix/blob/master/CHANGELOG.md#changed

omprakaash · 2024-03-16T14:18:26Z

Yeah, I could not find that in the change-log too. The change was done in this PR: nix-rust/nix#2100. Sorry, it seems that only write() has been changed. Close() still takes RawFd.

YJDoc2 · 2024-03-18T05:29:19Z

crates/libcontainer/src/process/fork.rs

        // Since nix = "0.27.1", `mmap()` requires a generic type `F: AsFd`.
        // `::<File>` doesn't have any meaning because we won't use it.
-        mman::mmap::<File>(
+        mman::mmap_anonymous(
            None,
            NonZeroUsize::new(default_stack_size).ok_or(CloneError::ZeroStackSize)?,
            mman::ProtFlags::PROT_READ | mman::ProtFlags::PROT_WRITE,
            mman::MapFlags::MAP_PRIVATE | mman::MapFlags::MAP_ANONYMOUS | mman::MapFlags::MAP_STACK,
-            None,
-            0,
        )


Need to check if this change is ok (should be, just have to confirm), but in that case, we should also remove the comment above, as not applicable anymore.

Hey, this still needs to be addressed, the function change is valid, so comment should be updated/removed.

crates/libcontainer/src/process/container_intermediate_process.rs

crates/libcontainer/src/process/fork.rs

utam0k · 2024-03-30T07:31:35Z

@omprakaash It seems CI failed with lint. May I ask you to check it?

omprakaash · 2024-03-31T05:19:58Z

Oh sorry about that, should have fixed the linting issues now.

utam0k · 2024-03-31T23:58:05Z

@omprakaash Thanks!

utam0k · 2024-03-31T23:59:55Z

@zahash @anti-entropy123 @YJDoc2 May I ask you to leave this PR to you?

zahash

looks good

anti-entropy123 · 2024-04-02T05:26:04Z

crates/libcontainer/src/container/tenant_builder.rs

@@ -148,13 +149,13 @@ impl TenantContainerBuilder {
        let mut notify_socket = NotifySocket::new(notify_path);
        notify_socket.notify_container_start()?;

-        close(write_end).map_err(LibcontainerError::OtherSyscall)?;
+        close(write_end.as_raw_fd()).map_err(LibcontainerError::OtherSyscall)?;


Since write_end is OwnedFd, why don't you close it by drop(write_end)?

#[stable(feature = "io_safety", since = "1.63.0")] impl Drop for OwnedFd { #[inline] fn drop(&mut self) { unsafe { ... let _ = libc::close(self.fd); ...

https://doc.rust-lang.org/nightly/src/std/os/fd/owned.rs.html#174-186

You beat me to it 😂 #2728 (comment)

YJDoc2

While other changes are ok, I have some points of concern here regarding the mix of OwnedFd and RawFd . Main point of concern for this PR, is that in several places, we are converting a RawFd to OwnedFd, and then calling close on it manually when the OwnedFd gets closed in its drop. I feel this should be addressed here.

crates/libcontainer/src/process/container_intermediate_process.rs

crates/libcontainer/src/container/tenant_builder.rs

crates/libcontainer/src/process/fork.rs

YJDoc2 · 2024-04-22T07:05:50Z

Hey, thanks for updating. Two notes :

there's conflict in lock file, you can recreate it or copy it over from master and run build to fix it
Can you add explicit drop call where there was close before, and comment the reasoning, maybe linking this PR there? That way in future we know for sure that we have taken care of the fds and the reason for calling drop instead of close

YJDoc2 · 2024-05-06T07:18:06Z

Hey @omprakaash , it seems there are still some more conflicts here. In case you are busy, is it ok if I resolve them and push? because the conflicts are in cargo* files, not code, it would be pretty straight forward to resolve. Apart from that, the code is lgtm.

omprakaash · 2024-05-16T16:14:21Z

Hey sorry for the late reply. I did not have the chance to look at this and missed your comment. Would be great if you could that. Thank you !

yihuaf · 2024-05-17T04:34:48Z

I will take care of this.

yihuaf · 2024-05-17T22:21:08Z

Merge conflict is resolved. I will look over the review comments over the weekend and address them.

commit f0a6b388e7f132648157ca2712e20a1ee93f265a Author: yihuaf <yihuaf@unkies.org> Date: Sun May 19 21:16:21 2024 -0700 Fix the explicit close Signed-off-by: yihuaf <yihuaf@unkies.org> commit 5e23878 Author: yihuaf <yihuaf@unkies.org> Date: Thu May 16 21:31:09 2024 -0700 Rebase to the latest main branch Signed-off-by: yihuaf <yihuaf@unkies.org> commit 3144355 Author: omprakaash <omsuseela@gmail.com> Date: Sat Mar 16 03:28:40 2024 +0000 Update nix to 0.28.0 Signed-off-by: omprakaash <omsuseela@gmail.com> Signed-off-by: om prakaash <omsuseela@gmail.com> Signed-off-by: yihuaf <yihuaf@unkies.org>

yihuaf · 2024-05-20T04:38:29Z

@YJDoc2 This is ready for review one last time. I have addressed the merge conflicts and went over the reviews. One note, the closing of the pipes (The fds that were converted from RawFd to OwnedFd) is actually required. We use the pipes as both a way to communicate between processes and as a barrier for synchronization. The finer control to explicitly close is required, instead of letting the scope takes care of the close through Drop trait. By closing one end of the pipe, we immediately signal to the other end of the pipe, so that any blocking calls on the pipe will immediately return. The PR already included the change, but I changed the comments to reflect this information. The previous comment implies that the drop was there only to maintain the legacy code, but the explicit close actual serves a purpose in this case, not just because we have to manually close due to the use of RawFd.

Let me know if there is anything else you would like to make change.

utam0k

Looks good code itself, but please wait for release v0.3.3 🙏 https://github.com/containers/youki/pulls

YJDoc2

Code looks good, but unfortunately there's another conflict due to latest release. Approving so that after resolving and @utam0k 's approval, this can get merged. Thank you @omprakaash and @yihuaf for taking care of this 🙏

utam0k

Thanks!

yihuaf · 2024-05-23T17:14:16Z

close #2723

utam0k added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Mar 16, 2024

YJDoc2 reviewed Mar 18, 2024

View reviewed changes

YJDoc2 mentioned this pull request Mar 26, 2024

Fully support OwnedFd #2417

Open

zahash reviewed Mar 26, 2024

View reviewed changes

crates/libcontainer/src/process/container_intermediate_process.rs Outdated Show resolved Hide resolved

anti-entropy123 reviewed Mar 26, 2024

View reviewed changes

crates/libcontainer/src/process/fork.rs Outdated Show resolved Hide resolved

omprakaash force-pushed the nix-update branch 2 times, most recently from e4ccbbd to 493da85 Compare March 27, 2024 21:27

omprakaash force-pushed the nix-update branch from 493da85 to 1edc090 Compare March 31, 2024 05:18

utam0k requested review from anti-entropy123, zahash and YJDoc2 April 1, 2024 00:00

utam0k assigned YJDoc2 and omprakaash Apr 1, 2024

zahash approved these changes Apr 1, 2024

View reviewed changes

anti-entropy123 reviewed Apr 2, 2024

View reviewed changes

YJDoc2 requested changes Apr 2, 2024

View reviewed changes

crates/libcontainer/src/process/container_intermediate_process.rs Outdated Show resolved Hide resolved

crates/libcontainer/src/container/tenant_builder.rs Outdated Show resolved Hide resolved

crates/libcontainer/src/process/fork.rs Show resolved Hide resolved

omprakaash force-pushed the nix-update branch 2 times, most recently from 0ec5908 to f3f417f Compare April 19, 2024 11:05

omprakaash force-pushed the nix-update branch 2 times, most recently from e4e7429 to 1123b43 Compare April 28, 2024 10:40

yihuaf force-pushed the nix-update branch 2 times, most recently from 630cccc to 5e23878 Compare May 17, 2024 22:19

yihuaf force-pushed the nix-update branch from 5e23878 to dbd2d32 Compare May 20, 2024 04:32

yihuaf assigned yihuaf and unassigned YJDoc2 May 20, 2024

yihuaf requested review from YJDoc2, anti-entropy123 and a team and removed request for anti-entropy123 May 20, 2024 04:38

utam0k requested changes May 21, 2024

View reviewed changes

YJDoc2 approved these changes May 23, 2024

View reviewed changes

utam0k approved these changes May 23, 2024

View reviewed changes

Merge branch 'main' into nix-update

8f004ec

utam0k enabled auto-merge (squash) May 23, 2024 12:18

utam0k merged commit a35d20f into containers:main May 23, 2024
28 checks passed

github-actions bot mentioned this pull request May 23, 2024

Release for v0.3.4 #2791

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update nix to 0.28.0 #2728

Update nix to 0.28.0 #2728

omprakaash commented Mar 16, 2024 •

edited

codecov-commenter commented Mar 16, 2024 •

edited

utam0k commented Mar 16, 2024

omprakaash commented Mar 16, 2024 •

edited

YJDoc2 Mar 18, 2024

YJDoc2 Apr 2, 2024

utam0k commented Mar 30, 2024

omprakaash commented Mar 31, 2024

utam0k commented Mar 31, 2024

utam0k commented Mar 31, 2024

zahash left a comment

anti-entropy123 Apr 2, 2024 •

edited

YJDoc2 Apr 2, 2024

YJDoc2 left a comment

YJDoc2 commented Apr 22, 2024

YJDoc2 commented May 6, 2024

omprakaash commented May 16, 2024

yihuaf commented May 17, 2024

yihuaf commented May 17, 2024

yihuaf commented May 20, 2024

utam0k left a comment

YJDoc2 left a comment

utam0k left a comment

yihuaf commented May 23, 2024

Update nix to 0.28.0 #2728

Update nix to 0.28.0 #2728

Conversation

omprakaash commented Mar 16, 2024 • edited

codecov-commenter commented Mar 16, 2024 • edited

Codecov Report

utam0k commented Mar 16, 2024

omprakaash commented Mar 16, 2024 • edited

YJDoc2 Mar 18, 2024

Choose a reason for hiding this comment

YJDoc2 Apr 2, 2024

Choose a reason for hiding this comment

utam0k commented Mar 30, 2024

omprakaash commented Mar 31, 2024

utam0k commented Mar 31, 2024

utam0k commented Mar 31, 2024

zahash left a comment

Choose a reason for hiding this comment

anti-entropy123 Apr 2, 2024 • edited

Choose a reason for hiding this comment

YJDoc2 Apr 2, 2024

Choose a reason for hiding this comment

YJDoc2 left a comment

Choose a reason for hiding this comment

YJDoc2 commented Apr 22, 2024

YJDoc2 commented May 6, 2024

omprakaash commented May 16, 2024

yihuaf commented May 17, 2024

yihuaf commented May 17, 2024

yihuaf commented May 20, 2024

utam0k left a comment

Choose a reason for hiding this comment

YJDoc2 left a comment

Choose a reason for hiding this comment

utam0k left a comment

Choose a reason for hiding this comment

yihuaf commented May 23, 2024

omprakaash commented Mar 16, 2024 •

edited

codecov-commenter commented Mar 16, 2024 •

edited

omprakaash commented Mar 16, 2024 •

edited

anti-entropy123 Apr 2, 2024 •

edited