NACK: [SRU][L, K, J, F][M, U][PATCH 0/1] UBUNTU: [Packaging] Expose base (parent) abi for derivatives

Thadeu Lima de Souza Cascardo cascardo at canonical.com
Wed Jul 5 09:17:36 UTC 2023


On Wed, Jul 05, 2023 at 11:01:35AM +0200, Roxana Nicolescu wrote:
> On 06-06-2023 12:38, Thadeu Lima de Souza Cascardo wrote:
> 
> > On Tue, Jun 06, 2023 at 09:43:36AM +0200, Roxana Nicolescu wrote:
> > > BugLink: https://bugs.launchpad.net/bugs/2017006
> > > 
> > > SRU Justification
> > > 
> > > [ Impact ]
> > > 
> > > Upstream stable added a change in the format of jbd2 in 5.15.87 that
> > > was cherry-picked to kinetic:linux.
> > > This is incompatible with the current changes in the lttng-module
> > > kinetic.
> > > 
> > > We previously encountered this issue for focal #lp2004644
> > > and we proactively released a new version for kinetic, but the fix did
> > > not work as expected.
> > > 
> > > As shown in the buglink, the build error is triggered because some
> > > function declaration does not match the kernel's.
> > > In `include/instrumentation/events/jbd2.h` in lttng-modules, there is
> > > a conditional declaration based on the kernel version.
> > > But for 5.19 kernels, the upstream kernel version has remained the
> > > same.
> > > 
> > > [ Fix ]
> > > Versioning based on linux upstream version does not work in this case.
> > > We expose the abi version of the kernel, but it alone will not solve
> > > this issue because we deliver many derivatives, each with its own
> > > version. Hence a base main abi version needs to be exposed in the
> > > headers.
> > I don't like this approach. lttng-modules and every other out-of-tree module
> > out there are doing the wrong thing. I worked with openvswitch in the past and
> > they kept out-of-tree modules even though the module was already in the tree,
> > and they kept it updated with the changing kernel ABI. What they did is that
> > they had their build look into the kernel headers for the function signatures
> > that were relevant and created a config file stating which signature each
> > relevant function had and then checking for that in their source code.
> > 
> > A couple of years ago, we had the same issue with wireguard and we attempted a
> > similar solution. There was a bug where it was not looking into the right
> > headers, but aside from that, it worked just fine. I believe the same solution
> > should be looked after here, and be considered for upstream too.
> > 
> > Cascardo.
> Hi Thadeau,
> 
> I went ahead and suggested this change to lttng-modules upstream.
> 
> They think maintaining a configure script will require a lot of effort and
> it's not adding any value
> 
> to their existing way of keeping up with kernel changes. And I agree with
> them.

Their module cannot build on some kernels because they are testing for version
numbers. And here we are introducing another version number to actually mean
(this other function signature is actually the one from this other major
version number here).

And are they the ones who are going to maintain that, or are we going to add
that to our copy of lttng? Then, of course, the burden of maintainership is not
falling into them.

They don't want to migrate to a model that is so much better to maintain
because the technical debt is too much at this point.


> 
> Therefore I am kindly asking to approve my initial suggestion. I need this
> at least for kinetic only because
> 
> kinetic is gonna reach eol very soon and it would be nice to ship a working
> module before that.
> 

One more reason not to add technical debt on our side just to fix something
that has been broken for this long on a dead-end series. I would go even
further and ask that we revisit what has been done in Jammy.

Honestly, if we really care about lttng, we should work with upstream to
migrate everything to the openvswitch model.

Cascardo.

> Thanks,
> Roxana
> 
> > > The actual fix for lttng-modules requires 2 steps:
> > > 1. Expose UTS_UBUNTU_BASE_RELEASE_ABI variable equal with the parent
> > > abi in <generated/utsrelease.h>. For backports with multiple
> > > inheritance (like jammy:linux-aws-5.19) the version from debian.master
> > > (kinetic:linux) will be used. In case there is no inheritance, the
> > > actual kernel abi will be used. This is needed for jammy, kinetic and
> > > lunar but it may be needed in the future for similar use cases,
> > > so it makes sense to push this to all releases.
> > > This patch will cover the first part.
> > > 2. Use this variable in the lttng-module to construct
> > > LTTNG_UBUNTU_BASE_VERSION_CODE and then use it for versioning:
> > > LTTNG_UBUNTU_BASE_VERSION_CODE >= LTTNG_UBUNTU_KERNEL_VERSION(5,19,0,42)
> > > The lttng-module fix will be done only for lunar, kinetic, and jammy.
> > > 
> > > [ Where problems would occur ]
> > > This patch alone should not have a big impact on our kernels.
> > > It will only expose a variable in the headers.
> > > In terms of the lttng-modules solution:
> > > We may notice new failures in ubuntu_lttng_smoke_test once this
> > > compiles and run. But it was tested locally and the results were good,
> > > so the probability is very very low.
> > > 
> > > [ Test Plan ]
> > > Since we hit backward issues in the past, multiple kernel versions
> > > were tested out to make sure this is the proper fix.
> > > General test instructions:
> > > 1. Install kinetic (desired version)
> > > 2. sudo apt install lttng-modules-dkms(or dpkg -i)
> > > 3. Depending on the version it may work or not
> > > 4. Additionally lttng-modules regression test should be triggered to
> > > confirm everything works fine.
> > > 
> > > My testing:
> > > I build fixed lttng-modules with the fixes described in launchpad.
> > > A. Kinetic kernel where I expose UTS_UBUNTU_BASE_RELEASE_ABI.
> > > The module built and rts passed:
> > > 09:32:05 INFO | PASSED (simple system call tracing with babeltrace)
> > > 09:32:05 INFO | Summary: 7 passed, 0 failed
> > > B. The break commit was introduced in 5.19.0.42. Versions between 42
> > > and 44 (depending on where this fix will land) will break.
> > > C. Tested with a kernel version that does not have the break commit:
> > > 5.19.0-41 to see if it's backwards compatible. Results were good.
> > > D. Tested with jammy-aws-5.19 that has this patch. It works fine.
> > > E. Tested with jammy-aws-5.19.0-1024 that does not have the break
> > > commit to see backwards compatibility. It succeeded.
> > > 
> > > Roxana Nicolescu (1):
> > >    UBUNTU: [Packaging] Expose base (parent) abi for derivatives
> > > 
> > >   debian/rules.d/0-common-vars.mk | 9 +++++++++
> > >   debian/rules.d/2-binary-arch.mk | 4 ++++
> > >   2 files changed, 13 insertions(+)
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > > 
> > > -- 
> > > kernel-team mailing list
> > > kernel-team at lists.ubuntu.com
> > > https://lists.ubuntu.com/mailman/listinfo/kernel-team



More information about the kernel-team mailing list