NO-JIRA: hack/build.sh: parallelize the build if MAKEFLAGS is set#10364
NO-JIRA: hack/build.sh: parallelize the build if MAKEFLAGS is set#10364jhixson74 wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
@jhixson74: This pull request explicitly references no jira issue. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
b58ef2e to
190b870
Compare
|
/cc @patrickdillon |
cc295b9 to
0e8c398
Compare
hack/build.sh
Outdated
|
|
||
| # shellcheck disable=SC2086 | ||
| go build ${GOFLAGS} -gcflags "${GCFLAGS}" -ldflags "${LDFLAGS}" -tags "${TAGS}" -o "${OUTPUT}" ./cmd/openshift-install | ||
| go build ${GOBUILDJOBS} ${GOFLAGS} -gcflags "${GCFLAGS}" -ldflags "${LDFLAGS}" -tags "${TAGS}" -o "${OUTPUT}" ./cmd/openshift-install |
There was a problem hiding this comment.
💡I notice we currently have the GOFLAGS. Should we use that instead of defining a new one? For example, we run:
GOFLAGS='-p=16 --mod=vendor' ./hack/build.sh
With this, I guess we just need to adjust cluster-api/Makefile so that go build considers GOFLAGS and hack/build.sh to pass down the env var?
There was a problem hiding this comment.
This breaks when go generate runs. That's why I did it this way.
There was a problem hiding this comment.
ahh whoops. Just curious, may I know how or why it failed (i.e. error message)?
There was a problem hiding this comment.
I guess I was asking because I haven't seen go generate failed in my local setup. Maybe, I am missing something 😓 Also, latest change might have broken the build in ci/prow/e2e-aws-ovn and ci/prow/images:
+ make '' -C cluster-api all
make: *** empty string invalid as file name. Stop.
There was a problem hiding this comment.
Bleh. Yeah, the shell linter wants variables quoted. But for cases like this, you intentionally don't quote them ;-) I will update.
There was a problem hiding this comment.
As for go generate, it honors GOFLAGS. -p is not a valid option to go generate
Ah right, sorry GOFLAGS is a built-in Golang env var, which applies to all go commands🤦 I have one more suggestion if it makes it easier:
We can define a variable GOBUILDFLAGS for specificially for building and pass it directly to go build:
go build ${GOBUILDFLAGS} ${GOFLAGS} -gcflags "${GCFLAGS}" -ldflags "${LDFLAGS}" -tags "${TAGS}" -o "${OUTPUT}" ./cmd/openshift-install
Then we can run build with any supported go flags instead of parsing the number out:
GOBUILDFLAGS="-p $(nproc)" ./hack/build.sh
There was a problem hiding this comment.
While that is a nice feature to have, it's outside of the scope here. This particular PR passes along the number of jobs to both make, and go build. For this to work, we now need to set GOBUILDFLAGS and MAKEFLAGS in the environment to keep both consistent. For now, I think this works fine as is.
There was a problem hiding this comment.
I met you half way. I do like the name GOBUILDFLAGS better. I also modified it here so that can be set in the environment. If MAKEFLAGS is set with -j, it will automagically set GOBUILDFLAGS.
There was a problem hiding this comment.
Ahh right, we need it for both make and go. The changes look good to me, thanks!
There was a problem hiding this comment.
Sorry for the long thread :(
Could you help me understand the reason to also set -p for go build? I thought we just need to allocate >= 1 make job slots via -j <n>, right? On main, we can already do MAKEFLAGS="-j 16" ./hack/build.sh, which does parallelize the capi provider build.
For go, go help build shows 👇, which is, in most cases, already building packages in parallel?
-p n
the number of programs, such as build commands or
test binaries, that can be run in parallel.
The default is GOMAXPROCS, normally the number of CPUs available.
b266623 to
1ec7cb3
Compare
hack/build.sh
Outdated
| release) | ||
| LDFLAGS="${LDFLAGS} -s -w" | ||
| TAGS="${TAGS} release" | ||
| TAGS="${TAGS} release" |
There was a problem hiding this comment.
There is a small empty character after this line (i.e. reported as ␦):
$ cat hack/build.sh | grep -A3 -- 'release)'
release)
LDFLAGS="${LDFLAGS} -s -w"
TAGS="${TAGS} release"␦
;;
Do you see it? Maybe we should remove it...
There was a problem hiding this comment.
Blah. That explains why there is a difference showing. I'm not sure what I did here, but I'll nuke it.
There was a problem hiding this comment.
Nice, I thought i was seeing things :D
hack/build.sh
Outdated
|
|
||
| # shellcheck disable=SC2086 | ||
| go build ${GOFLAGS} -gcflags "${GCFLAGS}" -ldflags "${LDFLAGS}" -tags "${TAGS}" -o "${OUTPUT}" ./cmd/openshift-install | ||
| go build ${GOBUILDJOBS} ${GOFLAGS} -gcflags "${GCFLAGS}" -ldflags "${LDFLAGS}" -tags "${TAGS}" -o "${OUTPUT}" ./cmd/openshift-install |
There was a problem hiding this comment.
As for go generate, it honors GOFLAGS. -p is not a valid option to go generate
Ah right, sorry GOFLAGS is a built-in Golang env var, which applies to all go commands🤦 I have one more suggestion if it makes it easier:
We can define a variable GOBUILDFLAGS for specificially for building and pass it directly to go build:
go build ${GOBUILDFLAGS} ${GOFLAGS} -gcflags "${GCFLAGS}" -ldflags "${LDFLAGS}" -tags "${TAGS}" -o "${OUTPUT}" ./cmd/openshift-install
Then we can run build with any supported go flags instead of parsing the number out:
GOBUILDFLAGS="-p $(nproc)" ./hack/build.sh
7ad7ce8 to
17958f0
Compare
hack/build.sh
Outdated
| if [ ${MAKEJOBS} -gt 0 ]; then | ||
| GOBUILDFLAGS="${GOBUILDFLAGS} -p ${MAKEJOBS}" | ||
| fi |
There was a problem hiding this comment.
| if [ ${MAKEJOBS} -gt 0 ]; then | |
| GOBUILDFLAGS="${GOBUILDFLAGS} -p ${MAKEJOBS}" | |
| fi | |
| if [ -n "${MAKEJOBS}" ] && [ "${MAKEJOBS}" -gt 0 ]; then | |
| GOBUILDFLAGS="${GOBUILDFLAGS} -p ${MAKEJOBS}" | |
| fi |
We should check for non-empty first before comparing the integer. If not, there is actually a silent failure:
+ '[' -gt 0 ']'
./hack/build.sh: line 26: [: -gt: unary operator expected
There was a problem hiding this comment.
Updated the regex to be more bulletproof. Now we won't get a non-digit. We either get null, or the number of jobs passed to -j.
80e9320 to
c486ac7
Compare
|
/test golint |
tthvo
left a comment
There was a problem hiding this comment.
Looks good to me!
@jhixson74 May I get help with #10364 (comment) before tagging?
c486ac7 to
170159b
Compare
I made one more optimization to building the capi providers. Please review ;-) go build -p works like make -j. It will create the number of threads specified. As you specified, if not, GOMAXPROCS is used, which is the number of cores. |
|
/hold It appears go changes aren't being noticed. |
170159b to
525f827
Compare
|
/hold cancel zip exits with code 12 if there is nothing to do, so added check for that. |
|
/test golint |
tthvo
left a comment
There was a problem hiding this comment.
/lgtm
The build is pretty fast now :D I guess e2e is passing the build means it's good :D
It will create the number of threads specified. As you specified, if not, GOMAXPROCS is used, which is the number of cores.
Oh thanks, I guess it's for making it consistent between make and go, but not a hard requirement to build in parallel...
|
Do you see improvements when using this? I'm curious about the results. go build already uses all cores; the make jobs are running in serial, but each job is a go build so is parallelized across all cores (right?) when I have tested (along time ago) I didn't see a lot of spare cpu space, except with the compression/zipping part It's not really clear to me why we should link the number of make jobs to the go |
| cd providers/$*; \ | ||
| if [ -f main.go ]; then path="."; else path=./vendor/`grep _ tools.go|awk '{ print $$2 }'|sed 's|"||g'`; fi; \ | ||
| go build -gcflags $(GCFLAGS) -ldflags $(LDFLAGS) -o ../../bin/$(TARGET_OS_ARCH)/cluster-api-provider-$* "$$path"; | ||
| go build $(GOBUILDFLAGS) -gcflags $(GCFLAGS) -ldflags $(LDFLAGS) -o ../../bin/$(TARGET_OS_ARCH)/cluster-api-provider-$* "$$path"; |
There was a problem hiding this comment.
hm, outside of your changes, but I notice that we're apparently not ${GOFLAGS} in the capi builds... I wonder if CI is using those 👀
There was a problem hiding this comment.
Looking at the build logs, it's drop -mod=vendor
2026-03-06T18:25:27.160016001Z go build -gcflags "" -ldflags "-s -w" -o ../../bin/linux_amd64/cluster-api-provider-openstack "$path";
vs
26-03-06T18:27:51.399947790Z + go build -mod=vendor -gcflags '' -ldflags ' -X github.com/openshift/installer/pkg/version.Raw=v1.4.21-pre-246-g170afd6c6ccd7679f4c55d91808988eeedbfe1fb-dirty -X github.com/openshift/installer/pkg/version.Commit=170afd6c6ccd7679f4c55d91808988eeedbfe1fb -X github.com/openshift/installer/
🤔
There was a problem hiding this comment.
Seems to not be an issue 😅
-mod mode
module download mode to use: readonly, vendor, or mod.
By default, if a vendor directory is present and the go version in go.mod
is 1.14 or higher, the go command acts as if -mod=vendor were set.
But we should probably fix that in case art changes anything in the future
There was a problem hiding this comment.
Aha, #10364 (comment) makes sense. I was wondering the same thing :D
There was a problem hiding this comment.
Looking at the build logs, it's drop -mod=vendor
2026-03-06T18:25:27.160016001Z go build -gcflags "" -ldflags "-s -w" -o ../../bin/linux_amd64/cluster-api-provider-openstack "$path";vs
26-03-06T18:27:51.399947790Z + go build -mod=vendor -gcflags '' -ldflags ' -X github.com/openshift/installer/pkg/version.Raw=v1.4.21-pre-246-g170afd6c6ccd7679f4c55d91808988eeedbfe1fb-dirty -X github.com/openshift/installer/pkg/version.Commit=170afd6c6ccd7679f4c55d91808988eeedbfe1fb -X github.com/openshift/installer/🤔
I haven't done anything with GOFLAGS here. I intentionally left it alone.
For me, running $ time ./hack/build.sh
# providers are built one at a time
real 0m49.057s
user 0m55.159s
sys 0m15.100s
$ time MAKEFLAGS="-j $(nproc)" ./hack/build.sh
# providers are built concurrently
real 0m31.646s
user 1m4.316s
sys 0m15.624s
🤔 Right, I had the same question in #10364 (comment). I assumed it was syncing the make and go allowed threads. Thinking about it again, I am now unsure too :( |
We can of course enable different knobs. None of this is on by default though, and is pretty much only useful to those of us working on it. Go uses all cores by default. Make does not. We can optimize this even more if we could get rid of build*.sh files and turn them into makefiles. What if I don't want all cores used? What if go is taking up too many resources on my laptop (hint: it does... all the time, and brings it to a screeching halt). That's why I've kept this little hack around for years. I'm attempting to (once again) bring it in ;-) Would you like me to decouple the number of make jobs from go build jobs? I'm fine with that. Or should I just close this out? |
|
@jhixson74: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |

Parallelize build when setting MAKEFLAGS. Keep default behavior otherwise.
To test:
env MAKEFLAGS="-j $(nproc)" ./hack/build.sh
This will pass along the output of nproc, or if you manually specify a number. make will get -j , and go build will get -p . Nothing changes if not setting MAKEFLAGS with a -j option.
Also updated the way we create the cluster-api.zip file. Instead of deleting it and zipping all the providers everytime hack/build.sh is invoked, we just update specific providers when changed and only update that provider in cluster-api.zip.