I don't think it's a good idea to start vendoring a branch of libcontainer. opencontainers/runc#470 fixes master, and there's no reason not to merge it if we can't figure out why the state transition stuff is broken. There's no reason we can't re-merge a fixed version of the state transition stuff later.

Because, if this gets put into 1.10, we now have to keep that branch around in perpetuity (who knows when someone will be trying to figure out a bug in 1.10, and they run hack/vendor.sh to vendor code they think will fix the problem). It's just a weird choice to make, when we have a PR that fixes the problem.

However, if you're committed to us vendoring a branch, you should cherry-pick opencontainers/runc@bf899fe. It fixes a security bug.

Experimental failed on #19472, Janky on #19393 😢

Hm Windows timeout (I think), but the first error looks weird;

05:24:01 INFO: Attempting curl to ping the DIND daemon
05:24:01 ERROR: Invalid syntax. Default option is not allowed more than '1' time(s).
05:24:01 Type "TIMEOUT /?" for usage.

….

05:24:01 -----------------------------------------------
05:24:01 Windows CI Timeout Detection - End of debugging
05:24:01 ———————————————————————

Windows error is unrelated

LGTM

LGTM

@calavera @tiborvass @crosbymichael Can you please explain why you all LGTM'd and merged a vendor for a non-master branch of runc, when opencontainers/runc#470 actually fixes the problems, passes all tests, has a vendor PR open that also passes all tests and merges cleanly on master, and has two LGTMs? More importantly, is there a reason why my comment wasn't even addressed?

The reason why I thought it was important to deal with this problem is not only because hot-fixes like this have tendency to stick around, but also because I actually have to bisect docker quite regularly, and usually run ./hack/vendor.sh in order to test some upstream changes to see if we should backport them. Yes, I get it, you don't support any previous versions. But other people (like me) do. Why make it hard for those people?

For future reference to anyone who needed to run hack/vendor.sh after the branch has been removed (such as people in the future who are bisecting Docker), here is the relevant information about this patch:

The commit opencontainers/runc@47e3f834d73e76bc2a6a585b48d2a93325b34979 was based on the commit opencontainers/runc@d97d5e8b007e4657316eed76ea30bc0f690230cf. The following commits were cherry-picked from master:

• opencontainers/runc@41edbeb
• opencontainers/runc@f048eaf
• opencontainers/runc@9c41e83

@cyphar Since Docker already released RC, we can't vendor runc master, so cherry-pick important bug fixes would be the way to go. After 1.10 released, we can vendor the whole runc mater again.

In this model, I think the path sanitation patch which fixes security problem can also be cherry-picked, we just need someone open the PR.

@cyphar we're still figuring out the process for vendoring and all suggestions to improve it are welcome.

Having said that, currently, the process does not prevent us from sending subsequent vendoring PRs if need be. It seems to me that opencontainers/runc#470 was not approved in time for this round of vendoring.

Your vendoring PR #19024 got indeed 2 LGTMs, but it was referencing your fork, which is not okay. We probably should make that clearer in the vendoring policy.

About vendoring commits that are not in upstream master, I don't directly see it as a problem and I don't quite understand the issue you have with bisecting, however I do see problems with our vendoring policy and I suggest we continue this discussion on the maintainers mailing list or in a PR to the vendoring policy.

Although we are in a release period, sorry for not addressing your issues in time.

@tiborvass opencontainers/runc#470 got one LGTM and one tentative LGTM ("I'm okay with this but ..."). That's what I was referring to, and that counts as 2 IMO.

About vendoring commits that are not in upstream master, I don't directly see it as a problem and I don't quite understand the issue you have with bisecting, however I do see problems with our vendoring policy and I suggest we continue this discussion on the maintainers mailing list or in a PR to the vendoring policy.

In SUSE, we backport fixes to bugs for 1.9.1 and (previously) 1.8.3. Quite a few bug fixes are in vendor commits (and sometimes bugs are caused by vendor commits). My workflow for such cases is something like:

• git bisect the night away until we hit the commit that broke it.
• If it's a vendor commit, test whether running hack/vendor.sh with different commits of the library fixes the problem (specifically if I'm trying to bisect a library that Docker depends on and Docker has a bug based on that library, this is quite important).
• Make a patch for SUSE distros.

Now, you could argue that it is possible to do the same thing without hack/vendor.sh. Yes I can. But it makes life significantly harder. The issue is that the hotfix branch is not going to be sticking around for the next few months, so when I eventually hit a bug in the window between this vendor and the next one, it's going to be a PITA to test.

I would appreciate it if the vendoring policy at least required some detailed decription of the non-master branch being vendored. I understand that hotfixes are necessary, and that sometimes it's unavoidable to vendor a branch, but the way it was done ("here's a cryptic commit hash, good luck understanding what patches we just pulled in, person debugging a problem 3 months from now") wasn't IMO how it should be done.

All of that being said, I'd be happy to start a dialogue about vendoring policy. Another weird thing that Docker appears to do is tag non-master branches with release tags (which is quite confusing).

I agree we should improve the process for cherry-picking patches of vendors in release period, I got the same problem sometimes, it's all for developers. What about we just backport patches without touching vendor ids, and keep these patch in separate?

@cyphar @hqhq thanks for your input, but please let's stop hijacking this merged PR. We can continue on a PR changing the vendor policy file, or in a new GH issue.