stdmalloc build of cockroach 22.1.5 #5
Merged
Conversation
This file contains 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
|
Among the changes not yet incorporated into build.sh: CockroachDB have stopped releasing built source tarballs. Instead, their releases page points to the download link for a GitHub release tarball, which I believe just comes from Here's what I did instead: (this is edited from what I actually ran but it should be close to that) My latest attempt fails thusly: I am not sure what this file is, but running it directly fails the same way: |
|
For my own reference, to reconstruct the patch files, I created a parallel directory structure in gopath: That is:
For each patch:
To reset my workspace without having to re-clone (which takes several minutes), I wound up with this: That may have missed some stuff. I'd run Then I'd do this to reset the rest of stuff: |
|
The current status is that the build fails as mentioned above. It's failing to run |
|
I'm not sure what was going on with my previous runs, but removing the above as well as I see that the caller does expect two values from this function now: but ours only returns one, the |
This was referenced Aug 17, 2022
Closed
|
On Josh's recommendation, I took the latest (vendored) pebble vfs and massaged it to work. Here's the delta: With that new file being: // Copyright 2020 The LevelDB-Go and Pebble Authors. All rights reserved. Use
// of this source code is governed by a BSD-style license that can be found in
// the LICENSE file.
// +build illumos
package vfs
import "golang.org/x/sys/unix"
func (defaultFS) GetDiskUsage(path string) (DiskUsage, error) {
stat := unix.Statvfs_t{}
if err := unix.Statvfs(path, &stat); err != nil {
return DiskUsage{}, err
}
freeBytes := uint64(stat.Bsize) * uint64(stat.Bfree)
availBytes := uint64(stat.Bsize) * uint64(stat.Bavail)
totalBytes := uint64(stat.Bsize) * uint64(stat.Blocks)
return DiskUsage{
AvailBytes: availBytes,
TotalBytes: totalBytes,
UsedBytes: totalBytes - freeBytes,
}, nil
}(same as before) it differs from disk_usage_unix.go only like this: |
|
The latest problem: It looks suspicious to me that it's trying to use a file called "CCL" in the "oss" build. I confirmed we are running the My next step is to try to build their Docker builder image (the Dockerfile for which is in build/builder), then see if I can use that to make an OSS build. |
|
Okay, I sort of successfully built the builder image, but trying to run the entrypoint interactively gave me an error message: Perhaps inadvisedly, I skipped this and ran with the previous layer. In that I ran this: That build ultimately failed with: This looks unrelated to the problem I was trying to check above. I was going to file a few bugs, but I checked the build instructions and found that |
|
Ran into another issue with the stock OSS build using their Docker builder and filed cockroachdb/cockroach#86403. |
|
That turned out to be a legit CockroachDB OSS build issue. It's fixed by cockroachdb/cockroach#86425. I verified this by cherry picking that fix onto v22.1.5. I was able to successfully build a v22.1.5 binary. I have not really tested it. A side effect of using a git clone here is that the Build Tag is actually correct: I believe the "dirty" note is because of the patch files. I was tempted to remove this, but I think it's more transparent to show that this isn't quite commit 7ec579a4. |
|
The omicron test suite passed with expected changes: dap@ivanova omicron-fixes $ git diff
diff --git a/nexus/tests/output/test-explain-output b/nexus/tests/output/test-explain-output
index 0f065162..b2e6e555 100644
--- a/nexus/tests/output/test-explain-output
+++ b/nexus/tests/output/test-explain-output
@@ -3,5 +3,5 @@ vectorized: true
• scan
missing stats
- table: test_users@primary
+ table: test_users@test_users_pkey
spans: [/'00000000-0000-0000-0000-000000000000' - /'00000000-0000-0000-0000-000000000000']
\ No newline at end of file
diff --git a/tools/cockroachdb_version b/tools/cockroachdb_version
index d44d5b1b..c8f8a62e 100644
--- a/tools/cockroachdb_version
+++ b/tools/cockroachdb_version
@@ -1 +1 @@
-v21.2.9
+v22.1.5-1-g7ec579a4-dirty |
|
The web UI is at least not totally broken:
|
|
Updating my notes above: my current workflow for testing changes is to do the following at the top level (garbage-compactor/cockroach): and the following within the clone at cache/gopath/src/github.com/cockroachdb/cockroach: One could as well remove the whole |
|
Okay, I think this is ready for review. Summary of changes here:The goals:
What came along for the ride:
I'd appreciate a sanity check on the removed and added patch files! I hope that any issues would result in build failures, but I know that's not always the case (as in the case of the libedit changes). TestingOn the latest commit (d762111), I have tested:
|
|
Note that since I last updated this, 22.1.7 has been released and contains the fix for one of the things I patched. When we're ready to move forward here, we should drop that patch and update to 22.1.7 instead. |
|
Cockroach have published a technical advisory that may affect us. We'll want to move to 22.1.9 when we're ready. |
jclulow
reviewed
Oct 21, 2022
There was a problem hiding this comment.
It might be good to do a similar check for Node 16.3.0 to what we're already doing for Go, but I don't feel that strongly about it.
One thing I've noticed is that the version stamp Build Tag field now says v22.1.5-dirty, which I assume is an artefact of our having patched things after cloning:
$ ./cockroach-v22.1.5/bin/cockroach version
Build Tag: v22.1.5-dirty
Build Time: 2022/10/21 00:18:30
Distribution: OSS
Platform: illumos amd64 (x86_64-pc-solaris2.11)
Go Version: go1.17.13
C Compiler: gcc 10.3.0
Build Commit ID: a30a663cbd9323d34d50f343dd038af64671e25f
Build Type: release
$ ./cockroach-v22.1.5/bin/cockroach version --build-tag
v22.1.5-dirty
I'm not sure to what extent this will matter.
I added something for this. I decided to check the node on your PATH rather than the installed package, since you could have the right package installed but having something else on your PATH.
Yeah, I decided to leave that because I felt it was more transparent about what we're doing. I may regret this if someone uses that in the future to disclaim responsibility. It might be cleaner to have our own RFD 211-style fork where we commit our patches. We could use the usual git/GitHub processes for managing patches and changes. Then the output here would be a (non-dirty) commit on our branch (and one could easily see what's different from the tip of the release branch). Since it sounds like you're okay with the other changes here, I think the next steps for me will be to see if I can build v22.1.9 without further work, then run that through an Omicron test suite, and then hopefully land this. |
jclulow
approved these changes
Oct 21, 2022
|
The new build has some of the same issues we were seeing before: oxidecomputer/omicron#1223 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is very much a work in progress.
I'm trying to get a stdmalloc build of the latest stable Cockroach because that's (a) useful anyway and (b) the next step in debugging the various CockroachDB crashes we've seen.
Status:
Remaining steps here:
cockroach versionworks,cockroach sqlworks (and so do the line editing basics)cockroachcan be plugged directly into a download URL. That won't be true for the fork that we've built.