Upgrade to Go 1.20.1 #44620

neersighted · 2022-12-09T16:02:13Z

Changes to enable Go 1.20; @corhere has taken over this PR.

Prerequisites:

thaJeztah · 2022-12-09T18:28:39Z

Hm... 🤔

Setup go version spec 1.20rc1
Attempting to download 1.20rc1...
matching 1.20rc1...
Not found in manifest.  Falling back to download directly from Go
Error: Unable to find Go version '1.20rc1' for platform linux and architecture x64.

Oh! Wonder if they (for the fallback) try to download 1.20rc1 without the go prefix 🤔

thaJeztah · 2022-12-09T18:34:43Z

https://github.com/actions/setup-go/blob/30c39bfe0c7338d0d8e99486938f1066b2f92108/src/installer.ts#L77

`Not found in manifest.  Falling back to download directly from Go'

https://github.com/actions/setup-go/blob/30c39bfe0c7338d0d8e99486938f1066b2f92108/src/installer.ts#L245

let version = makeSemver(candidate.version);

https://github.com/actions/setup-go/blob/30c39bfe0c7338d0d8e99486938f1066b2f92108/src/installer.ts#L249-L253

      goFile = candidate.files.find(file => {
        core.debug(
          `${file.arch}===${archFilter} && ${file.os}===${platFilter}`
        );
        return file.arch === archFilter && file.os === platFilter;

So, hm.. at a cursory glance; they download https://go.dev/dl/?mode=json&include=all

try to match a SemVer version (?), which go doesn't use
try to match against x64, but Go uses GOOS format, so the list will have amd64

thaJeztah · 2022-12-12T19:43:30Z

LOL; this one is even more funny; this is the failure on Windows setting up Go;

Setup go version spec 1.20rc1
Attempting to download 1.20rc1...
matching 1.20rc1...
Not found in manifest.  Falling back to download directly from Go
Error: Unable to find Go version '1.20rc1' for platform linux and architecture x64.

"You had one job..."

neersighted · 2022-12-12T21:18:00Z

It looks like this is fundamentally actions/setup-go#63, which is pretty stalled out upstream. We can use the RC by modifying our version strings to match GHA's expectations, but I think this is another case for my plan to centralize the Go version somewhere and plumb it through with scripts.

neersighted · 2023-01-10T17:07:09Z

Okay, this is ready for review in the sense that while it should not be merged, we are seeing the real CI failures and can start addressing them.

PTAL @thaJeztah @corhere @cpuguy83 @tianon

thaJeztah · 2023-01-11T23:42:40Z

This one failure looks possibly genuine; I kicked ci at least once, but think it was the same failure first time;

=== FAIL: amd64.integration-cli TestDockerCLIRunSuite/TestCmdCannotBeInvoked (0.39s)
    docker_cli_run_test.go:3667: assertion failed: 
        Command:  /usr/local/cli/docker run --name testCmdCannotBeInvoked busybox /etc
        ExitCode: 125
        Error:    exit status 125
        Stdout:   
        Stderr:   /usr/local/cli/docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "/etc": is a directory: unknown.
        time="2023-01-11T22:33:24Z" level=error msg="error waiting for container: context canceled" 
        
        
        Failures:
        ExitCode was 125 expected 126
        Expected error to contain "exit status 126"
    --- FAIL: TestDockerCLIRunSuite/TestCmdCannotBeInvoked (0.39s)

=== FAIL: amd64.integration-cli TestDockerCLIRunSuite (206.38s)

thaJeztah · 2023-01-11T23:43:36Z

Golangci-lint failure is likely because golangci is not yet compatible with go1.20

neersighted · 2023-01-11T23:44:45Z

It looks like the OOM killer to me, though I'm not sure how to get dmesg from GHA without adding it as an explicit step (I'll try that next).

I don't think golangci-lint should need explicit compatibility with Go 1.20, and there are no open issues on their tracker that correlate. Is this typical behavior during a Go upgrade?

thaJeztah · 2023-01-11T23:46:50Z

Haven't looked closely at the output. Perhaps you're right and I made the wrong assumption.

It is common though that golangci-lint fails to complete successfully on Go updates (although usually it gives an informative error message). We could try running it locally in a go1.20 container

thaJeztah · 2023-01-11T23:49:48Z

The test failure could be string matching; I think those status code are based on output of "why the command failed" (it's ugly, but don't think there's better solutions for some of that); perhaps the error response from Golang was updated. I recently made some updates in that area in #43248

edit: code is here;

moby/daemon/errors.go

Lines 183 to 201 in 6d212fa

    
           // isInvalidCommand tries to detect if the reason the container failed to start 
        
           // was due to an invalid command for the container (command not found, or not 
        
           // a valid executable). 
        
           func isInvalidCommand(errMessage string) bool { 
        
           	errMessage = strings.ToLower(errMessage) 
        
           	errMessages := []string{ 
        
           		"executable file not found", 
        
           		"no such file or directory", 
        
           		"system cannot find the file specified", 
        
           		"failed to run runc create/exec call", 
        
           	} 
        
           	for _, msg := range errMessages { 
        
           		if strings.Contains(errMessage, msg) { 
        
           			return true 
        
           		} 
        
           	} 
        
           	return false 
        
           }

thaJeztah · 2023-01-19T19:26:21Z

rc3 is available now

corhere · 2023-02-03T21:41:02Z

daemon/errors.go

+	// syscall.EACCESS to syscall.EISDIR. Unfortunately docker/cli checks
+	// whether the error message contains syscall.EACCESS.Error() to
+	// determine whether to exit with code 126 or 125, so we have little


For reference, this is the offending docker/cli code.

Oh my god, when I glanced at this failure, I thought it might be something like that, but I did not fully comprehend the magnitude of coupling.

docker/cli#4001

corhere · 2023-02-03T22:10:56Z

daemon/errors.go

+	// Go 1.20 changed the error for attempting to execute a directory from
+	// syscall.EACCESS to syscall.EISDIR. Unfortunately docker/cli checks


This is a compatibility issue which manifests when runc is compiled against Go 1.20, irrespective of the Go version that Moby is compiled with.

corhere · 2023-02-03T22:43:02Z

The TestIntegration/TestFileOpCopyIncludeExclude/worker=dockerd buildkit integration test is failing, but it is also failing on master so is unlikely to be a Go 1.20 problem. Everything else is green; I think we're good to go on Go 1.20!

thaJeztah · 2023-02-14T20:52:33Z

Some questions;

Was the distribution/xfer: fix download fencepost bug commit intended to be part of the go1.20 related work?
I see that (but perhaps it's GitHub's presentation) the Go1.20.0 commit is first in the list of commits, but the golangci-lint and fixes related to go1.20 changes are after that; should the order be reversed?
I'm wondering if most (if not all) changes related to the deprecation and some other changes could already work on go1.19 (if so, we could consider including those into the 23.0 branch already in case that needs to be updated to go1.20 in future)

corhere · 2023-02-14T21:10:51Z

Was the distribution/xfer: fix download fencepost bug commit intended to be part of the go1.20 related work?

Yes, through a long causal chain. golangci-lint had to be bumped to a version which supports Go 1.20. The bumped golangci-lint flagged a loop-closure bug in the distribution/xfer unit tests. Fixing the loop-closure bug revealed that some of the unintentionally-skipped subtests fail. That commit gets the tests passing again.

I see that (but perhaps it's GitHub's presentation) the Go1.20.0 commit is first in the list of commits, but the golangci-lint and fixes related to go1.20 changes are after that; should the order be reversed?

That depends on your opinions on test-driven development. I feel it would be dishonest to invert the history, fixing things before they could have been known to be broken.

I'm wondering if most (if not all) changes related to the deprecation and some other changes could already work on go1.19 (if so, we could consider including those into the 23.0 branch already in case that needs to be updated to go1.20 in future)

Yes, all the changes could be backported.

thaJeztah · 2023-02-15T09:26:59Z

That depends on your opinions on test-driven development. I feel it would be dishonest to invert the history, fixing things before they could have been known to be broken.

Only in this case, such changes were not due to the Go update, but because we updated a linter?

I can see where you're coming from, but I think "rewriting history" is already something we done with our rebase+squash/cleanup based workflow, that that should not be an issue. W.r.t. an "honest history"; where suitable, we can of course still include a mention of Go and/or Golang-CI versions in the commit message ("Older versions of GolangCI did not catch this error", or "In preparation of newer Go versions, which no longer allow this")

What we tended to do in the past is to;

Prepare changes needed for the Go update (if any) that already applied before the update itself
This may include addressing issues caught by newer versions of linters
Update the linter
Update Go, and changes that must go together with the Go update

Most of the time we did the preparation steps in a separate PR, both for visibility, as well as make backporting those changes to release branch(es) non-ambiguous (including it in the same PR as the Go update could otherwise mean "this is a partial backport, with commits X Y and Z omitted").

In this approach;

All changes leading up to the Go (and/or Linter) update would be tested in CI before updating Go itself; this allowed us to be sure those changes work with previous Go versions (thus could qualify for backporting if desired).
The Go update PR itself would be an "epic" (have a checkbox list with changes) so that we can still keep track of things related to the update (FWIW; this is one reason I tend to backport individual Go updates (e.g. see [20.10 backport] update to go1.19.6 #44990) - I track back all Go updates, and look if potentially other changes were not yet included in the branch. Having the individual commits in backports also helps correlate the changes with the main branch)
At that point, the Go update itself is more of a "ceremony" (it would only have the Go version update and only changes that MUST go together with the update (breaking changes))

corhere · 2023-02-15T17:20:18Z

@thaJeztah you make a very compelling argument. I've captured that in the repo wiki for the benefit of the next person to take on the task of bumping the Go version.

Signed-off-by: Bjorn Neergaard <bneergaard@mirantis.com> Co-authored-by: Cory Snider <csnider@mirantis.com> Signed-off-by: Cory Snider <csnider@mirantis.com>

Signed-off-by: Cory Snider <csnider@mirantis.com>

thaJeztah

LGTM

neersighted · 2023-02-22T22:37:47Z

LGTM

neersighted force-pushed the go_1.20 branch from 7671a26 to 4bdbeb4 Compare December 9, 2022 16:03

thaJeztah changed the title ~~[DNM] go1.2rc1 test~~ [DNM] go1.20rc1 test Dec 9, 2022

neersighted force-pushed the go_1.20 branch from 4bdbeb4 to d190934 Compare December 12, 2022 19:05

neersighted force-pushed the go_1.20 branch 6 times, most recently from e56cbab to 7d08fda Compare January 10, 2023 16:42

neersighted changed the title ~~[DNM] go1.20rc1 test~~ [DNM] go1.20rc2 test Jan 10, 2023

neersighted marked this pull request as ready for review January 10, 2023 17:06

neersighted requested a review from tianon as a code owner January 10, 2023 17:06

neersighted force-pushed the go_1.20 branch 2 times, most recently from 7d33e86 to ece4f68 Compare January 11, 2023 22:09

neersighted force-pushed the go_1.20 branch from ece4f68 to f055d17 Compare January 19, 2023 19:29

neersighted changed the title ~~[DNM] go1.20rc2 test~~ [DNM] go1.20rc3 test Jan 19, 2023

neersighted force-pushed the go_1.20 branch from f055d17 to b4440e3 Compare February 3, 2023 12:50

neersighted changed the title ~~[DNM] go1.20rc3 test~~ [DNM] go1.20 test Feb 3, 2023

corhere force-pushed the go_1.20 branch from f6e5ac7 to 5912bfa Compare February 3, 2023 17:32

corhere force-pushed the go_1.20 branch from 6f4839a to d3ae0f2 Compare February 3, 2023 21:35

corhere reviewed Feb 3, 2023

View reviewed changes

neersighted changed the title ~~[DNM] go1.20 test~~ Go 1.20 support Feb 13, 2023

corhere force-pushed the go_1.20 branch from d3ae0f2 to 51eeb19 Compare February 13, 2023 18:03

corhere requested a review from samuelkarp as a code owner February 13, 2023 18:24

thaJeztah added status/2-code-review impact/changelog area/packaging labels Feb 15, 2023

thaJeztah added this to the v-next milestone Feb 15, 2023

neersighted and others added 2 commits February 22, 2023 16:37

Upgrade to Go 1.20

d4e7a87

Signed-off-by: Bjorn Neergaard <bneergaard@mirantis.com> Co-authored-by: Cory Snider <csnider@mirantis.com> Signed-off-by: Cory Snider <csnider@mirantis.com>

Upgrade to Go 1.20.1

cd779da

Signed-off-by: Cory Snider <csnider@mirantis.com>

corhere force-pushed the go_1.20 branch from 1312066 to cd779da Compare February 22, 2023 21:38

corhere requested a review from thaJeztah February 22, 2023 21:38

corhere changed the title ~~Go 1.20 support~~ Upgrade to Go 1.20.1 Feb 22, 2023

thaJeztah approved these changes Feb 22, 2023

View reviewed changes

neersighted merged commit 89ac206 into moby:master Feb 22, 2023

neersighted deleted the go_1.20 branch February 22, 2023 22:37

corhere added the process/cherry-pick label Feb 23, 2023

corhere mentioned this pull request Feb 23, 2023

[23.0 backport] Upgrade to Go 1.20.1 #45069

Closed

corhere added process/cherry-picked and removed process/cherry-pick labels Feb 27, 2023

corhere mentioned this pull request Jun 23, 2023

[23.0 backport] Update Go to go1.20.5 #45803

Merged

thaJeztah mentioned this pull request Oct 20, 2023

[20.10] Update to go1.20.10, golang/org/x/net v0.17.0 #46692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Go 1.20.1 #44620

Upgrade to Go 1.20.1 #44620

neersighted commented Dec 9, 2022 •

edited by corhere

thaJeztah commented Dec 9, 2022

thaJeztah commented Dec 9, 2022 •

edited

thaJeztah commented Dec 12, 2022

neersighted commented Dec 12, 2022

neersighted commented Jan 10, 2023

thaJeztah commented Jan 11, 2023

thaJeztah commented Jan 11, 2023

neersighted commented Jan 11, 2023

thaJeztah commented Jan 11, 2023

thaJeztah commented Jan 11, 2023 •

edited

thaJeztah commented Jan 19, 2023

corhere Feb 3, 2023

neersighted Feb 3, 2023

corhere Feb 3, 2023

corhere Feb 3, 2023

corhere commented Feb 3, 2023

thaJeztah commented Feb 14, 2023

corhere commented Feb 14, 2023

thaJeztah commented Feb 15, 2023

corhere commented Feb 15, 2023

thaJeztah left a comment

neersighted commented Feb 22, 2023

		// Go 1.20 changed the error for attempting to execute a directory from
		// syscall.EACCESS to syscall.EISDIR. Unfortunately docker/cli checks

Upgrade to Go 1.20.1 #44620

Upgrade to Go 1.20.1 #44620

Conversation

neersighted commented Dec 9, 2022 • edited by corhere

thaJeztah commented Dec 9, 2022

thaJeztah commented Dec 9, 2022 • edited

thaJeztah commented Dec 12, 2022

neersighted commented Dec 12, 2022

neersighted commented Jan 10, 2023

thaJeztah commented Jan 11, 2023

thaJeztah commented Jan 11, 2023

neersighted commented Jan 11, 2023

thaJeztah commented Jan 11, 2023

thaJeztah commented Jan 11, 2023 • edited

thaJeztah commented Jan 19, 2023

corhere Feb 3, 2023

Choose a reason for hiding this comment

neersighted Feb 3, 2023

Choose a reason for hiding this comment

corhere Feb 3, 2023

Choose a reason for hiding this comment

corhere Feb 3, 2023

Choose a reason for hiding this comment

corhere commented Feb 3, 2023

thaJeztah commented Feb 14, 2023

corhere commented Feb 14, 2023

thaJeztah commented Feb 15, 2023

corhere commented Feb 15, 2023

thaJeztah left a comment

Choose a reason for hiding this comment

neersighted commented Feb 22, 2023

neersighted commented Dec 9, 2022 •

edited by corhere

thaJeztah commented Dec 9, 2022 •

edited

thaJeztah commented Jan 11, 2023 •

edited