[PATCH 0/6] Add github CI

[Nir Soffer]

Replace the broken travis CI configuration with github actions.

The new CI revealed some issues with chown(), and some flaky or broken tests
with some python versions.

The chown() issues are fixed in this change, the tests issues need
investigation.

Nir Soffer (6):
  Remove travis CI configuration
  Log uid and gid when running in unprivileged mode
  Change rundir ownership only if running as root
  Don't change socket ownership
  Add python 3.11 to tox
  Add github actions based CI

.github/workflows/ci.yml | 48 ++++++++++++++++++++++++++++++++++++++++
 .travis.yml              | 26 ----------------------
 src/lockfile.c           | 12 +++++-----
 src/main.c               | 10 ++-------
 tox.ini                  |  4 ++--
 5 files changed, 59 insertions(+), 41 deletions(-)
 create mode 100644 .github/workflows/ci.yml
 delete mode 100644 .travis.yml

[PATCH 1/6] Remove travis CI configuration

[Nir Soffer]

The travis CI is broken for long time.

Signed-off-by: Nir Soffer 
---
 .travis.yml | 26 --------------------------
 1 file changed, 26 deletions(-)
 delete mode 100644 .travis.yml

diff --git a/.travis.yml b/.travis.yml
deleted file mode 100644
index 56bcc8c..0000000
--- a/.travis.yml
+++ /dev/null
@@ -1,26 +0,0 @@
-dist: bionic
-
-language: python
-
-python:
-  - "3.6"
-  - "3.7"
-  - "3.8"
-  - "3.9-dev"
-
-addons:
-  apt:
-    packages:
-    - gcc
-    - libaio-dev
-    - libblkid-dev
-    - make
-
-install:
-    - pip install flake8
-
-script:
-    - make BUILDARGS="--build-lib=."
-    - source tests/env.sh
-    - pytest
-    - flake8 --statistics tests python

[PATCH 2/6] Log uid and gid when running in unprivileged mode

[Nir Soffer]

This helps to debug issues in remote CI.

Example logs (from the tests):

2022-11-27 16:34:46 326 [2767]: Running in unprivileged mode uid=1001 gid=1001

Signed-off-by: Nir Soffer 
---
 src/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/main.c b/src/main.c
index a9ceee5..f0182b6 100644
--- a/src/main.c
+++ b/src/main.c
@@ -1755,21 +1755,22 @@ static int do_daemon(void)
    	return rv;
    strcpy(client[helper_ci].owner_name, "helper");

setup_signals();
    setup_logging();

if (strcmp(run_dir, DEFAULT_RUN_DIR))
    	log_warn("Using non-standard run directory '%s'", run_dir);

if (!privileged)
-		log_warn("Running in unprivileged mode");
+		log_warn("Running in unprivileged mode uid=%d gid=%d",
+			 com.uid, com.gid);

/* If we run as root, make run_dir owned by root, so we can create the
     * lockfile when selinux disables DAC_OVERRIDE.
     * See https://danwalsh.livejournal.com/79643.html */

fd = lockfile(run_dir, SANLK_LOCKFILE_NAME, com.uid,
    	      privileged ? 0 : com.gid);
    if (fd < 0) {
    	close_logging();
    	return fd;

[PATCH 3/6] Change rundir ownership only if running as root

[Nir Soffer]

This works when testing locally, but fails in ubuntu CI:

Using non-standard run directory '/tmp/sanlock'
    Running in unprivileged mode uid=1001 gid=1001
    lockfile chown error /tmp/sanlock: Operation not permitted

The ownership change is required only when starting a root to avoid
issues with selinux.

Signed-off-by: Nir Soffer 
---
 src/lockfile.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/lockfile.c b/src/lockfile.c
index cffaaff..fe15bd0 100644
--- a/src/lockfile.c
+++ b/src/lockfile.c
@@ -40,25 +40,27 @@ int lockfile(const char *dir, const char *name, int uid, int gid)
     * starting as root. */

old_umask = umask(0002);
    rv = mkdir(dir, 0775);
    if (rv < 0 && errno != EEXIST) {
    	umask(old_umask);
    	return rv;
    }
    umask(old_umask);

-	rv = chown(dir, uid, gid);
-	if (rv < 0) {
-		log_error("lockfile chown error %s: %s",
-			  dir, strerror(errno));
-		return rv;
+	if (geteuid() == 0) {
+		rv = chown(dir, uid, gid);
+		if (rv < 0) {
+			log_error("lockfile chown error %s: %s",
+				  dir, strerror(errno));
+			return rv;
+		}
    }

snprintf(path, PATH_MAX, "%s/%s", dir, name);

fd = open(path, O_CREAT|O_WRONLY|O_CLOEXEC, 0644);
    if (fd < 0) {
    	log_error("lockfile open error %s: %s",
    		  path, strerror(errno));
    	return -1;
    }

[PATCH 4/6] Don't change socket ownership

[Nir Soffer]

We bind the socket after dropping privileges so the socket is already
owned by the current user and group and there is no need to change
ownership.

This works when running in unprivileged mode locally but fails on Ubuntu
CI:

Using non-standard run directory '/tmp/sanlock'
    Running in unprivileged mode uid=1001 gid=1001
    sanlock daemon started 3.8.5 host 64b4f365-e153-4e0f-b4bf-c462c37739e7.fv-az41-301 (fv-az41-301)
    could not set socket /tmp/sanlock/sanlock.sock permissions: Operation not permitted

Signed-off-by: Nir Soffer 
---
 src/main.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/src/main.c b/src/main.c
index f0182b6..3944452 100644
--- a/src/main.c
+++ b/src/main.c
@@ -1385,27 +1385,20 @@ static int setup_listener(void)

unlink(addr.sun_path);
    rv = bind(fd, (struct sockaddr *) &addr, sizeof(struct sockaddr_un));
    if (rv < 0)
    	goto exit_fail;

rv = chmod(addr.sun_path, DEFAULT_SOCKET_MODE);
    if (rv < 0)
    	goto exit_fail;

-	rv = chown(addr.sun_path, com.uid, com.gid);
-	if (rv < 0) {
-		log_error("could not set socket %s permissions: %s",
-			addr.sun_path, strerror(errno));
-		goto exit_fail;
-	}
-
    rv = listen(fd, 5);
    if (rv < 0)
    	goto exit_fail;

fcntl(fd, F_SETFL, fcntl(fd, F_GETFL, 0) | O_NONBLOCK);

ci = client_add(fd, process_listener, NULL);
    if (ci < 0)
    	goto exit_fail;

[PATCH 4/6] Don't change socket ownership

[Nir Soffer]

On Sun, Nov 27, 2022 at 7:39 PM Nir Soffer  wrote:
> We bind the socket after dropping privileges so the socket is already
> owned by the current user and group and there is no need to change
> ownership.
>
> This works when running in unprivileged mode locally but fails on Ubuntu
> CI:
> Using non-standard run directory '/tmp/sanlock'
> Running in unprivileged mode uid=1001 gid=1001
> sanlock daemon started 3.8.5 host
>
> 64b4f365-e153-4e0f-b4bf-c462c37739e7.fv-az41-301 (fv-az41-301)
>     could not set socket /tmp/sanlock/sanlock.sock permissions: Operation
> not permitted
> Signed-off-by: Nir Soffer
>
> src/main.c | 7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/src/main.c b/src/main.c
> index f0182b6..3944452 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -1385,27 +1385,20 @@ static int setup_listener(void)
>     unlink(addr.sun_path);
>     rv = bind(fd, (struct sockaddr *) &addr, sizeof(struct
>
> sockaddr_un));
>         if (rv < 0)
>                 goto exit_fail;
>     rv = chmod(addr.sun_path, DEFAULT_SOCKET_MODE);
>     if (rv < 0)
>             goto exit_fail;
>
>   rv = chown(addr.sun_path, com.uid, com.gid);
>   if (rv < 0) {
>           log_error("could not set socket %s permissions: %s",
>                   addr.sun_path, strerror(errno));
>           goto exit_fail;
>   }

This was added in

commit d8805d428ff7c8994a43cc5265eb5379f4db1057
Author: Federico Simoncelli 
Date:   Wed Apr 20 13:59:33 2011 +0100

daemon: configurable socket permissions

When it was added sanlock did not dropped privileges.

We started to drop privileges in:

commit 5c51530b5bccb8cfadc8a8591558b9ab99b95cb3
Author: Federico Simoncelli 
Date:   Wed Jul 18 12:44:15 2012 -0500

daemon: drop root privileges and use the helper

Since that commit, chown() is called from the user running sanlock, trying
to change
the ownership to the same uid/gid.

-
>     rv = listen(fd, 5);
>     if (rv < 0)
>             goto exit_fail;
>
>     fcntl(fd, F_SETFL, fcntl(fd, F_GETFL, 0) | O_NONBLOCK);
>
>     ci = client_add(fd, process_listener, NULL);
>     if (ci < 0)
>             goto exit_fail;
>
> --
> 2.38.1

[PATCH 5/6] Add python 3.11 to tox

[Nir Soffer]

The version can be tested now in github actions on ubuntu latest.

Signed-off-by: Nir Soffer 
---
 tox.ini | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tox.ini b/tox.ini
index c0cf5a1..04676a9 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,32 +1,32 @@
 # Tox (http://tox.testrun.org/) is a tool for running tests
 # in multiple virtualenvs. This configuration file will run the
 # test suite on all supported python versions. To use it, "pip install tox"
 # and then run "tox" from this directory.

[tox]
-envlist = py{36,37,38,39,310},flake8
+envlist = py{36,37,38,39,310,311},flake8
 skipsdist = True
 skip_missing_interpreters = True

[testenv]
 passenv = *
 setenv =
     LD_LIBRARY_PATH={env:PWD}/wdmd:{env:PWD}/src
     SANLOCK_PRIVILEGED=0
     SANLOCK_RUN_DIR=/tmp/sanlock
 whitelist_externals = make
 deps =
     pytest
     userstorage>=0.5.1
 commands =
-    py{36,37,38,39,310}: make BUILDARGS="--build-lib={envsitepackagesdir}"
+    py{36,37,38,39,310,311}: make BUILDARGS="--build-lib={envsitepackagesdir}"
     pytest {posargs}

[testenv:flake8]
 deps = flake8
 commands = flake8 --statistics tests python

[pytest]
 # Notes:
 # --basetemp: we must use /var/tmp as sanlock uses direct I/O.
 # -rxX show extra test summary: (x)failed, (X)passed,

[PATCH 6/6] Add github actions based CI

[Nir Soffer]

Add github workflow running on github for every push and pull requests.

The CI run the tests with multiple python versions, ensuring that
sanlock builds and run on Ubuntu latest (20.04), and the python bindings
is compatible with python 3.6-3.10.

The new CI revealed these issues:

- Unneeded chown in lockfile() when not running as root
- Unneeded chown in setup_listener()
- test_write_lockspace_4k[user_4k_path0-1048576] is flaky in python 3.6
- test_write_lockspace_4k[user_4k_path0-1048576] broken in python 3.9
- test_write_resource_invalid_disk[disk5] broken in python 3.9, 3.10, 3.11

Example run:
https://github.com/nirs/sanlock/actions/runs/3559245867/

Signed-off-by: Nir Soffer 
---
 .github/workflows/ci.yml | 48 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 .github/workflows/ci.yml

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
new file mode 100644
index 0000000..125eee6
--- /dev/null
+++ b/.github/workflows/ci.yml
@@ -0,0 +1,48 @@
+# Copyright (C) 2022 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+
+name: CI
+
+on:
+  - push
+  - pull_request
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+          - python_version: '3.6'
+          - python_version: '3.7'
+          - python_version: '3.8'
+          - python_version: '3.9'
+          - python_version: '3.10'
+          - python_version: '3.11'
+    steps:
+      - name: Checkout source
+        uses: actions/checkout@v3
+
+      - name: Setup python
+        uses: actions/setup-python@v4
+        with:
+          python-version: ${{matrix.python_version}}
+          cache: 'pip'
+          check-latest: true
+
+      - name: Install required packages
+        run: sudo apt install -y gcc make libaio-dev libblkid-dev uuid-dev
+
+      - name: Install pip requirements
+        run: pip install -r requirements.txt
+
+      - name: Setup userstorage
+        run: userstorage create tests/storage.py
+
+      - name: Run tests
+        run: tox

[PATCH 6/6] Add github actions based CI

[Albert Esteve]

On Sun, Nov 27, 2022 at 6:39 PM Nir Soffer  wrote:
> Add github workflow running on github for every push and pull requests.
>
> The CI run the tests with multiple python versions, ensuring that
> sanlock builds and run on Ubuntu latest (20.04), and the python bindings
> is compatible with python 3.6-3.10.
>
> The new CI revealed these issues:
> Unneeded chown in lockfile() when not running as root
> Unneeded chown in setup_listener()
> test_write_lockspace_4k[user_4k_path0-1048576] is flaky in python 3.6
> test_write_lockspace_4k[user_4k_path0-1048576] broken in python 3.9
> test_write_resource_invalid_disk[disk5] broken in python 3.9, 3.10, 3.11

Before adding the CI jobs they should be all green.

What do you plan to do with this?
Either issues are fixed in this (or separate) PR, or failing tests are set
temporarily with xfail until they are fixed.
> Example run:
> https://github.com/nirs/sanlock/actions/runs/3559245867/
> Signed-off-by: Nir Soffer
>
> .github/workflows/ci.yml | 48 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 .github/workflows/ci.yml
>
> diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
> new file mode 100644
> index 0000000..125eee6
> --- /dev/null
> +++ b/.github/workflows/ci.yml
> @@ -0,0 +1,48 @@
> +# Copyright (C) 2022 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
>
> +name: CI
>
> +on:
> push
> pull_request
>
> +jobs:
> test:
> runs-on: ubuntu-latest
> strategy:
>  fail-fast: false
>  matrix:
>    include:
>      - python_version: '3.6'
>      - python_version: '3.7'
>      - python_version: '3.8'
>      - python_version: '3.9'
>      - python_version: '3.10'
>      - python_version: '3.11'
> steps:
>  - name: Checkout source
>    uses: actions/checkout@v3
>  - name: Setup python
>    uses: actions/setup-python@v4
>    with:
>      python-version: ${{matrix.python_version}}
>      cache: 'pip'
>      check-latest: true
>  - name: Install required packages
>    run: sudo apt install -y gcc make libaio-dev libblkid-dev uuid-dev
>  - name: Install pip requirements
>    run: pip install -r requirements.txt
>  - name: Setup userstorage
>    run: userstorage create tests/storage.py
>  - name: Run tests
>    run: tox
>
> --
> 2.38.1

[PATCH 6/6] Add github actions based CI

[Albert Esteve]

Nevermind, I saw the other patch :)

On Mon, Dec 12, 2022 at 4:42 PM Albert Esteve  wrote:
> On Sun, Nov 27, 2022 at 6:39 PM Nir Soffer  wrote:
> > Add github workflow running on github for every push and pull requests.
> >
> > The CI run the tests with multiple python versions, ensuring that
> > sanlock builds and run on Ubuntu latest (20.04), and the python bindings
> > is compatible with python 3.6-3.10.
> >
> > The new CI revealed these issues:
> > Unneeded chown in lockfile() when not running as root
> > Unneeded chown in setup_listener()
> > test_write_lockspace_4k[user_4k_path0-1048576] is flaky in python 3.6
> > test_write_lockspace_4k[user_4k_path0-1048576] broken in python 3.9
> > test_write_resource_invalid_disk[disk5] broken in python 3.9, 3.10, 3.11
>
> Before adding the CI jobs they should be all green.
>
> What do you plan to do with this?
> Either issues are fixed in this (or separate) PR, or failing tests are set
> temporarily with xfail until they are fixed.
> > Example run:
> > https://github.com/nirs/sanlock/actions/runs/3559245867/
> > Signed-off-by: Nir Soffer
> >
> > .github/workflows/ci.yml | 48 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >  create mode 100644 .github/workflows/ci.yml
> >
> > diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
> > new file mode 100644
> > index 0000000..125eee6
> > --- /dev/null
> > +++ b/.github/workflows/ci.yml
> > @@ -0,0 +1,48 @@
> > +# Copyright (C) 2022 Red Hat, Inc.
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> >
> > +name: CI
> >
> > +on:
> > push
> > pull_request
> >
> > +jobs:
> > test:
> > runs-on: ubuntu-latest
> > strategy:
> >  fail-fast: false
> >  matrix:
> >    include:
> >      - python_version: '3.6'
> >      - python_version: '3.7'
> >      - python_version: '3.8'
> >      - python_version: '3.9'
> >      - python_version: '3.10'
> >      - python_version: '3.11'
> > steps:
> >  - name: Checkout source
> >    uses: actions/checkout@v3
> >  - name: Setup python
> >    uses: actions/setup-python@v4
> >    with:
> >      python-version: ${{matrix.python_version}}
> >      cache: 'pip'
> >      check-latest: true
> >  - name: Install required packages
> >    run: sudo apt install -y gcc make libaio-dev libblkid-dev
> >
> > uuid-dev
> >  - name: Install pip requirements
> >    run: pip install -r requirements.txt
> >  - name: Setup userstorage
> >    run: userstorage create tests/storage.py
> >  - name: Run tests
> >    run: tox
> >
> > --
> > 2.38.1