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

Bump up Golang version #85

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

vharsh
Copy link

@vharsh vharsh commented Mar 7, 2022

What type of PR is this?
/kind cleanup
#SODACODE2022

What this PR does / why we need it:

  • Bumps up Golang version

Which issue(s) this PR fixes:

Fixes #83

Test Report Added?:

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind TESTED

/kind NOT-TESTED

Special notes for your reviewer:

Breaking changes

  1. Post Golang 1.16, calls to os.Exit(0) will now fail the unit tests.
  2. [WIP] Some Metrics tests are failing for Ceph, I'm taking a look at them.

Harsh Vardhan added 2 commits March 6, 2022 22:42
Signed-off-by: Harsh Vardhan <[email protected]>
Signed-off-by: Harsh Vardhan <[email protected]>
@skdwriting
Copy link
Contributor

skdwriting commented Mar 15, 2022

Could you please provide the testing done after the version upgrade?
Build and test updates can be provided. I see the CI failed with the upgrade!

Fix: Prior Go 1.16, an os.Exit(0) was not considered as a Test failure.
Ref: https://tip.golang.org/doc/go1.16#go-test
@skdwriting
Copy link
Contributor

Please mark the PR for #SODACODE2022 in the description

@asifdxtreme
Copy link
Member

@vharsh Can you please look at the failing CI jobs

@vharsh
Copy link
Author

vharsh commented Mar 17, 2022

@vharsh Can you please look at the failing CI jobs

I'm on it, it has broken from Golang 1.14 and it's lacking some metric Component name from the 15th metric, I'm trying to understand how the tests were passing all this while.

@skdwriting
Copy link
Contributor

@vharsh thanks...

@vharsh
Copy link
Author

vharsh commented Apr 19, 2022

I did some debugging again this long weekend, sharing all what I've found.

What factors are causing the Ceph metrics to fail?

  • The upgraded Golang(from 1.14) version has some changes to json.Unmarshall(....), which is not allowing stringified values like "osd.0"(which clearly isn't a floating point Number) into the json.Number type (which is an alias for the string type) into the Name field( from metrics_cli.go -> cephOSDDF.OSDNodes[*].Name ), which should likely have been a string, a JSON unmarshall operation is failing.
  • I've verified the expected values changes across go 1.13.x & v1.18.x, there have been some change in behaviour and this cannot be fixed by just fixing the expectedValues in the unit-test.

Some errors only visible in the newer Golang v1.17 or v1.18

E0419 12:02:09.894663    8531 metrics_cli.go:52] file ReadDefaultConfigFile can't readrados: No such file or directory
E0419 12:02:09.895347    8531 metrics_cli.go:461] unmarshal failed

Things I need to look further:

  • Go-Ceph bindings have are pinned to a very old revision from 2017, some changes might be required to change the expected internal types to string.
  • Should I bump up the Go-Ceph bindings to a release post 2020, so that dock users can get newer Ceph versions and likely the more supported & stable versions?

@asifdxtreme
Copy link
Member

@vharsh Thanks for debugging this, feel free to bump the go-ceph to newer versions.

@sushanthakumar
Copy link
Contributor

@vharsh Can you please look at @asifdxtreme suggestion and check further, thanks

@vharsh
Copy link
Author

vharsh commented May 5, 2022

@sushanthakumar Yeah, I was investigating this last weekend, I'll take a look at it soon-ish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade the project to use Go 1.17+
4 participants