Skip to content

[DNM] Test CI with runc 1.2#48336

Closed
rata wants to merge 2 commits intomoby:masterfrom
rata:runc-1.2-debug
Closed

[DNM] Test CI with runc 1.2#48336
rata wants to merge 2 commits intomoby:masterfrom
rata:runc-1.2-debug

Conversation

@rata
Copy link
Contributor

@rata rata commented Aug 15, 2024

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@rata rata force-pushed the runc-1.2-debug branch 4 times, most recently from 0f86b83 to 64708e9 Compare August 15, 2024 11:18
@rata
Copy link
Contributor Author

rata commented Aug 19, 2024

Let's unify the debugging on this other PR: #48160. I've updated with my (not very useful) findings: #48160 (comment)

run: |
echo ${{ steps.tests.outputs.matrix }}

integration-cli:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do they still fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda I added them back and they don't fail.

I removed them to speed-up the CI run with several tests (bisect, etc.). But it makes sense to add them back once I got a result, to verify the result

@rata rata force-pushed the runc-1.2-debug branch 4 times, most recently from 00aba42 to c56c2cd Compare August 28, 2024 13:22
@rata rata force-pushed the runc-1.2-debug branch 13 times, most recently from 2f76d53 to 2b723e7 Compare August 30, 2024 13:19
Comment on lines +948 to +953
retry:
nwList, nw6List, err := ifaceAddrs(bridgeName)
if err != nil {
if errors.Is(err, unix.EINTR) {
goto retry
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you hit this in practice? If yes, it's better to patch github.com/docker/libnetwork/ns directly (I understand you're just testing here).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I hit it in CI several times (not a lot, I think). I agree patching it in the right place seems better, as you noted, this is just for testing runc 1.2.0-rc.2 :)

@AkihiroSuda
Copy link
Member

Could you rebase?

rc.2 release notes: https://github.com/opencontainers/runc/releases/tag/v1.2.0-rc.2
rc.1 release notes: https://github.com/opencontainers/runc/releases/tag/v1.2.0-rc.1

Breaking changes and deprecations are included below;

Breaking changes:

Several aspects of how mount options work has been adjusted in a way that
could theoretically break users that have very strange mount option strings.
This was necessary to fix glaring issues in how mount options were being
treated. The key changes are:

- Mount options on bind-mounts that clear a mount flag are now always
  applied. Previously, if a user requested a bind-mount with only clearing
  options (such as rw,exec,dev) the options would be ignored and the
  original bind-mount options would be set. Unfortunately this also means
  that container configurations which specified only clearing mount options
  will now actually get what they asked for, which could break existing
  containers (though it seems unlikely that a user who requested a specific
  mount option would consider it "broken" to get the mount options they
  asked foruser who requested a specific mount option would consider it
  "broken" to get the mount options they asked for). This also allows us to
  silently add locked mount flags the user did not explicitly request to be
  cleared in rootless mode, allowing for easier use of bind-mounts for
  rootless containers.
- Container configurations using bind-mounts with superblock mount flags
  (i.e. filesystem-specific mount flags, referred to as "data" in
  mount(2), as opposed to VFS generic mount flags like MS_NODEV) will
  now return an error. This is because superblock mount flags will also
  affect the host mount (as the superblock is shared when bind-mounting),
  which is obviously not acceptable. Previously, these flags were silently
  ignored so this change simply tells users that runc cannot fulfil their
  request rather than just ignoring it.

Deprecated

- runc option --criu is now ignored (with a warning), and the option will
  be removed entirely in a future release. Users who need a non-standard
  criu binary should rely on the standard way of looking up binaries in
  $PATH.
- runc kill option -a is now deprecated. Previously, it had to be specified
  to kill a container (with SIGKILL) which does not have its own private PID
  namespace (so that runc would send SIGKILL to all processes). Now, this is
  done automatically.
- github.com/opencontainers/runc/libcontainer/user is now deprecated, please
  use github.com/moby/sys/user instead. It will be removed in a future
  release.

[@kolyshkin: set CC and CFLAGS, add xx-verify for runc-dmz, use
EXTRA_BUILDTAGS (no need to specify default tags like seccomp)].

Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@rata rata force-pushed the runc-1.2-debug branch 2 times, most recently from 4181aee to 68d033d Compare October 21, 2024 12:49
@rata
Copy link
Contributor Author

rata commented Oct 21, 2024

@AkihiroSuda I was aiming to debug the issues, not really merge this. And with the overlayfs trick for protecting /proc/self/exe, this got fast again that no failures are detected (I pushed here using the latest runc from main).

I do think we should wait for the container to be ready, though. If you think so too, I can try to revive that. But I'll be happy if you want to take the patches and do the PR yourself. Otherwise, I can do it but in a few days/weeks, as I'm super busy right now (I don't think there is urgency now, especially since the test pass fine with the latest improvements. Do you think so too?)

EDIT: It seems one test in one worker failed. I have repushed. But it's defeinitely not so many tests in all workers failing, though.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
@AkihiroSuda
Copy link
Member

Thanks @rata

I do think we should wait for the container to be ready, though. If you think so too, I can try to revive that. But I'll be happy if you want to take the patches and do the PR yourself. Otherwise

PR:

@rata rata closed this Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants