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

[Chore] Fix test cases querying OpenShift user workload monitoring stack. #2984

Merged
merged 1 commit into from
May 23, 2024

Conversation

IshwarKanse
Copy link
Contributor

@IshwarKanse IshwarKanse commented May 23, 2024

Description:

In OpenShift 4.16 tokens for service account are not generated by default and we have to now use the TokenRequest API to create token. The PR fixes the check_metrics.sh script to create a token from prometheus-user-workload SA.

Testing:
Tested on OpenShift 4.15 and 4.16:

--- PASS: chainsaw (173.73s)
    --- PASS: chainsaw/monitoring (96.42s)
    --- PASS: chainsaw/otlp-metrics-traces (77.32s)
PASS
Tests Summary...
- Passed  tests 2
- Failed  tests 0
- Skipped tests 0
Done.

@IshwarKanse IshwarKanse requested a review from a team as a code owner May 23, 2024 05:58
@IshwarKanse IshwarKanse changed the title [Chore] Fix test cases querying OpenShift user workload monitoring stack. [Chore] Fix test cases querying OpenShift user workload monitoring stack and make it easier to build container images on non-amd64 systems. May 23, 2024
@swiatekm
Copy link
Contributor

Does running docker build -t ${IMG} . really not work on an ARM Mac? What's the error message?

@IshwarKanse
Copy link
Contributor Author

IshwarKanse commented May 23, 2024

@swiatekm-sumo It works if we are building for native platform like ARM (With Mac M2 ARM) or system with amd64 but if we want to build for non-native arch, then we need to use the flags in the PR.

With the flags added, we can build for any arch, for example from Mac ARM:

GOOS=linux GOARCH=amd64 ARCH=amd64 BUNDLE_IMG=docker.io/${USER}/opentelemetry-operator-bundle:latest IMG=docker.io/${USER}/opentelemetry-operator:latest make container container-push bundle-build bundle-push

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

I don't like the Makefile change, and I'd like to discuss it further. The test changes are fine, and if you submit them in a separate PR, I'll approve them.

Makefile Outdated
@@ -293,7 +293,7 @@ scorecard-tests: operator-sdk
.PHONY: container
container: GOOS = linux
container: manager
docker build -t ${IMG} .
docker buildx build --load --platform linux/${ARCH} -t ${IMG} .
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to require buildx, we should document it in CONTRIBUTING.md. As is, you're changing the requirements for all contributors.

Separately, if we're going to make platforms configurable via an env variable, it should just be ${PLATFORM} and not ARCH which is confusingly not consistent with GOARCH.

Finally, I'd like to understand why the original command doesn't work on ARM Macs. From what I can see in Docker's documentation, buildkit should work fine in that environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@swiatekm-sumo Docker buildx uses Buildkit. BuildKit is the default builder for users on Docker Desktop, and Docker Engine as of version 23.0. From the Docker CLI, I see docker build is just an alias for docker buildx build.

docker build --help
Start a build

Usage: docker buildx build [OPTIONS] PATH | URL | -

So it shouldn't matter if we specify docker build or docker buildx build.

To build amd64 images on MAC ARM, we need to run the following commands.

$ export DOCKER_DEFAULT_PLATFORM=linux/amd64

$ TARGETALLOCATOR_IMG=quay.io/rhn_support_ikanse/ta:latest GOOS=linux GOARCH=amd64 ARCH=amd64 make container-target-allocator

CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o cmd/otel-allocator/bin/targetallocator_amd64 -ldflags "-s -w" ./cmd/otel-allocator
docker build -t quay.io/rhn_support_ikanse/ta:latest cmd/otel-allocator
[+] Building 12.2s (10/10) FINISHED                                                                    docker-container:default
 => [internal] booting buildkit                                                                                            8.7s
 => => pulling image moby/buildkit:buildx-stable-1                                                                         8.3s
 => => creating container buildx_buildkit_default                                                                          0.4s
 => [internal] load build definition from Dockerfile                                                                       0.0s
 => => transferring dockerfile: 457B                                                                                       0.0s
 => [internal] load metadata for docker.io/library/alpine:3.19                                                             2.0s
 => [internal] load .dockerignore                                                                                          0.0s
 => => transferring context: 2B                                                                                            0.0s
 => [certificates 1/2] FROM docker.io/library/alpine:3.19@sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761  0.0s
 => => resolve docker.io/library/alpine:3.19@sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761f8e1ad6b       0.0s
 => [stage-1 1/3] WORKDIR /root/                                                                                           0.0s
 => [internal] load build context                                                                                          1.3s
 => => transferring context: 94.50MB                                                                                       1.3s
 => CACHED [certificates 2/2] RUN apk --no-cache add ca-certificates                                                       0.0s
 => CACHED [stage-1 2/3] COPY --from=certificates /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt    0.0s
 => CACHED [stage-1 3/3] COPY bin/targetallocator_amd64 ./main                                                             0.0s
WARNING: No output specified with docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load

$ TARGETALLOCATOR_IMG=quay.io/rhn_support_ikanse/ta:latest GOOS=linux GOARCH=amd64 ARCH=amd64 make container-target-allocator-push 
docker push quay.io/rhn_support_ikanse/ta:latest
Error response from daemon: failed to find image quay.io/rhn_support_ikanse/ta:latest: quay.io/rhn_support_ikanse/ta:latest: image not known
make: *** [container-target-allocator-push] Error 1

Since Mac and Windows use Docker machine which uses a linux VM to run the Docker daemon, we need to load the build images in Docker instance by specifying --load from the docker build command as seen from the above warning or set a default to load for the build driver. https://docs.docker.com/build/drivers/#load-by-default Also since we are not specifying --platform in the Docker build command, we need to set export DOCKER_DEFAULT_PLATFORM=linux/amd64 in the env vars.

I'll open a new PR for the Makefile changes and the documentation in CONTRIBUTING.md as we need to get this fix for the tests merged. We can discuss on how to make it more intuitive to contributors to build for multiple platforms.

@IshwarKanse IshwarKanse changed the title [Chore] Fix test cases querying OpenShift user workload monitoring stack and make it easier to build container images on non-amd64 systems. [Chore] Fix test cases querying OpenShift user workload monitoring stack. May 23, 2024
@jaronoff97 jaronoff97 merged commit 59a7c25 into open-telemetry:main May 23, 2024
33 checks passed
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants