feat: reduce number of containers #243
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kubevirt-bot
added
dco-signoff: yes
openshift-ci
bot
added
the
approved
0xFelix
reviewed
Apr 24, 2023
build/Dockerfile.DiskVirt
Outdated
| USER_NAME=${TASK_NAME} \ | ||
| HOME=/home/${TASK_NAME} | ||
|
|
||
| USER 0 |
There was a problem hiding this comment.
Yes, the libguestfs does not allow to do user modifications without switching to root. Then on line 34 it is switched back to regular user
build/Dockerfile.DiskVirt
Outdated
| CGO_ENABLED=0 GOOS=linux go build -o /out/${TASK_NAME} cmd/${TASK_NAME}/main.go; \ | ||
| done | ||
|
|
||
| FROM quay.io/kubevirt/libguestfs-tools:v0.59.0 |
There was a problem hiding this comment.
Make the kubevirt version a variable?
ksimon1
force-pushed
the
reduce-containers
branch
from
3e1e8f8
to
1887193
Compare
ksimon1
added a commit
to ksimon1/release
that referenced
this pull request
Apr 24, 2023
project kubevirt tekton tasks project will use only 2 containers - kubevirt/kubevirt-tekton-tasks#243. This commit adjusts settings for openshift ci Signed-off-by: Karel Šimon <ksimon@redhat.com>
openshift-ci bot
pushed a commit
to openshift/release
that referenced
this pull request
Apr 24, 2023
…38645) project kubevirt tekton tasks project will use only 2 containers - kubevirt/kubevirt-tekton-tasks#243. This commit adjusts settings for openshift ci Signed-off-by: Karel Šimon <ksimon@redhat.com>
|
/retest |
1 similar comment
|
/retest |
ksimon1
force-pushed
the
reduce-containers
branch
5 times, most recently
from
43bcf85
to
cb8f09e
Compare
|
/hold WIP |
kubevirt-bot
added
the
do-not-merge/hold
ksimon1
force-pushed
the
reduce-containers
branch
3 times, most recently
from
a9d8974
to
aa31216
Compare
|
/hold cancel |
kubevirt-bot
removed
the
do-not-merge/hold
0xFelix
reviewed
Apr 28, 2023
| @@ -191,7 +193,7 @@ spec: | |||
| description: The namespace of a VM that was created. | |||
| steps: | |||
| - name: createvm | |||
| image: "quay.io/kubevirt/tekton-task-create-vm:v0.14.0" | |||
| image: "quay.io/kubevirt/tekton-tasks:v0.14.0" | |||
| command: | |||
| - create-vm | |||
There was a problem hiding this comment.
Some tasks do not need a user modifications.
Another problem is, that the entrypoint is doing some nasty things with quotation marks of parameters and e.g. modify-template task has problem with it.
| command: | ||
| - entrypoint | ||
| args: | ||
| - '--verbose' | ||
| - $(params.verbose) | ||
| env: | ||
| - name: COMMAND |
There was a problem hiding this comment.
Do we really need the COMMAND env or could the command to be executed be the first arg in a step?
There was a problem hiding this comment.
I've tried both approaches, but I like this one better because I don't know if the arguments are guaranteed in the order. Through the env variable, it is always certain that the command will be there.
ksimon1
force-pushed
the
reduce-containers
branch
from
aa31216
to
0d9d84a
Compare
feat: reduce number of dockerfiles this commit reduces number of builded containers to just 2. One container for disk-virt tasks and second for everything else. The disk-virt containers requires separate container, because they need different base image (kubevirt libguestfs which is distroless and we can't install there anything). Other tasks share the same container, because they don't need anything special. This change will help to decrease maintenance effort, because we will not have to take care about 9 containers, but only about two. This commit also reduce number of dockerfiles to just two (one for disk-virt tasks, second for everything else). This will help to reduce time needed to e.g. bump golang version, or bump base images versions. It will also reduce complexity of build scripts and it will reduce build time. New containers will have names: kubevirt/tekton-tasks kubevirt/tekton-tasks-disk-virt kubevirt/tekton-tasks container contains :copy-template, create-vm, execute-in-vm, generate-ssh-keys, modify-data-object, modify-vm-template and wait-for-vmi-status tasks. kubevirt/tekton-tasks-disk-virt container contains: disk-virt-customize and disk-virt-sysprep Signed-off-by: Karel Šimon <ksimon@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0xFelix, ksimon1 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 |
ascerra
pushed a commit
to ascerra/release
that referenced
this pull request
May 8, 2023
…penshift#38645) project kubevirt tekton tasks project will use only 2 containers - kubevirt/kubevirt-tekton-tasks#243. This commit adjusts settings for openshift ci Signed-off-by: Karel Šimon <ksimon@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
feat: reduce number of containers
feat: reduce number of dockerfiles
this commit reduces number of builded containers to just 2. One container for disk-virt tasks and second for everything else. The disk-virt containers requires separate container, because they need different base image (kubevirt libguestfs which is distroless and we can't install there anything). Other tasks share the same container, because they don't need anything special.
This change will help to decrease maintenance effort, because we will not have to take care about 9 containers, but only about two. This commit also reduce number of dockerfiles to just two (one for disk-virt tasks, second for everything else). This will help to reduce time needed to e.g. bump golang version, or bump base images versions. It will also reduce complexity of build scripts and it will reduce build time.
New containers will have names:
kubevirt/tekton-tasks
kubevirt/tekton-tasks-disk-virt
kubevirt/tekton-tasks container contains :copy-template, create-vm, execute-in-vm, generate-ssh-keys, modify-data-object, modify-vm-template and wait-for-vmi-status tasks.
kubevirt/tekton-tasks-disk-virt container contains: disk-virt-customize and disk-virt-sysprep
Release note: