Skip to content
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

Start adding a more useful python module around libpacemaker #3813

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,9 @@ AS_IF([test x"${PYTHON}" != x""], [AC_PATH_PROG([PYTHON], [$PYTHON])])

dnl Require a minimum Python version
AM_PATH_PYTHON([3.6])
PKG_CHECK_MODULES([PYTHON], [python])
AC_SUBST([PYTHON_LIBS])
AC_SUBST([PYTHON_CFLAGS])

AC_PROG_LN_S
AC_PROG_MKDIR_P
Expand Down
4 changes: 2 additions & 2 deletions mk/python.mk
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

.PHONY: pylint
pylint: $(PYCHECKFILES)
PYTHONPATH=$(abs_top_builddir)/python \
PYTHONPATH=$(abs_top_builddir)/python:$(abs_top_builddir)/python/pacemaker/.libs \
pylint --rcfile $(top_srcdir)/python/pylintrc $(PYCHECKFILES)

# Disabled warnings:
Expand All @@ -27,5 +27,5 @@ pylint: $(PYCHECKFILES)
# Disable docstrings warnings on unit tests.
.PHONY: pyflake
pyflake: $(PYCHECKFILES)
PYTHONPATH=$(abs_top_builddir)/python \
PYTHONPATH=$(abs_top_builddir)/python:$(abs_top_builddir)/python/pacemaker/.libs \
flake8 --ignore=W503,E402,E501,F401 --per-file-ignores="tests/*:D100,D101,D102,D104" $(PYCHECKFILES)
5 changes: 3 additions & 2 deletions python/Makefile.am
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#
# Copyright 2023-2024 the Pacemaker project contributors
# Copyright 2023-2025 the Pacemaker project contributors
#
# The version control history for this file may have further details.
#
Expand All @@ -22,4 +22,5 @@ check-local:
if [ "x$(top_srcdir)" != "x$(top_builddir)" ]; then \
cp -r $(top_srcdir)/python/* $(abs_top_builddir)/python/; \
fi
PYTHONPATH=$(top_builddir)/python $(PYTHON) -m unittest discover -v -s $(top_builddir)/python/tests
PYTHONPATH=$(top_builddir)/python:$(top_builddir)/python/pacemaker/.libs \
$(PYTHON) -m unittest discover -v -s $(top_builddir)/python/tests
13 changes: 11 additions & 2 deletions python/pacemaker/Makefile.am
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#
# Copyright 2023-2024 the Pacemaker project contributors
# Copyright 2023-2025 the Pacemaker project contributors
#
# The version control history for this file may have further details.
#
Expand All @@ -9,9 +9,18 @@

include $(top_srcdir)/mk/common.mk

pyexec_LTLIBRARIES = _pcmksupport.la

_pcmksupport_la_SOURCES = pcmksupport.c
_pcmksupport_la_CPPFLAGS = $(PYTHON_CFLAGS)
_pcmksupport_la_LDFLAGS = $(AM_LDFLAGS) -module -avoid-version -export-symbols-regex PyInit__pcmksupport
_pcmksupport_la_LIBADD = $(top_builddir)/lib/pacemaker/libpacemaker.la

pkgpython_PYTHON = __init__.py \
_library.py \
exitstatus.py
exceptions.py \
exitstatus.py \
resource.py

nodist_pkgpython_PYTHON = buildoptions.py

Expand Down
6 changes: 4 additions & 2 deletions python/pacemaker/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,7 @@
__copyright__ = "Copyright 2023-2024 the Pacemaker project contributors"
__license__ = "GNU Lesser General Public License version 2.1 or later (LGPLv2.1+)"

from pacemaker.buildoptions import BuildOptions
from pacemaker.exitstatus import ExitStatus
from .buildoptions import BuildOptions
from .exitstatus import ExitStatus
from . import exceptions
from . import resource
9 changes: 9 additions & 0 deletions python/pacemaker/exceptions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
"""A module providing exceptions that can be raised by the Pacemaker module."""

__all__ = ["PacemakerError"]
__copyright__ = "Copyright 2025 the Pacemaker project contributors"
__license__ = "GNU Lesser General Public License version 2.1 or later (LGPLv2.1+)"


class PacemakerError(Exception):
"""Base exception class for all Pacemaker errors."""
82 changes: 82 additions & 0 deletions python/pacemaker/pcmksupport.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
* Copyright 2025 the Pacemaker project contributors
*
* The version control history for this file may have further details.
*
* This source code is licensed under the GNU Lesser General Public License
* version 2.1 or later (LGPLv2.1+) WITHOUT ANY WARRANTY.
*/

#include <Python.h>
#include <libxml/tree.h>

#include <pacemaker.h>

/* This file defines a c-based low level module that wraps libpacemaker
* functions and returns python objects. This is necessary because most
* libpacemaker functions return an xmlNode **, which needs to be coerced
* through the PyCapsule type into something that libxml2's python
* bindings can work with.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage (or necessity) of using PyCapsule, etc., rather than ctypes or cffi for our use case?

I have not created Python bindings myself. Before I do any more research, I'm wondering what you've found on this topic and what led you to PyCapsule.

Note from the Python documentation (first URL):

The C extension interface is specific to CPython, and extension modules do not work on other Python implementations. In many cases, it is possible to avoid writing C extensions and preserve portability to other implementations. For example, if your use case is calling C library functions or system calls, you should consider using the ctypes module or the cffi library rather than writing custom C code. These modules let you write Python code to interface with C code and are more portable between implementations of Python than writing and compiling a C extension module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, using PyCapsule is the only way I've been able to make anything that doesn't just segfault.

Here's what I want:

  • We call a libpacemaker function - say, pcmk_list_standards. That gives us a return code and an xmlNodePtr *.
  • I don't want our python bindings to give the user XML anything. I want the bindings to return python types. So, I need to convert that XML into python.
  • The easiest way of doing that is by using libxml2, which conveniently also has python bindings. That means we can write the type conversion crud in python instead of C, which sounds a lot better considering we want the output to be python.

Basically, I want to take something that was allocated and generated by libxml2 in C (the xmlNodePtr *) and pass it through our own python code and libxml2's python bindings, back into libxml2 in C during the process of converting the types. There's something about this process that just segfaults unless I use the PyCapsule type. Almost everything else in this PR is just boilerplate necessary to be able to use that type.

My first approach was to use ctypes just like we are doing for ExitStatus, but I couldn't get any result besides segfaults there. I think what we'd need to do is define a structure class that mirrors an xmlNode, which seems like way too much work. That's why I turned to this approach. I haven't tried the ffi module yet.

Honestly, I would love to nuke the entire C module portion of this and do everything in python with ctypes/ffi. I just can't figure it out yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance:

clumens@spire ~/src/pacemaker main|✚1❯ git diff
diff --git a/python/pacemaker/_library.py b/python/pacemaker/_library.py
index 1cd1bd500d..fe8d83fa25 100644
--- a/python/pacemaker/_library.py
+++ b/python/pacemaker/_library.py
@@ -1,6 +1,6 @@
 """A module providing private library management code."""
 
-__all__ = ["_libcrmcommon"]
+__all__ = ["_libcrmcommon", "_libpacemker"]
 __copyright__ = "Copyright 2024-2025 the Pacemaker project contributors"
 __license__ = "GNU Lesser General Public License version 2.1 or later (LGPLv2.1+)"
 
@@ -36,3 +36,7 @@ def load_library(basename):
 
 _libcrmcommon = load_library("crmcommon")
 _libcrmcommon.crm_exit_str.restype = ctypes.c_char_p
+
+_libpacemaker = load_library("pacemaker")
+_libpacemaker.pcmk_list_standards.restype = ctypes.c_int
+_libpacemaker.pcmk_list_standards.argtypes = [ ctypes.POINTER(ctypes.c_void_p) ]
clumens@spire ~/src/pacemaker main|✚1❯ PYTHONPATH=python LD_LIBRARY_PATH=lib/common/.libs python
Python 3.12.7 (main, Oct  1 2024, 00:00:00) [GCC 14.2.1 20240912 (Red Hat 14.2.1-3)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pacemaker
>>> import ctypes
>>> import libxml2
>>> xxx = ctypes.pointer(ctypes.c_void_p())
>>> pacemaker._library._libpacemaker.pcmk_list_standards(xxx)
0
>>> libxml2.xmlDoc(xxx)
<xmlDoc (None) object at 0x7febd4df11c0>
>>> libxml2.xmlDoc(xxx).xpathEval("/pacemaker-result")
zsh: segmentation fault  PYTHONPATH=python LD_LIBRARY_PATH=lib/common/.libs python

It feels really close - in particular, I don't think the void pointer type is correct here. But I'm not sure how to hand it an xmlNodePtr. ctypes really only knows about basic types. Beyond that, you're supposed to define classes that derive from Structure. But, an xmlNode comes from libxml2 so I don't want to define a class for that and I don't see anywhere that they define one.

*/

/* Base exception class for any errors in the _pcmksupport module */
static PyObject *PacemakerError;

PyMODINIT_FUNC PyInit__pcmksupport(void);

static PyObject *
py_list_standards(PyObject *self, PyObject *args)
{
int rc;
xmlNodePtr xml = NULL;

if (!PyArg_ParseTuple(args, "")) {
return NULL;
}

rc = pcmk_list_standards(&xml);
if (rc != pcmk_rc_ok) {
PyErr_SetString(PacemakerError, pcmk_rc_str(rc));
return NULL;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure what to do here - on the one hand, pcmk_* functions return a standard Pacemaker return code, so that seems like what we should raise. However, we don't expose those return codes in the python module at the moment (which... maybe we should do that?). On the other hand, the returned XML already contains the exit code. However, that's the kind of value a process should return, not a function.

}

return PyCapsule_New(xml, "xmlNodePtr", NULL);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the commit message, I think these functions could almost certainly be auto-generated by a quick script based on a single line description of their name, argument types, and return value.

static PyMethodDef pcmksupportMethods[] = {
{ "list_standards", py_list_standards, METH_VARARGS, NULL },
{ NULL, NULL, 0, NULL }
};

static struct PyModuleDef moduledef = {
PyModuleDef_HEAD_INIT,
"_pcmksupport",
NULL,
-1,
pcmksupportMethods,
NULL,
NULL,
NULL,
NULL
};

PyMODINIT_FUNC
PyInit__pcmksupport(void)
{
PyObject *module = PyModule_Create(&moduledef);

if (module == NULL) {
return NULL;
}

/* Add the base exception to the module */
PacemakerError = PyErr_NewException("_pcmksupport.PacemakerError", NULL, NULL);

/* FIXME: When we can support Python >= 3.10, we can use PyModule_AddObjectRef */
if (PyModule_AddObject(module, "PacemakerError", PacemakerError) < 0) {
Py_XDECREF(PacemakerError);
return NULL;
}

return module;
}
22 changes: 22 additions & 0 deletions python/pacemaker/resource.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
"""A module for managing cluster resources."""

__all__ = ["list_standards"]
__copyright__ = "Copyright 2025 the Pacemaker project contributors"
__license__ = "GNU Lesser General Public License version 2.1 or later (LGPLv2.1+)"

import libxml2

import _pcmksupport
from pacemaker.exceptions import PacemakerError


def list_standards():
"""Return a list of supported resource standards."""
try:
xml = _pcmksupport.list_standards()
except _pcmksupport.PacemakerError as e:
raise PacemakerError(*e.args) from None

doc = libxml2.xmlDoc(xml)

return [item.getContent() for item in doc.xpathEval("/pacemaker-result/standards/item")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention here (and in any further functions) would be to convert the returned XML into a python native type, like a list of strings here. That's what I'm going through so much trouble everywhere to deal with the XML types.

2 changes: 1 addition & 1 deletion python/pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ unsafe-load-any-extension=no
# A comma-separated list of package or module names from where C extensions may
# be loaded. Extensions are loading into the active Python interpreter and may
# run arbitrary code
extension-pkg-allow-list=
extension-pkg-allow-list=_pcmksupport

# Minimum supported python version
# CHANGED
Expand Down
8 changes: 5 additions & 3 deletions rpm/pacemaker.spec.in
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ Requires: %{python_name}-%{name} = %{version}-%{release}
Requires: %{python_path}
BuildRequires: %{python_name}-devel
BuildRequires: %{python_name}-setuptools
BuildRequires: %{python_name}-libxml2

# Pacemaker requires a minimum libqb functionality
Requires: libqb >= 1.0.1
Expand Down Expand Up @@ -373,7 +374,6 @@ License: LGPL-2.1-or-later
Summary: Python libraries for Pacemaker
Requires: %{python_path}
Requires: %{pkgname_pcmk_libs} = %{version}-%{release}
BuildArch: noarch

%description -n %{python_name}-%{name}
Pacemaker is an advanced, scalable High-Availability cluster resource
Expand Down Expand Up @@ -516,8 +516,9 @@ popd

%check
make %{_smp_mflags} check
{ cts/cts-scheduler --run load-stopped-loop \
&& cts/cts-cli -V \
{ PYTHONPATH=python/pacemaker/.libs \
cts/cts-scheduler --run load-stopped-loop \
&& PYTHONPATH=python/pacemaker/.libs cts/cts-cli -V \
&& touch .CHECKED
} 2>&1 | sed 's/[fF]ail/faiil/g' # prevent false positives in rpmlint
[ -f .CHECKED ] && rm -f -- .CHECKED
Expand Down Expand Up @@ -747,6 +748,7 @@ exit 0
%doc ChangeLog.md

%files -n %{python_name}-%{name}
%{python3_sitearch}/_pcmksupport.so
%{python3_sitelib}/pacemaker/
%{python3_sitelib}/pacemaker-*.egg-info
%exclude %{python3_sitelib}/pacemaker/_cts/
Expand Down