-
Notifications
You must be signed in to change notification settings - Fork 66
fix(ws): backend dockerfile #386
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
fix(ws): backend dockerfile #386
Conversation
0ffaa72
to
a89a08e
Compare
workspaces/backend/Dockerfile
Outdated
RUN go mod edit -replace=github.com/kubeflow/notebooks/workspaces/controller=./controller | ||
|
||
# Download dependencies | ||
RUN go mod download |
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.
These back-to-back RUN
commands can be more efficiently combined to reduce a spurious layer
RUN go mod edit -replace=github.com/kubeflow/notebooks/workspaces/controller=./controller && \
go mod download
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 will change that, tnx
COPY backend/openapi/ openapi/ | ||
|
||
# Build the Go application | ||
RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -a -o backend ./cmd/main.go |
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.
Unrelated to changes in this PR - but I'm confused by the use case of specifying TARGETARCH
here...
Our Makefile
never sets this build argument... so do we even need it? Under what circumstance would it need to be defined?
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.
Hey, we need the TARGETOS and TARGETARCH for the make docker-buildx command.
When you use docker buildx build --platform=..., Docker automatically sets build-time environment variables like TARGETARCH and TARGETOS for each platform you're building for.
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.
Ok, thank you for highlighting this! TIL
💯
However, from my preliminary research - just sharing for awareness - no changes needed/required on this PR - it seems that podman
does NOT automatically set these variables... so while we have a CONTAINER_TOOL
"abstraction" in our Makefile
to "support" running with podman
- it won't actually be comparable to what docker
does (without user intervention)
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.
Thank you for the comment, I checked it too, and I saw that in podman
we don't have the buildx
command at all, so we will need to change that too (but let's leave it to another ticket for replacing podman with docker)
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.
Agreed - definitely a follow up item we can/should discuss more how to handle!
workspaces/backend/Makefile
Outdated
.PHONY: docker-buildx | ||
docker-buildx: ## Build and push docker image for the manager for cross-platform support | ||
# copy existing Dockerfile and insert --platform=${BUILDPLATFORM} into Dockerfile.cross, and preserve the original Dockerfile | ||
sed -e '1 s/\(^FROM\)/FROM --platform=\$$\{BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\$$\{BUILDPLATFORM\}/' Dockerfile > Dockerfile.cross |
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.
Again, unrelated to the changes in this PR - but I am confused as to why we have 2 separate expressions ( -e
) in this sed
command...
Our Dockerfile
has a comment on Line 1 - so (at least today) - the initial -e
command will never pass. And since we are not relying on that initial command - unclear why we even attempt to support it (?)
Clearly the 2nd expression is "robust enough" - as we are relying on it today...
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.
You are right, I will remove it.
In order to properly test this change - I wanted to get the container image running in k8s The following was my testing methodology:
All API calls tested above returned successfully with expected JSON payloads reflecting the state of my cluster! |
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.
Apologies I did not catch this on my first review - but when re-reviewing the changes today - I paid more attention to the build output and noticed this:
$ gmake docker-build docker-push IMG=quay.io/rh-ee-astonebe/kubeflow-notebooks-v2:backend-containerize
docker build -f Dockerfile -t quay.io/rh-ee-astonebe/kubeflow-notebooks-v2:backend-containerize ..
DEPRECATED: The legacy builder is deprecated and will be removed in a future release.
Install the buildx component to build images with BuildKit:
https://docs.docker.com/go/buildx/
Sending build context to Docker daemon 1.21GB
Specifically this:
Sending build context to Docker daemon 1.21GB
1.21GB being sent in context seemed... a bit much 😇
What I think is happening:
- We (rightly) changed the build context to
..
docker
will look for a.dockerignore
file at the root of the provided build context directory- we have no
.dockerignore
file defined inworkspaces/
- EVERYTHING in the developer's repo will get packaged up in build context 😱
Granted, when using CI/CD to build an image - there would not be as much "stuff" expected to be present in the repo - but on my workstation, the bin/
directories of both backend
and controller
, in addition to all the files (includind node_modules
) are getting slurped up into context.
Seems like we probably would want to define a .dockerignore
in workspaces/
to filter out all the directories we'd never expect/want to include. Definitely want to include a comment in that file (since its kinda "hanging out" in that directory with no obvious indication how it would be used otherwise)
Signed-off-by: Liav Weiss (EXT-Nokia) <[email protected]>
Signed-off-by: Liav Weiss (EXT-Nokia) <[email protected]>
/lgtm This fix work on me now, I am able to build/push the backend image successfully. Generated logs:
Without that fix I had this error:
|
@atheo89: changing LGTM is restricted to collaborators In 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 kubernetes/test-infra repository. |
3de7a76
to
f0f0a93
Compare
Signed-off-by: Liav Weiss (EXT-Nokia) <[email protected]>
f0f0a93
to
5a57dac
Compare
@andyatmiami I fixed the issue, I think frontend code is unnecessary too. |
/retest |
Confirming that the build context size is now nice and minimal on the latest changes 💯
An almost 1000x reduction is "pretty good" 😎 |
/lgtm
|
Signed-off-by: Mathew Wicks <[email protected]>
Thanks @liavweiss and @andyatmiami, now we can get the manifests for backend set up (and e2e tests). /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thesuperzapper The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/ok-to-test |
* feat(ws): Properly containerize backend component kubeflow#323 Signed-off-by: Liav Weiss (EXT-Nokia) <[email protected]> * feat(ws): Properly containerize backend component kubeflow#323 Signed-off-by: Liav Weiss (EXT-Nokia) <[email protected]> * feat(ws): Properly containerize backend component kubeflow#323 Signed-off-by: Liav Weiss (EXT-Nokia) <[email protected]> * mathew: revert typo Signed-off-by: Mathew Wicks <[email protected]> --------- Signed-off-by: Liav Weiss (EXT-Nokia) <[email protected]> Signed-off-by: Mathew Wicks <[email protected]> Co-authored-by: Liav Weiss (EXT-Nokia) <[email protected]> Co-authored-by: Mathew Wicks <[email protected]>
closes: #323
In this pr I fixed the make docker-build + make docker-buildx.
NOTE: When you run "make docker-buildx" you will get this ERROR:
"ERROR: failed to solve: failed to push nbv2-backend:latest: push access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed
make: [Makefile:135: docker-buildx] Error 1 (ignored)",
Because we didn't specify the right registry in the Makefile.