Skip to content
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

Upgrade to go 1.22.1 (with fix for dockerfiles) #1764

Merged
merged 8 commits into from Mar 12, 2024

Conversation

gargnitingoogle
Copy link
Collaborator

@gargnitingoogle gargnitingoogle commented Mar 4, 2024

Description

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - Sanity testing
  2. Unit tests - NA
  3. Integration tests - Passed.
  4. Perf tests
+--------+------------+--------------+------------+--------------+--------------+
| Branch | File Size  |   Read BW    |  Write BW  | RandRead BW  | RandWrite BW |
+--------+------------+--------------+------------+--------------+--------------+
| Master |  0.25MiB   | 536.37MiB/s  | 1.26MiB/s  |  77.95MiB/s  |  1.08MiB/s   |
|   PR   |  0.25MiB   | 518.86MiB/s  |  1.3MiB/s  |  70.12MiB/s  |  1.31MiB/s   |
|        |            |              |            |              |              |
| Master | 48.828MiB  | 4645.71MiB/s | 80.55MiB/s | 1238.05MiB/s |  79.37MiB/s  |
|   PR   | 48.828MiB  | 4477.76MiB/s | 79.18MiB/s | 1277.78MiB/s |  80.71MiB/s  |
|        |            |              |            |              |              |
| Master | 976.562MiB | 4406.04MiB/s | 33.37MiB/s | 477.44MiB/s  |  32.95MiB/s  |
|   PR   | 976.562MiB | 4423.47MiB/s | 33.91MiB/s | 608.93MiB/s  |  31.9MiB/s   |
|        |            |              |            |              |              |
+--------+------------+--------------+------------+--------------+--------------+

@gargnitingoogle gargnitingoogle force-pushed the gargnitin/fix-dockerfile-go-get branch 3 times, most recently from eb68b71 to 8685637 Compare March 5, 2024 10:49
@gargnitingoogle gargnitingoogle changed the title Replace go get with go install in Dockerfiles Fix dockerfile for go 1.22 Mar 5, 2024
@gargnitingoogle gargnitingoogle changed the title Fix dockerfile for go 1.22 Upgrade to go 1.22 Mar 5, 2024
@gargnitingoogle gargnitingoogle force-pushed the gargnitin/fix-dockerfile-go-get branch from 8685637 to e2d814e Compare March 5, 2024 11:05
@gargnitingoogle gargnitingoogle changed the title Upgrade to go 1.22 Upgrade to go 1.22 (with fix for dockerfiles) Mar 5, 2024
@gargnitingoogle gargnitingoogle added execute-perf-test Execute performance test in PR execute-integration-tests Run only integration tests labels Mar 5, 2024
@gargnitingoogle gargnitingoogle marked this pull request as ready for review March 5, 2024 11:20
@gargnitingoogle gargnitingoogle force-pushed the gargnitin/fix-dockerfile-go-get branch from e2d814e to 6952e54 Compare March 6, 2024 04:37
@gargnitingoogle
Copy link
Collaborator Author

Link to presubmit tests passing log: logs

@gargnitingoogle gargnitingoogle removed execute-perf-test Execute performance test in PR execute-integration-tests Run only integration tests labels Mar 6, 2024
@gargnitingoogle gargnitingoogle force-pushed the gargnitin/fix-dockerfile-go-get branch from 6952e54 to 1c51816 Compare March 7, 2024 04:19
@gargnitingoogle gargnitingoogle marked this pull request as draft March 7, 2024 04:20
@gargnitingoogle
Copy link
Collaborator Author

Marking as draft until I try out the switch to git clone from go get in the dockerfiles.

@gargnitingoogle gargnitingoogle marked this pull request as ready for review March 7, 2024 14:37
@gargnitingoogle gargnitingoogle added execute-perf-test Execute performance test in PR execute-integration-tests Run only integration tests labels Mar 7, 2024
@gargnitingoogle gargnitingoogle force-pushed the gargnitin/fix-dockerfile-go-get branch from f3af723 to d45d298 Compare March 8, 2024 10:12
@gargnitingoogle gargnitingoogle assigned raj-prince and unassigned sethiay Mar 8, 2024
Copy link
Collaborator

@raj-prince raj-prince left a comment

Choose a reason for hiding this comment

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

Changes look good to me, other than one confirmation comment.

Other than that, as we discussed offline.

  • We should sanity it by running it with louhi test-flow.
  • Also, should check the impact of these changes with GKE team.

@gargnitingoogle
Copy link
Collaborator Author

  • We should sanity it by running it with louhi test-flow.

Ran it once and build went through. I have given it another run after adding the --depth 1 flag, to reduce cloning time.

  • Also, should check the impact of these changes with GKE team.

Will check with @songjiaxun .

Reapply "Upgrade to golang 1.22 (#1753)" (#1766)

This reverts commit a63a46f and fixes
merge conflicts with golang 1.21.7 upgrade which went in earlier.
Does the following
1. Create a go module (go.mod) in a directory package_gcsfuse,
   to bypass the change in go get behaviour introduced in 1.17
   and enforced in go 1.22.
2. Fix some paths and commands etc. accordingly in the dockerfile.
Does the following
1. Create a go module (go.mod) in a directory package_gcsfuse,
   to bypass the change in go get behaviour introduced in 1.17
   and enforced in go 1.22.
2. Fix some paths and commands etc. accordingly in the dockerfile.
Fetch only the last commit state in docker files
during git clone to reduce its runtime.
This reduces the runtime of git clone dramatically.
@gargnitingoogle gargnitingoogle changed the title Upgrade to go 1.22 (with fix for dockerfiles) Upgrade to go 1.22.1 (with fix for dockerfiles) Mar 12, 2024
@gargnitingoogle
Copy link
Collaborator Author

gargnitingoogle commented Mar 12, 2024

@raj-prince

  • We should sanity it by running it with louhi test-flow.

Ran it once and build went through. I have given it another run after adding the --depth 1 flag, to reduce cloning time.

I checked Louhi test-flow with --depth 1 and that worked too.

  • Also, should check the impact of these changes with GKE team.

Will check with @songjiaxun .

@songjiaxun shared over chat that "GKE Louhi pipeline and the testgrid uses tools/package_gcsfuse_docker/Dockerfile to build gcsfuse binary, so as long as the Dockerfile works, out pipeline should be fine.". So I think we're good there.

Also, I upgraded this PR to 1.22.1 (from 1.22) as @songjiaxun requested.

@gargnitingoogle
Copy link
Collaborator Author

@raj-prince please approve this. This has passed the integration and perf tests, and louhi-test workflow.

@gargnitingoogle gargnitingoogle merged commit ef48953 into master Mar 12, 2024
8 checks passed
@gargnitingoogle gargnitingoogle deleted the gargnitin/fix-dockerfile-go-get branch March 12, 2024 10:51
Tulsishah pushed a commit that referenced this pull request Mar 15, 2024
* Upgrade to golang 1.22 (without dockerfile fix)

Reapply "Upgrade to golang 1.22 (#1753)" (#1766)

This reverts commit a63a46f and fixes
merge conflicts with golang 1.21.7 upgrade which went in earlier.

* Fix containerize_gcsfuse/dockerfile for go 1.22

Does the following
1. Create a go module (go.mod) in a directory package_gcsfuse,
   to bypass the change in go get behaviour introduced in 1.17
   and enforced in go 1.22.
2. Fix some paths and commands etc. accordingly in the dockerfile.

* Fix package_gcsfuse/dockerfile for go 1.22

Does the following
1. Create a go module (go.mod) in a directory package_gcsfuse,
   to bypass the change in go get behaviour introduced in 1.17
   and enforced in go 1.22.
2. Fix some paths and commands etc. accordingly in the dockerfile.

* Switch containerize*/Dockerfile to use git clone

* Switch package*/Dockerfile to use git clone

* Replace /go with ${GOPATH}

* Minimize run of git clone

Fetch only the last commit state in docker files
during git clone to reduce its runtime.
This reduces the runtime of git clone dramatically.

* Upgrade from go 1.22.0 to 1.22.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execute-integration-tests Run only integration tests execute-perf-test Execute performance test in PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants