Merged
Conversation
thaJeztah
commented
Jul 17, 2019
Dockerfile
Outdated
Member
Author
There was a problem hiding this comment.
opened checkpoint-restore/criu#743 for this as well
Member
Author
|
Windows build is failing, but no details what exactly is failing 😞 |
Member
Author
|
And new LOL, I don't see a difference; diff --git a/pkg/parsers/kernel/kernel_windows.go b/pkg/parsers/kernel/kernel_windows.go
index b7b15a1fd2..a0712ce633 100644
--- a/pkg/parsers/kernel/kernel_windows.go
+++ b/pkg/parsers/kernel/kernel_windows.go
@@ -44,7 +44,7 @@ func GetKernelVersion() (*VersionInfo, error) {
}
KVI.major = int(dwVersion & 0xFF)
- KVI.minor = int((dwVersion & 0XFF00) >> 8)
+ KVI.minor = int((dwVersion & 0xFF00) >> 8)
KVI.build = int((dwVersion & 0xFFFF0000) >> 16)
return KVI, nilOh! lowercase |
Member
Author
|
Failures on Experimental, Janky, PowerPC, and s390x: https://jenkins.dockerproject.org/job/Docker-PRs/54911/console Failures on Windows RS1 (fails to build) https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS1/25943/console Failures on Windows RS5 (fails to build) https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS5-Process/3036/console |
Member
Author
|
This one passes locally; |
Member
Author
|
Logs for the failing And all logs of the tests and daemon: |
thaJeztah
commented
Jul 17, 2019
Member
Author
There was a problem hiding this comment.
forcing IPv4 for the ping; see if that's making a difference
Member
Author
|
One thing I noticed on my machine (Docker for Mac) is that starting the daemon is really, really slow; looks like creating iptables rules takes a lot longer; That's almost 7 seconds! If I start the daemon with Details |
Member
Author
|
Switching to master, and using Go 1.12.7 on stretch; And without iptables: Details |
Member
Author
|
This branch again, using Go And without iptables So looks to be due to buster (perhaps the iptables version in buster switching to nftables?) Details |
Member
Author
|
Added a bit of debugging for ``, and printing Nothing that really stands out there |
Member
Author
|
Ok, found the problem (w.r.t. the failing tests); it's indeed because the CI machines run on Ubuntu 16.04, which uses legacy iptables, whereas the Debian buster image uses Added a hacky few lines to the https://wiki.debian.org/iptables
|
Member
Author
|
Looks like the performance issue is due to nftables; Switch to iptables-legacy: Switch to iptables-nft: |
Member
Author
|
switching to iptables-legacy fixes the tests; |
tianon
reviewed
Jul 22, 2019
hack/dind
Outdated
Member
There was a problem hiding this comment.
Shouldn't this happen in the Dockerfile instead so we don't accidentally apply this change somewhere less harmless than a container like on a contributor's host machine?
Member
Author
There was a problem hiding this comment.
It's really a quick hack to see if it did the job
Surprisingly; when I did it as a RUN step, it gave a "permission denied; need to be root"
Member
There was a problem hiding this comment.
Oh that's really odd -- AFAICT, we're not using USER or anything so it already should be root. 😓
(And doing update-alternatives shouldn't be running iptables or anything that might need additional privileges.)
Member
Author
There was a problem hiding this comment.
That's what I would expect as well; didn't look closely yet, but I suspect it's calling iptables (perhaps just to check if it's installed?)
Member
Author
|
Arg; forgot to remove |
Member
|
Instead of removing it, I'd recommend switching it to be |
Member
Author
|
Yes, that's what I actually meant 😅 - updated 🤞 |
Member
Author
|
Will have to look into the Windows build failure; the file that it marks missing is on the repository; https://github.com/moby/moby/blob/master/vendor/cloud.google.com/go/compute/metadata/metadata.go |
73e405d to
eb4289e
Compare
Contributor
Member
Author
|
Yes, was actually thinking of separating it from this PR (although it is "accepted" by older versions of Go) |
8f9487c to
ccdaf19
Compare
ccdaf19 to
c4e96d8
Compare
Member
Author
|
Interesting; this is why Looking at this, it looks like |
c4e96d8 to
810e9b2
Compare
Member
Author
|
ha! looks like Still wondering why it switched to use |
810e9b2 to
2e19350
Compare
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2e19350 to
38e4ae3
Compare
Member
Author
|
@kolyshkin @tiborvass @tianon this is all green now; ready for review! |
Contributor
|
@seemethere @zelahi I'm afraid if we merge this, release scripts somewhere will not add the GO111MODULE=off. |
seemethere
approved these changes
Sep 19, 2019
tiborvass
approved these changes
Sep 19, 2019
This was referenced Feb 22, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on top of / depends on (when backporting, these are also needed):
I'll rebase once those are merged
testing if things work or break