-
Notifications
You must be signed in to change notification settings - Fork 359
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
Disable cgo for all Antrea binaries #5988
Disable cgo for all Antrea binaries #5988
Conversation
We can revert some commits which had to be introduced to fix the UBI build specifically because cgo was enabled |
Instead of selectively disabling cgo for some binaries (e.g., release assets), we now unconditionally disable cgo for all binaries, even those that only run inside the container image for which they were built (e.g., antrea-controller). After some analysis, there seems to be no downside in doing this. We also get some benefits such as reduced build time for the default make command. Fixes antrea-io#5724 Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
This reverts commit 2f8441b. Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
This reverts commit 2afab06. Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
c03439d
to
81b2320
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disabling cgo for Flow-Aggregator looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, one question.
@@ -12,30 +12,9 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
ARG BUILD_TAG | |||
FROM registry.access.redhat.com/ubi8 as antrea-build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the FYI. I can rebase the PR later.
BTW I think this change is to revert #5723
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luolanzone I think it's because it no longer relies on glibc shipped with ubi8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. This change just reverts an earlier commit that I had to be introduced because of a glibc mismatch (I preserved the individual "revert" commits in the history for this PR, so you can take a look). Now that we disable cgo, the Antrea binaries no longer have a dependency on libc, and if there is a mismatch in version between the libc included in the golang Docker image and the libc included in the ubi Docker image, it doesn't matter anymore. Therefore we can revert to the golang Docker image, to avoid having to install Go manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also disable cgo PHONY "bin" in Makefile
? I am asking this because we have another issue #5967 for VM agent case, which may build antrea-agent for VM scenario inside a Docker container runtime using Ubuntu 22.04, but run it on a VM on a different Linux OS version (e.g. ubuntu 18.04), then agent may fail because of the missing libraries.
-coverprofile=.coverage/coverage-unit.txt -covermode=atomic \ | ||
antrea.io/antrea/cmd/... antrea.io/antrea/pkg/... antrea.io/antrea/multicluster/cmd/... antrea.io/antrea/multicluster/controllers/... | ||
|
||
.PHONY: .windows-test-unit | ||
.windows-test-unit: .coverage | ||
@echo | ||
@echo "==> Running unit tests <==" | ||
$(GO) test -race -coverpkg=antrea.io/antrea/cmd/...,antrea.io/antrea/pkg/... \ | ||
CGO_ENABLED=1 $(GO) test -race -coverpkg=antrea.io/antrea/cmd/...,antrea.io/antrea/pkg/... \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used to see build errors with enabling cgo on Windows, does this change work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cgo is required to run the race detector:
On macOS, the race detector has been rewritten not to use cgo: race-detector-enabled programs can be built and run without Xcode. On Linux and other Unix systems, and on Windows, a host C toolchain is required to use the race detector.
This is the only place where we enable it. By default, cgo is now disabled for every build operation.
/test-vm-e2e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
-coverprofile=.coverage/coverage-unit.txt -covermode=atomic \ | ||
antrea.io/antrea/cmd/... antrea.io/antrea/pkg/... antrea.io/antrea/multicluster/cmd/... antrea.io/antrea/multicluster/controllers/... | ||
|
||
.PHONY: .windows-test-unit | ||
.windows-test-unit: .coverage | ||
@echo | ||
@echo "==> Running unit tests <==" | ||
$(GO) test -race -coverpkg=antrea.io/antrea/cmd/...,antrea.io/antrea/pkg/... \ | ||
CGO_ENABLED=1 $(GO) test -race -coverpkg=antrea.io/antrea/cmd/...,antrea.io/antrea/pkg/... \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -12,30 +12,9 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
ARG BUILD_TAG | |||
FROM registry.access.redhat.com/ubi8 as antrea-build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luolanzone I think it's because it no longer relies on glibc shipped with ubi8.
Cgo is disabled for everything now (except when enabling the race detector for unit tests). |
/test-vm-e2e |
/test-all |
One test case is currently failing for |
@wenyingd does this need to be backported to older releases? |
Sorry for missing the ask, yes, we need to back port the change. cc @luolanzone |
* Disable cgo for all Antrea binaries Instead of selectively disabling cgo for some binaries (e.g., release assets), we now unconditionally disable cgo for all binaries, even those that only run inside the container image for which they were built (e.g., antrea-controller). After some analysis, there seems to be no downside in doing this. We also get some benefits such as reduced build time for the default make command. Fixes antrea-io#5724 * Revert "Add git to antrea-build image for UBI build (antrea-io#5727)" This reverts commit 2f8441b. * Revert "Fix antrea-ubi image build (antrea-io#5723)" This reverts commit 2afab06. --------- Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
I have backported this to v1.15, I suppose v1.14 is also needed? @antoninbas @wenyingd |
Focusing on VM Agent case, it is supposed to be required after we bump up the base Ubuntu to 22.04 in the antrea images. So we shall back port the change to all the versions which are still in the maintenance cycle. |
Because we are now disabling cgo when building all Antrea binaries, there is no need to install Go manually when building the UBI images. It was required before because of incompatibility between the glibc versions in the golang image and the ubi8 base image. We can now use the standard golang image to compile the Antrea binaries (without cgo, there is no dependency on glibc), then copy these binaries to the ubi8 image. PR antrea-io#5988, which disabled cgo, removed the unnecessary code from Dockerfile.build.ubi, but not from the other Dockerfiles (Dockerfile.build.agent.ubi and Dockerfile.build.controller.ubi). Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
* Disable cgo for all Antrea binaries Instead of selectively disabling cgo for some binaries (e.g., release assets), we now unconditionally disable cgo for all binaries, even those that only run inside the container image for which they were built (e.g., antrea-controller). After some analysis, there seems to be no downside in doing this. We also get some benefits such as reduced build time for the default make command. Fixes #5724 * Revert "Add git to antrea-build image for UBI build (#5727)" This reverts commit 2f8441b. * Revert "Fix antrea-ubi image build (#5723)" This reverts commit 2afab06. --------- Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Because we are now disabling cgo when building all Antrea binaries, there is no need to install Go manually when building the UBI images. It was required before because of incompatibility between the glibc versions in the golang image and the ubi8 base image. We can now use the standard golang image to compile the Antrea binaries (without cgo, there is no dependency on glibc), then copy these binaries to the ubi8 image. PR #5988, which disabled cgo, removed the unnecessary code from Dockerfile.build.ubi, but not from the other Dockerfiles (Dockerfile.build.agent.ubi and Dockerfile.build.controller.ubi). Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
* Disable cgo for all Antrea binaries Instead of selectively disabling cgo for some binaries (e.g., release assets), we now unconditionally disable cgo for all binaries, even those that only run inside the container image for which they were built (e.g., antrea-controller). After some analysis, there seems to be no downside in doing this. We also get some benefits such as reduced build time for the default make command. Fixes antrea-io#5724 * Revert "Add git to antrea-build image for UBI build (antrea-io#5727)" This reverts commit 2f8441b. * Revert "Fix antrea-ubi image build (antrea-io#5723)" This reverts commit 2afab06. --------- Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
* Disable cgo for all Antrea binaries Instead of selectively disabling cgo for some binaries (e.g., release assets), we now unconditionally disable cgo for all binaries, even those that only run inside the container image for which they were built (e.g., antrea-controller). After some analysis, there seems to be no downside in doing this. We also get some benefits such as reduced build time for the default make command. Fixes #5724 * Revert "Add git to antrea-build image for UBI build (#5727)" This reverts commit 2f8441b. * Revert "Fix antrea-ubi image build (#5723)" This reverts commit 2afab06. --------- Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
* Disable cgo for all Antrea binaries Instead of selectively disabling cgo for some binaries (e.g., release assets), we now unconditionally disable cgo for all binaries, even those that only run inside the container image for which they were built (e.g., antrea-controller). After some analysis, there seems to be no downside in doing this. We also get some benefits such as reduced build time for the default make command. Fixes antrea-io#5724 * Revert "Add git to antrea-build image for UBI build (antrea-io#5727)" This reverts commit 2f8441b. * Revert "Fix antrea-ubi image build (antrea-io#5723)" This reverts commit 2afab06. --------- Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
* Disable cgo for all Antrea binaries Instead of selectively disabling cgo for some binaries (e.g., release assets), we now unconditionally disable cgo for all binaries, even those that only run inside the container image for which they were built (e.g., antrea-controller). After some analysis, there seems to be no downside in doing this. We also get some benefits such as reduced build time for the default make command. Fixes #5724 * Revert "Add git to antrea-build image for UBI build (#5727)" This reverts commit 2f8441b. * Revert "Fix antrea-ubi image build (#5723)" This reverts commit 2afab06. --------- Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Instead of selectively disabling cgo for some binaries (e.g., release
assets), we now unconditionally disable cgo for all binaries, even those
that only run inside the container image for which they were built
(e.g., antrea-controller). After some analysis, there seems to be no
downside in doing this. We also get some benefits such as reduced build
time for the default make command.
Fixes #5724