6:31 a.m.
since v1:
- .bash_logout is removed from the user home environment (causes builds to fail
on Debian)
- gitlab-runner is now downloaded to and executed from user's bin/ directory
instead of using /usr/local/bin
- both RC and systemd service files now define the user explicitly that the init
system changes to when spawning the gitlab-runner service
- the gitlab flavour tasks file now relies on shell's 'creates' module
argument
so that the agent registration is idempotent rather than touching the
dirs/files explicitly
- both gitlab URL and runner token are now files editable under
~/.config/lcitool
Both the config.toml improvement in terms of replacing the ~/.config/lcitool
files as discussed during the v1 review process as well as cloudinit usage is
planned for a separate respective series, so for now, we'll have to live with 2
new config files and cloud images will need to be built manually.
Erik Skultety (6):
guests: users: Discard the .bash_logout skeleton file
guests: users: Create a bin/ directory in the flavor user's home
guests: templates: Introduce a gitlab-runner systemd service template
guests: templates: Introduce a gitlab-runner RC init service template
guests: Introduce the new 'gitlab' flavor
guests: lcitool: Enable the new 'gitlab' flavor in the lcitool script
guests/lcitool | 50 ++++++++++++++++--
guests/playbooks/update/main.yml | 5 ++
guests/playbooks/update/tasks/gitlab.yml | 52 +++++++++++++++++++
guests/playbooks/update/tasks/users.yml | 10 +++-
.../update/templates/gitlab-runner.j2 | 31 +++++++++++
.../update/templates/gitlab-runner.service.j2 | 14 +++++
6 files changed, 156 insertions(+), 6 deletions(-)
create mode 100644 guests/playbooks/update/tasks/gitlab.yml
create mode 100644 guests/playbooks/update/templates/gitlab-runner.j2
create mode 100644 guests/playbooks/update/templates/gitlab-runner.service.j2
--
2.25.1
6:31 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 1/6] guests: users: Discard the .bash_logout skeleton file
As part of the users tasks, we remove the .profile file after we create
our own .bash_profile. As part of that effort, let's remove the .bash_logout
skeleton file as well, the reason being gitlab's runner agent which
has a hard time initializing a build environment on Debian where the
system populates the .bash_logout with console cleaning code which
messes up the runner and causes the build to fail instantly [1].
Since Debian is the only distro actually populating the file and the
machines are assumed to be accessed over SSH, we can safely drop it.
[1] https://gitlab.com/gitlab-org/gitlab-runner/issues/4449
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
guests/playbooks/update/tasks/users.yml | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/guests/playbooks/update/tasks/users.yml
b/guests/playbooks/update/tasks/users.yml
index 30e5fca..a07349f 100644
--- a/guests/playbooks/update/tasks/users.yml
+++ b/guests/playbooks/update/tasks/users.yml
@@ -63,9 +63,10 @@
- bash_profile
- bashrc
-- name: '{{ flavor }}: Remove existing shell profile'
+- name: '{{ flavor }}: Remove unnecessary home skeleton files'
file:
path: /home/{{ flavor }}/.{{ item }}
state: absent
with_items:
- profile
+ - bash_logout
--
2.25.1
4:12 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 1/6] guests: users: Discard the .bash_logout skeleton file
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
As part of the users tasks, we remove the .profile file after we
create
our own .bash_profile. As part of that effort, let's remove the .bash_logout
skeleton file as well, the reason being gitlab's runner agent which
has a hard time initializing a build environment on Debian where the
system populates the .bash_logout with console cleaning code which
messes up the runner and causes the build to fail instantly [1].
Since Debian is the only distro actually populating the file and the
machines are assumed to be accessed over SSH, we can safely drop it.
[1] https://gitlab.com/gitlab-org/gitlab-runner/issues/4449
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
guests/playbooks/update/tasks/users.yml | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
6:31 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 2/6] guests: users: Create a bin/ directory in the flavor user's home
We're creating a dedicated user to run the gitlab agent, so why not
store the agent within the user profile and execute it from there.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
guests/playbooks/update/tasks/users.yml | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/guests/playbooks/update/tasks/users.yml
b/guests/playbooks/update/tasks/users.yml
index a07349f..4b09416 100644
--- a/guests/playbooks/update/tasks/users.yml
+++ b/guests/playbooks/update/tasks/users.yml
@@ -70,3 +70,10 @@
with_items:
- profile
- bash_logout
+
+- name: '{{ flavor }}: Create /home/{{ flavor }}/bin directory'
+ file:
+ path: /home/{{ flavor }}/bin
+ state: directory
+ owner: '{{ flavor }}'
+ group: '{{ flavor }}'
--
2.25.1
6:37 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 2/6] guests: users: Create a bin/ directory in the flavor user's home
On Tue, Apr 07, 2020 at 01:31:17PM +0200, Erik Skultety wrote:
We're creating a dedicated user to run the gitlab agent, so why
not
store the agent within the user profile and execute it from there.
I'm wary of this as it seems like it can create a exploit vector.
ie malicious code running as the gitlab account can replace the
gitlab agent binary in its $HOME.
Shouldn't the binary be in /usr/local/bin and owned by root so
it is completely separated ?
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
guests/playbooks/update/tasks/users.yml | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/guests/playbooks/update/tasks/users.yml
b/guests/playbooks/update/tasks/users.yml
index a07349f..4b09416 100644
--- a/guests/playbooks/update/tasks/users.yml
+++ b/guests/playbooks/update/tasks/users.yml
@@ -70,3 +70,10 @@
with_items:
- profile
- bash_logout
+
+- name: '{{ flavor }}: Create /home/{{ flavor }}/bin directory'
+ file:
+ path: /home/{{ flavor }}/bin
+ state: directory
+ owner: '{{ flavor }}'
+ group: '{{ flavor }}'
--
2.25.1
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
6:45 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 2/6] guests: users: Create a bin/ directory in the flavor user's home
On Tue, Apr 07, 2020 at 12:37:01PM +0100, Daniel P. Berrangé wrote:
On Tue, Apr 07, 2020 at 01:31:17PM +0200, Erik Skultety wrote:
> We're creating a dedicated user to run the gitlab agent, so why not
> store the agent within the user profile and execute it from there.
I'm wary of this as it seems like it can create a exploit vector.
ie malicious code running as the gitlab account can replace the
gitlab agent binary in its $HOME.
Shouldn't the binary be in /usr/local/bin and owned by root so
it is completely separated ?
That's what I've done in v1 (though not because of the possible attack vector
you mention), but it was suggested to move it to user's $HOME [1].
[1] https://www.redhat.com/archives/libvir-list/2020-March/msg01424.html
I'll change it to the original version on my local branch.
6:48 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 2/6] guests: users: Create a bin/ directory in the flavor user's home
On Tue, Apr 07, 2020 at 01:45:46PM +0200, Erik Skultety wrote:
On Tue, Apr 07, 2020 at 12:37:01PM +0100, Daniel P. Berrangé wrote:
> On Tue, Apr 07, 2020 at 01:31:17PM +0200, Erik Skultety wrote:
> > We're creating a dedicated user to run the gitlab agent, so why not
> > store the agent within the user profile and execute it from there.
>
> I'm wary of this as it seems like it can create a exploit vector.
> ie malicious code running as the gitlab account can replace the
> gitlab agent binary in its $HOME.
>
> Shouldn't the binary be in /usr/local/bin and owned by root so
> it is completely separated ?
That's what I've done in v1 (though not because of the possible attack vector
you mention), but it was suggested to move it to user's $HOME [1].
[1] https://www.redhat.com/archives/libvir-list/2020-March/msg01424.html
I'll change it to the original version on my local branch.
Hmm, for that matter, we shouldn't store the config file in the
/home/gitlab/.gitlab-runner directory either.
Essentially we should try to assume anything in $HOME is subjec to
arbitrary deletion in order to restore a clean state, so we shouldn't
try to keep important files there.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
9:30 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 2/6] guests: users: Create a bin/ directory in the flavor user's home
On Tue, Apr 07, 2020 at 12:48:34PM +0100, Daniel P. Berrangé wrote:
On Tue, Apr 07, 2020 at 01:45:46PM +0200, Erik Skultety wrote:
> On Tue, Apr 07, 2020 at 12:37:01PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Apr 07, 2020 at 01:31:17PM +0200, Erik Skultety wrote:
> > > We're creating a dedicated user to run the gitlab agent, so why not
> > > store the agent within the user profile and execute it from there.
> >
> > I'm wary of this as it seems like it can create a exploit vector.
> > ie malicious code running as the gitlab account can replace the
> > gitlab agent binary in its $HOME.
> >
> > Shouldn't the binary be in /usr/local/bin and owned by root so
> > it is completely separated ?
>
> That's what I've done in v1 (though not because of the possible attack
vector
> you mention), but it was suggested to move it to user's $HOME [1].
> [1] https://www.redhat.com/archives/libvir-list/2020-March/msg01424.html
>
> I'll change it to the original version on my local branch.
Hmm, for that matter, we shouldn't store the config file in the
/home/gitlab/.gitlab-runner directory either.
Yes, I'll make sure the config is under the default system location in
/etc/gitlab-runner/ with read permissions for the gitlab user.
Thanks,
--
Erik Skultety
6:31 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 3/6] guests: templates: Introduce a gitlab-runner systemd service template
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
.../update/templates/gitlab-runner.service.j2 | 14 ++++++++++++++
1 file changed, 14 insertions(+)
create mode 100644 guests/playbooks/update/templates/gitlab-runner.service.j2
diff --git a/guests/playbooks/update/templates/gitlab-runner.service.j2
b/guests/playbooks/update/templates/gitlab-runner.service.j2
new file mode 100644
index 0000000..5d494b2
--- /dev/null
+++ b/guests/playbooks/update/templates/gitlab-runner.service.j2
@@ -0,0 +1,14 @@
+[Unit]
+Description=GitLab Runner
+After=network.target
+ConditionFileIsExecutable=/home/gitlab/bin/gitlab-runner
+
+[Service]
+User=gitlab
+WorkingDirectory=/home/gitlab
+ExecStart=/home/gitlab/bin/gitlab-runner run --config
/home/gitlab/.gitlab-runner/config.toml
+Restart=always
+
+[Install]
+WantedBy=multi-user.target
+
--
2.25.1
6:31 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 4/6] guests: templates: Introduce a gitlab-runner RC init service template
Based on https://docs.gitlab.com/runner/install/freebsd.html#installing-gitlab-runner
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
.../update/templates/gitlab-runner.j2 | 31 +++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 guests/playbooks/update/templates/gitlab-runner.j2
diff --git a/guests/playbooks/update/templates/gitlab-runner.j2
b/guests/playbooks/update/templates/gitlab-runner.j2
new file mode 100644
index 0000000..bfd722b
--- /dev/null
+++ b/guests/playbooks/update/templates/gitlab-runner.j2
@@ -0,0 +1,31 @@
+#!/bin/sh
+# PROVIDE: gitlab_runner
+# REQUIRE: DAEMON NETWORKING
+# BEFORE:
+# KEYWORD:
+
+. /etc/rc.subr
+
+name="gitlab_runner"
+rcvar="gitlab_runner_enable"
+
+user="gitlab"
+user_home="/home/gitlab"
+command="/home/gitlab/bin/gitlab-runner"
+command_args="run --config ${user_home}/.gitlab-runner/config.toml"
+pidfile="/var/run/${name}.pid"
+
+start_cmd="gitlab_runner_start"
+
+gitlab_runner_start()
+{
+ export USER=${user}
+ export HOME=${user_home}
+ if checkyesno ${rcvar}; then
+ cd ${user_home}
+ /usr/sbin/daemon -u ${user} -p ${pidfile} ${command} ${command_args} >
/var/log/gitlab-runner.log 2>&1
+ fi
+}
+
+load_rc_config $name
+run_rc_command $1
--
2.25.1
6:31 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 5/6] guests: Introduce the new 'gitlab' flavor
With the recent efforts in upstream libvirt to centralize our CI on
gitlab, let's add a new gitlab-specific flavor along with related
playbook tasks. This flavour revolves around installing and configuring
the gitlab-runner agent binary which requires the per-project
registration token to be specified in order for the runner to be
successfully registered with the gitlab server.
Note that as part of the registration process each runner acquires a new
unique access token. This means that we must ensure that the
registration is run only on the first update, otherwise a new runner
with a new access token is registered with the gitlab project.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
guests/playbooks/update/main.yml | 5 +++
guests/playbooks/update/tasks/gitlab.yml | 52 ++++++++++++++++++++++++
2 files changed, 57 insertions(+)
create mode 100644 guests/playbooks/update/tasks/gitlab.yml
diff --git a/guests/playbooks/update/main.yml b/guests/playbooks/update/main.yml
index a5a4de8..371e53d 100644
--- a/guests/playbooks/update/main.yml
+++ b/guests/playbooks/update/main.yml
@@ -58,3 +58,8 @@
- include: '{{ playbook_base }}/tasks/jenkins.yml'
when:
- flavor == 'jenkins'
+
+ # Install the Gitlab runner agent
+ - include: '{{ playbook_base }}/tasks/gitlab.yml'
+ when:
+ - flavor == 'gitlab'
diff --git a/guests/playbooks/update/tasks/gitlab.yml
b/guests/playbooks/update/tasks/gitlab.yml
new file mode 100644
index 0000000..b8f731d
--- /dev/null
+++ b/guests/playbooks/update/tasks/gitlab.yml
@@ -0,0 +1,52 @@
+---
+- name: Define gitlab-related facts
+ set_fact:
+ gitlab_url: '{{ lookup("file", gitlab_url_file) }}'
+ gitlab_runner_secret: '{{ lookup("file", gitlab_runner_token_file)
}}'
+ gitlab_runner_download_url:
https://gitlab-runner-downloads.s3.amazonaws.com/latest/binaries/gitlab-r...
ansible_system|lower }}-amd64
+ gitlab_runner_config_path: '/home/gitlab/.gitlab-runner/config.toml'
+
+- name: Download gitlab-runner agent
+ get_url:
+ url: '{{ gitlab_runner_download_url }}'
+ dest: /home/gitlab/bin/gitlab-runner
+ owner: gitlab
+ group: gitlab
+ mode: '0775'
+ force: yes
+
+- name: Register the gitlab-runner agent
+ become: true
+ become_user: gitlab
+ shell: '/home/gitlab/bin/gitlab-runner register --non-interactive --config {{
gitlab_runner_config_path }} --registration-token {{ gitlab_runner_secret }} --url {{
gitlab_url }} --executor shell --tag-list {{ inventory_hostname }}'
+ args:
+ creates: '{{ gitlab_runner_config_path }}'
+
+- block:
+ - name: Install the gitlab-runner service unit
+ template:
+ src: '{{ playbook_base }}/templates/gitlab-runner.service.j2'
+ dest: /etc/systemd/system/gitlab-runner.service
+
+ - name: Enable the gitlab-runner service
+ systemd:
+ name: gitlab-runner
+ state: started
+ enabled: yes
+ daemon_reload: yes
+ when: ansible_service_mgr == 'systemd'
+
+- block:
+ - name: Install the gitlab_runner rc service script
+ template:
+ src: '{{ playbook_base }}/templates/gitlab-runner.j2'
+ dest: '/usr/local/etc/rc.d/gitlab_runner'
+ mode: '0755'
+
+ - name: Enable the gitlab-runner rc service
+ service:
+ name: gitlab_runner
+ state: started
+ enabled: yes
+ when: ansible_service_mgr != 'systemd'
+
--
2.25.1
5:05 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 5/6] guests: Introduce the new 'gitlab' flavor
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
+- name: Register the gitlab-runner agent
+ become: true
+ become_user: gitlab
+ shell: '/home/gitlab/bin/gitlab-runner register --non-interactive --config {{
gitlab_runner_config_path }} --registration-token {{ gitlab_runner_secret }} --url {{
gitlab_url }} --executor shell --tag-list {{ inventory_hostname }}'
You didn't answer in the other thread, so I'll ask again here: is the
idea that we're going to use only the FreeBSD runners to supplement
the shared runners for the existing unit tests, and all Linux runners
are only going to be used for integration tests later on, hence why
we need to use the shell executor rather than the docker executor?
Additional nit: instead of using {{ inventory_hostname }} as tag, we
can have a nicer tag by using {{ os_name|lower }}-{{ os_version }}.
It would also be a good idea to quote all command arguments.
The rest looks good, but given the potential security issue raised by
Dan I'll wait for v3 before handing out actual ACKs :)
--
Andrea Bolognani / Red Hat / Virtualization
5:28 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 5/6] guests: Introduce the new 'gitlab' flavor
On Wed, Apr 08, 2020 at 12:05:11PM +0200, Andrea Bolognani wrote:
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
> +- name: Register the gitlab-runner agent
> + become: true
> + become_user: gitlab
> + shell: '/home/gitlab/bin/gitlab-runner register --non-interactive --config {{
gitlab_runner_config_path }} --registration-token {{ gitlab_runner_secret }} --url {{
gitlab_url }} --executor shell --tag-list {{ inventory_hostname }}'
You didn't answer in the other thread, so I'll ask again here: is the
idea that we're going to use only the FreeBSD runners to supplement
the shared runners for the existing unit tests, and all Linux runners
are only going to be used for integration tests later on, hence why
we need to use the shell executor rather than the docker executor?
Why not both? We can always extend the capacity with VM builders, although it's
true that functional tests was what I had in mind originally (+the FreeBSD
exception like you already mentioned). Either way, I don't understand why we
should force usage of the docker executor for the VMs which we can use for
builds. The way I'm looking at the setup is: container setup vs VM setup, with
both being consistent in their own respective category, IOW, why should the
setup of the VM in terms of the gitlab-runner be different when running
functional tests vs builds? So, I'd like to stay with the shell executor on VMs
in all cases and not fragment the setup even further.
Furthermore, with the OpenShift infra we got, I see very little to no value in
using the VMs to extend our build capacity.
Additional nit: instead of using {{ inventory_hostname }} as tag, we
can have a nicer tag by using {{ os_name|lower }}-{{ os_version }}.
Sure, I don't mind that change.
It would also be a good idea to quote all command arguments.
Will do.
--
Erik Skultety
8:21 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 5/6] guests: Introduce the new 'gitlab' flavor
On Wed, 2020-04-08 at 12:28 +0200, Erik Skultety wrote:
On Wed, Apr 08, 2020 at 12:05:11PM +0200, Andrea Bolognani wrote:
> You didn't answer in the other thread, so I'll ask again here: is the
> idea that we're going to use only the FreeBSD runners to supplement
> the shared runners for the existing unit tests, and all Linux runners
> are only going to be used for integration tests later on, hence why
> we need to use the shell executor rather than the docker executor?
Why not both? We can always extend the capacity with VM builders, although it's
true that functional tests was what I had in mind originally (+the FreeBSD
exception like you already mentioned). Either way, I don't understand why we
should force usage of the docker executor for the VMs which we can use for
builds. The way I'm looking at the setup is: container setup vs VM setup, with
both being consistent in their own respective category, IOW, why should the
setup of the VM in terms of the gitlab-runner be different when running
functional tests vs builds? So, I'd like to stay with the shell executor on VMs
in all cases and not fragment the setup even further.
Because all the builds that currently exist are defined in terms of
containers, so when you have something like
x86-fedora-31:
script:
...
image: quay.io/libvirt/buildenv-libvirt-fedora-31:latest
you cannot just run the same job on a worker that is configured to
use the shell executor.
I guess you could drop the image element and replace it with
tags:
- fedora-31
but then you'd either have to duplicate the job definition, or to
only have the new one which then would not work for forks and merge
requests, so that makes it less interesting.
Furthermore, with the OpenShift infra we got, I see very little to no
value in
using the VMs to extend our build capacity.
I don't understand what you're trying to say here at all, sorry.
--
Andrea Bolognani / Red Hat / Virtualization
8:56 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 5/6] guests: Introduce the new 'gitlab' flavor
On Wed, Apr 08, 2020 at 03:21:00PM +0200, Andrea Bolognani wrote:
On Wed, 2020-04-08 at 12:28 +0200, Erik Skultety wrote:
> On Wed, Apr 08, 2020 at 12:05:11PM +0200, Andrea Bolognani wrote:
> > You didn't answer in the other thread, so I'll ask again here: is the
> > idea that we're going to use only the FreeBSD runners to supplement
> > the shared runners for the existing unit tests, and all Linux runners
> > are only going to be used for integration tests later on, hence why
> > we need to use the shell executor rather than the docker executor?
>
> Why not both? We can always extend the capacity with VM builders, although it's
> true that functional tests was what I had in mind originally (+the FreeBSD
> exception like you already mentioned). Either way, I don't understand why we
> should force usage of the docker executor for the VMs which we can use for
> builds. The way I'm looking at the setup is: container setup vs VM setup, with
> both being consistent in their own respective category, IOW, why should the
> setup of the VM in terms of the gitlab-runner be different when running
> functional tests vs builds? So, I'd like to stay with the shell executor on VMs
> in all cases and not fragment the setup even further.
Because all the builds that currently exist are defined in terms of
containers, so when you have something like
x86-fedora-31:
script:
...
image: quay.io/libvirt/buildenv-libvirt-fedora-31:latest
you cannot just run the same job on a worker that is configured to
use the shell executor.
I guess you could drop the image element and replace it with
tags:
- fedora-31
^This is what I had in mind
but then you'd either have to duplicate the job definition, or to
only have the new one which then would not work for forks and merge
requests, so that makes it less interesting.
We'll have to duplicate it for FreeBSD anyway, so I don't understand why should
do it differently for other VM distros.
> Furthermore, with the OpenShift infra we got, I see very little to no value in
> using the VMs to extend our build capacity.
I don't understand what you're trying to say here at all, sorry.
What I meant is that I don't see much value to run the builds in VMs since we
have a bunch of images ready where we can already execute the builds so it's
basically only about having resources to spawn the containers and that's where
OpenShift comes in.
I understand you reminding me that private runners cannot be used to run on
merge requests (and forks for obvious reasons), but the same applies to VMs
we're discussing in this thread. So, I wouldn't focus primarily on running the
builds there is what I'm saying.
--
Erik Skultety
11:13 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 5/6] guests: Introduce the new 'gitlab' flavor
On Wed, 2020-04-08 at 15:56 +0200, Erik Skultety wrote:
On Wed, Apr 08, 2020 at 03:21:00PM +0200, Andrea Bolognani wrote:
> I guess you could drop the image element and replace it with
>
> tags:
> - fedora-31
^This is what I had in mind
> but then you'd either have to duplicate the job definition, or to
> only have the new one which then would not work for forks and merge
> requests, so that makes it less interesting.
We'll have to duplicate it for FreeBSD anyway, so I don't understand why should
do it differently for other VM distros.
We will not duplicate it, because there is no existing container
based build for FreeBSD: the FreeBSD builds can, by definition, only
happen inside VMs.
> I don't understand what you're trying to say here at
all, sorry.
What I meant is that I don't see much value to run the builds in VMs since we
have a bunch of images ready where we can already execute the builds
Totally agree with you up until this point: this is exactly what I
was trying to convey.
so it's
basically only about having resources to spawn the containers and that's where
OpenShift comes in.
I still don't understand how OpenShift is part of the picture. I have
probably either missed or forgotten something O:-)
I understand you reminding me that private runners cannot be used to
run on
merge requests (and forks for obvious reasons), but the same applies to VMs
we're discussing in this thread. So, I wouldn't focus primarily on running the
builds there is what I'm saying.
I think we're kind of talking past each other at this point :)
To reiterate/clarify: I'm perfectly okay with using Linux VMs for
integration testing only and leaving builds to shared runner and
FreeBSD VMs. I don't think we should use Linux VMs for builds unless
we use the docker executor for them, but then again I don't think we
really need to use Linux VMs for builds in the first place.
--
Andrea Bolognani / Red Hat / Virtualization
6:31 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 6/6] guests: lcitool: Enable the new 'gitlab' flavor in the lcitool script
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
guests/lcitool | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 45 insertions(+), 5 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 689a8cf..d9b2372 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -175,7 +175,7 @@ class Config:
)
)
- if flavor not in ["test", "jenkins"]:
+ if flavor not in ["test", "jenkins", "gitlab"]:
raise Exception("Invalid flavor '{}'".format(flavor))
return flavor
@@ -185,7 +185,7 @@ class Config:
# The vault password is only needed for the jenkins flavor, but in
# that case we want to make sure there's *something* in there
- if self.get_flavor() != "test":
+ if self.get_flavor() == "jenkins":
vault_pass_file = self._get_config_file("vault-password")
try:
@@ -217,6 +217,38 @@ class Config:
return root_pass_file
+ def get_gitlab_runner_token_file(self):
+ gitlab_runner_token_file =
self._get_config_file("gitlab-runner-token")
+
+ try:
+ with open(gitlab_runner_token_file, "r") as infile:
+ if not infile.readline().strip():
+ raise ValueError
+ except Exception as ex:
+ raise Exception(
+ "Missing or invalid gitlab runner token file ({}): {}".format(
+ gitlab_runner_token_file, ex
+ )
+ )
+
+ return gitlab_runner_token_file
+
+ def get_gitlab_url_file(self):
+ gitlab_url_file = self._get_config_file("gitlab-url")
+
+ try:
+ with open(gitlab_url_file, "r") as infile:
+ if not infile.readline().strip():
+ raise ValueError
+ except Exception as ex:
+ raise Exception(
+ "Missing or invalid gitlab url file ({}): {}".format(
+ gitlab_url_file, ex
+ )
+ )
+
+ return gitlab_url_file
+
class Inventory:
@@ -449,6 +481,8 @@ class Application:
flavor = self._config.get_flavor()
vault_pass_file = self._config.get_vault_password_file()
root_pass_file = self._config.get_root_password_file()
+ gitlab_url_file = self._config.get_gitlab_url_file()
+ gitlab_runner_token_file = self._config.get_gitlab_runner_token_file()
ansible_hosts = ",".join(self._inventory.expand_pattern(hosts))
selected_projects = self._projects.expand_pattern(projects)
@@ -469,7 +503,7 @@ class Application:
playbook_base = os.path.join(base, "playbooks", playbook)
playbook_path = os.path.join(playbook_base, "main.yml")
- extra_vars = json.dumps({
+ extra_vars_d = {
"base": base,
"playbook_base": playbook_base,
"root_password_file": root_pass_file,
@@ -477,7 +511,13 @@ class Application:
"selected_projects": selected_projects,
"git_remote": git_remote,
"git_branch": git_branch,
- })
+ }
+
+ if flavor == "gitlab":
+ extra_vars_d.update([
+ ("gitlab_url_file", gitlab_url_file),
+ ("gitlab_runner_token_file", gitlab_runner_token_file),
+ ])
ansible_playbook = distutils.spawn.find_executable("ansible-playbook")
if ansible_playbook is None:
@@ -486,7 +526,7 @@ class Application:
cmd = [
ansible_playbook,
"--limit", ansible_hosts,
- "--extra-vars", extra_vars,
+ "--extra-vars", json.dumps(extra_vars_d),
]
# Provide the vault password if available
--
2.25.1
4:49 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 6/6] guests: lcitool: Enable the new 'gitlab' flavor in the lcitool script
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
# The vault password is only needed for the jenkins flavor,
but in
# that case we want to make sure there's *something* in there
- if self.get_flavor() != "test":
+ if self.get_flavor() == "jenkins":
vault_pass_file = self._get_config_file("vault-password")
You need something similar to this in your new functions, otherwise
trying even if you've configured lcitool to use the test flavor
you'll still get
$ ./lcitool update all all
./lcitool: Missing or invalid gitlab url file ...
+ def get_gitlab_runner_token_file(self):
+ gitlab_runner_token_file =
self._get_config_file("gitlab-runner-token")
+
+ try:
+ with open(gitlab_runner_token_file, "r") as infile:
+ if not infile.readline().strip():
+ raise ValueError
+ except Exception as ex:
+ raise Exception(
+ "Missing or invalid gitlab runner token file ({}): {}".format(
Please capitalize GitLab properly in user-visible strings.
- extra_vars = json.dumps({
+ extra_vars_d = {
You don't really need to change the name of the variable.
+ if flavor == "gitlab":
+ extra_vars_d.update([
+ ("gitlab_url_file", gitlab_url_file),
+ ("gitlab_runner_token_file", gitlab_runner_token_file),
+ ])
You can use a dict as argument to update(), which is more natural
than a list of tuples.
But in reality I think you can skip the check entirely and add both
keys to extra_vars unconditionally: after you've fixed the issue I
pointed out above, they will either be set (gitlab flavor) or None
(any other flavor), and even in the latter case it's fine to pass
them to Ansible as they will simply not be used.
Everything else looks good.
--
Andrea Bolognani / Red Hat / Virtualization
5:42 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 6/6] guests: lcitool: Enable the new 'gitlab' flavor in the lcitool script
On Wed, Apr 08, 2020 at 11:49:45AM +0200, Andrea Bolognani wrote:
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
> # The vault password is only needed for the jenkins flavor, but in
> # that case we want to make sure there's *something* in there
> - if self.get_flavor() != "test":
> + if self.get_flavor() == "jenkins":
> vault_pass_file = self._get_config_file("vault-password")
You need something similar to this in your new functions, otherwise
trying even if you've configured lcitool to use the test flavor
you'll still get
$ ./lcitool update all all
./lcitool: Missing or invalid gitlab url file ...
Doh! Clearly I didn't test regressions :(
> + def get_gitlab_runner_token_file(self):
> + gitlab_runner_token_file =
self._get_config_file("gitlab-runner-token")
> +
> + try:
> + with open(gitlab_runner_token_file, "r") as infile:
> + if not infile.readline().strip():
> + raise ValueError
> + except Exception as ex:
> + raise Exception(
> + "Missing or invalid gitlab runner token file ({}):
{}".format(
Please capitalize GitLab properly in user-visible strings.
> - extra_vars = json.dumps({
> + extra_vars_d = {
You don't really need to change the name of the variable.
Right, although there are certain good practices like adding _l to lists and _d
to dicts, so that it's immediately visible what kind of object you're working
with when you don't have the initialization of the object right in front of
your eyes.
> + if flavor == "gitlab":
> + extra_vars_d.update([
> + ("gitlab_url_file", gitlab_url_file),
> + ("gitlab_runner_token_file", gitlab_runner_token_file),
> + ])
You can use a dict as argument to update(), which is more natural
than a list of tuples.
^This is highly subjective I'd say, the readability doesn't really change if
you don't count an extra pair of parentheses, but oki, I'll change it...
But in reality I think you can skip the check entirely and add both
keys to extra_vars unconditionally: after you've fixed the issue I
pointed out above, they will either be set (gitlab flavor) or None
(any other flavor), and even in the latter case it's fine to pass
them to Ansible as they will simply not be used.
I thought about this and I simply didn't want to pass undefined variables to
ansible even if it doesn't use them, although I guess with the changes I'm
currently working on it won't matter as I'm planning to stop passing visible
extra vars on the command-line completely, so I think I can live with this
suggestion.
--
Erik Skultety
7:52 a.m.
New subject: [libvirt-jenkins-ci PATCH v2 6/6] guests: lcitool: Enable the new 'gitlab' flavor in the lcitool script
On Wed, 2020-04-08 at 12:42 +0200, Erik Skultety wrote:
On Wed, Apr 08, 2020 at 11:49:45AM +0200, Andrea Bolognani wrote:
> On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
> > - extra_vars = json.dumps({
> > + extra_vars_d = {
>
> You don't really need to change the name of the variable.
Right, although there are certain good practices like adding _l to lists and _d
to dicts, so that it's immediately visible what kind of object you're working
with when you don't have the initialization of the object right in front of
your eyes.
I wasn't aware of these good practices, but either way we're not
currently using them in lcitool so adopting them in a single case
out of a thousand lines of code doesn't make a lot of sense to me.
--
Andrea Bolognani / Red Hat / Virtualization
1689
days inactive
1690
days old
19 comments
3 participants
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Erik Skultety