* [PATCH v5 1/5] prserv: declare "max_package_pr" client hook
2024-04-29 20:14 [PATCH v5 0/5] prserv: add support for an "upstream" server michael.opdenacker
@ 2024-04-29 20:14 ` michael.opdenacker
2024-04-29 20:14 ` [PATCH v5 2/5] prserv: move code from __init__ to bitbake-prserv michael.opdenacker
` (3 subsequent siblings)
4 siblings, 0 replies; 20+ messages in thread
From: michael.opdenacker @ 2024-04-29 20:14 UTC (permalink / raw)
To: bitbake-devel
Cc: Michael Opdenacker, Joshua Watt, Tim Orling, Thomas Petazzoni
From: Michael Opdenacker <michael.opdenacker@bootlin.com>
Add missing declaration for the max_package_pr client hook
Signed-off-by: Michael Opdenacker <michael.opdenacker@bootlin.com>
Cc: Joshua Watt <JPEWhacker@gmail.com>
Cc: Tim Orling <ticotimo@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
lib/prserv/client.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/prserv/client.py b/lib/prserv/client.py
index 8471ee3046..99fc4e0f7f 100644
--- a/lib/prserv/client.py
+++ b/lib/prserv/client.py
@@ -65,7 +65,7 @@ class PRAsyncClient(bb.asyncrpc.AsyncClient):
class PRClient(bb.asyncrpc.Client):
def __init__(self):
super().__init__()
- self._add_methods("getPR", "test_pr", "test_package", "importone", "export", "is_readonly")
+ self._add_methods("getPR", "test_pr", "test_package", "max_package_pr", "importone", "export", "is_readonly")
def _get_async_client(self):
return PRAsyncClient()
--
2.34.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v5 2/5] prserv: move code from __init__ to bitbake-prserv
2024-04-29 20:14 [PATCH v5 0/5] prserv: add support for an "upstream" server michael.opdenacker
2024-04-29 20:14 ` [PATCH v5 1/5] prserv: declare "max_package_pr" client hook michael.opdenacker
@ 2024-04-29 20:14 ` michael.opdenacker
2024-05-02 16:57 ` [bitbake-devel] " Khem Raj
2024-04-29 20:14 ` [PATCH v5 3/5] prserv: add "upstream" server support michael.opdenacker
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: michael.opdenacker @ 2024-04-29 20:14 UTC (permalink / raw)
To: bitbake-devel
Cc: Michael Opdenacker, Joshua Watt, Tim Orling, Thomas Petazzoni
From: Michael Opdenacker <michael.opdenacker@bootlin.com>
This script was the only user of this code.
Signed-off-by: Michael Opdenacker <michael.opdenacker@bootlin.com>
Cc: Joshua Watt <JPEWhacker@gmail.com>
Cc: Tim Orling <ticotimo@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
bin/bitbake-prserv | 9 ++++++++-
lib/prserv/__init__.py | 13 -------------
2 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/bin/bitbake-prserv b/bin/bitbake-prserv
index ad0a069401..920663a1d8 100755
--- a/bin/bitbake-prserv
+++ b/bin/bitbake-prserv
@@ -21,6 +21,13 @@ VERSION = "1.1.0"
PRHOST_DEFAULT="0.0.0.0"
PRPORT_DEFAULT=8585
+def init_logger(logfile, loglevel):
+ numeric_level = getattr(logging, loglevel.upper(), None)
+ if not isinstance(numeric_level, int):
+ raise ValueError("Invalid log level: %s" % loglevel)
+ FORMAT = "%(asctime)-15s %(message)s"
+ logging.basicConfig(level=numeric_level, filename=logfile, format=FORMAT)
+
def main():
parser = argparse.ArgumentParser(
description="BitBake PR Server. Version=%s" % VERSION,
@@ -72,7 +79,7 @@ def main():
)
args = parser.parse_args()
- prserv.init_logger(os.path.abspath(args.log), args.loglevel)
+ init_logger(os.path.abspath(args.log), args.loglevel)
if args.start:
ret=prserv.serv.start_daemon(args.file, args.host, args.port, os.path.abspath(args.log), args.read_only)
diff --git a/lib/prserv/__init__.py b/lib/prserv/__init__.py
index 0e0aa34d0e..94658b815d 100644
--- a/lib/prserv/__init__.py
+++ b/lib/prserv/__init__.py
@@ -5,16 +5,3 @@
#
__version__ = "1.0.0"
-
-import os, time
-import sys, logging
-
-def init_logger(logfile, loglevel):
- numeric_level = getattr(logging, loglevel.upper(), None)
- if not isinstance(numeric_level, int):
- raise ValueError("Invalid log level: %s" % loglevel)
- FORMAT = "%(asctime)-15s %(message)s"
- logging.basicConfig(level=numeric_level, filename=logfile, format=FORMAT)
-
-class NotFoundError(Exception):
- pass
--
2.34.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [bitbake-devel] [PATCH v5 2/5] prserv: move code from __init__ to bitbake-prserv
2024-04-29 20:14 ` [PATCH v5 2/5] prserv: move code from __init__ to bitbake-prserv michael.opdenacker
@ 2024-05-02 16:57 ` Khem Raj
2024-05-03 8:04 ` Michael Opdenacker
0 siblings, 1 reply; 20+ messages in thread
From: Khem Raj @ 2024-05-02 16:57 UTC (permalink / raw)
To: michael.opdenacker
Cc: bitbake-devel, Joshua Watt, Tim Orling, Thomas Petazzoni
Hi Michael
I am seeing a problem with this series, specifically following
patchset is causing a regression
* ea1d7b8a36e bitbake: prserv: add bitbake selftests
* 5c66d345300 bitbake: prserv: import simplification
* c23f2c1680f bitbake: prserv: store_value() improvements
* 395b321af29 bitbake: prserv: avoid possible race condition in database code
* 8022dc1029d bitbake: prserv: enable database sharing
* b81bfdfbed8 bitbake: prserv: add "upstream" server support
* 9539ce3b14a bitbake: prserv: move code from __init__ to bitbake-prserv
* 4ec1553710b bitbake: prserv: declare "max_package_pr" client hook
error seen during packagedata task of base-files
WARNING: base-files-3.0.14-r0 do_packagedata: Error talking to server:
Connection closed
ERROR: Error from client ('127.0.0.1', 59846): 'PRTable' object has no
attribute 'sync'
Traceback (most recent call last):
File "/mnt/b/yoe/master/sources/poky/bitbake/lib/bb/asyncrpc/serv.py",
line 278, in PRServer._client_handler(socket=<bb.asyncrpc.connection.StreamConnection
object at 0x794b1e009110>):
client = self.accept_client(socket)
> await client.process_requests()
except Exception as e:
File "/mnt/b/yoe/master/sources/poky/bitbake/lib/prserv/serv.py",
line 171, in PRServerClient.process_requests():
try:
> await super().process_requests()
finally:
File "/mnt/b/yoe/master/sources/poky/bitbake/lib/bb/asyncrpc/serv.py",
line 92, in PRServerClient.process_requests():
try:
> response = await self.dispatch_message(d)
except InvokeError as e:
File "/mnt/b/yoe/master/sources/poky/bitbake/lib/prserv/serv.py",
line 45, in PRServerClient.dispatch_message(msg={'get-pr': {'version':
'base-files-3.0.14-r0', 'pkgarch': 'imx8qm_var_som', 'checksum':
'0d287eea3da9a2573dfd3fbc709ef8697f2553563ff1211efc733f215d510663',
'history': False}}):
except:
> self.server.table.sync()
raise
AttributeError: 'PRTable' object has no attribute 'sync'
To reproduce this you will need to add a bbappend for base-files from
https://github.com/varigit/meta-variscite-bsp-imx/blob/hardknott/recipes-core/base-files/base-files_%25.bbappend
You can make the overrides match the machine you might be building if
not one of variscite machine.
Thanks
-Khem
On Mon, Apr 29, 2024 at 1:14 PM Michael Opdenacker via
lists.openembedded.org
<michael.opdenacker=bootlin.com@lists.openembedded.org> wrote:
>
> From: Michael Opdenacker <michael.opdenacker@bootlin.com>
>
> This script was the only user of this code.
>
> Signed-off-by: Michael Opdenacker <michael.opdenacker@bootlin.com>
> Cc: Joshua Watt <JPEWhacker@gmail.com>
> Cc: Tim Orling <ticotimo@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
> bin/bitbake-prserv | 9 ++++++++-
> lib/prserv/__init__.py | 13 -------------
> 2 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/bin/bitbake-prserv b/bin/bitbake-prserv
> index ad0a069401..920663a1d8 100755
> --- a/bin/bitbake-prserv
> +++ b/bin/bitbake-prserv
> @@ -21,6 +21,13 @@ VERSION = "1.1.0"
> PRHOST_DEFAULT="0.0.0.0"
> PRPORT_DEFAULT=8585
>
> +def init_logger(logfile, loglevel):
> + numeric_level = getattr(logging, loglevel.upper(), None)
> + if not isinstance(numeric_level, int):
> + raise ValueError("Invalid log level: %s" % loglevel)
> + FORMAT = "%(asctime)-15s %(message)s"
> + logging.basicConfig(level=numeric_level, filename=logfile, format=FORMAT)
> +
> def main():
> parser = argparse.ArgumentParser(
> description="BitBake PR Server. Version=%s" % VERSION,
> @@ -72,7 +79,7 @@ def main():
> )
>
> args = parser.parse_args()
> - prserv.init_logger(os.path.abspath(args.log), args.loglevel)
> + init_logger(os.path.abspath(args.log), args.loglevel)
>
> if args.start:
> ret=prserv.serv.start_daemon(args.file, args.host, args.port, os.path.abspath(args.log), args.read_only)
> diff --git a/lib/prserv/__init__.py b/lib/prserv/__init__.py
> index 0e0aa34d0e..94658b815d 100644
> --- a/lib/prserv/__init__.py
> +++ b/lib/prserv/__init__.py
> @@ -5,16 +5,3 @@
> #
>
> __version__ = "1.0.0"
> -
> -import os, time
> -import sys, logging
> -
> -def init_logger(logfile, loglevel):
> - numeric_level = getattr(logging, loglevel.upper(), None)
> - if not isinstance(numeric_level, int):
> - raise ValueError("Invalid log level: %s" % loglevel)
> - FORMAT = "%(asctime)-15s %(message)s"
> - logging.basicConfig(level=numeric_level, filename=logfile, format=FORMAT)
> -
> -class NotFoundError(Exception):
> - pass
> --
> 2.34.1
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#16151): https://lists.openembedded.org/g/bitbake-devel/message/16151
> Mute This Topic: https://lists.openembedded.org/mt/105808694/1997914
> Group Owner: bitbake-devel+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [raj.khem@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [bitbake-devel] [PATCH v5 2/5] prserv: move code from __init__ to bitbake-prserv
2024-05-02 16:57 ` [bitbake-devel] " Khem Raj
@ 2024-05-03 8:04 ` Michael Opdenacker
2024-05-03 14:14 ` Khem Raj
0 siblings, 1 reply; 20+ messages in thread
From: Michael Opdenacker @ 2024-05-03 8:04 UTC (permalink / raw)
To: Khem Raj
Cc: michael.opdenacker, bitbake-devel, Joshua Watt, Tim Orling,
Thomas Petazzoni
Hi Khem
On 5/2/24 at 18:57, Khem Raj wrote:
> Hi Michael
>
> I am seeing a problem with this series, specifically following
> patchset is causing a regression
>
> * ea1d7b8a36e bitbake: prserv: add bitbake selftests
> * 5c66d345300 bitbake: prserv: import simplification
> * c23f2c1680f bitbake: prserv: store_value() improvements
> * 395b321af29 bitbake: prserv: avoid possible race condition in database code
> * 8022dc1029d bitbake: prserv: enable database sharing
> * b81bfdfbed8 bitbake: prserv: add "upstream" server support
> * 9539ce3b14a bitbake: prserv: move code from __init__ to bitbake-prserv
> * 4ec1553710b bitbake: prserv: declare "max_package_pr" client hook
>
>
> error seen during packagedata task of base-files
>
> WARNING: base-files-3.0.14-r0 do_packagedata: Error talking to server:
> Connection closed
> ERROR: Error from client ('127.0.0.1', 59846): 'PRTable' object has no
> attribute 'sync'
> Traceback (most recent call last):
Thanks for the review. Actually, did you test with the V6 update I sent?
Cheers
Michael.
--
Michael Opdenacker, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [bitbake-devel] [PATCH v5 2/5] prserv: move code from __init__ to bitbake-prserv
2024-05-03 8:04 ` Michael Opdenacker
@ 2024-05-03 14:14 ` Khem Raj
2024-05-08 13:35 ` Richard Purdie
0 siblings, 1 reply; 20+ messages in thread
From: Khem Raj @ 2024-05-03 14:14 UTC (permalink / raw)
To: Michael Opdenacker
Cc: Joshua Watt, Thomas Petazzoni, Tim Orling, bitbake-devel
[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]
On Fri, May 3, 2024 at 1:04 AM Michael Opdenacker <
michael.opdenacker@bootlin.com> wrote:
> Hi Khem
>
> On 5/2/24 at 18:57, Khem Raj wrote:
> > Hi Michael
> >
> > I am seeing a problem with this series, specifically following
> > patchset is causing a regression
> >
> > * ea1d7b8a36e bitbake: prserv: add bitbake selftests
> > * 5c66d345300 bitbake: prserv: import simplification
> > * c23f2c1680f bitbake: prserv: store_value() improvements
> > * 395b321af29 bitbake: prserv: avoid possible race condition in database
> code
> > * 8022dc1029d bitbake: prserv: enable database sharing
> > * b81bfdfbed8 bitbake: prserv: add "upstream" server support
> > * 9539ce3b14a bitbake: prserv: move code from __init__ to bitbake-prserv
> > * 4ec1553710b bitbake: prserv: declare "max_package_pr" client hook
> >
> >
> > error seen during packagedata task of base-files
> >
> > WARNING: base-files-3.0.14-r0 do_packagedata: Error talking to server:
> > Connection closed
> > ERROR: Error from client ('127.0.0.1', 59846): 'PRTable' object has no
> > attribute 'sync'
> > Traceback (most recent call last):
>
>
> Thanks for the review. Actually, did you test with the V6 update I sent?
I think it was version in master-next that Richard has staged
>
> Cheers
> Michael.
>
> --
> Michael Opdenacker, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
>
[-- Attachment #2: Type: text/html, Size: 2113 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [bitbake-devel] [PATCH v5 2/5] prserv: move code from __init__ to bitbake-prserv
2024-05-03 14:14 ` Khem Raj
@ 2024-05-08 13:35 ` Richard Purdie
2024-05-08 15:13 ` Khem Raj
0 siblings, 1 reply; 20+ messages in thread
From: Richard Purdie @ 2024-05-08 13:35 UTC (permalink / raw)
To: raj.khem, Michael Opdenacker
Cc: Joshua Watt, Thomas Petazzoni, Tim Orling, bitbake-devel
On Fri, 2024-05-03 at 07:14 -0700, Khem Raj via lists.openembedded.org
wrote:
>
>
> On Fri, May 3, 2024 at 1:04 AM Michael Opdenacker
> <michael.opdenacker@bootlin.com> wrote:
> > Hi Khem
> >
> > On 5/2/24 at 18:57, Khem Raj wrote:
> > > Hi Michael
> > >
> > > I am seeing a problem with this series, specifically following
> > > patchset is causing a regression
> > >
> > > * ea1d7b8a36e bitbake: prserv: add bitbake selftests
> > > * 5c66d345300 bitbake: prserv: import simplification
> > > * c23f2c1680f bitbake: prserv: store_value() improvements
> > > * 395b321af29 bitbake: prserv: avoid possible race condition in
> > > database code
> > > * 8022dc1029d bitbake: prserv: enable database sharing
> > > * b81bfdfbed8 bitbake: prserv: add "upstream" server support
> > > * 9539ce3b14a bitbake: prserv: move code from __init__ to
> > > bitbake-prserv
> > > * 4ec1553710b bitbake: prserv: declare "max_package_pr" client
> > > hook
> > >
> > >
> > > error seen during packagedata task of base-files
> > >
> > > WARNING: base-files-3.0.14-r0 do_packagedata: Error talking to
> > > server:
> > > Connection closed
> > > ERROR: Error from client ('127.0.0.1', 59846): 'PRTable' object
> > > has no
> > > attribute 'sync'
> > > Traceback (most recent call last):
> >
> >
> > Thanks for the review. Actually, did you test with the V6 update I
> > sent?
> >
>
>
> I think it was version in master-next that Richard has staged
>
It was against v6 so I'm not able to merge these until that is fixed.
Cheers,
Richard
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [bitbake-devel] [PATCH v5 2/5] prserv: move code from __init__ to bitbake-prserv
2024-05-08 13:35 ` Richard Purdie
@ 2024-05-08 15:13 ` Khem Raj
2024-05-08 17:40 ` Michael Opdenacker
0 siblings, 1 reply; 20+ messages in thread
From: Khem Raj @ 2024-05-08 15:13 UTC (permalink / raw)
To: Richard Purdie
Cc: Michael Opdenacker, Joshua Watt, Thomas Petazzoni, Tim Orling,
bitbake-devel
On Wed, May 8, 2024 at 6:35 AM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Fri, 2024-05-03 at 07:14 -0700, Khem Raj via lists.openembedded.org
> wrote:
> >
> >
> > On Fri, May 3, 2024 at 1:04 AM Michael Opdenacker
> > <michael.opdenacker@bootlin.com> wrote:
> > > Hi Khem
> > >
> > > On 5/2/24 at 18:57, Khem Raj wrote:
> > > > Hi Michael
> > > >
> > > > I am seeing a problem with this series, specifically following
> > > > patchset is causing a regression
> > > >
> > > > * ea1d7b8a36e bitbake: prserv: add bitbake selftests
> > > > * 5c66d345300 bitbake: prserv: import simplification
> > > > * c23f2c1680f bitbake: prserv: store_value() improvements
> > > > * 395b321af29 bitbake: prserv: avoid possible race condition in
> > > > database code
> > > > * 8022dc1029d bitbake: prserv: enable database sharing
> > > > * b81bfdfbed8 bitbake: prserv: add "upstream" server support
> > > > * 9539ce3b14a bitbake: prserv: move code from __init__ to
> > > > bitbake-prserv
> > > > * 4ec1553710b bitbake: prserv: declare "max_package_pr" client
> > > > hook
> > > >
> > > >
> > > > error seen during packagedata task of base-files
> > > >
> > > > WARNING: base-files-3.0.14-r0 do_packagedata: Error talking to
> > > > server:
> > > > Connection closed
> > > > ERROR: Error from client ('127.0.0.1', 59846): 'PRTable' object
> > > > has no
> > > > attribute 'sync'
> > > > Traceback (most recent call last):
> > >
> > >
> > > Thanks for the review. Actually, did you test with the V6 update I
> > > sent?
> > >
> >
> >
> > I think it was version in master-next that Richard has staged
> >
>
> It was against v6 so I'm not able to merge these until that is fixed.
>
Right, I will try a clean from scratch build and see if it was something
intermediate to be sure.
> Cheers,
>
> Richard
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [bitbake-devel] [PATCH v5 2/5] prserv: move code from __init__ to bitbake-prserv
2024-05-08 15:13 ` Khem Raj
@ 2024-05-08 17:40 ` Michael Opdenacker
2024-05-14 17:03 ` Khem Raj
0 siblings, 1 reply; 20+ messages in thread
From: Michael Opdenacker @ 2024-05-08 17:40 UTC (permalink / raw)
To: Khem Raj, Richard Purdie
Cc: michael.opdenacker, Joshua Watt, Thomas Petazzoni, Tim Orling,
bitbake-devel
Richard, Khem,
On 5/8/24 at 20:43, Khem Raj wrote:
> On Wed, May 8, 2024 at 6:35 AM Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
>> On Fri, 2024-05-03 at 07:14 -0700, Khem Raj via lists.openembedded.org
>> wrote:
>>>
>>> On Fri, May 3, 2024 at 1:04 AM Michael Opdenacker
>>> <michael.opdenacker@bootlin.com> wrote:
>>>> Hi Khem
>>>>
>>>> On 5/2/24 at 18:57, Khem Raj wrote:
>>>>> Hi Michael
>>>>>
>>>>> I am seeing a problem with this series, specifically following
>>>>> patchset is causing a regression
>>>>>
>>>>> * ea1d7b8a36e bitbake: prserv: add bitbake selftests
>>>>> * 5c66d345300 bitbake: prserv: import simplification
>>>>> * c23f2c1680f bitbake: prserv: store_value() improvements
>>>>> * 395b321af29 bitbake: prserv: avoid possible race condition in
>>>>> database code
>>>>> * 8022dc1029d bitbake: prserv: enable database sharing
>>>>> * b81bfdfbed8 bitbake: prserv: add "upstream" server support
>>>>> * 9539ce3b14a bitbake: prserv: move code from __init__ to
>>>>> bitbake-prserv
>>>>> * 4ec1553710b bitbake: prserv: declare "max_package_pr" client
>>>>> hook
>>>>>
>>>>>
>>>>> error seen during packagedata task of base-files
>>>>>
>>>>> WARNING: base-files-3.0.14-r0 do_packagedata: Error talking to
>>>>> server:
>>>>> Connection closed
>>>>> ERROR: Error from client ('127.0.0.1', 59846): 'PRTable' object
>>>>> has no
>>>>> attribute 'sync'
>>>>> Traceback (most recent call last):
>>>>
>>>> Thanks for the review. Actually, did you test with the V6 update I
>>>> sent?
>>>>
>>>
>>> I think it was version in master-next that Richard has staged
>>>
>> It was against v6 so I'm not able to merge these until that is fixed.
>>
> Right, I will try a clean from scratch build and see if it was something
> intermediate to be sure.
Thanks for testing.
I confirm the issue is still there. My apologies for this overlook!
This is a busy week for me, but I will submit a fix as soon as I get the
time, and before the end of the week anyway.
Kind regards
Michael.
--
Michael Opdenacker, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [bitbake-devel] [PATCH v5 2/5] prserv: move code from __init__ to bitbake-prserv
2024-05-08 17:40 ` Michael Opdenacker
@ 2024-05-14 17:03 ` Khem Raj
0 siblings, 0 replies; 20+ messages in thread
From: Khem Raj @ 2024-05-14 17:03 UTC (permalink / raw)
To: Michael Opdenacker
Cc: Richard Purdie, Joshua Watt, Thomas Petazzoni, Tim Orling,
bitbake-devel
On Wed, May 8, 2024 at 10:41 AM Michael Opdenacker
<michael.opdenacker@bootlin.com> wrote:
>
> Richard, Khem,
>
> On 5/8/24 at 20:43, Khem Raj wrote:
> > On Wed, May 8, 2024 at 6:35 AM Richard Purdie
> > <richard.purdie@linuxfoundation.org> wrote:
> >> On Fri, 2024-05-03 at 07:14 -0700, Khem Raj via lists.openembedded.org
> >> wrote:
> >>>
> >>> On Fri, May 3, 2024 at 1:04 AM Michael Opdenacker
> >>> <michael.opdenacker@bootlin.com> wrote:
> >>>> Hi Khem
> >>>>
> >>>> On 5/2/24 at 18:57, Khem Raj wrote:
> >>>>> Hi Michael
> >>>>>
> >>>>> I am seeing a problem with this series, specifically following
> >>>>> patchset is causing a regression
> >>>>>
> >>>>> * ea1d7b8a36e bitbake: prserv: add bitbake selftests
> >>>>> * 5c66d345300 bitbake: prserv: import simplification
> >>>>> * c23f2c1680f bitbake: prserv: store_value() improvements
> >>>>> * 395b321af29 bitbake: prserv: avoid possible race condition in
> >>>>> database code
> >>>>> * 8022dc1029d bitbake: prserv: enable database sharing
> >>>>> * b81bfdfbed8 bitbake: prserv: add "upstream" server support
> >>>>> * 9539ce3b14a bitbake: prserv: move code from __init__ to
> >>>>> bitbake-prserv
> >>>>> * 4ec1553710b bitbake: prserv: declare "max_package_pr" client
> >>>>> hook
> >>>>>
> >>>>>
> >>>>> error seen during packagedata task of base-files
> >>>>>
> >>>>> WARNING: base-files-3.0.14-r0 do_packagedata: Error talking to
> >>>>> server:
> >>>>> Connection closed
> >>>>> ERROR: Error from client ('127.0.0.1', 59846): 'PRTable' object
> >>>>> has no
> >>>>> attribute 'sync'
> >>>>> Traceback (most recent call last):
> >>>>
> >>>> Thanks for the review. Actually, did you test with the V6 update I
> >>>> sent?
> >>>>
> >>>
> >>> I think it was version in master-next that Richard has staged
> >>>
> >> It was against v6 so I'm not able to merge these until that is fixed.
> >>
> > Right, I will try a clean from scratch build and see if it was something
> > intermediate to be sure.
>
>
> Thanks for testing.
> I confirm the issue is still there. My apologies for this overlook!
> This is a busy week for me, but I will submit a fix as soon as I get the
> time, and before the end of the week anyway.
FWIW the new patchset seems to work well for issue I was seeing
>
> Kind regards
> Michael.
>
> --
> Michael Opdenacker, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 3/5] prserv: add "upstream" server support
2024-04-29 20:14 [PATCH v5 0/5] prserv: add support for an "upstream" server michael.opdenacker
2024-04-29 20:14 ` [PATCH v5 1/5] prserv: declare "max_package_pr" client hook michael.opdenacker
2024-04-29 20:14 ` [PATCH v5 2/5] prserv: move code from __init__ to bitbake-prserv michael.opdenacker
@ 2024-04-29 20:14 ` michael.opdenacker
2024-04-29 20:14 ` [PATCH v5 4/5] prserv: enable database sharing michael.opdenacker
2024-04-29 20:14 ` [PATCH v5 5/5] prserv: add bitbake selftests michael.opdenacker
4 siblings, 0 replies; 20+ messages in thread
From: michael.opdenacker @ 2024-04-29 20:14 UTC (permalink / raw)
To: bitbake-devel
Cc: Michael Opdenacker, Joshua Watt, Tim Orling, Thomas Petazzoni
From: Michael Opdenacker <michael.opdenacker@bootlin.com>
Introduce a PRSERVER_UPSTREAM variable that makes the
local PR server connect to an "upstream" one.
This makes it possible to implement local fixes to an
upstream package (revision "x", in a way that gives the local
update priority (revision "x.y").
Update the calculation of the new revisions to support the
case when prior revisions are not integers, but have
an "x.y..." format."
Set the comments in the handle_get_pr() function in serv.py
for details about the calculation of the local revision.
This is done by going on supporting the "history" mode that
wasn't used so far (revisions can return to a previous historical value),
in addition to the default "no history" mode (revisions can never decrease).
Rather than storing the history mode in the database table
itself (i.e. "PRMAIN_hist" and "PRMAIN_nohist"), the history mode
is now passed through the client requests. As a consequence, the
table name is now "PRMAIN", which is incompatible with what
was generated before, but avoids confusion if we kept the "PRMAIN_nohist"
name for both "history" and "no history" modes.
Update the server version to "2.0.0".
Signed-off-by: Michael Opdenacker <michael.opdenacker@bootlin.com>
Cc: Joshua Watt <JPEWhacker@gmail.com>
Cc: Tim Orling <ticotimo@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
bin/bitbake-prserv | 17 +++-
lib/prserv/__init__.py | 90 +++++++++++++++++-
lib/prserv/client.py | 17 ++--
lib/prserv/db.py | 205 ++++++++++++++++++++++-------------------
lib/prserv/serv.py | 134 +++++++++++++++++++++++----
5 files changed, 338 insertions(+), 125 deletions(-)
diff --git a/bin/bitbake-prserv b/bin/bitbake-prserv
index 920663a1d8..580e021fda 100755
--- a/bin/bitbake-prserv
+++ b/bin/bitbake-prserv
@@ -16,7 +16,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(os.path.dirname(__file__)), "lib
import prserv
import prserv.serv
-VERSION = "1.1.0"
+VERSION = "2.0.0"
PRHOST_DEFAULT="0.0.0.0"
PRPORT_DEFAULT=8585
@@ -77,12 +77,25 @@ def main():
action="store_true",
help="open database in read-only mode",
)
+ parser.add_argument(
+ "-u",
+ "--upstream",
+ default=os.environ.get("PRSERVER_UPSTREAM", None),
+ help="Upstream PR service (host:port)",
+ )
args = parser.parse_args()
init_logger(os.path.abspath(args.log), args.loglevel)
if args.start:
- ret=prserv.serv.start_daemon(args.file, args.host, args.port, os.path.abspath(args.log), args.read_only)
+ ret=prserv.serv.start_daemon(
+ args.file,
+ args.host,
+ args.port,
+ os.path.abspath(args.log),
+ args.read_only,
+ args.upstream
+ )
elif args.stop:
ret=prserv.serv.stop_daemon(args.host, args.port)
else:
diff --git a/lib/prserv/__init__.py b/lib/prserv/__init__.py
index 94658b815d..a817b03c1e 100644
--- a/lib/prserv/__init__.py
+++ b/lib/prserv/__init__.py
@@ -4,4 +4,92 @@
# SPDX-License-Identifier: GPL-2.0-only
#
-__version__ = "1.0.0"
+
+__version__ = "2.0.0"
+
+import logging
+logger = logging.getLogger("BitBake.PRserv")
+
+from bb.asyncrpc.client import parse_address, ADDR_TYPE_UNIX, ADDR_TYPE_WS
+
+def create_server(addr, dbpath, upstream=None, read_only=False):
+ from . import serv
+
+ s = serv.PRServer(dbpath, upstream=upstream, read_only=read_only)
+ host, port = addr.split(":")
+ s.start_tcp_server(host, int(port))
+
+ return s
+
+def increase_revision(ver):
+ """Take a revision string such as "1" or "1.2.3" or even a number and increase its last number
+ This fails if the last number is not an integer"""
+
+ fields=str(ver).split('.')
+ last = fields[-1]
+
+ try:
+ val = int(last)
+ except Exception as e:
+ logger.critical("Unable to increase revision value %s: %s" % (ver, e))
+ raise e
+
+ return ".".join(fields[0:-1] + list(str(val + 1)))
+
+def _revision_greater_or_equal(rev1, rev2):
+ """Compares x.y.z revision numbers, using integer comparison
+ Returns True if rev1 is greater or equal to rev2"""
+
+ fields1 = rev1.split(".")
+ fields2 = rev2.split(".")
+ l1 = len(fields1)
+ l2 = len(fields2)
+
+ for i in range(l1):
+ val1 = int(fields1[i])
+ if i < l2:
+ val2 = int(fields2[i])
+ if val2 < val1:
+ return True
+ elif val2 > val1:
+ return False
+ else:
+ return True
+ return True
+
+def revision_smaller(rev1, rev2):
+ """Compares x.y.z revision numbers, using integer comparison
+ Returns True if rev1 is strictly smaller than rev2"""
+ return not(_revision_greater_or_equal(rev1, rev2))
+
+def revision_greater(rev1, rev2):
+ """Compares x.y.z revision numbers, using integer comparison
+ Returns True if rev1 is strictly greater than rev2"""
+ return _revision_greater_or_equal(rev1, rev2) and (rev1 != rev2)
+
+def create_client(addr):
+ from . import client
+
+ c = client.PRClient()
+
+ try:
+ (typ, a) = parse_address(addr)
+ c.connect_tcp(*a)
+ return c
+ except Exception as e:
+ c.close()
+ raise e
+
+async def create_async_client(addr):
+ from . import client
+
+ c = client.PRAsyncClient()
+
+ try:
+ (typ, a) = parse_address(addr)
+ await c.connect_tcp(*a)
+ return c
+
+ except Exception as e:
+ await c.close()
+ raise e
diff --git a/lib/prserv/client.py b/lib/prserv/client.py
index 99fc4e0f7f..565c6f3872 100644
--- a/lib/prserv/client.py
+++ b/lib/prserv/client.py
@@ -6,6 +6,7 @@
import logging
import bb.asyncrpc
+from . import create_async_client
logger = logging.getLogger("BitBake.PRserv")
@@ -13,16 +14,16 @@ class PRAsyncClient(bb.asyncrpc.AsyncClient):
def __init__(self):
super().__init__("PRSERVICE", "1.0", logger)
- async def getPR(self, version, pkgarch, checksum):
+ async def getPR(self, version, pkgarch, checksum, history=False):
response = await self.invoke(
- {"get-pr": {"version": version, "pkgarch": pkgarch, "checksum": checksum}}
+ {"get-pr": {"version": version, "pkgarch": pkgarch, "checksum": checksum, "history": history}}
)
if response:
return response["value"]
- async def test_pr(self, version, pkgarch, checksum):
+ async def test_pr(self, version, pkgarch, checksum, history=False):
response = await self.invoke(
- {"test-pr": {"version": version, "pkgarch": pkgarch, "checksum": checksum}}
+ {"test-pr": {"version": version, "pkgarch": pkgarch, "checksum": checksum, "history": history}}
)
if response:
return response["value"]
@@ -41,16 +42,16 @@ class PRAsyncClient(bb.asyncrpc.AsyncClient):
if response:
return response["value"]
- async def importone(self, version, pkgarch, checksum, value):
+ async def importone(self, version, pkgarch, checksum, value, history=False):
response = await self.invoke(
- {"import-one": {"version": version, "pkgarch": pkgarch, "checksum": checksum, "value": value}}
+ {"import-one": {"version": version, "pkgarch": pkgarch, "checksum": checksum, "value": value, "history": history}}
)
if response:
return response["value"]
- async def export(self, version, pkgarch, checksum, colinfo):
+ async def export(self, version, pkgarch, checksum, colinfo, history=False):
response = await self.invoke(
- {"export": {"version": version, "pkgarch": pkgarch, "checksum": checksum, "colinfo": colinfo}}
+ {"export": {"version": version, "pkgarch": pkgarch, "checksum": checksum, "colinfo": colinfo, "history": history}}
)
if response:
return (response["metainfo"], response["datainfo"])
diff --git a/lib/prserv/db.py b/lib/prserv/db.py
index eb41508198..b2520f3158 100644
--- a/lib/prserv/db.py
+++ b/lib/prserv/db.py
@@ -10,6 +10,8 @@ import errno
import prserv
import time
+from . import increase_revision, revision_greater, revision_smaller
+
try:
import sqlite3
except ImportError:
@@ -32,15 +34,11 @@ if sqlversion[0] < 3 or (sqlversion[0] == 3 and sqlversion[1] < 3):
#
class PRTable(object):
- def __init__(self, conn, table, nohist, read_only):
+ def __init__(self, conn, table, read_only):
self.conn = conn
- self.nohist = nohist
self.read_only = read_only
self.dirty = False
- if nohist:
- self.table = "%s_nohist" % table
- else:
- self.table = "%s_hist" % table
+ self.table = table
if self.read_only:
table_exists = self._execute(
@@ -53,8 +51,8 @@ class PRTable(object):
(version TEXT NOT NULL, \
pkgarch TEXT NOT NULL, \
checksum TEXT NOT NULL, \
- value INTEGER, \
- PRIMARY KEY (version, pkgarch, checksum));" % self.table)
+ value TEXT, \
+ PRIMARY KEY (version, pkgarch, checksum, value));" % self.table)
def _execute(self, *query):
"""Execute a query, waiting to acquire a lock if necessary"""
@@ -68,6 +66,28 @@ class PRTable(object):
continue
raise exc
+ def _extremum_value(self, rows, is_max):
+ value = None
+
+ for row in rows:
+ current_value = row[0]
+ if value is None:
+ value = current_value
+ else:
+ if is_max:
+ is_new_extremum = revision_greater(current_value, value)
+ else:
+ is_new_extremum = revision_smaller(current_value, value)
+ if is_new_extremum:
+ value = current_value
+ return value
+
+ def _max_value(self, rows):
+ return self._extremum_value(rows, True)
+
+ def _min_value(self, rows):
+ return self._extremum_value(rows, False)
+
def sync(self):
if not self.read_only:
self.conn.commit()
@@ -102,101 +122,93 @@ class PRTable(object):
else:
return False
- def find_value(self, version, pkgarch, checksum):
+
+ def find_package_max_value(self, version, pkgarch):
+ """Returns the greatest value for (version, pkgarch), or None if not found. Doesn't create a new value"""
+
+ data = self._execute("SELECT value FROM %s where version=? AND pkgarch=?;" % (self.table),
+ (version, pkgarch))
+ rows = data.fetchall()
+ value = self._max_value(rows)
+ return value
+
+ def find_value(self, version, pkgarch, checksum, history=False):
"""Returns the value for the specified checksum if found or None otherwise."""
- data=self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
- (version, pkgarch, checksum))
- row=data.fetchone()
- if row is not None:
- return row[0]
+ if history:
+ return self.find_min_value(version, pkgarch, checksum)
else:
- return None
+ return self.find_max_value(version, pkgarch, checksum)
- def find_max_value(self, version, pkgarch):
- """Returns the greatest value for (version, pkgarch), or None if not found. Doesn't create a new value"""
- data = self._execute("SELECT max(value) FROM %s where version=? AND pkgarch=?;" % (self.table),
+ def _find_extremum_value(self, version, pkgarch, checksum, is_max):
+ """Returns the maximum (if is_max is True) or minimum (if is_max is False) value
+ for (version, pkgarch, checksum), or None if not found. Doesn't create a new value"""
+
+ data = self._execute("SELECT value FROM %s where version=? AND pkgarch=? AND checksum=?;" % (self.table),
+ (version, pkgarch, checksum))
+ rows = data.fetchall()
+ return self._extremum_value(rows, is_max)
+
+ def find_max_value(self, version, pkgarch, checksum):
+ return self._find_extremum_value(version, pkgarch, checksum, True)
+
+ def find_min_value(self, version, pkgarch, checksum):
+ return self._find_extremum_value(version, pkgarch, checksum, False)
+
+ def find_new_subvalue(self, version, pkgarch, base):
+ """Take and increase the greatest "<base>.y" value for (version, pkgarch), or return "<base>.0" if not found.
+ This doesn't store a new value."""
+
+ data = self._execute("SELECT value FROM %s where version=? AND pkgarch=? AND value LIKE '%s.%%';" % (self.table, base),
(version, pkgarch))
- row = data.fetchone()
- if row is not None:
- return row[0]
- else:
- return None
+ rows = data.fetchall()
+ value = self._max_value(rows)
- def _get_value_hist(self, version, pkgarch, checksum):
- data=self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
- (version, pkgarch, checksum))
- row=data.fetchone()
- if row is not None:
- return row[0]
+ if value is not None:
+ return increase_revision(value)
else:
- #no value found, try to insert
- if self.read_only:
- data = self._execute("SELECT ifnull(max(value)+1, 0) FROM %s where version=? AND pkgarch=?;" % (self.table),
- (version, pkgarch))
- row = data.fetchone()
- if row is not None:
- return row[0]
- else:
- return 0
+ return base + ".0"
- try:
- self._execute("INSERT INTO %s VALUES (?, ?, ?, (select ifnull(max(value)+1, 0) from %s where version=? AND pkgarch=?));"
- % (self.table, self.table),
- (version, pkgarch, checksum, version, pkgarch))
- except sqlite3.IntegrityError as exc:
- logger.error(str(exc))
+ def store_value(self, version, pkgarch, checksum, value):
+ """Store new value in the database"""
- self.dirty = True
+ try:
+ self._execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
+ (version, pkgarch, checksum, value))
+ except sqlite3.IntegrityError as exc:
+ logger.error(str(exc))
- data=self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
- (version, pkgarch, checksum))
- row=data.fetchone()
- if row is not None:
- return row[0]
- else:
- raise prserv.NotFoundError
+ self.dirty = True
- def _get_value_no_hist(self, version, pkgarch, checksum):
- data=self._execute("SELECT value FROM %s \
- WHERE version=? AND pkgarch=? AND checksum=? AND \
- value >= (select max(value) from %s where version=? AND pkgarch=?);"
- % (self.table, self.table),
- (version, pkgarch, checksum, version, pkgarch))
- row=data.fetchone()
- if row is not None:
- return row[0]
- else:
- #no value found, try to insert
- if self.read_only:
- data = self._execute("SELECT ifnull(max(value)+1, 0) FROM %s where version=? AND pkgarch=?;" % (self.table),
- (version, pkgarch))
- return data.fetchone()[0]
+ def _get_value(self, version, pkgarch, checksum, history):
- try:
- self._execute("INSERT OR REPLACE INTO %s VALUES (?, ?, ?, (select ifnull(max(value)+1, 0) from %s where version=? AND pkgarch=?));"
- % (self.table, self.table),
- (version, pkgarch, checksum, version, pkgarch))
- except sqlite3.IntegrityError as exc:
- logger.error(str(exc))
- self.conn.rollback()
+ max_value = self.find_package_max_value(version, pkgarch)
- self.dirty = True
+ if max_value is None:
+ # version, pkgarch completely unknown. Return initial value.
+ return "0"
- data=self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
- (version, pkgarch, checksum))
- row=data.fetchone()
- if row is not None:
- return row[0]
- else:
- raise prserv.NotFoundError
+ value = self.find_value(version, pkgarch, checksum, history)
- def get_value(self, version, pkgarch, checksum):
- if self.nohist:
- return self._get_value_no_hist(version, pkgarch, checksum)
+ if value is None:
+ # version, pkgarch found but not checksum. Create a new value from the maximum one
+ return increase_revision(max_value)
+
+ if history:
+ return value
+
+ # "no history" mode - If the value is not the maximum value for the package, need to increase it.
+ if max_value > value:
+ return increase_revision(max_value)
else:
- return self._get_value_hist(version, pkgarch, checksum)
+ return value
+
+ def get_value(self, version, pkgarch, checksum, history):
+ value = self._get_value(version, pkgarch, checksum, history)
+ if not self.read_only:
+ self.store_value(version, pkgarch, checksum, value)
+ return value
def _import_hist(self, version, pkgarch, checksum, value):
if self.read_only:
@@ -252,13 +264,13 @@ class PRTable(object):
else:
return None
- def importone(self, version, pkgarch, checksum, value):
- if self.nohist:
- return self._import_no_hist(version, pkgarch, checksum, value)
- else:
+ def importone(self, version, pkgarch, checksum, value, history=False):
+ if history:
return self._import_hist(version, pkgarch, checksum, value)
+ else:
+ return self._import_no_hist(version, pkgarch, checksum, value)
- def export(self, version, pkgarch, checksum, colinfo):
+ def export(self, version, pkgarch, checksum, colinfo, history=False):
metainfo = {}
#column info
if colinfo:
@@ -278,12 +290,12 @@ class PRTable(object):
#data info
datainfo = []
- if self.nohist:
+ if history:
+ sqlstmt = "SELECT * FROM %s as T1 WHERE 1=1 " % self.table
+ else:
sqlstmt = "SELECT T1.version, T1.pkgarch, T1.checksum, T1.value FROM %s as T1, \
(SELECT version, pkgarch, max(value) as maxvalue FROM %s GROUP BY version, pkgarch) as T2 \
WHERE T1.version=T2.version AND T1.pkgarch=T2.pkgarch AND T1.value=T2.maxvalue " % (self.table, self.table)
- else:
- sqlstmt = "SELECT * FROM %s as T1 WHERE 1=1 " % self.table
sqlarg = []
where = ""
if version:
@@ -322,9 +334,8 @@ class PRTable(object):
class PRData(object):
"""Object representing the PR database"""
- def __init__(self, filename, nohist=True, read_only=False):
+ def __init__(self, filename, read_only=False):
self.filename=os.path.abspath(filename)
- self.nohist=nohist
self.read_only = read_only
#build directory hierarchy
try:
@@ -351,7 +362,7 @@ class PRData(object):
if tblname in self._tables:
return self._tables[tblname]
else:
- tableobj = self._tables[tblname] = PRTable(self.connection, tblname, self.nohist, self.read_only)
+ tableobj = self._tables[tblname] = PRTable(self.connection, tblname, self.read_only)
return tableobj
def __delitem__(self, tblname):
diff --git a/lib/prserv/serv.py b/lib/prserv/serv.py
index dc4be5b620..05573d06cc 100644
--- a/lib/prserv/serv.py
+++ b/lib/prserv/serv.py
@@ -12,6 +12,7 @@ import sqlite3
import prserv
import prserv.db
import errno
+from . import create_async_client, revision_smaller, increase_revision
import bb.asyncrpc
logger = logging.getLogger("BitBake.PRserv")
@@ -51,8 +52,9 @@ class PRServerClient(bb.asyncrpc.AsyncServerConnection):
version = request["version"]
pkgarch = request["pkgarch"]
checksum = request["checksum"]
+ history = request["history"]
- value = self.server.table.find_value(version, pkgarch, checksum)
+ value = self.server.table.find_value(version, pkgarch, checksum, history)
return {"value": value}
async def handle_test_package(self, request):
@@ -68,22 +70,110 @@ class PRServerClient(bb.asyncrpc.AsyncServerConnection):
version = request["version"]
pkgarch = request["pkgarch"]
- value = self.server.table.find_max_value(version, pkgarch)
+ value = self.server.table.find_package_max_value(version, pkgarch)
return {"value": value}
async def handle_get_pr(self, request):
version = request["version"]
pkgarch = request["pkgarch"]
checksum = request["checksum"]
+ history = request["history"]
- response = None
- try:
- value = self.server.table.get_value(version, pkgarch, checksum)
- response = {"value": value}
- except prserv.NotFoundError:
- self.logger.error("failure storing value in database for (%s, %s)",version, checksum)
+ if self.upstream_client is None:
+ value = self.server.table.get_value(version, pkgarch, checksum, history)
+ return {"value": value}
- return response
+ # We have an upstream server.
+ # Check whether the local server already knows the requested configuration.
+ # If the configuration is a new one, the generated value we will add will
+ # depend on what's on the upstream server. That's why we're calling find_value()
+ # instead of get_value() directly.
+
+ value = self.server.table.find_value(version, pkgarch, checksum, history)
+ upstream_max = await self.upstream_client.max_package_pr(version, pkgarch)
+
+ if value is not None:
+
+ # The configuration is already known locally.
+
+ if history:
+ value = self.server.table.get_value(version, pkgarch, checksum, history)
+ else:
+ existing_value = value
+ # In "no history", we need to make sure the value doesn't decrease
+ # and is at least greater than the maximum upstream value
+ # and the maximum local value
+
+ local_max = self.server.table.find_package_max_value(version, pkgarch)
+ if revision_smaller(value, local_max):
+ value = increase_revision(local_max)
+
+ if revision_smaller(value, upstream_max):
+ # Ask upstream whether it knows the checksum
+ upstream_value = await self.upstream_client.test_pr(version, pkgarch, checksum)
+ if upstream_value is None:
+ # Upstream doesn't have our checksum, let create a new one
+ value = upstream_max + ".0"
+ else:
+ # Fine to take the same value as upstream
+ value = upstream_max
+
+ if not value == existing_value and not self.server.read_only:
+ self.server.table.store_value(version, pkgarch, checksum, value)
+
+ return {"value": value}
+
+ # The configuration is a new one for the local server
+ # Let's ask the upstream server whether it knows it
+
+ known_upstream = await self.upstream_client.test_package(version, pkgarch)
+
+ if not known_upstream:
+
+ # The package is not known upstream, must be a local-only package
+ # Let's compute the PR number using the local-only method
+
+ value = self.server.table.get_value(version, pkgarch, checksum, history)
+ return {"value": value}
+
+ # The package is known upstream, let's ask the upstream server
+ # whether it knows our new output hash
+
+ value = await self.upstream_client.test_pr(version, pkgarch, checksum)
+
+ if value is not None:
+
+ # Upstream knows this output hash, let's store it and use it too.
+
+ if not self.server.read_only:
+ self.server.table.store_value(version, pkgarch, checksum, value)
+ # If the local server is read only, won't be able to store the new
+ # value in the database and will have to keep asking the upstream server
+ return {"value": value}
+
+ # The output hash doesn't exist upstream, get the most recent number from upstream (x)
+ # Then, we want to have a new PR value for the local server: x.y
+
+ upstream_max = await self.upstream_client.max_package_pr(version, pkgarch)
+ # Here we know that the package is known upstream, so upstream_max can't be None
+ subvalue = self.server.table.find_new_subvalue(version, pkgarch, upstream_max)
+
+ if not self.server.read_only:
+ self.server.table.store_value(version, pkgarch, checksum, subvalue)
+
+ return {"value": subvalue}
+
+ async def process_requests(self):
+ if self.server.upstream is not None:
+ self.upstream_client = await create_async_client(self.server.upstream)
+ else:
+ self.upstream_client = None
+
+ try:
+ await super().process_requests()
+ finally:
+ if self.upstream_client is not None:
+ await self.upstream_client.close()
async def handle_import_one(self, request):
response = None
@@ -92,8 +182,9 @@ class PRServerClient(bb.asyncrpc.AsyncServerConnection):
pkgarch = request["pkgarch"]
checksum = request["checksum"]
value = request["value"]
+ history = request["history"]
- value = self.server.table.importone(version, pkgarch, checksum, value)
+ value = self.server.table.importone(version, pkgarch, checksum, value, history)
if value is not None:
response = {"value": value}
@@ -104,9 +195,10 @@ class PRServerClient(bb.asyncrpc.AsyncServerConnection):
pkgarch = request["pkgarch"]
checksum = request["checksum"]
colinfo = request["colinfo"]
+ history = request["history"]
try:
- (metainfo, datainfo) = self.server.table.export(version, pkgarch, checksum, colinfo)
+ (metainfo, datainfo) = self.server.table.export(version, pkgarch, checksum, colinfo, history)
except sqlite3.Error as exc:
self.logger.error(str(exc))
metainfo = datainfo = None
@@ -117,11 +209,12 @@ class PRServerClient(bb.asyncrpc.AsyncServerConnection):
return {"readonly": self.server.read_only}
class PRServer(bb.asyncrpc.AsyncServer):
- def __init__(self, dbfile, read_only=False):
+ def __init__(self, dbfile, read_only=False, upstream=None):
super().__init__(logger)
self.dbfile = dbfile
self.table = None
self.read_only = read_only
+ self.upstream = upstream
def accept_client(self, socket):
return PRServerClient(socket, self)
@@ -134,6 +227,9 @@ class PRServer(bb.asyncrpc.AsyncServer):
self.logger.info("Started PRServer with DBfile: %s, Address: %s, PID: %s" %
(self.dbfile, self.address, str(os.getpid())))
+ if self.upstream is not None:
+ self.logger.info("And upstream PRServer: %s " % (self.upstream))
+
return tasks
async def stop(self):
@@ -147,14 +243,15 @@ class PRServer(bb.asyncrpc.AsyncServer):
self.table.sync()
class PRServSingleton(object):
- def __init__(self, dbfile, logfile, host, port):
+ def __init__(self, dbfile, logfile, host, port, upstream):
self.dbfile = dbfile
self.logfile = logfile
self.host = host
self.port = port
+ self.upstream = upstream
def start(self):
- self.prserv = PRServer(self.dbfile)
+ self.prserv = PRServer(self.dbfile, upstream=self.upstream)
self.prserv.start_tcp_server(socket.gethostbyname(self.host), self.port)
self.process = self.prserv.serve_as_process(log_level=logging.WARNING)
@@ -233,7 +330,7 @@ def run_as_daemon(func, pidfile, logfile):
os.remove(pidfile)
os._exit(0)
-def start_daemon(dbfile, host, port, logfile, read_only=False):
+def start_daemon(dbfile, host, port, logfile, read_only=False, upstream=None):
ip = socket.gethostbyname(host)
pidfile = PIDPREFIX % (ip, port)
try:
@@ -249,7 +346,7 @@ def start_daemon(dbfile, host, port, logfile, read_only=False):
dbfile = os.path.abspath(dbfile)
def daemon_main():
- server = PRServer(dbfile, read_only=read_only)
+ server = PRServer(dbfile, read_only=read_only, upstream=upstream)
server.start_tcp_server(ip, port)
server.serve_forever()
@@ -336,6 +433,9 @@ def auto_start(d):
host = host_params[0].strip().lower()
port = int(host_params[1])
+
+ upstream = d.getVar("PRSERV_UPSTREAM") or None
+
if is_local_special(host, port):
import bb.utils
cachedir = (d.getVar("PERSISTENT_DIR") or d.getVar("CACHE"))
@@ -350,7 +450,7 @@ def auto_start(d):
auto_shutdown()
if not singleton:
bb.utils.mkdirhier(cachedir)
- singleton = PRServSingleton(os.path.abspath(dbfile), os.path.abspath(logfile), host, port)
+ singleton = PRServSingleton(os.path.abspath(dbfile), os.path.abspath(logfile), host, port, upstream)
singleton.start()
if singleton:
host = singleton.host
--
2.34.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v5 4/5] prserv: enable database sharing
2024-04-29 20:14 [PATCH v5 0/5] prserv: add support for an "upstream" server michael.opdenacker
` (2 preceding siblings ...)
2024-04-29 20:14 ` [PATCH v5 3/5] prserv: add "upstream" server support michael.opdenacker
@ 2024-04-29 20:14 ` michael.opdenacker
2024-04-29 20:34 ` Joshua Watt
2024-04-29 20:14 ` [PATCH v5 5/5] prserv: add bitbake selftests michael.opdenacker
4 siblings, 1 reply; 20+ messages in thread
From: michael.opdenacker @ 2024-04-29 20:14 UTC (permalink / raw)
To: bitbake-devel
Cc: Michael Opdenacker, Joshua Watt, Tim Orling, Thomas Petazzoni
From: Michael Opdenacker <michael.opdenacker@bootlin.com>
sqlite3 can allow multiple processes to access the database
simultaneously, but it must be opened correctly. The key change is that
the database is no longer opened in "exclusive" mode (defaulting to
shared mode). In addition, the journal is set to "WAL" mode, as this is
the most efficient for dealing with simultaneous access between
different processes. In order to keep the database performance,
synchronous mode is set to "off". The WAL journal will protect against
incomplete transactions in any given client, however the database will
not be protected against unexpected power loss from the OS (which is a
fine trade off for performance, and also the same as the previous
implementation).
The use of a database cursor enabled to remove the _execute() wrapper.
The cursor automatically makes sure that the query happens in an atomic
transaction and commits when finished.
This also removes the need for a "dirty" flag for the database and
for explicit database syncing, which simplifies the code.
Signed-off-by: Michael Opdenacker <michael.opdenacker@bootlin.com>
Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
Cc: Tim Orling <ticotimo@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
lib/prserv/db.py | 322 +++++++++++++++++++++------------------------
lib/prserv/serv.py | 8 --
2 files changed, 151 insertions(+), 179 deletions(-)
diff --git a/lib/prserv/db.py b/lib/prserv/db.py
index b2520f3158..f430586d73 100644
--- a/lib/prserv/db.py
+++ b/lib/prserv/db.py
@@ -8,21 +8,13 @@ import logging
import os.path
import errno
import prserv
-import time
+import sqlite3
+from contextlib import closing
from . import increase_revision, revision_greater, revision_smaller
-try:
- import sqlite3
-except ImportError:
- from pysqlite2 import dbapi2 as sqlite3
-
logger = logging.getLogger("BitBake.PRserv")
-sqlversion = sqlite3.sqlite_version_info
-if sqlversion[0] < 3 or (sqlversion[0] == 3 and sqlversion[1] < 3):
- raise Exception("sqlite3 version 3.3.0 or later is required.")
-
#
# "No History" mode - for a given query tuple (version, pkgarch, checksum),
# the returned value will be the largest among all the values of the same
@@ -31,40 +23,28 @@ if sqlversion[0] < 3 or (sqlversion[0] == 3 and sqlversion[1] < 3):
# "History" mode - Return a new higher value for previously unseen query
# tuple (version, pkgarch, checksum), otherwise return historical value.
# Value can decrement if returning to a previous build.
-#
class PRTable(object):
def __init__(self, conn, table, read_only):
self.conn = conn
self.read_only = read_only
- self.dirty = False
self.table = table
- if self.read_only:
- table_exists = self._execute(
- "SELECT count(*) FROM sqlite_master \
- WHERE type='table' AND name='%s'" % (self.table))
- if not table_exists:
- raise prserv.NotFoundError
- else:
- self._execute("CREATE TABLE IF NOT EXISTS %s \
- (version TEXT NOT NULL, \
- pkgarch TEXT NOT NULL, \
- checksum TEXT NOT NULL, \
- value TEXT, \
- PRIMARY KEY (version, pkgarch, checksum, value));" % self.table)
-
- def _execute(self, *query):
- """Execute a query, waiting to acquire a lock if necessary"""
- start = time.time()
- end = start + 20
- while True:
- try:
- return self.conn.execute(*query)
- except sqlite3.OperationalError as exc:
- if "is locked" in str(exc) and end > time.time():
- continue
- raise exc
+ with closing(self.conn.cursor()) as cursor:
+ if self.read_only:
+ table_exists = cursor.execute(
+ "SELECT count(*) FROM sqlite_master \
+ WHERE type='table' AND name='%s'" % (self.table))
+ if not table_exists:
+ raise prserv.NotFoundError
+ else:
+ cursor.execute("CREATE TABLE IF NOT EXISTS %s \
+ (version TEXT NOT NULL, \
+ pkgarch TEXT NOT NULL, \
+ checksum TEXT NOT NULL, \
+ value TEXT, \
+ PRIMARY KEY (version, pkgarch, checksum, value));" % self.table)
+ self.conn.commit()
def _extremum_value(self, rows, is_max):
value = None
@@ -88,49 +68,42 @@ class PRTable(object):
def _min_value(self, rows):
return self._extremum_value(rows, False)
- def sync(self):
- if not self.read_only:
- self.conn.commit()
- self._execute("BEGIN EXCLUSIVE TRANSACTION")
-
- def sync_if_dirty(self):
- if self.dirty:
- self.sync()
- self.dirty = False
-
def test_package(self, version, pkgarch):
"""Returns whether the specified package version is found in the database for the specified architecture"""
# Just returns the value if found or None otherwise
- data=self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=?;" % self.table,
- (version, pkgarch))
- row=data.fetchone()
- if row is not None:
- return True
- else:
- return False
+ with closing(self.conn.cursor()) as cursor:
+ data=cursor.execute("SELECT value FROM %s WHERE version=? AND pkgarch=?;" % self.table,
+ (version, pkgarch))
+ row=data.fetchone()
+ if row is not None:
+ return True
+ else:
+ return False
def test_value(self, version, pkgarch, value):
"""Returns whether the specified value is found in the database for the specified package and architecture"""
# Just returns the value if found or None otherwise
- data=self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? and value=?;" % self.table,
- (version, pkgarch, value))
- row=data.fetchone()
- if row is not None:
- return True
- else:
- return False
+ with closing(self.conn.cursor()) as cursor:
+ data=cursor.execute("SELECT value FROM %s WHERE version=? AND pkgarch=? and value=?;" % self.table,
+ (version, pkgarch, value))
+ row=data.fetchone()
+ if row is not None:
+ return True
+ else:
+ return False
def find_package_max_value(self, version, pkgarch):
"""Returns the greatest value for (version, pkgarch), or None if not found. Doesn't create a new value"""
- data = self._execute("SELECT value FROM %s where version=? AND pkgarch=?;" % (self.table),
- (version, pkgarch))
- rows = data.fetchall()
- value = self._max_value(rows)
- return value
+ with closing(self.conn.cursor()) as cursor:
+ data = cursor.execute("SELECT value FROM %s where version=? AND pkgarch=?;" % (self.table),
+ (version, pkgarch))
+ rows = data.fetchall()
+ value = self._max_value(rows)
+ return value
def find_value(self, version, pkgarch, checksum, history=False):
"""Returns the value for the specified checksum if found or None otherwise."""
@@ -145,10 +118,11 @@ class PRTable(object):
"""Returns the maximum (if is_max is True) or minimum (if is_max is False) value
for (version, pkgarch, checksum), or None if not found. Doesn't create a new value"""
- data = self._execute("SELECT value FROM %s where version=? AND pkgarch=? AND checksum=?;" % (self.table),
- (version, pkgarch, checksum))
- rows = data.fetchall()
- return self._extremum_value(rows, is_max)
+ with closing(self.conn.cursor()) as cursor:
+ data = cursor.execute("SELECT value FROM %s where version=? AND pkgarch=? AND checksum=?;" % (self.table),
+ (version, pkgarch, checksum))
+ rows = data.fetchall()
+ return self._extremum_value(rows, is_max)
def find_max_value(self, version, pkgarch, checksum):
return self._find_extremum_value(version, pkgarch, checksum, True)
@@ -160,26 +134,27 @@ class PRTable(object):
"""Take and increase the greatest "<base>.y" value for (version, pkgarch), or return "<base>.0" if not found.
This doesn't store a new value."""
- data = self._execute("SELECT value FROM %s where version=? AND pkgarch=? AND value LIKE '%s.%%';" % (self.table, base),
- (version, pkgarch))
- rows = data.fetchall()
- value = self._max_value(rows)
+ with closing(self.conn.cursor()) as cursor:
+ data = cursor.execute("SELECT value FROM %s where version=? AND pkgarch=? AND value LIKE '%s.%%';" % (self.table, base),
+ (version, pkgarch))
+ rows = data.fetchall()
+ value = self._max_value(rows)
- if value is not None:
- return increase_revision(value)
- else:
- return base + ".0"
+ if value is not None:
+ return increase_revision(value)
+ else:
+ return base + ".0"
def store_value(self, version, pkgarch, checksum, value):
"""Store new value in the database"""
- try:
- self._execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
- (version, pkgarch, checksum, value))
- except sqlite3.IntegrityError as exc:
- logger.error(str(exc))
-
- self.dirty = True
+ with closing(self.conn.cursor()) as cursor:
+ try:
+ cursor.execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
+ (version, pkgarch, checksum, value))
+ except sqlite3.IntegrityError as exc:
+ logger.error(str(exc))
+ self.conn.commit()
def _get_value(self, version, pkgarch, checksum, history):
@@ -215,54 +190,56 @@ class PRTable(object):
return None
val = None
- data = self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
+ with closing(self.conn.cursor()) as cursor:
+ data = cursor.execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
(version, pkgarch, checksum))
- row = data.fetchone()
- if row is not None:
- val=row[0]
- else:
- #no value found, try to insert
- try:
- self._execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
- (version, pkgarch, checksum, value))
- except sqlite3.IntegrityError as exc:
- logger.error(str(exc))
+ row = data.fetchone()
+ if row is not None:
+ val=row[0]
+ else:
+ #no value found, try to insert
+ try:
+ cursor.execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
+ (version, pkgarch, checksum, value))
+ except sqlite3.IntegrityError as exc:
+ logger.error(str(exc))
- self.dirty = True
+ self.conn.commit()
- data = self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
+ data = cursor.execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
(version, pkgarch, checksum))
- row = data.fetchone()
- if row is not None:
- val = row[0]
+ row = data.fetchone()
+ if row is not None:
+ val = row[0]
return val
def _import_no_hist(self, version, pkgarch, checksum, value):
if self.read_only:
return None
- try:
- #try to insert
- self._execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
- (version, pkgarch, checksum, value))
- except sqlite3.IntegrityError as exc:
- #already have the record, try to update
+ with closing(self.conn.cursor()) as cursor:
try:
- self._execute("UPDATE %s SET value=? WHERE version=? AND pkgarch=? AND checksum=? AND value<?"
- % (self.table),
- (value, version, pkgarch, checksum, value))
+ #try to insert
+ cursor.execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
+ (version, pkgarch, checksum, value))
except sqlite3.IntegrityError as exc:
- logger.error(str(exc))
+ #already have the record, try to update
+ try:
+ cursor.execute("UPDATE %s SET value=? WHERE version=? AND pkgarch=? AND checksum=? AND value<?"
+ % (self.table),
+ (value, version, pkgarch, checksum, value))
+ except sqlite3.IntegrityError as exc:
+ logger.error(str(exc))
- self.dirty = True
+ self.conn.commit()
- data = self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=? AND value>=?;" % self.table,
+ data = cursor.execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=? AND value>=?;" % self.table,
(version, pkgarch, checksum, value))
- row=data.fetchone()
- if row is not None:
- return row[0]
- else:
- return None
+ row=data.fetchone()
+ if row is not None:
+ return row[0]
+ else:
+ return None
def importone(self, version, pkgarch, checksum, value, history=False):
if history:
@@ -272,56 +249,57 @@ class PRTable(object):
def export(self, version, pkgarch, checksum, colinfo, history=False):
metainfo = {}
- #column info
- if colinfo:
- metainfo["tbl_name"] = self.table
- metainfo["core_ver"] = prserv.__version__
- metainfo["col_info"] = []
- data = self._execute("PRAGMA table_info(%s);" % self.table)
+ with closing(self.conn.cursor()) as cursor:
+ #column info
+ if colinfo:
+ metainfo["tbl_name"] = self.table
+ metainfo["core_ver"] = prserv.__version__
+ metainfo["col_info"] = []
+ data = cursor.execute("PRAGMA table_info(%s);" % self.table)
+ for row in data:
+ col = {}
+ col["name"] = row["name"]
+ col["type"] = row["type"]
+ col["notnull"] = row["notnull"]
+ col["dflt_value"] = row["dflt_value"]
+ col["pk"] = row["pk"]
+ metainfo["col_info"].append(col)
+
+ #data info
+ datainfo = []
+
+ if history:
+ sqlstmt = "SELECT * FROM %s as T1 WHERE 1=1 " % self.table
+ else:
+ sqlstmt = "SELECT T1.version, T1.pkgarch, T1.checksum, T1.value FROM %s as T1, \
+ (SELECT version, pkgarch, max(value) as maxvalue FROM %s GROUP BY version, pkgarch) as T2 \
+ WHERE T1.version=T2.version AND T1.pkgarch=T2.pkgarch AND T1.value=T2.maxvalue " % (self.table, self.table)
+ sqlarg = []
+ where = ""
+ if version:
+ where += "AND T1.version=? "
+ sqlarg.append(str(version))
+ if pkgarch:
+ where += "AND T1.pkgarch=? "
+ sqlarg.append(str(pkgarch))
+ if checksum:
+ where += "AND T1.checksum=? "
+ sqlarg.append(str(checksum))
+
+ sqlstmt += where + ";"
+
+ if len(sqlarg):
+ data = cursor.execute(sqlstmt, tuple(sqlarg))
+ else:
+ data = cursor.execute(sqlstmt)
for row in data:
- col = {}
- col["name"] = row["name"]
- col["type"] = row["type"]
- col["notnull"] = row["notnull"]
- col["dflt_value"] = row["dflt_value"]
- col["pk"] = row["pk"]
- metainfo["col_info"].append(col)
-
- #data info
- datainfo = []
-
- if history:
- sqlstmt = "SELECT * FROM %s as T1 WHERE 1=1 " % self.table
- else:
- sqlstmt = "SELECT T1.version, T1.pkgarch, T1.checksum, T1.value FROM %s as T1, \
- (SELECT version, pkgarch, max(value) as maxvalue FROM %s GROUP BY version, pkgarch) as T2 \
- WHERE T1.version=T2.version AND T1.pkgarch=T2.pkgarch AND T1.value=T2.maxvalue " % (self.table, self.table)
- sqlarg = []
- where = ""
- if version:
- where += "AND T1.version=? "
- sqlarg.append(str(version))
- if pkgarch:
- where += "AND T1.pkgarch=? "
- sqlarg.append(str(pkgarch))
- if checksum:
- where += "AND T1.checksum=? "
- sqlarg.append(str(checksum))
-
- sqlstmt += where + ";"
-
- if len(sqlarg):
- data = self._execute(sqlstmt, tuple(sqlarg))
- else:
- data = self._execute(sqlstmt)
- for row in data:
- if row["version"]:
- col = {}
- col["version"] = row["version"]
- col["pkgarch"] = row["pkgarch"]
- col["checksum"] = row["checksum"]
- col["value"] = row["value"]
- datainfo.append(col)
+ if row["version"]:
+ col = {}
+ col["version"] = row["version"]
+ col["pkgarch"] = row["pkgarch"]
+ col["checksum"] = row["checksum"]
+ col["value"] = row["value"]
+ datainfo.append(col)
return (metainfo, datainfo)
def dump_db(self, fd):
@@ -345,14 +323,15 @@ class PRData(object):
raise e
uri = "file:%s%s" % (self.filename, "?mode=ro" if self.read_only else "")
logger.debug("Opening PRServ database '%s'" % (uri))
- self.connection=sqlite3.connect(uri, uri=True, isolation_level="EXCLUSIVE", check_same_thread = False)
+ self.connection=sqlite3.connect(uri, uri=True)
self.connection.row_factory=sqlite3.Row
- if not self.read_only:
- self.connection.execute("pragma synchronous = off;")
- self.connection.execute("PRAGMA journal_mode = MEMORY;")
+ self.connection.execute("PRAGMA synchronous = OFF;")
+ self.connection.execute("PRAGMA journal_mode = WAL;")
+ self.connection.commit()
self._tables={}
def disconnect(self):
+ self.connection.commit()
self.connection.close()
def __getitem__(self, tblname):
@@ -370,3 +349,4 @@ class PRData(object):
del self._tables[tblname]
logger.info("drop table %s" % (tblname))
self.connection.execute("DROP TABLE IF EXISTS %s;" % tblname)
+ self.connection.commit()
diff --git a/lib/prserv/serv.py b/lib/prserv/serv.py
index 05573d06cc..fd673b1851 100644
--- a/lib/prserv/serv.py
+++ b/lib/prserv/serv.py
@@ -44,8 +44,6 @@ class PRServerClient(bb.asyncrpc.AsyncServerConnection):
except:
self.server.table.sync()
raise
- else:
- self.server.table.sync_if_dirty()
async def handle_test_pr(self, request):
'''Finds the PR value corresponding to the request. If not found, returns None and doesn't insert a new value'''
@@ -233,15 +231,9 @@ class PRServer(bb.asyncrpc.AsyncServer):
return tasks
async def stop(self):
- self.table.sync_if_dirty()
self.db.disconnect()
await super().stop()
- def signal_handler(self):
- super().signal_handler()
- if self.table:
- self.table.sync()
-
class PRServSingleton(object):
def __init__(self, dbfile, logfile, host, port, upstream):
self.dbfile = dbfile
--
2.34.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 4/5] prserv: enable database sharing
2024-04-29 20:14 ` [PATCH v5 4/5] prserv: enable database sharing michael.opdenacker
@ 2024-04-29 20:34 ` Joshua Watt
2024-04-30 12:58 ` Michael Opdenacker
0 siblings, 1 reply; 20+ messages in thread
From: Joshua Watt @ 2024-04-29 20:34 UTC (permalink / raw)
To: michael.opdenacker; +Cc: bitbake-devel, Tim Orling, Thomas Petazzoni
On Mon, Apr 29, 2024 at 2:14 PM <michael.opdenacker@bootlin.com> wrote:
>
> From: Michael Opdenacker <michael.opdenacker@bootlin.com>
>
> sqlite3 can allow multiple processes to access the database
> simultaneously, but it must be opened correctly. The key change is that
> the database is no longer opened in "exclusive" mode (defaulting to
> shared mode). In addition, the journal is set to "WAL" mode, as this is
> the most efficient for dealing with simultaneous access between
> different processes. In order to keep the database performance,
> synchronous mode is set to "off". The WAL journal will protect against
> incomplete transactions in any given client, however the database will
> not be protected against unexpected power loss from the OS (which is a
> fine trade off for performance, and also the same as the previous
> implementation).
>
> The use of a database cursor enabled to remove the _execute() wrapper.
> The cursor automatically makes sure that the query happens in an atomic
> transaction and commits when finished.
>
> This also removes the need for a "dirty" flag for the database and
> for explicit database syncing, which simplifies the code.
>
> Signed-off-by: Michael Opdenacker <michael.opdenacker@bootlin.com>
> Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
> Cc: Tim Orling <ticotimo@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
> lib/prserv/db.py | 322 +++++++++++++++++++++------------------------
> lib/prserv/serv.py | 8 --
> 2 files changed, 151 insertions(+), 179 deletions(-)
>
> diff --git a/lib/prserv/db.py b/lib/prserv/db.py
> index b2520f3158..f430586d73 100644
> --- a/lib/prserv/db.py
> +++ b/lib/prserv/db.py
> @@ -8,21 +8,13 @@ import logging
> import os.path
> import errno
> import prserv
> -import time
> +import sqlite3
>
> +from contextlib import closing
> from . import increase_revision, revision_greater, revision_smaller
>
> -try:
> - import sqlite3
> -except ImportError:
> - from pysqlite2 import dbapi2 as sqlite3
> -
> logger = logging.getLogger("BitBake.PRserv")
>
> -sqlversion = sqlite3.sqlite_version_info
> -if sqlversion[0] < 3 or (sqlversion[0] == 3 and sqlversion[1] < 3):
> - raise Exception("sqlite3 version 3.3.0 or later is required.")
> -
> #
> # "No History" mode - for a given query tuple (version, pkgarch, checksum),
> # the returned value will be the largest among all the values of the same
> @@ -31,40 +23,28 @@ if sqlversion[0] < 3 or (sqlversion[0] == 3 and sqlversion[1] < 3):
> # "History" mode - Return a new higher value for previously unseen query
> # tuple (version, pkgarch, checksum), otherwise return historical value.
> # Value can decrement if returning to a previous build.
> -#
>
> class PRTable(object):
> def __init__(self, conn, table, read_only):
> self.conn = conn
> self.read_only = read_only
> - self.dirty = False
> self.table = table
>
> - if self.read_only:
> - table_exists = self._execute(
> - "SELECT count(*) FROM sqlite_master \
> - WHERE type='table' AND name='%s'" % (self.table))
> - if not table_exists:
> - raise prserv.NotFoundError
> - else:
> - self._execute("CREATE TABLE IF NOT EXISTS %s \
> - (version TEXT NOT NULL, \
> - pkgarch TEXT NOT NULL, \
> - checksum TEXT NOT NULL, \
> - value TEXT, \
> - PRIMARY KEY (version, pkgarch, checksum, value));" % self.table)
> -
> - def _execute(self, *query):
> - """Execute a query, waiting to acquire a lock if necessary"""
> - start = time.time()
> - end = start + 20
> - while True:
> - try:
> - return self.conn.execute(*query)
> - except sqlite3.OperationalError as exc:
> - if "is locked" in str(exc) and end > time.time():
> - continue
> - raise exc
> + with closing(self.conn.cursor()) as cursor:
> + if self.read_only:
> + table_exists = cursor.execute(
> + "SELECT count(*) FROM sqlite_master \
> + WHERE type='table' AND name='%s'" % (self.table))
> + if not table_exists:
> + raise prserv.NotFoundError
This is a race if you run the read-only server first, which seems like
a bad user experience. I'd probably create the table even if the
server is read-only (I'm pretty sure hash server works this way also)
> + else:
> + cursor.execute("CREATE TABLE IF NOT EXISTS %s \
> + (version TEXT NOT NULL, \
> + pkgarch TEXT NOT NULL, \
> + checksum TEXT NOT NULL, \
> + value TEXT, \
> + PRIMARY KEY (version, pkgarch, checksum, value));" % self.table)
> + self.conn.commit()
>
> def _extremum_value(self, rows, is_max):
> value = None
> @@ -88,49 +68,42 @@ class PRTable(object):
> def _min_value(self, rows):
> return self._extremum_value(rows, False)
>
> - def sync(self):
> - if not self.read_only:
> - self.conn.commit()
> - self._execute("BEGIN EXCLUSIVE TRANSACTION")
> -
> - def sync_if_dirty(self):
> - if self.dirty:
> - self.sync()
> - self.dirty = False
> -
> def test_package(self, version, pkgarch):
> """Returns whether the specified package version is found in the database for the specified architecture"""
>
> # Just returns the value if found or None otherwise
> - data=self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=?;" % self.table,
> - (version, pkgarch))
> - row=data.fetchone()
> - if row is not None:
> - return True
> - else:
> - return False
> + with closing(self.conn.cursor()) as cursor:
> + data=cursor.execute("SELECT value FROM %s WHERE version=? AND pkgarch=?;" % self.table,
> + (version, pkgarch))
> + row=data.fetchone()
> + if row is not None:
> + return True
> + else:
> + return False
>
> def test_value(self, version, pkgarch, value):
> """Returns whether the specified value is found in the database for the specified package and architecture"""
>
> # Just returns the value if found or None otherwise
> - data=self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? and value=?;" % self.table,
> - (version, pkgarch, value))
> - row=data.fetchone()
> - if row is not None:
> - return True
> - else:
> - return False
> + with closing(self.conn.cursor()) as cursor:
> + data=cursor.execute("SELECT value FROM %s WHERE version=? AND pkgarch=? and value=?;" % self.table,
> + (version, pkgarch, value))
> + row=data.fetchone()
> + if row is not None:
> + return True
> + else:
> + return False
>
>
> def find_package_max_value(self, version, pkgarch):
> """Returns the greatest value for (version, pkgarch), or None if not found. Doesn't create a new value"""
>
> - data = self._execute("SELECT value FROM %s where version=? AND pkgarch=?;" % (self.table),
> - (version, pkgarch))
> - rows = data.fetchall()
> - value = self._max_value(rows)
> - return value
> + with closing(self.conn.cursor()) as cursor:
> + data = cursor.execute("SELECT value FROM %s where version=? AND pkgarch=?;" % (self.table),
> + (version, pkgarch))
> + rows = data.fetchall()
> + value = self._max_value(rows)
> + return value
>
> def find_value(self, version, pkgarch, checksum, history=False):
> """Returns the value for the specified checksum if found or None otherwise."""
> @@ -145,10 +118,11 @@ class PRTable(object):
> """Returns the maximum (if is_max is True) or minimum (if is_max is False) value
> for (version, pkgarch, checksum), or None if not found. Doesn't create a new value"""
>
> - data = self._execute("SELECT value FROM %s where version=? AND pkgarch=? AND checksum=?;" % (self.table),
> - (version, pkgarch, checksum))
> - rows = data.fetchall()
> - return self._extremum_value(rows, is_max)
> + with closing(self.conn.cursor()) as cursor:
> + data = cursor.execute("SELECT value FROM %s where version=? AND pkgarch=? AND checksum=?;" % (self.table),
> + (version, pkgarch, checksum))
> + rows = data.fetchall()
> + return self._extremum_value(rows, is_max)
>
> def find_max_value(self, version, pkgarch, checksum):
> return self._find_extremum_value(version, pkgarch, checksum, True)
> @@ -160,26 +134,27 @@ class PRTable(object):
> """Take and increase the greatest "<base>.y" value for (version, pkgarch), or return "<base>.0" if not found.
> This doesn't store a new value."""
>
> - data = self._execute("SELECT value FROM %s where version=? AND pkgarch=? AND value LIKE '%s.%%';" % (self.table, base),
> - (version, pkgarch))
> - rows = data.fetchall()
> - value = self._max_value(rows)
> + with closing(self.conn.cursor()) as cursor:
> + data = cursor.execute("SELECT value FROM %s where version=? AND pkgarch=? AND value LIKE '%s.%%';" % (self.table, base),
> + (version, pkgarch))
> + rows = data.fetchall()
> + value = self._max_value(rows)
>
> - if value is not None:
> - return increase_revision(value)
> - else:
> - return base + ".0"
> + if value is not None:
> + return increase_revision(value)
> + else:
> + return base + ".0"
>
> def store_value(self, version, pkgarch, checksum, value):
> """Store new value in the database"""
>
> - try:
> - self._execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
> - (version, pkgarch, checksum, value))
> - except sqlite3.IntegrityError as exc:
> - logger.error(str(exc))
> -
> - self.dirty = True
> + with closing(self.conn.cursor()) as cursor:
> + try:
> + cursor.execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
> + (version, pkgarch, checksum, value))
> + except sqlite3.IntegrityError as exc:
> + logger.error(str(exc))
> + self.conn.commit()
>
> def _get_value(self, version, pkgarch, checksum, history):
>
> @@ -215,54 +190,56 @@ class PRTable(object):
> return None
>
> val = None
> - data = self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
> + with closing(self.conn.cursor()) as cursor:
> + data = cursor.execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
> (version, pkgarch, checksum))
> - row = data.fetchone()
> - if row is not None:
> - val=row[0]
> - else:
> - #no value found, try to insert
> - try:
> - self._execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
> - (version, pkgarch, checksum, value))
> - except sqlite3.IntegrityError as exc:
> - logger.error(str(exc))
> + row = data.fetchone()
> + if row is not None:
> + val=row[0]
> + else:
> + #no value found, try to insert
> + try:
> + cursor.execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
> + (version, pkgarch, checksum, value))
> + except sqlite3.IntegrityError as exc:
> + logger.error(str(exc))
You probably want to make this whole SELECT / INSERT block a loop, and
instead of repeating the SELECT below, retry the loop on an integrity
error. The only reason you should be getting an integrity error is if
you have a race between multiple servers accessing the database at
once. In that case, looping back and trying to fall out in the
previous SELECT is better.
This is especially important if you delete entries in the database, as
in pathological cases you might have to loop several times before you
fall out of the loop (although typically, you would only need to retry
once).
I'd recommend this paradigm any other places you do SELECT / INSERT
where the INSERT depends on the SELECT.
>
> - self.dirty = True
> + self.conn.commit()
>
> - data = self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
> + data = cursor.execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=?;" % self.table,
> (version, pkgarch, checksum))
> - row = data.fetchone()
> - if row is not None:
> - val = row[0]
> + row = data.fetchone()
> + if row is not None:
> + val = row[0]
> return val
>
> def _import_no_hist(self, version, pkgarch, checksum, value):
> if self.read_only:
> return None
>
> - try:
> - #try to insert
> - self._execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
> - (version, pkgarch, checksum, value))
> - except sqlite3.IntegrityError as exc:
> - #already have the record, try to update
> + with closing(self.conn.cursor()) as cursor:
> try:
> - self._execute("UPDATE %s SET value=? WHERE version=? AND pkgarch=? AND checksum=? AND value<?"
> - % (self.table),
> - (value, version, pkgarch, checksum, value))
> + #try to insert
> + cursor.execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
> + (version, pkgarch, checksum, value))
> except sqlite3.IntegrityError as exc:
> - logger.error(str(exc))
> + #already have the record, try to update
> + try:
> + cursor.execute("UPDATE %s SET value=? WHERE version=? AND pkgarch=? AND checksum=? AND value<?"
> + % (self.table),
> + (value, version, pkgarch, checksum, value))
> + except sqlite3.IntegrityError as exc:
> + logger.error(str(exc))
Looping is suggested here also.
>
> - self.dirty = True
> + self.conn.commit()
>
> - data = self._execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=? AND value>=?;" % self.table,
> + data = cursor.execute("SELECT value FROM %s WHERE version=? AND pkgarch=? AND checksum=? AND value>=?;" % self.table,
> (version, pkgarch, checksum, value))
You already have all the data for everything you inserted.... I'd
probably just return based on what you already know instead of another
SELECT, otherwise this could race (with a deletion) and fail.
Alternatively, I think sqlite can return the data for the
just-inserted-row, but I forget how off the top of my head.
> - row=data.fetchone()
> - if row is not None:
> - return row[0]
> - else:
> - return None
> + row=data.fetchone()
> + if row is not None:
> + return row[0]
> + else:
> + return None
>
> def importone(self, version, pkgarch, checksum, value, history=False):
> if history:
> @@ -272,56 +249,57 @@ class PRTable(object):
>
> def export(self, version, pkgarch, checksum, colinfo, history=False):
> metainfo = {}
> - #column info
> - if colinfo:
> - metainfo["tbl_name"] = self.table
> - metainfo["core_ver"] = prserv.__version__
> - metainfo["col_info"] = []
> - data = self._execute("PRAGMA table_info(%s);" % self.table)
> + with closing(self.conn.cursor()) as cursor:
> + #column info
> + if colinfo:
> + metainfo["tbl_name"] = self.table
> + metainfo["core_ver"] = prserv.__version__
> + metainfo["col_info"] = []
> + data = cursor.execute("PRAGMA table_info(%s);" % self.table)
> + for row in data:
> + col = {}
> + col["name"] = row["name"]
> + col["type"] = row["type"]
> + col["notnull"] = row["notnull"]
> + col["dflt_value"] = row["dflt_value"]
> + col["pk"] = row["pk"]
> + metainfo["col_info"].append(col)
> +
> + #data info
> + datainfo = []
> +
> + if history:
> + sqlstmt = "SELECT * FROM %s as T1 WHERE 1=1 " % self.table
> + else:
> + sqlstmt = "SELECT T1.version, T1.pkgarch, T1.checksum, T1.value FROM %s as T1, \
> + (SELECT version, pkgarch, max(value) as maxvalue FROM %s GROUP BY version, pkgarch) as T2 \
> + WHERE T1.version=T2.version AND T1.pkgarch=T2.pkgarch AND T1.value=T2.maxvalue " % (self.table, self.table)
> + sqlarg = []
> + where = ""
> + if version:
> + where += "AND T1.version=? "
> + sqlarg.append(str(version))
> + if pkgarch:
> + where += "AND T1.pkgarch=? "
> + sqlarg.append(str(pkgarch))
> + if checksum:
> + where += "AND T1.checksum=? "
> + sqlarg.append(str(checksum))
> +
> + sqlstmt += where + ";"
> +
> + if len(sqlarg):
> + data = cursor.execute(sqlstmt, tuple(sqlarg))
> + else:
> + data = cursor.execute(sqlstmt)
> for row in data:
> - col = {}
> - col["name"] = row["name"]
> - col["type"] = row["type"]
> - col["notnull"] = row["notnull"]
> - col["dflt_value"] = row["dflt_value"]
> - col["pk"] = row["pk"]
> - metainfo["col_info"].append(col)
> -
> - #data info
> - datainfo = []
> -
> - if history:
> - sqlstmt = "SELECT * FROM %s as T1 WHERE 1=1 " % self.table
> - else:
> - sqlstmt = "SELECT T1.version, T1.pkgarch, T1.checksum, T1.value FROM %s as T1, \
> - (SELECT version, pkgarch, max(value) as maxvalue FROM %s GROUP BY version, pkgarch) as T2 \
> - WHERE T1.version=T2.version AND T1.pkgarch=T2.pkgarch AND T1.value=T2.maxvalue " % (self.table, self.table)
> - sqlarg = []
> - where = ""
> - if version:
> - where += "AND T1.version=? "
> - sqlarg.append(str(version))
> - if pkgarch:
> - where += "AND T1.pkgarch=? "
> - sqlarg.append(str(pkgarch))
> - if checksum:
> - where += "AND T1.checksum=? "
> - sqlarg.append(str(checksum))
> -
> - sqlstmt += where + ";"
> -
> - if len(sqlarg):
> - data = self._execute(sqlstmt, tuple(sqlarg))
> - else:
> - data = self._execute(sqlstmt)
> - for row in data:
> - if row["version"]:
> - col = {}
> - col["version"] = row["version"]
> - col["pkgarch"] = row["pkgarch"]
> - col["checksum"] = row["checksum"]
> - col["value"] = row["value"]
> - datainfo.append(col)
> + if row["version"]:
> + col = {}
> + col["version"] = row["version"]
> + col["pkgarch"] = row["pkgarch"]
> + col["checksum"] = row["checksum"]
> + col["value"] = row["value"]
> + datainfo.append(col)
> return (metainfo, datainfo)
>
> def dump_db(self, fd):
> @@ -345,14 +323,15 @@ class PRData(object):
> raise e
> uri = "file:%s%s" % (self.filename, "?mode=ro" if self.read_only else "")
> logger.debug("Opening PRServ database '%s'" % (uri))
> - self.connection=sqlite3.connect(uri, uri=True, isolation_level="EXCLUSIVE", check_same_thread = False)
> + self.connection=sqlite3.connect(uri, uri=True)
> self.connection.row_factory=sqlite3.Row
> - if not self.read_only:
> - self.connection.execute("pragma synchronous = off;")
> - self.connection.execute("PRAGMA journal_mode = MEMORY;")
> + self.connection.execute("PRAGMA synchronous = OFF;")
> + self.connection.execute("PRAGMA journal_mode = WAL;")
> + self.connection.commit()
> self._tables={}
>
> def disconnect(self):
> + self.connection.commit()
> self.connection.close()
>
> def __getitem__(self, tblname):
> @@ -370,3 +349,4 @@ class PRData(object):
> del self._tables[tblname]
> logger.info("drop table %s" % (tblname))
> self.connection.execute("DROP TABLE IF EXISTS %s;" % tblname)
> + self.connection.commit()
> diff --git a/lib/prserv/serv.py b/lib/prserv/serv.py
> index 05573d06cc..fd673b1851 100644
> --- a/lib/prserv/serv.py
> +++ b/lib/prserv/serv.py
> @@ -44,8 +44,6 @@ class PRServerClient(bb.asyncrpc.AsyncServerConnection):
> except:
> self.server.table.sync()
> raise
> - else:
> - self.server.table.sync_if_dirty()
>
> async def handle_test_pr(self, request):
> '''Finds the PR value corresponding to the request. If not found, returns None and doesn't insert a new value'''
> @@ -233,15 +231,9 @@ class PRServer(bb.asyncrpc.AsyncServer):
> return tasks
>
> async def stop(self):
> - self.table.sync_if_dirty()
> self.db.disconnect()
> await super().stop()
>
> - def signal_handler(self):
> - super().signal_handler()
> - if self.table:
> - self.table.sync()
> -
> class PRServSingleton(object):
> def __init__(self, dbfile, logfile, host, port, upstream):
> self.dbfile = dbfile
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 4/5] prserv: enable database sharing
2024-04-29 20:34 ` Joshua Watt
@ 2024-04-30 12:58 ` Michael Opdenacker
2024-04-30 13:23 ` Joshua Watt
0 siblings, 1 reply; 20+ messages in thread
From: Michael Opdenacker @ 2024-04-30 12:58 UTC (permalink / raw)
To: Joshua Watt; +Cc: bitbake-devel, Tim Orling, Thomas Petazzoni
Hi Joshua
Many thanks for the review and guidance. See my comments and questions
below.
On 4/29/24 at 22:34, Joshua Watt wrote:
>> + with closing(self.conn.cursor()) as cursor:
>> + if self.read_only:
>> + table_exists = cursor.execute(
>> + "SELECT count(*) FROM sqlite_master \
>> + WHERE type='table' AND name='%s'" % (self.table))
>> + if not table_exists:
>> + raise prserv.NotFoundError
> This is a race if you run the read-only server first, which seems like
> a bad user experience. I'd probably create the table even if the
> server is read-only (I'm pretty sure hash server works this way also)
Fixed. Thanks.
>
>> + row = data.fetchone()
>> + if row is not None:
>> + val=row[0]
>> + else:
>> + #no value found, try to insert
>> + try:
>> + cursor.execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
>> + (version, pkgarch, checksum, value))
>> + except sqlite3.IntegrityError as exc:
>> + logger.error(str(exc))
> You probably want to make this whole SELECT / INSERT block a loop, and
> instead of repeating the SELECT below, retry the loop on an integrity
> error. The only reason you should be getting an integrity error is if
> you have a race between multiple servers accessing the database at
> once. In that case, looping back and trying to fall out in the
> previous SELECT is better.
>
> This is especially important if you delete entries in the database, as
> in pathological cases you might have to loop several times before you
> fall out of the loop (although typically, you would only need to retry
> once).
>
> I'd recommend this paradigm any other places you do SELECT / INSERT
> where the INSERT depends on the SELECT.
This is actually coming from the prserv legacy code, which I haven't
really touched yet.
Is this kind of event really expected to happen with sqlite3? I see that
hashserve only checks for "sqlite3.IntegrityError" once, so maybe it's
too pessimistic to anticipate errors here... Do we also want to support
multiple builders accessing the same PR database?
Anyway, if I really have to do it, I guess I'd also have to do it in the
store_value() function which currently just logs an error without
retrying. But if we loop, doesn't it add the risk to create an endless
loop?
Cheers,
Michael.
--
Michael Opdenacker, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 4/5] prserv: enable database sharing
2024-04-30 12:58 ` Michael Opdenacker
@ 2024-04-30 13:23 ` Joshua Watt
2024-04-30 13:55 ` Joshua Watt
0 siblings, 1 reply; 20+ messages in thread
From: Joshua Watt @ 2024-04-30 13:23 UTC (permalink / raw)
To: Michael Opdenacker; +Cc: bitbake-devel, Tim Orling, Thomas Petazzoni
On Tue, Apr 30, 2024 at 6:58 AM Michael Opdenacker
<michael.opdenacker@bootlin.com> wrote:
>
> Hi Joshua
>
> Many thanks for the review and guidance. See my comments and questions
> below.
>
> On 4/29/24 at 22:34, Joshua Watt wrote:
> >> + with closing(self.conn.cursor()) as cursor:
> >> + if self.read_only:
> >> + table_exists = cursor.execute(
> >> + "SELECT count(*) FROM sqlite_master \
> >> + WHERE type='table' AND name='%s'" % (self.table))
> >> + if not table_exists:
> >> + raise prserv.NotFoundError
> > This is a race if you run the read-only server first, which seems like
> > a bad user experience. I'd probably create the table even if the
> > server is read-only (I'm pretty sure hash server works this way also)
>
>
> Fixed. Thanks.
>
> >
> >> + row = data.fetchone()
> >> + if row is not None:
> >> + val=row[0]
> >> + else:
> >> + #no value found, try to insert
> >> + try:
> >> + cursor.execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
> >> + (version, pkgarch, checksum, value))
> >> + except sqlite3.IntegrityError as exc:
> >> + logger.error(str(exc))
> > You probably want to make this whole SELECT / INSERT block a loop, and
> > instead of repeating the SELECT below, retry the loop on an integrity
> > error. The only reason you should be getting an integrity error is if
> > you have a race between multiple servers accessing the database at
> > once. In that case, looping back and trying to fall out in the
> > previous SELECT is better.
> >
> > This is especially important if you delete entries in the database, as
> > in pathological cases you might have to loop several times before you
> > fall out of the loop (although typically, you would only need to retry
> > once).
> >
> > I'd recommend this paradigm any other places you do SELECT / INSERT
> > where the INSERT depends on the SELECT.
>
> This is actually coming from the prserv legacy code, which I haven't
> really touched yet.
>
> Is this kind of event really expected to happen with sqlite3? I see that
> hashserve only checks for "sqlite3.IntegrityError" once, so maybe it's
> too pessimistic to anticipate errors here... Do we also want to support
> multiple builders accessing the same PR database?
The hash server API and databases are designed to avoid a SELECT /
INSERT dependency. If you look, the APIs are either just reading with
SELECT, or doing an INSERT that ignores if the entry already exists.
After the INSERT, it sometimes does a SELECT to try and read back the
inserted data, but this is fine as long as it correctly handles the
case where it might not get back the data that was just inserted (if
it's racing with a delete), which it does correctly. (e.g. INSERT ->
SELECT is fine, SELECT -> INSERT requires a retry).
>
> Anyway, if I really have to do it, I guess I'd also have to do it in the
> store_value() function which currently just logs an error without
> retrying. But if we loop, doesn't it add the risk to create an endless
> loop?
Sans bugs, no. In order to loop again, the database state must change
between the SELECT and INSERT statements (remember, a cursor is not a
read-lock on the database). As such, some other server sharing the
database *must* have made progress and successfully done an INSERT, so
holistically, the entire system (all servers) have made progress and
it's not an endless loop.
>
> Cheers,
> Michael.
>
> --
>
> Michael Opdenacker, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 4/5] prserv: enable database sharing
2024-04-30 13:23 ` Joshua Watt
@ 2024-04-30 13:55 ` Joshua Watt
2024-04-30 14:38 ` Michael Opdenacker
0 siblings, 1 reply; 20+ messages in thread
From: Joshua Watt @ 2024-04-30 13:55 UTC (permalink / raw)
To: Michael Opdenacker; +Cc: bitbake-devel, Tim Orling, Thomas Petazzoni
On Tue, Apr 30, 2024 at 7:23 AM Joshua Watt <jpewhacker@gmail.com> wrote:
>
> On Tue, Apr 30, 2024 at 6:58 AM Michael Opdenacker
> <michael.opdenacker@bootlin.com> wrote:
> >
> > Hi Joshua
> >
> > Many thanks for the review and guidance. See my comments and questions
> > below.
> >
> > On 4/29/24 at 22:34, Joshua Watt wrote:
> > >> + with closing(self.conn.cursor()) as cursor:
> > >> + if self.read_only:
> > >> + table_exists = cursor.execute(
> > >> + "SELECT count(*) FROM sqlite_master \
> > >> + WHERE type='table' AND name='%s'" % (self.table))
> > >> + if not table_exists:
> > >> + raise prserv.NotFoundError
> > > This is a race if you run the read-only server first, which seems like
> > > a bad user experience. I'd probably create the table even if the
> > > server is read-only (I'm pretty sure hash server works this way also)
> >
> >
> > Fixed. Thanks.
> >
> > >
> > >> + row = data.fetchone()
> > >> + if row is not None:
> > >> + val=row[0]
> > >> + else:
> > >> + #no value found, try to insert
> > >> + try:
> > >> + cursor.execute("INSERT INTO %s VALUES (?, ?, ?, ?);" % (self.table),
> > >> + (version, pkgarch, checksum, value))
> > >> + except sqlite3.IntegrityError as exc:
> > >> + logger.error(str(exc))
> > > You probably want to make this whole SELECT / INSERT block a loop, and
> > > instead of repeating the SELECT below, retry the loop on an integrity
> > > error. The only reason you should be getting an integrity error is if
> > > you have a race between multiple servers accessing the database at
> > > once. In that case, looping back and trying to fall out in the
> > > previous SELECT is better.
> > >
> > > This is especially important if you delete entries in the database, as
> > > in pathological cases you might have to loop several times before you
> > > fall out of the loop (although typically, you would only need to retry
> > > once).
> > >
> > > I'd recommend this paradigm any other places you do SELECT / INSERT
> > > where the INSERT depends on the SELECT.
> >
> > This is actually coming from the prserv legacy code, which I haven't
> > really touched yet.
> >
> > Is this kind of event really expected to happen with sqlite3? I see that
> > hashserve only checks for "sqlite3.IntegrityError" once, so maybe it's
> > too pessimistic to anticipate errors here... Do we also want to support
> > multiple builders accessing the same PR database?
>
> The hash server API and databases are designed to avoid a SELECT /
> INSERT dependency. If you look, the APIs are either just reading with
> SELECT, or doing an INSERT that ignores if the entry already exists.
> After the INSERT, it sometimes does a SELECT to try and read back the
> inserted data, but this is fine as long as it correctly handles the
> case where it might not get back the data that was just inserted (if
> it's racing with a delete), which it does correctly. (e.g. INSERT ->
> SELECT is fine, SELECT -> INSERT requires a retry).
Sorry, also forgot to mention: if your use case is _just_ a read-write
server and a read-only server, then you are correct, you do not need
to worry about retries. I think that's your only use case for this
work, so the above statements are just for posterity, as I suspect we
will be doing more work on the PR server after you finish the current
work :)
>
> >
> > Anyway, if I really have to do it, I guess I'd also have to do it in the
> > store_value() function which currently just logs an error without
> > retrying. But if we loop, doesn't it add the risk to create an endless
> > loop?
>
> Sans bugs, no. In order to loop again, the database state must change
> between the SELECT and INSERT statements (remember, a cursor is not a
> read-lock on the database). As such, some other server sharing the
> database *must* have made progress and successfully done an INSERT, so
> holistically, the entire system (all servers) have made progress and
> it's not an endless loop.
>
>
> >
> > Cheers,
> > Michael.
> >
> > --
> >
> > Michael Opdenacker, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> >
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 4/5] prserv: enable database sharing
2024-04-30 13:55 ` Joshua Watt
@ 2024-04-30 14:38 ` Michael Opdenacker
0 siblings, 0 replies; 20+ messages in thread
From: Michael Opdenacker @ 2024-04-30 14:38 UTC (permalink / raw)
To: Joshua Watt; +Cc: bitbake-devel, Tim Orling, Thomas Petazzoni
Hi Joshua,
On 4/30/24 at 15:55, Joshua Watt wrote:
>
> Sorry, also forgot to mention: if your use case is _just_ a read-write
> server and a read-only server, then you are correct, you do not need
> to worry about retries. I think that's your only use case for this
> work, so the above statements are just for posterity, as I suspect we
> will be doing more work on the PR server after you finish the current
> work :)
Great, good to know thanks!
This will make things simpler indeed.
Cheers
Michael.
--
Michael Opdenacker, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 5/5] prserv: add bitbake selftests
2024-04-29 20:14 [PATCH v5 0/5] prserv: add support for an "upstream" server michael.opdenacker
` (3 preceding siblings ...)
2024-04-29 20:14 ` [PATCH v5 4/5] prserv: enable database sharing michael.opdenacker
@ 2024-04-29 20:14 ` michael.opdenacker
2024-04-29 21:08 ` Joshua Watt
4 siblings, 1 reply; 20+ messages in thread
From: michael.opdenacker @ 2024-04-29 20:14 UTC (permalink / raw)
To: bitbake-devel
Cc: Michael Opdenacker, Joshua Watt, Tim Orling, Thomas Petazzoni
From: Michael Opdenacker <michael.opdenacker@bootlin.com>
Run them with "bitbake-selftest prserv.tests"
Signed-off-by: Michael Opdenacker <michael.opdenacker@bootlin.com>
Cc: Joshua Watt <JPEWhacker@gmail.com>
Cc: Tim Orling <ticotimo@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
bin/bitbake-selftest | 2 +
lib/prserv/tests.py | 416 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 418 insertions(+)
create mode 100644 bitbake/lib/prserv/tests.py
diff --git a/bin/bitbake-selftest b/bin/bitbake-selftest
index f25f23b1ae..ce901232fe 100755
--- a/bin/bitbake-selftest
+++ b/bin/bitbake-selftest
@@ -15,6 +15,7 @@ import unittest
try:
import bb
import hashserv
+ import prserv
import layerindexlib
except RuntimeError as exc:
sys.exit(str(exc))
@@ -33,6 +34,7 @@ tests = ["bb.tests.codeparser",
"bb.tests.utils",
"bb.tests.compression",
"hashserv.tests",
+ "prserv.tests",
"layerindexlib.tests.layerindexobj",
"layerindexlib.tests.restapi",
"layerindexlib.tests.cooker"]
diff --git a/lib/prserv/tests.py b/lib/prserv/tests.py
new file mode 100644
index 0000000000..051a03c876
--- /dev/null
+++ b/lib/prserv/tests.py
@@ -0,0 +1,416 @@
+#! /usr/bin/env python3
+#
+# Copyright (C) 2024 BitBake Contributors
+#
+# SPDX-License-Identifier: GPL-2.0-only
+#
+
+from . import create_server, create_client, increase_revision, revision_greater, revision_smaller, _revision_greater_or_equal
+import prserv.db as db
+from bb.asyncrpc import InvokeError
+import logging
+import os
+import sys
+import tempfile
+import unittest
+import socket
+import subprocess
+from pathlib import Path
+
+THIS_DIR = Path(__file__).parent
+BIN_DIR = THIS_DIR.parent.parent / "bin"
+
+version = "dummy-1.0-r0"
+pkgarch = "core2-64"
+other_arch = "aarch64"
+
+checksumX = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4f0"
+checksum0 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a0"
+checksum1 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a1"
+checksum2 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a2"
+checksum3 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a3"
+checksum4 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a4"
+checksum5 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a5"
+checksum6 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a6"
+checksum7 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a7"
+checksum8 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a8"
+checksum9 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a9"
+checksum10 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4aa"
+
+def server_prefunc(server, name):
+ logging.basicConfig(level=logging.DEBUG, filename='prserv-%s.log' % name, filemode='w',
+ format='%(levelname)s %(filename)s:%(lineno)d %(message)s')
+ server.logger.debug("Running server %s" % name)
+ sys.stdout = open('prserv-stdout-%s.log' % name, 'w')
+ sys.stderr = sys.stdout
+
+class PRTestSetup(object):
+
+ def start_server(self, name, dbfile, upstream=None, read_only=False, prefunc=server_prefunc):
+
+ def cleanup_server(server):
+ if server.process.exitcode is not None:
+ return
+ server.process.terminate()
+ server.process.join()
+
+ server = create_server(socket.gethostbyname("localhost") + ":0",
+ dbfile,
+ upstream=upstream,
+ read_only=read_only)
+
+ server.serve_as_process(prefunc=prefunc, args=(name,))
+ self.addCleanup(cleanup_server, server)
+
+ return server
+
+ def start_client(self, server_address):
+ def cleanup_client(client):
+ client.close()
+
+ client = create_client(server_address)
+ self.addCleanup(cleanup_client, client)
+
+ return client
+
+class FunctionTests(unittest.TestCase):
+
+ def test_0a_increase_revision(self):
+ self.assertEqual(increase_revision("1"), "2")
+ self.assertEqual(increase_revision("1.0"), "1.1")
+ self.assertEqual(increase_revision("1.1.1"), "1.1.2")
+ self.assertEqual(increase_revision("1.1.1.3"), "1.1.1.4")
+ self.assertRaises(ValueError, increase_revision, "1.a")
+ self.assertRaises(ValueError, increase_revision, "1.")
+ self.assertRaises(ValueError, increase_revision, "")
+
+ def test_0b_revision_greater_or_equal(self):
+ self.assertTrue(_revision_greater_or_equal("2", "2"))
+ self.assertTrue(_revision_greater_or_equal("2", "1"))
+ self.assertTrue(_revision_greater_or_equal("10", "2"))
+ self.assertTrue(_revision_greater_or_equal("1.10", "1.2"))
+ self.assertFalse(_revision_greater_or_equal("1.2", "1.10"))
+ self.assertTrue(_revision_greater_or_equal("1.10", "1"))
+ self.assertTrue(_revision_greater_or_equal("1.10.1", "1.10"))
+ self.assertFalse(_revision_greater_or_equal("1.10.1", "1.10.2"))
+ self.assertTrue(_revision_greater_or_equal("1.10.1", "1.10.1"))
+ self.assertTrue(_revision_greater_or_equal("1.10.1", "1"))
+ self.assertTrue(revision_greater("1.20", "1.3"))
+ self.assertTrue(revision_smaller("1.3", "1.20"))
+
+ # DB tests
+
+ def test_0b_db(self):
+ dbfile = "testtable.sqlite3"
+ if os.path.exists(dbfile):
+ os.remove(dbfile)
+
+ self.db = db.PRData(dbfile)
+ self.table = self.db["PRMAIN"]
+
+ self.table.store_value(version, pkgarch, checksum0, "0")
+ self.table.store_value(version, pkgarch, checksum1, "1")
+ # "No history" mode supports multiple PRs for the same checksum
+ self.table.store_value(version, pkgarch, checksum0, "2")
+ self.table.store_value(version, pkgarch, checksum2, "1.0")
+
+ self.assertTrue(self.table.test_package(version, pkgarch))
+ self.assertFalse(self.table.test_package(version, other_arch))
+
+ self.assertTrue(self.table.test_value(version, pkgarch, "0"))
+ self.assertTrue(self.table.test_value(version, pkgarch, "1"))
+ self.assertTrue(self.table.test_value(version, pkgarch, "2"))
+
+ self.assertEqual(self.table.find_package_max_value(version, pkgarch), "2")
+
+ self.assertEqual(self.table.find_min_value(version, pkgarch, checksum0), "0")
+ self.assertEqual(self.table.find_max_value(version, pkgarch, checksum0), "2")
+
+ # Test history modes
+ self.assertEqual(self.table.find_value(version, pkgarch, checksum0, True), "0")
+ self.assertEqual(self.table.find_value(version, pkgarch, checksum0, False), "2")
+
+ self.assertEqual(self.table.find_new_subvalue(version, pkgarch, "3"), "3.0")
+ self.assertEqual(self.table.find_new_subvalue(version, pkgarch, "1"), "1.1")
+
+ # Revision comparison tests
+ self.table.store_value(version, pkgarch, checksum1, "1.3")
+ self.table.store_value(version, pkgarch, checksum1, "1.20")
+ self.assertEqual(self.table.find_min_value(version, pkgarch, checksum1), "1")
+ self.assertEqual(self.table.find_max_value(version, pkgarch, checksum1), "1.20")
+
+class PRBasicTests(PRTestSetup, unittest.TestCase):
+
+ def setUp(self):
+ dbfile = "prtest-basic.sqlite3"
+ if os.path.exists(dbfile):
+ os.remove(dbfile)
+
+ self.server1 = self.start_server("basic", dbfile)
+ self.client1 = self.start_client(self.server1.address)
+
+ def test_1a_basic(self):
+
+ # Checks on non existing configuration
+
+ result = self.client1.test_pr(version, pkgarch, checksum0)
+ self.assertIsNone(result, "test_pr should return 'None' for a non existing PR")
+
+ result = self.client1.test_package(version, pkgarch)
+ self.assertFalse(result, "test_package should return 'False' for a non existing PR")
+
+ result = self.client1.max_package_pr(version, pkgarch)
+ self.assertIsNone(result, "max_package_pr should return 'None' for a non existing PR")
+
+ # Add a first configuration
+
+ result = self.client1.getPR(version, pkgarch, checksum0)
+ self.assertEqual(result, "0", "getPR: initial PR of a package should be '0'")
+
+ result = self.client1.test_pr(version, pkgarch, checksum0)
+ self.assertEqual(result, "0", "test_pr should return '0' here, matching the result of getPR")
+
+ result = self.client1.test_package(version, pkgarch)
+ self.assertTrue(result, "test_package should return 'True' for an existing PR")
+
+ result = self.client1.max_package_pr(version, pkgarch)
+ self.assertEqual(result, "0", "max_package_pr should return '0' in the current test series")
+
+ # Check that the same request gets the same value
+
+ result = self.client1.getPR(version, pkgarch, checksum0)
+ self.assertEqual(result, "0", "getPR: asking for the same PR a second time in a row should return the same value.")
+
+ # Add new configurations
+
+ result = self.client1.getPR(version, pkgarch, checksum1)
+ self.assertEqual(result, "1", "getPR: second PR of a package should be '1'")
+
+ result = self.client1.test_pr(version, pkgarch, checksum1)
+ self.assertEqual(result, "1", "test_pr should return '1' here, matching the result of getPR")
+
+ result = self.client1.max_package_pr(version, pkgarch)
+ self.assertEqual(result, "1", "max_package_pr should return '1' in the current test series")
+
+ result = self.client1.getPR(version, pkgarch, checksum2)
+ self.assertEqual(result, "2", "getPR: second PR of a package should be '2'")
+
+ result = self.client1.test_pr(version, pkgarch, checksum2)
+ self.assertEqual(result, "2", "test_pr should return '2' here, matching the result of getPR")
+
+ result = self.client1.max_package_pr(version, pkgarch)
+ self.assertEqual(result, "2", "max_package_pr should return '2' in the current test series")
+
+ result = self.client1.getPR(version, pkgarch, checksum3)
+ self.assertEqual(result, "3", "getPR: second PR of a package should be '3'")
+
+ result = self.client1.test_pr(version, pkgarch, checksum3)
+ self.assertEqual(result, "3", "test_pr should return '3' here, matching the result of getPR")
+
+ result = self.client1.max_package_pr(version, pkgarch)
+ self.assertEqual(result, "3", "max_package_pr should return '3' in the current test series")
+
+ # Ask again for the first configuration
+
+ result = self.client1.getPR(version, pkgarch, checksum0)
+ self.assertEqual(result, "4", "getPR: should return '4' in this configuration")
+
+ # Ask again with explicit "no history" mode
+
+ result = self.client1.getPR(version, pkgarch, checksum0, False)
+ self.assertEqual(result, "4", "getPR: should return '4' in this configuration")
+
+ # Ask again with explicit "history" mode. This should return the first recorded PR for checksum0
+
+ result = self.client1.getPR(version, pkgarch, checksum0, True)
+ self.assertEqual(result, "0", "getPR: should return '0' in this configuration")
+
+ # Check again that another pkgarg resets the counters
+
+ result = self.client1.test_pr(version, other_arch, checksum0)
+ self.assertIsNone(result, "test_pr should return 'None' for a non existing PR")
+
+ result = self.client1.test_package(version, other_arch)
+ self.assertFalse(result, "test_package should return 'False' for a non existing PR")
+
+ result = self.client1.max_package_pr(version, other_arch)
+ self.assertIsNone(result, "max_package_pr should return 'None' for a non existing PR")
+
+ # Now add the configuration
+
+ result = self.client1.getPR(version, other_arch, checksum0)
+ self.assertEqual(result, "0", "getPR: initial PR of a package should be '0'")
+
+ result = self.client1.test_pr(version, other_arch, checksum0)
+ self.assertEqual(result, "0", "test_pr should return '0' here, matching the result of getPR")
+
+ result = self.client1.test_package(version, other_arch)
+ self.assertTrue(result, "test_package should return 'True' for an existing PR")
+
+ result = self.client1.max_package_pr(version, other_arch)
+ self.assertEqual(result, "0", "max_package_pr should return '0' in the current test series")
+
+ def test_1b_is_readonly(self):
+ result = self.client1.is_readonly()
+ self.assertFalse(result, "Server should not be described as 'read-only'")
+
+class PRReadOnlyTests(PRTestSetup, unittest.TestCase):
+
+ def setUp(self):
+ # Here we're using the database test table, as for reasons not understood yet,
+ # the other tables we created from the PR server in prior tests, have empty contents!
+ dbfile = "testtable.sqlite3"
+ self.server1 = self.start_server("basic-readonly", dbfile, read_only=True)
+ self.client1 = self.start_client(self.server1.address)
+
+ def test_2a_read_only_server(self):
+
+ result = self.client1.is_readonly()
+ self.assertTrue(result, "Database should be described as 'read-only'")
+
+ # Checks on non existing configuration
+ self.assertIsNone(self.client1.test_pr(version, pkgarch, checksumX))
+ self.assertFalse(self.client1.test_package("unknown", pkgarch))
+
+ # Look up an existing configuration
+ self.assertEqual(self.client1.getPR(version, pkgarch, checksum0), "2") # "no history" mode
+ self.assertEqual(self.client1.getPR(version, pkgarch, checksum0, True), "0") # "history" mode
+ self.assertEqual(self.client1.getPR(version, pkgarch, checksum2), "3")
+ self.assertEqual(self.client1.getPR(version, pkgarch, checksum2, True), "1.0")
+ self.assertEqual(self.client1.max_package_pr(version, pkgarch), "2")
+
+ # Try to insert a new value. The value should be correct given the read-only dababase, but won't be saved
+ self.assertEqual(self.client1.getPR(version, pkgarch, checksum3), "3")
+ # Same of another value
+ self.assertEqual(self.client1.getPR(version, pkgarch, checksum3), "3")
+
+class PRUpstreamTests(PRTestSetup, unittest.TestCase):
+
+ def setUp(self):
+
+ dbfile2 = "prtest-upstream2.sqlite3"
+ if os.path.exists(dbfile2):
+ os.remove(dbfile2)
+
+ self.server2 = self.start_server("upstream2", dbfile2)
+ self.client2 = self.start_client(self.server2.address)
+
+ dbfile1 = "prtest-upstream1.sqlite3"
+ if os.path.exists(dbfile1):
+ os.remove(dbfile1)
+
+ self.server1 = self.start_server("upstream1", dbfile1, upstream=self.server2.address)
+ self.client1 = self.start_client(self.server1.address)
+
+ dbfile0 = "prtest-local.sqlite3"
+ if os.path.exists(dbfile0):
+ os.remove(dbfile0)
+
+ self.server0 = self.start_server("local", dbfile0, upstream=self.server1.address)
+ self.client0 = self.start_client(self.server0.address)
+ self.shared_db = dbfile0
+
+ def test_3a_upstream(self):
+
+ # For identical checksums, all servers should return the same PR
+
+ result = self.client2.getPR(version, pkgarch, checksum0)
+ self.assertEqual(result, "0", "getPR: initial PR of a package should be '0'")
+
+ result = self.client1.getPR(version, pkgarch, checksum0)
+ self.assertEqual(result, "0", "getPR: initial PR of a package should be '0' (same as upstream)")
+
+ result = self.client0.getPR(version, pkgarch, checksum0)
+ self.assertEqual(result, "0", "getPR: initial PR of a package should be '0' (same as upstream)")
+
+ # Now introduce new checksums on server1 for, same version
+
+ result = self.client1.getPR(version, pkgarch, checksum1)
+ self.assertEqual(result, "0.0", "getPR: first PR of a package which has a different checksum upstream should be '0.0'")
+
+ result = self.client1.getPR(version, pkgarch, checksum2)
+ self.assertEqual(result, "0.1", "getPR: second PR of a package that has a different checksum upstream should be '0.1'")
+
+ # Now introduce checksums on server0 for, same version
+
+ result = self.client1.getPR(version, pkgarch, checksum1)
+ self.assertEqual(result, "0.2", "getPR: can't decrease for known PR")
+
+ result = self.client1.getPR(version, pkgarch, checksum2)
+ self.assertEqual(result, "0.3")
+
+ result = self.client1.max_package_pr(version, pkgarch)
+ self.assertEqual(result, "0.3")
+
+ result = self.client0.getPR(version, pkgarch, checksum3)
+ self.assertEqual(result, "0.3.0", "getPR: first PR of a package that doesn't exist upstream should be '0.3.0'")
+
+ result = self.client0.getPR(version, pkgarch, checksum4)
+ self.assertEqual(result, "0.3.1", "getPR: second PR of a package that doesn't exist upstream should be '0.3.1'")
+
+ result = self.client0.getPR(version, pkgarch, checksum3)
+ self.assertEqual(result, "0.3.2")
+
+ # More upstream updates
+ # Here, we assume no communication between server2 and server0. server2 only impacts server0
+ # after impacting server1
+
+ self.assertEqual(self.client2.getPR(version, pkgarch, checksum5), "1")
+ self.assertEqual(self.client1.getPR(version, pkgarch, checksum6), "1.0")
+ self.assertEqual(self.client1.getPR(version, pkgarch, checksum7), "1.1")
+ self.assertEqual(self.client0.getPR(version, pkgarch, checksum8), "1.1.0")
+ self.assertEqual(self.client0.getPR(version, pkgarch, checksum9), "1.1.1")
+
+ # "history" mode tests
+
+ self.assertEqual(self.client2.getPR(version, pkgarch, checksum0, True), "0")
+ self.assertEqual(self.client1.getPR(version, pkgarch, checksum2, True), "0.1")
+ self.assertEqual(self.client0.getPR(version, pkgarch, checksum3, True), "0.3.0")
+
+ # More "no history" mode tests
+
+ self.assertEqual(self.client2.getPR(version, pkgarch, checksum0), "2")
+ self.assertEqual(self.client1.getPR(version, pkgarch, checksum0), "2") # Same as upstream
+ self.assertEqual(self.client0.getPR(version, pkgarch, checksum0), "2") # Same as upstream
+ self.assertEqual(self.client1.getPR(version, pkgarch, checksum7), "3") # This could be surprising, but since the previous revision was "2", increasing it yields "3".
+ # We don't know how many upstream servers we have
+ def test_3b_readonly(self):
+
+ # Start read-only server
+ self.server_ro = self.start_server("local-ro", self.shared_db, upstream=self.server1.address, read_only=True)
+ self.client_ro = self.start_client(self.server_ro.address)
+
+ self.assertTrue(self.client_ro.is_readonly(), "Database should be described as 'read-only'")
+
+ # Checks on non existing configurations
+ self.assertIsNone(self.client_ro.test_pr(version, pkgarch, checksumX))
+ self.assertFalse(self.client_ro.test_package("unknown", pkgarch))
+
+ # Look up existing configurations
+ self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum0), "2") # "no history" mode
+ #self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum0, True), "0") # "history" mode
+ #self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum3), "0.3.0")
+ #self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum3, True), "0.3.0")
+ self.assertEqual(self.client_ro.max_package_pr(version, pkgarch), "3")
+
+ # Try to insert a new value. The value should be correct given the read-only dababase, but won't be saved
+ self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum7), "3")
+ # Same of another value which only exists in the upstream upstream
+ # It's as it's a completely new one. We may want the upstream server to ask its upstream server for this value though.
+ self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum8), "4")
+ # Try to insert a completely new value. It should also return "4" as the previous value wasn't saved by the read-only server.
+ self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum10), "4")
+
+class ScriptTests(unittest.TestCase):
+
+ def test_5a_start_bitbake_prserv(self):
+ try:
+ subprocess.check_call([BIN_DIR / "bitbake-prserv", "--start", "-f", "testtable.sqlite3"])
+ except subprocess.CalledProcessError as e:
+ self.fail("Failed to start bitbake-prserv: %s" % e.returncode)
+
+ def test_5b_stop_bitbake_prserv(self):
+ try:
+ subprocess.check_call([BIN_DIR / "bitbake-prserv", "--stop"])
+ except subprocess.CalledProcessError as e:
+ self.fail("Failed to stop bitbake-prserv: %s" % e.returncode)
--
2.34.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 5/5] prserv: add bitbake selftests
2024-04-29 20:14 ` [PATCH v5 5/5] prserv: add bitbake selftests michael.opdenacker
@ 2024-04-29 21:08 ` Joshua Watt
2024-04-30 8:29 ` Michael Opdenacker
0 siblings, 1 reply; 20+ messages in thread
From: Joshua Watt @ 2024-04-29 21:08 UTC (permalink / raw)
To: michael.opdenacker; +Cc: bitbake-devel, Tim Orling, Thomas Petazzoni
On Mon, Apr 29, 2024 at 2:14 PM <michael.opdenacker@bootlin.com> wrote:
>
> From: Michael Opdenacker <michael.opdenacker@bootlin.com>
>
> Run them with "bitbake-selftest prserv.tests"
>
> Signed-off-by: Michael Opdenacker <michael.opdenacker@bootlin.com>
> Cc: Joshua Watt <JPEWhacker@gmail.com>
> Cc: Tim Orling <ticotimo@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
> bin/bitbake-selftest | 2 +
> lib/prserv/tests.py | 416 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 418 insertions(+)
> create mode 100644 bitbake/lib/prserv/tests.py
>
> diff --git a/bin/bitbake-selftest b/bin/bitbake-selftest
> index f25f23b1ae..ce901232fe 100755
> --- a/bin/bitbake-selftest
> +++ b/bin/bitbake-selftest
> @@ -15,6 +15,7 @@ import unittest
> try:
> import bb
> import hashserv
> + import prserv
> import layerindexlib
> except RuntimeError as exc:
> sys.exit(str(exc))
> @@ -33,6 +34,7 @@ tests = ["bb.tests.codeparser",
> "bb.tests.utils",
> "bb.tests.compression",
> "hashserv.tests",
> + "prserv.tests",
> "layerindexlib.tests.layerindexobj",
> "layerindexlib.tests.restapi",
> "layerindexlib.tests.cooker"]
> diff --git a/lib/prserv/tests.py b/lib/prserv/tests.py
> new file mode 100644
> index 0000000000..051a03c876
> --- /dev/null
> +++ b/lib/prserv/tests.py
> @@ -0,0 +1,416 @@
> +#! /usr/bin/env python3
> +#
> +# Copyright (C) 2024 BitBake Contributors
> +#
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +
> +from . import create_server, create_client, increase_revision, revision_greater, revision_smaller, _revision_greater_or_equal
> +import prserv.db as db
> +from bb.asyncrpc import InvokeError
> +import logging
> +import os
> +import sys
> +import tempfile
> +import unittest
> +import socket
> +import subprocess
> +from pathlib import Path
> +
> +THIS_DIR = Path(__file__).parent
> +BIN_DIR = THIS_DIR.parent.parent / "bin"
> +
> +version = "dummy-1.0-r0"
> +pkgarch = "core2-64"
> +other_arch = "aarch64"
> +
> +checksumX = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4f0"
> +checksum0 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a0"
> +checksum1 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a1"
> +checksum2 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a2"
> +checksum3 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a3"
> +checksum4 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a4"
> +checksum5 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a5"
> +checksum6 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a6"
> +checksum7 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a7"
> +checksum8 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a8"
> +checksum9 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4a9"
> +checksum10 = "51bf8189dbe9ea81fa6dd89608bf19380c437a9cf12f6c6239887801ba4ab4aa"
> +
> +def server_prefunc(server, name):
> + logging.basicConfig(level=logging.DEBUG, filename='prserv-%s.log' % name, filemode='w',
> + format='%(levelname)s %(filename)s:%(lineno)d %(message)s')
> + server.logger.debug("Running server %s" % name)
> + sys.stdout = open('prserv-stdout-%s.log' % name, 'w')
> + sys.stderr = sys.stdout
> +
> +class PRTestSetup(object):
> +
> + def start_server(self, name, dbfile, upstream=None, read_only=False, prefunc=server_prefunc):
> +
> + def cleanup_server(server):
> + if server.process.exitcode is not None:
> + return
> + server.process.terminate()
> + server.process.join()
> +
> + server = create_server(socket.gethostbyname("localhost") + ":0",
> + dbfile,
> + upstream=upstream,
> + read_only=read_only)
> +
> + server.serve_as_process(prefunc=prefunc, args=(name,))
> + self.addCleanup(cleanup_server, server)
> +
> + return server
> +
> + def start_client(self, server_address):
> + def cleanup_client(client):
> + client.close()
> +
> + client = create_client(server_address)
> + self.addCleanup(cleanup_client, client)
> +
> + return client
> +
> +class FunctionTests(unittest.TestCase):
> +
> + def test_0a_increase_revision(self):
> + self.assertEqual(increase_revision("1"), "2")
> + self.assertEqual(increase_revision("1.0"), "1.1")
> + self.assertEqual(increase_revision("1.1.1"), "1.1.2")
> + self.assertEqual(increase_revision("1.1.1.3"), "1.1.1.4")
> + self.assertRaises(ValueError, increase_revision, "1.a")
> + self.assertRaises(ValueError, increase_revision, "1.")
> + self.assertRaises(ValueError, increase_revision, "")
> +
> + def test_0b_revision_greater_or_equal(self):
> + self.assertTrue(_revision_greater_or_equal("2", "2"))
> + self.assertTrue(_revision_greater_or_equal("2", "1"))
> + self.assertTrue(_revision_greater_or_equal("10", "2"))
> + self.assertTrue(_revision_greater_or_equal("1.10", "1.2"))
> + self.assertFalse(_revision_greater_or_equal("1.2", "1.10"))
> + self.assertTrue(_revision_greater_or_equal("1.10", "1"))
> + self.assertTrue(_revision_greater_or_equal("1.10.1", "1.10"))
> + self.assertFalse(_revision_greater_or_equal("1.10.1", "1.10.2"))
> + self.assertTrue(_revision_greater_or_equal("1.10.1", "1.10.1"))
> + self.assertTrue(_revision_greater_or_equal("1.10.1", "1"))
> + self.assertTrue(revision_greater("1.20", "1.3"))
> + self.assertTrue(revision_smaller("1.3", "1.20"))
> +
> + # DB tests
> +
> + def test_0b_db(self):
> + dbfile = "testtable.sqlite3"
> + if os.path.exists(dbfile):
> + os.remove(dbfile)
> +
> + self.db = db.PRData(dbfile)
> + self.table = self.db["PRMAIN"]
> +
> + self.table.store_value(version, pkgarch, checksum0, "0")
> + self.table.store_value(version, pkgarch, checksum1, "1")
> + # "No history" mode supports multiple PRs for the same checksum
> + self.table.store_value(version, pkgarch, checksum0, "2")
> + self.table.store_value(version, pkgarch, checksum2, "1.0")
> +
> + self.assertTrue(self.table.test_package(version, pkgarch))
> + self.assertFalse(self.table.test_package(version, other_arch))
> +
> + self.assertTrue(self.table.test_value(version, pkgarch, "0"))
> + self.assertTrue(self.table.test_value(version, pkgarch, "1"))
> + self.assertTrue(self.table.test_value(version, pkgarch, "2"))
> +
> + self.assertEqual(self.table.find_package_max_value(version, pkgarch), "2")
> +
> + self.assertEqual(self.table.find_min_value(version, pkgarch, checksum0), "0")
> + self.assertEqual(self.table.find_max_value(version, pkgarch, checksum0), "2")
> +
> + # Test history modes
> + self.assertEqual(self.table.find_value(version, pkgarch, checksum0, True), "0")
> + self.assertEqual(self.table.find_value(version, pkgarch, checksum0, False), "2")
> +
> + self.assertEqual(self.table.find_new_subvalue(version, pkgarch, "3"), "3.0")
> + self.assertEqual(self.table.find_new_subvalue(version, pkgarch, "1"), "1.1")
> +
> + # Revision comparison tests
> + self.table.store_value(version, pkgarch, checksum1, "1.3")
> + self.table.store_value(version, pkgarch, checksum1, "1.20")
> + self.assertEqual(self.table.find_min_value(version, pkgarch, checksum1), "1")
> + self.assertEqual(self.table.find_max_value(version, pkgarch, checksum1), "1.20")
> +
> +class PRBasicTests(PRTestSetup, unittest.TestCase):
> +
> + def setUp(self):
> + dbfile = "prtest-basic.sqlite3"
It's generally considered bad form to dump test files in the users CWD
and also blindly os.remove() things there :)
In the hash server tests, we create a temporary directory and clean it
up when the test ends by doing this in setUp():
self.temp_dir = tempfile.TemporaryDirectory(prefix='bb-hashserv')
self.addCleanup(self.temp_dir.cleanup)
Then use self.temp_dir as the base whenever you need a path, e.g:
dbfile = os.path.join(self.temp_dir, "prtest-basic.sqlite3")
> + if os.path.exists(dbfile):
> + os.remove(dbfile)
> +
> + self.server1 = self.start_server("basic", dbfile)
> + self.client1 = self.start_client(self.server1.address)
> +
> + def test_1a_basic(self):
> +
> + # Checks on non existing configuration
> +
> + result = self.client1.test_pr(version, pkgarch, checksum0)
> + self.assertIsNone(result, "test_pr should return 'None' for a non existing PR")
> +
> + result = self.client1.test_package(version, pkgarch)
> + self.assertFalse(result, "test_package should return 'False' for a non existing PR")
> +
> + result = self.client1.max_package_pr(version, pkgarch)
> + self.assertIsNone(result, "max_package_pr should return 'None' for a non existing PR")
> +
> + # Add a first configuration
> +
> + result = self.client1.getPR(version, pkgarch, checksum0)
> + self.assertEqual(result, "0", "getPR: initial PR of a package should be '0'")
> +
> + result = self.client1.test_pr(version, pkgarch, checksum0)
> + self.assertEqual(result, "0", "test_pr should return '0' here, matching the result of getPR")
> +
> + result = self.client1.test_package(version, pkgarch)
> + self.assertTrue(result, "test_package should return 'True' for an existing PR")
> +
> + result = self.client1.max_package_pr(version, pkgarch)
> + self.assertEqual(result, "0", "max_package_pr should return '0' in the current test series")
> +
> + # Check that the same request gets the same value
> +
> + result = self.client1.getPR(version, pkgarch, checksum0)
> + self.assertEqual(result, "0", "getPR: asking for the same PR a second time in a row should return the same value.")
> +
> + # Add new configurations
> +
> + result = self.client1.getPR(version, pkgarch, checksum1)
> + self.assertEqual(result, "1", "getPR: second PR of a package should be '1'")
> +
> + result = self.client1.test_pr(version, pkgarch, checksum1)
> + self.assertEqual(result, "1", "test_pr should return '1' here, matching the result of getPR")
> +
> + result = self.client1.max_package_pr(version, pkgarch)
> + self.assertEqual(result, "1", "max_package_pr should return '1' in the current test series")
> +
> + result = self.client1.getPR(version, pkgarch, checksum2)
> + self.assertEqual(result, "2", "getPR: second PR of a package should be '2'")
> +
> + result = self.client1.test_pr(version, pkgarch, checksum2)
> + self.assertEqual(result, "2", "test_pr should return '2' here, matching the result of getPR")
> +
> + result = self.client1.max_package_pr(version, pkgarch)
> + self.assertEqual(result, "2", "max_package_pr should return '2' in the current test series")
> +
> + result = self.client1.getPR(version, pkgarch, checksum3)
> + self.assertEqual(result, "3", "getPR: second PR of a package should be '3'")
> +
> + result = self.client1.test_pr(version, pkgarch, checksum3)
> + self.assertEqual(result, "3", "test_pr should return '3' here, matching the result of getPR")
> +
> + result = self.client1.max_package_pr(version, pkgarch)
> + self.assertEqual(result, "3", "max_package_pr should return '3' in the current test series")
> +
> + # Ask again for the first configuration
> +
> + result = self.client1.getPR(version, pkgarch, checksum0)
> + self.assertEqual(result, "4", "getPR: should return '4' in this configuration")
> +
> + # Ask again with explicit "no history" mode
> +
> + result = self.client1.getPR(version, pkgarch, checksum0, False)
> + self.assertEqual(result, "4", "getPR: should return '4' in this configuration")
> +
> + # Ask again with explicit "history" mode. This should return the first recorded PR for checksum0
> +
> + result = self.client1.getPR(version, pkgarch, checksum0, True)
> + self.assertEqual(result, "0", "getPR: should return '0' in this configuration")
> +
> + # Check again that another pkgarg resets the counters
> +
> + result = self.client1.test_pr(version, other_arch, checksum0)
> + self.assertIsNone(result, "test_pr should return 'None' for a non existing PR")
> +
> + result = self.client1.test_package(version, other_arch)
> + self.assertFalse(result, "test_package should return 'False' for a non existing PR")
> +
> + result = self.client1.max_package_pr(version, other_arch)
> + self.assertIsNone(result, "max_package_pr should return 'None' for a non existing PR")
> +
> + # Now add the configuration
> +
> + result = self.client1.getPR(version, other_arch, checksum0)
> + self.assertEqual(result, "0", "getPR: initial PR of a package should be '0'")
> +
> + result = self.client1.test_pr(version, other_arch, checksum0)
> + self.assertEqual(result, "0", "test_pr should return '0' here, matching the result of getPR")
> +
> + result = self.client1.test_package(version, other_arch)
> + self.assertTrue(result, "test_package should return 'True' for an existing PR")
> +
> + result = self.client1.max_package_pr(version, other_arch)
> + self.assertEqual(result, "0", "max_package_pr should return '0' in the current test series")
> +
> + def test_1b_is_readonly(self):
> + result = self.client1.is_readonly()
> + self.assertFalse(result, "Server should not be described as 'read-only'")
> +
> +class PRReadOnlyTests(PRTestSetup, unittest.TestCase):
> +
> + def setUp(self):
> + # Here we're using the database test table, as for reasons not understood yet,
> + # the other tables we created from the PR server in prior tests, have empty contents!
> + dbfile = "testtable.sqlite3"
> + self.server1 = self.start_server("basic-readonly", dbfile, read_only=True)
> + self.client1 = self.start_client(self.server1.address)
> +
> + def test_2a_read_only_server(self):
> +
> + result = self.client1.is_readonly()
> + self.assertTrue(result, "Database should be described as 'read-only'")
> +
> + # Checks on non existing configuration
> + self.assertIsNone(self.client1.test_pr(version, pkgarch, checksumX))
> + self.assertFalse(self.client1.test_package("unknown", pkgarch))
> +
> + # Look up an existing configuration
> + self.assertEqual(self.client1.getPR(version, pkgarch, checksum0), "2") # "no history" mode
> + self.assertEqual(self.client1.getPR(version, pkgarch, checksum0, True), "0") # "history" mode
> + self.assertEqual(self.client1.getPR(version, pkgarch, checksum2), "3")
> + self.assertEqual(self.client1.getPR(version, pkgarch, checksum2, True), "1.0")
> + self.assertEqual(self.client1.max_package_pr(version, pkgarch), "2")
> +
> + # Try to insert a new value. The value should be correct given the read-only dababase, but won't be saved
> + self.assertEqual(self.client1.getPR(version, pkgarch, checksum3), "3")
> + # Same of another value
> + self.assertEqual(self.client1.getPR(version, pkgarch, checksum3), "3")
> +
> +class PRUpstreamTests(PRTestSetup, unittest.TestCase):
> +
> + def setUp(self):
> +
> + dbfile2 = "prtest-upstream2.sqlite3"
> + if os.path.exists(dbfile2):
> + os.remove(dbfile2)
> +
> + self.server2 = self.start_server("upstream2", dbfile2)
> + self.client2 = self.start_client(self.server2.address)
> +
> + dbfile1 = "prtest-upstream1.sqlite3"
> + if os.path.exists(dbfile1):
> + os.remove(dbfile1)
> +
> + self.server1 = self.start_server("upstream1", dbfile1, upstream=self.server2.address)
> + self.client1 = self.start_client(self.server1.address)
> +
> + dbfile0 = "prtest-local.sqlite3"
> + if os.path.exists(dbfile0):
> + os.remove(dbfile0)
> +
> + self.server0 = self.start_server("local", dbfile0, upstream=self.server1.address)
> + self.client0 = self.start_client(self.server0.address)
> + self.shared_db = dbfile0
> +
> + def test_3a_upstream(self):
> +
> + # For identical checksums, all servers should return the same PR
> +
> + result = self.client2.getPR(version, pkgarch, checksum0)
> + self.assertEqual(result, "0", "getPR: initial PR of a package should be '0'")
> +
> + result = self.client1.getPR(version, pkgarch, checksum0)
> + self.assertEqual(result, "0", "getPR: initial PR of a package should be '0' (same as upstream)")
> +
> + result = self.client0.getPR(version, pkgarch, checksum0)
> + self.assertEqual(result, "0", "getPR: initial PR of a package should be '0' (same as upstream)")
> +
> + # Now introduce new checksums on server1 for, same version
> +
> + result = self.client1.getPR(version, pkgarch, checksum1)
> + self.assertEqual(result, "0.0", "getPR: first PR of a package which has a different checksum upstream should be '0.0'")
> +
> + result = self.client1.getPR(version, pkgarch, checksum2)
> + self.assertEqual(result, "0.1", "getPR: second PR of a package that has a different checksum upstream should be '0.1'")
> +
> + # Now introduce checksums on server0 for, same version
> +
> + result = self.client1.getPR(version, pkgarch, checksum1)
> + self.assertEqual(result, "0.2", "getPR: can't decrease for known PR")
> +
> + result = self.client1.getPR(version, pkgarch, checksum2)
> + self.assertEqual(result, "0.3")
> +
> + result = self.client1.max_package_pr(version, pkgarch)
> + self.assertEqual(result, "0.3")
> +
> + result = self.client0.getPR(version, pkgarch, checksum3)
> + self.assertEqual(result, "0.3.0", "getPR: first PR of a package that doesn't exist upstream should be '0.3.0'")
> +
> + result = self.client0.getPR(version, pkgarch, checksum4)
> + self.assertEqual(result, "0.3.1", "getPR: second PR of a package that doesn't exist upstream should be '0.3.1'")
> +
> + result = self.client0.getPR(version, pkgarch, checksum3)
> + self.assertEqual(result, "0.3.2")
> +
> + # More upstream updates
> + # Here, we assume no communication between server2 and server0. server2 only impacts server0
> + # after impacting server1
> +
> + self.assertEqual(self.client2.getPR(version, pkgarch, checksum5), "1")
> + self.assertEqual(self.client1.getPR(version, pkgarch, checksum6), "1.0")
> + self.assertEqual(self.client1.getPR(version, pkgarch, checksum7), "1.1")
> + self.assertEqual(self.client0.getPR(version, pkgarch, checksum8), "1.1.0")
> + self.assertEqual(self.client0.getPR(version, pkgarch, checksum9), "1.1.1")
> +
> + # "history" mode tests
> +
> + self.assertEqual(self.client2.getPR(version, pkgarch, checksum0, True), "0")
> + self.assertEqual(self.client1.getPR(version, pkgarch, checksum2, True), "0.1")
> + self.assertEqual(self.client0.getPR(version, pkgarch, checksum3, True), "0.3.0")
> +
> + # More "no history" mode tests
> +
> + self.assertEqual(self.client2.getPR(version, pkgarch, checksum0), "2")
> + self.assertEqual(self.client1.getPR(version, pkgarch, checksum0), "2") # Same as upstream
> + self.assertEqual(self.client0.getPR(version, pkgarch, checksum0), "2") # Same as upstream
> + self.assertEqual(self.client1.getPR(version, pkgarch, checksum7), "3") # This could be surprising, but since the previous revision was "2", increasing it yields "3".
> + # We don't know how many upstream servers we have
> + def test_3b_readonly(self):
> +
> + # Start read-only server
> + self.server_ro = self.start_server("local-ro", self.shared_db, upstream=self.server1.address, read_only=True)
> + self.client_ro = self.start_client(self.server_ro.address)
> +
> + self.assertTrue(self.client_ro.is_readonly(), "Database should be described as 'read-only'")
> +
> + # Checks on non existing configurations
> + self.assertIsNone(self.client_ro.test_pr(version, pkgarch, checksumX))
> + self.assertFalse(self.client_ro.test_package("unknown", pkgarch))
> +
> + # Look up existing configurations
> + self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum0), "2") # "no history" mode
> + #self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum0, True), "0") # "history" mode
> + #self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum3), "0.3.0")
> + #self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum3, True), "0.3.0")
> + self.assertEqual(self.client_ro.max_package_pr(version, pkgarch), "3")
> +
> + # Try to insert a new value. The value should be correct given the read-only dababase, but won't be saved
> + self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum7), "3")
> + # Same of another value which only exists in the upstream upstream
> + # It's as it's a completely new one. We may want the upstream server to ask its upstream server for this value though.
> + self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum8), "4")
> + # Try to insert a completely new value. It should also return "4" as the previous value wasn't saved by the read-only server.
> + self.assertEqual(self.client_ro.getPR(version, pkgarch, checksum10), "4")
> +
> +class ScriptTests(unittest.TestCase):
> +
> + def test_5a_start_bitbake_prserv(self):
> + try:
> + subprocess.check_call([BIN_DIR / "bitbake-prserv", "--start", "-f", "testtable.sqlite3"])
> + except subprocess.CalledProcessError as e:
> + self.fail("Failed to start bitbake-prserv: %s" % e.returncode)
> +
> + def test_5b_stop_bitbake_prserv(self):
> + try:
> + subprocess.check_call([BIN_DIR / "bitbake-prserv", "--stop"])
> + except subprocess.CalledProcessError as e:
> + self.fail("Failed to stop bitbake-prserv: %s" % e.returncode)
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 5/5] prserv: add bitbake selftests
2024-04-29 21:08 ` Joshua Watt
@ 2024-04-30 8:29 ` Michael Opdenacker
0 siblings, 0 replies; 20+ messages in thread
From: Michael Opdenacker @ 2024-04-30 8:29 UTC (permalink / raw)
To: Joshua Watt; +Cc: bitbake-devel, Tim Orling, Thomas Petazzoni
Hi Joshua
Many thanks for the review!
On 4/29/24 at 23:08, Joshua Watt wrote:
>> +
>> +class PRBasicTests(PRTestSetup, unittest.TestCase):
>> +
>> + def setUp(self):
>> + dbfile = "prtest-basic.sqlite3"
> It's generally considered bad form to dump test files in the users CWD
> and also blindly os.remove() things there :)
>
> In the hash server tests, we create a temporary directory and clean it
> up when the test ends by doing this in setUp():
>
> self.temp_dir = tempfile.TemporaryDirectory(prefix='bb-hashserv')
> self.addCleanup(self.temp_dir.cleanup)
>
> Then use self.temp_dir as the base whenever you need a path, e.g:
>
> dbfile = os.path.join(self.temp_dir, "prtest-basic.sqlite3")
Done! Good catch.
I remember now keeping the database files when I was implementing the
tests so that I could inspect the databases manually.
At least with this approach, I don't have to remove the database file if
it exists! This almost makes the code smaller :)
This will be included in my next branch update.
Thanks!
Michael.
--
Michael Opdenacker, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread