-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: rework example pipelines #550
Conversation
metadata: | ||
name: pipeline | ||
apiVersion: rbac.authorization.k8s.io/v1 | ||
kind: ClusterRole |
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.
Do we still need to ship this role or was this fixed in Tekton/OpenShift Pipelines in the meantime?
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.
yes, we still need it - I tested it with openshift pipelines 1.10.2 (newest) and SA is still not deployed to openshift|kube namespaces
- name: virtioContainerDiskName | ||
description: Reference to the containerdisk containing the virtio-win drivers ISO. | ||
type: string | ||
default: quay.io/kubevirt/virtio-container-disk:latest |
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.
The latest
tag hasn't been updated in two years. This could be an issue.
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.
It can't because the value is replaced by https://github.com/kubevirt/ssp-operator/blob/master/internal/operands/tekton-pipelines/tekton-pipelines.go#L161
taskRef: | ||
kind: Task | ||
name: wait-for-vmi-status | ||
- name: create-base-dv |
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.
Isn't the base-dv enough, why do we need a separate root disk that is cloned after the install? Can't the base-dv be used as root disk directly?
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.
All gets complicated, because of the disk name and multiple runs use case. In templates, the disk name was derived from VM name and vm name was generated with some random hash. So you could run the pipeline multiple times. But while creating VM from VM manifest, the datavolume can have only a single hardcoded name (it might use generateName, but then we could not copy it to kubevirt-os-images, because the pvc will have different name than VM and vm name and namespace is the only information we get from create-vm task).
- description: Namespace of the created base DataVolume | ||
name: baseDvNamespace | ||
value: $(tasks.create-base-dv.results.namespace) | ||
tasks: |
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.
Put this block at the start of the pipeline spec?
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 prefer the order: params, tasks, finally, results
kind: Task | ||
name: wait-for-vmi-status | ||
timeout: 2h0m0s | ||
- name: create-base-dv |
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.
As above, is cloning twice really necessary?
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.
It might be possible to do a single copy, but I need to test it, because it will mean to run VM in kubevirt-os-images namespace
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.
Bios installer is not cloing twice while efi installer still is?
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.
efi installer has to clone twice due to the fact, that pipelineRun can run in any namespace. One of the efi pipelineRun pod is mounting the PVC with iso. The PVC has to be in the same namespace as the pod (cross namespace is not supported during mount). So at the end when VM is finished, the result PVC has to be copied to result namespace.
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
/hold I need to do additional changes |
30b2bef
to
e7a11f3
Compare
/retest |
3a768fb
to
90dfa29
Compare
/hold cancel |
@akrejcir, @jcanocan, @opokornyy can you please review? |
/retest |
@@ -29,17 +29,22 @@ done | |||
# SECRET | |||
accessKeyId="/tmp/secrets/accessKeyId" | |||
secretKey="/tmp/secrets/secretKey" | |||
namespace="kubevirt" |
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.
Use different namespace to test pipelines by default?
name: pipeline | ||
--- | ||
apiVersion: v1 | ||
kind: ServiceAccount |
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 this become an issue when pipeline
SA is already there, i.e. will ssp-operator and tektoncd/operator fight for it?
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.
Currently the SA is deployed only to ^openshift|^kube namespace. Howewer in the future there might be a time, when this bug will be fixed and ssp and tekton operator will fight over this SA. I will add a check to ssp if the SA already exists
4b77915
to
10a3448
Compare
kind: DataVolume | ||
metadata: | ||
annotations: | ||
"cdi.kubevirt.io/storage.bind.immediate.requested": "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.
Quotes used here but in L187 not?
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.
Good catch, I updated it, but for some unknow reasons, it was reverted back
kind: Task | ||
name: wait-for-vmi-status | ||
timeout: 2h0m0s | ||
- name: create-base-dv |
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
5683820
to
dec3e9a
Compare
/retest |
1 similar comment
/retest |
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.
Looks good to me.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0xFelix 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 |
/retest |
1 similar comment
/retest |
dec3e9a
to
15722e9
Compare
/retest |
f5a3692
to
994357d
Compare
Common-templates are going to be deprecated in some future version. This commit is changing example pipelines to remove all automation related to templates and replaces it with create-vm-from-manifest task. This change helps to unify pipelines for okd and kubernetes, so now we can use single version for okd and k8s. windows-bios-installer pipeline is redesigned to run only in a single namespace, which helps to reduce number of steps and complexity. windows-customize pipeline is redesigned to run only in a single namespace which helps to reduce number of steps and complexity. Due to changes in pipelines, there were done necessary changes in RBAC - objects separated in different file, rules defined in bios-installer pipeline were replaced by roleBindings to original task's clusterRoles. Signed-off-by: Karel Šimon <ksimon@redhat.com>
994357d
to
14fbce5
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
/lgtm |
What this PR does / why we need it:
feat: rework example pipelines
Common-templates are going to be deprecated in some future version. This commit is changing example pipelines to remove all automation related to templates and replaces it with create-vm-from-manifest task.
This change helps to unify pipelines for okd and kubernetes, so now we can use single version for okd and k8s.
Release note: