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
Bump hcsshim to get some fixes. #42270
Conversation
This also requires bumping winio. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Heh, looks like you lost a small race with @katiewasnothere / #42269, although you've also updated go-winio here (but that's the only difference I see). Edit: and the link to microsoft/hcsshim@e811ee7 (microsoft/hcsshim#991) over there, which I gather is the specific commit that fixes the customer issues? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if CI is running again; windows CI is currently failing (is being worked on)
@cpuguy83 @katiewasnothere any possibility to open up about which kind of which kind of issue(s) are expected to be fixed by this? |
@olljanat Specific case is too much logging in errors. |
Ok this looks like a regression / change in behavior:
Does hcsshim return a But we should update our API docs to reflect that a 400 error can be returned; |
Hmm... funny, so looking at the test, older API versions could return a moby/integration-cli/docker_api_exec_test.go Lines 195 to 199 in 08e6790
|
Actually thinking now if 400 is correct; the request itself for this case (start a container) is valid, but the container configuration is invalid, so 🤷♂️ depends on how you look at it I guess. |
I suspect the problem is here; Lines 141 to 147 in 7b9275c
Looking at the error message returned (wrapped for readability);
✅ The string does contain Line 147 in 7b9275c
❌ The string does not contain cmd (invalid ), and because of that, contains(errDesc, cmd) won't match; Line 144 in 7b9275c
Erm... so actually that second is not true (for some reason); it does match; it looks for entrypoint (perhaps that's an empty string, so will result in "match anything"; Line 270 in 7b9275c
Entrypoint and Argss , because we trigger an event; Line 183 in 7b9275c
And when we do match, we return a Line 150 in 7b9275c
So was it previously broken? Is it broken on Linux (if we return a 500 there)? |
@cpuguy83 could you have a peek at the failures? (I tried finding what's causing it in my comment above) |
It seems like specifically because we took out the extra error details the error check does not match anymore. |
Whether or not the command path is in the error message is a an implementation detail. For example, on Windows the only reason this ever matched was because it dumped the entire container config into the error message, but this had nothing to do with the actual error. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Wait.. why is this failing? Broken package? https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-42270/3/pipeline/224
Let me restart |
This one I've seen a couple of time srecently, but seems like a newly introduced problem (just not on this PR, I think?)
This one looks legit, I don't think I've seen that.
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@tianon - good to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
closes #42269
This also requires bumping winio.
This addresses some customer issues for us.