-
Notifications
You must be signed in to change notification settings - Fork 82
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
Building multi-arch Images #1103
base: master
Are you sure you want to change the base?
Conversation
I will go through the changes later today. Based on a quick scan of the included files, I suggest not to include deploy/kubeplus-chart/kubeplus-components-6.yaml in the PR. We want to maintain official image tags from cloudark-kubeplus GCR repo when we publish the KubePlus Helm chart. |
Yeah, will update that part later today needed to get everything at least deployed to validate. I will move that to the values.yaml |
abead1b
to
3c10a9f
Compare
@devdattakulkarni I intend to switch the scripts to use https://docs.docker.com/engine/reference/commandline/buildx/ that of course will require a once time pre step
For now, the actions only build the latest but we will need a release process to allow us to build tagged images. I am thinking we align the chart version with the tags then we can keep the individual components versions in the |
@@ -0,0 +1,52 @@ | |||
name: Build consumerui |
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 @enyachoke
Can you point me to a good resource to come up to speed on Github workflows? I haven't studied them in great details. This link has some introductory material, but I was not able to find information about aspects of the syntax that you have used below, such as the release action ('release'), types of releases ('published', 'edited'), the various actions (actions/checkout@v3 - what does that mean), etc.
At a high level, I am able to understand what this file is doing but would like to precisely understand the meaning of each step.
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 am running this workflow on push to master to build the latest and on release to build the releases. See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#release for details on this trigger.
actions/checkout@v3
Checks out the repository in the GitHub actions env
docker/setup-qemu-action
Install QEMU to allow us to run multi-arch builds
docker/setup-buildx-action
This does the step I mentioned above
$ docker buildx create --name mybuilder
$ docker buildx use mybuilder
which is necessary for buildx
docker/login-action@v2
This logs into the registry and you will have to setup this secrets
REGISTRY=gcr.io
REGISTRY_USERNAME=cloudark-kubeplus
REGISTRY_PASSWORD=thepassword for the registry
docker/metadata-action@v4
This prepares the tags needed in the image build step. This step uses conditionals and will create for example
gcr.io/cloudark-kubeplus/consumerui:latest
when the actions are running for the default branch which in this case is master or gcr.io/cloudark-kubeplus/consumerui:0.0.1
if the action is triggered by a release. Not we extra the version in the step Set Version
docker/build-push-action@v4
Finally this step build and pushes the images.
@@ -0,0 +1,50 @@ | |||
name: Build mutating-webhook-helper |
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.
mutating-webhook-helper is not used anymore.
We can exclude it from the github workflow.
consumerui/Dockerfile
Outdated
ADD grapher.py /root/grapher.py | ||
ADD templates /root/templates | ||
ADD static /root/static | ||
ADD consumerui/requirements.txt /root/requirements.txt |
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.
Why is this change needed?
I see this type of change in all the Dockerfiles below. Why is that needed?
Won't this affect local builds which are needed during development?
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.
We are building the binaries within the Dockerfile, and the context is at the base of the repository. We could have kept the context within the service folders but I think some of the go applications reference components in the base of the project and Docker does not allow us to do something like ../
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.
What a believe we need to do is update the scripts used to build the development images and have the context set to the base of the project.
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 went through the matrix strategy docs. I am still unclear on how the local builds will happen.
The current workflow to test any new code change is as follows:
- make code changes
- use the build scripts to create the latest tag
- change the appropriate container in the kubeplus deployment yaml in deploy/kubeplus-chart/template to use the latest tag
- deploy the kubeplus chart from deploy/kubeplus-chart
The changes to the Dockerfiles will break this workflow.
deploy/Dockerfile
Outdated
RUN apt-get update && apt-get install -y openssl curl jq python3 python3-pip && pip3 install pyyaml | ||
ADD deploy/webhook-create-self-signed-ca-cert.sh / | ||
RUN arch=$(arch | sed s/aarch64/arm64/ | sed s/x86_64/amd64/) && cd /root/ && curl -LO "https://dl.k8s.io/release/v1.26.0/bin/linux/${arch}/kubectl" | ||
COPY deploy/kubeplus-non-pod-resources.yaml /root/. |
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.
Same comment as that for Dockerfile for consumerui.
RUN cp /root/kubectl bin/. && chmod +x /root/kubectl && chmod +x bin/kubectl && chmod +x /root/kubeconfiggenerator.sh && cp /root/helm bin/. && chmod +x /root/helm && chmod +x bin/helm | ||
|
||
#ENTRYPOINT ["/root/kubeconfiggenerator.sh"] | ||
|
||
#ENTRYPOINT ["/root/kubeconfiggenerator.sh"] |
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.
Extra. Can be deleted.
mutating-webhook-helper/Dockerfile
Outdated
@@ -1,5 +1,10 @@ | |||
FROM ubuntu:20.04 | |||
FROM golang:1.20.1-bullseye as builder |
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.
mutating-webhook-helper is not being used. So we can ignore this file.
kubeconfiggenerator: | ||
image: | ||
registry: gcr.io | ||
repository: cloudark-kubeplus/kubeconfiggenerator |
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.
is there an accidental newline after "cloudark-" ?
deploy/kubeconfiggenerator.py
Outdated
@@ -493,6 +493,8 @@ def flatten(yaml_contents, flattened, types_dict, prefix=''): | |||
inner_prop_dict = {} | |||
prop_dict = {'properties': inner_prop_dict} | |||
prop_dict['type'] = 'object' | |||
#Allow | |||
prop_dict['additionalProperties'] = True |
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.
Is this change for the requirement that you had identified regarding supporting empty annotations?
If so, it will be better to separate it in a separate PR. That way it will be easier to merge it while we are still going through the build related changes.
e830704
to
13001c2
Compare
@enyachoke Hello. I hope things are going well for you. |
RUN curl -LO "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/amd64/kubectl" && sudo install -o root -g root -m 0755 kubectl /usr/local/bin/kubectl | ||
|
||
RUN cp /usr/bin/python3.8 /usr/bin/python | ||
RUN arch=$(arch | sed s/aarch64/arm64/ | sed s/x86_64/amd64/) && \ |
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.
What is output of arch on a arm machine? Why do we need two sed commands?
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.
On Linux the arch command will return aarch64 for ARM and x86_64 for AMD64 machine. Most ackage are published as either arm64 or amd64 so we need to run sed to translate.
Why do we need two sed commands?
Not the or (|)
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 think you have added this file just as a way to know how the rendered values.yaml will look. Correct?
@enyachoke Thanks for updating the PR. I went through it and have added couple of questions/comments. Right now the CI is failing. Need to look into it. |
I see some tests are failing. I am not sure how I could fix those |
Hmm.. I want to rule out any issues with the tests themselves. Sometimes that happens. The best way to know that is to push another change and see if the tests fail or succeed. I will be working on one of the issues in next couple days. Once I submit a PR for that, we can see if the tests are failing for that PR too. |
This PR makes changes to the dockerfiles to compile the source code in a multi step build. This then opens up the possibility of building images for multiple architectures. I need this change since I am currently evaluating this in a K3s cluster running on arm64 nodes.