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

[Roxana Nicolescu]

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.

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.

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