diff mbox series

[scarthgap,1/9] cve_check: Use a local copy of the database during builds

Message ID 468a9077d9b3466d60ee8c5b6144529ea5e5c849.1723636705.git.steve@sakoman.com
State Rejected
Delegated to: Steve Sakoman
Headers show
Series [scarthgap,1/9] cve_check: Use a local copy of the database during builds | expand

Commit Message

Steve Sakoman Aug. 14, 2024, 12:02 p.m. UTC 
From: Richard Purdie <richard.purdie@linuxfoundation.org>

Rtaher than trying to use a sqlite database over NFS from DL_DIR, work from
a local copy in STAGING DIR after fetching.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
(cherry picked from commit 03596904392d257572a905a182b92c780d636744)
Signed-off-by: Steve Sakoman <steve@sakoman.com>
---
 meta/classes/cve-check.bbclass                 |  7 ++++---
 .../meta/cve-update-nvd2-native.bb             | 18 +++++++++++++-----
 2 files changed, 17 insertions(+), 8 deletions(-)

Comments

Marta Rybczynska Aug. 14, 2024, 2:25 p.m. UTC | #1 
On Wed, Aug 14, 2024 at 2:02 PM Steve Sakoman via lists.openembedded.org
<steve=sakoman.com@lists.openembedded.org> wrote:

> From: Richard Purdie <richard.purdie@linuxfoundation.org>
>
> Rtaher than trying to use a sqlite database over NFS from DL_DIR, work from
> a local copy in STAGING DIR after fetching.
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> (cherry picked from commit 03596904392d257572a905a182b92c780d636744)
> Signed-off-by: Steve Sakoman <steve@sakoman.com>
>


This patch is changing the ABI (well... variables). I think we should come
back to the old naming
for the downloaded copy before backporting.

Regards,
Marta
Steve Sakoman Aug. 14, 2024, 2:33 p.m. UTC | #2 
Thanks for reviewing Marta!

I'll drop this backported patch for both scarthgap and kirkstone and
wait for appropriate versions of the patch to be submitted.

Steve

On Wed, Aug 14, 2024 at 7:25 AM Marta Rybczynska <rybczynska@gmail.com> wrote:
>
>
>
> On Wed, Aug 14, 2024 at 2:02 PM Steve Sakoman via lists.openembedded.org <steve=sakoman.com@lists.openembedded.org> wrote:
>>
>> From: Richard Purdie <richard.purdie@linuxfoundation.org>
>>
>> Rtaher than trying to use a sqlite database over NFS from DL_DIR, work from
>> a local copy in STAGING DIR after fetching.
>>
>> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
>> (cherry picked from commit 03596904392d257572a905a182b92c780d636744)
>> Signed-off-by: Steve Sakoman <steve@sakoman.com>
>
>
>
> This patch is changing the ABI (well... variables). I think we should come back to the old naming
> for the downloaded copy before backporting.
>
> Regards,
> Marta
Steve Sakoman Oct. 22, 2024, 12:59 p.m. UTC | #3 
Hi Marta,

We are still getting database errors on scarthgap (and occasionally on
kirkstone if memory serves), so I think we need to reconsider
backporting this patch.

Can you suggest changes that would make it acceptable to you?

Steve

On Wed, Aug 14, 2024 at 7:33 AM Steve Sakoman via
lists.openembedded.org <steve=sakoman.com@lists.openembedded.org>
wrote:
>
> Thanks for reviewing Marta!
>
> I'll drop this backported patch for both scarthgap and kirkstone and
> wait for appropriate versions of the patch to be submitted.
>
> Steve
>
> On Wed, Aug 14, 2024 at 7:25 AM Marta Rybczynska <rybczynska@gmail.com> wrote:
> >
> >
> >
> > On Wed, Aug 14, 2024 at 2:02 PM Steve Sakoman via lists.openembedded.org <steve=sakoman.com@lists.openembedded.org> wrote:
> >>
> >> From: Richard Purdie <richard.purdie@linuxfoundation.org>
> >>
> >> Rtaher than trying to use a sqlite database over NFS from DL_DIR, work from
> >> a local copy in STAGING DIR after fetching.
> >>
> >> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> >> (cherry picked from commit 03596904392d257572a905a182b92c780d636744)
> >> Signed-off-by: Steve Sakoman <steve@sakoman.com>
> >
> >
> >
> > This patch is changing the ABI (well... variables). I think we should come back to the old naming
> > for the downloaded copy before backporting.
> >
> > Regards,
> > Marta
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#203323): https://lists.openembedded.org/g/openembedded-core/message/203323
> Mute This Topic: https://lists.openembedded.org/mt/107893246/3620601
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [steve@sakoman.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Richard Purdie Oct. 22, 2024, 1:09 p.m. UTC | #4 
On Tue, 2024-10-22 at 05:59 -0700, Steve Sakoman via lists.openembedded.org wrote:
> Hi Marta,
> 
> We are still getting database errors on scarthgap (and occasionally on
> kirkstone if memory serves), so I think we need to reconsider
> backporting this patch.
> 
> Can you suggest changes that would make it acceptable to you?

Just for complete context, moving the database on master did isolate the database issues to scarthgap so the problem is in the older branches at this point.

Cheers,

Richard
Marta Rybczynska Oct. 25, 2024, 5:08 a.m. UTC | #5 
On Tue, Oct 22, 2024 at 3:09 PM Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Tue, 2024-10-22 at 05:59 -0700, Steve Sakoman via
> lists.openembedded.org wrote:
> > Hi Marta,
> >
> > We are still getting database errors on scarthgap (and occasionally on
> > kirkstone if memory serves), so I think we need to reconsider
> > backporting this patch.
> >
> > Can you suggest changes that would make it acceptable to you?
>
> Just for complete context, moving the database on master did isolate the
> database issues to scarthgap so the problem is in the older branches at
> this point.
>
>
Hello all,
To make a summary, this patch did not solve the issue on master, it was the
change of the database path that did. Is this correct? And we still do not
know what the cause is? So, when we change the path on all branches, the
issue is likely to come back ?

My problem with this patch is that it changes names of variables that are
in use. It should at least not fail silently for people who decide to use a
pinned database.

Do you see another solution than renaming the _DLDIR_ version to the old
name?

Kind regards,
Marta
Richard Purdie Oct. 29, 2024, 10:40 p.m. UTC | #6 
On Fri, 2024-10-25 at 07:08 +0200, Marta Rybczynska wrote:
> To make a summary, this patch did not solve the issue on master, it
> was the change of the database path that did.

Originally, I couldn't isolate the change to tell or not. Now, by
changing the database path we've shown the issue is occurring on the
branches without the patch.

>  Is this correct? And we still do not know what the cause is? So,
> when we change the path on all branches, the issue is likely to come
> back ?

I think if we move the database operations to the local filesystem and
out of DL_DIR, we're likely to stop seeing the issues. We know they're
isolated to something about the older releases.

> My problem with this patch is that it changes names of variables that
> are in use. It should at least not fail silently for people who
> decide to use a pinned database. 
> 
> Do you see another solution than renaming the _DLDIR_ version to the
> old name?

We either backport the patch as is and have the same functionality
across the branches, or we do some special version of the patch for the
LTS branches. In many ways I think it might be better to have things
consistent rather than doing something special with all the confusion
that can cause, even if people do have to actively adapt to the change.

Cheers,

Richard
Marta Rybczynska Nov. 5, 2024, 11:55 a.m. UTC | #7 
On Wed, 30 Oct 2024, 07:40 Richard Purdie, <
richard.purdie@linuxfoundation.org> wrote:

> On Fri, 2024-10-25 at 07:08 +0200, Marta Rybczynska wrote:
> > To make a summary, this patch did not solve the issue on master, it
> > was the change of the database path that did.
>
> Originally, I couldn't isolate the change to tell or not. Now, by
> changing the database path we've shown the issue is occurring on the
> branches without the patch.
>
> >  Is this correct? And we still do not know what the cause is? So,
> > when we change the path on all branches, the issue is likely to come
> > back ?
>
> I think if we move the database operations to the local filesystem and
> out of DL_DIR, we're likely to stop seeing the issues. We know they're
> isolated to something about the older releases.
>
> > My problem with this patch is that it changes names of variables that
> > are in use. It should at least not fail silently for people who
> > decide to use a pinned database.
> >
> > Do you see another solution than renaming the _DLDIR_ version to the
> > old name?
>
> We either backport the patch as is and have the same functionality
> across the branches, or we do some special version of the patch for the
> LTS branches. In many ways I think it might be better to have things
> consistent rather than doing something special with all the confusion
> that can cause, even if people do have to actively adapt to the change.
>

Well, I'm not happy with a change of variables in existing releases.
However the split would be complicated either way.

And as a bonus, I'm not able to reproduce on my systems (the lack of NFS
might be a factor).

So, I'll say go for this backport even if unhappy with it.

Kind regards,
Marta

>
>
Steve Sakoman Nov. 5, 2024, 1:53 p.m. UTC | #8 
On Tue, Nov 5, 2024 at 3:55 AM Marta Rybczynska <rybczynska@gmail.com> wrote:
>
>
>
> On Wed, 30 Oct 2024, 07:40 Richard Purdie, <richard.purdie@linuxfoundation.org> wrote:
>>
>> On Fri, 2024-10-25 at 07:08 +0200, Marta Rybczynska wrote:
>> > To make a summary, this patch did not solve the issue on master, it
>> > was the change of the database path that did.
>>
>> Originally, I couldn't isolate the change to tell or not. Now, by
>> changing the database path we've shown the issue is occurring on the
>> branches without the patch.
>>
>> >  Is this correct? And we still do not know what the cause is? So,
>> > when we change the path on all branches, the issue is likely to come
>> > back ?
>>
>> I think if we move the database operations to the local filesystem and
>> out of DL_DIR, we're likely to stop seeing the issues. We know they're
>> isolated to something about the older releases.
>>
>> > My problem with this patch is that it changes names of variables that
>> > are in use. It should at least not fail silently for people who
>> > decide to use a pinned database.
>> >
>> > Do you see another solution than renaming the _DLDIR_ version to the
>> > old name?
>>
>> We either backport the patch as is and have the same functionality
>> across the branches, or we do some special version of the patch for the
>> LTS branches. In many ways I think it might be better to have things
>> consistent rather than doing something special with all the confusion
>> that can cause, even if people do have to actively adapt to the change.
>
>
> Well, I'm not happy with a change of variables in existing releases. However the split would be complicated either way.
>
> And as a bonus, I'm not able to reproduce on my systems (the lack of NFS might be a factor).
>
> So, I'll say go for this backport even if unhappy with it.

Thanks Marta, I will proceed with that.

Steve4
diff mbox series

Patch

diff --git a/meta/classes/cve-check.bbclass b/meta/classes/cve-check.bbclass
index 93a2a1413d..c946de29a4 100644
--- a/meta/classes/cve-check.bbclass
+++ b/meta/classes/cve-check.bbclass
@@ -31,8 +31,9 @@ 
 CVE_PRODUCT ??= "${BPN}"
 CVE_VERSION ??= "${PV}"
 
-CVE_CHECK_DB_DIR ?= "${DL_DIR}/CVE_CHECK"
-CVE_CHECK_DB_FILE ?= "${CVE_CHECK_DB_DIR}/nvdcve_2-1.db"
+CVE_CHECK_DB_FILENAME ?= "nvdcve_2-1.db"
+CVE_CHECK_DB_DIR ?= "${STAGING_DIR}/CVE_CHECK"
+CVE_CHECK_DB_FILE ?= "${CVE_CHECK_DB_DIR}/${CVE_CHECK_DB_FILENAME}"
 CVE_CHECK_DB_FILE_LOCK ?= "${CVE_CHECK_DB_FILE}.lock"
 
 CVE_CHECK_LOG ?= "${T}/cve.log"
@@ -198,7 +199,7 @@  python do_cve_check () {
 }
 
 addtask cve_check before do_build
-do_cve_check[depends] = "cve-update-nvd2-native:do_fetch"
+do_cve_check[depends] = "cve-update-nvd2-native:do_unpack"
 do_cve_check[nostamp] = "1"
 
 python cve_check_cleanup () {
diff --git a/meta/recipes-core/meta/cve-update-nvd2-native.bb b/meta/recipes-core/meta/cve-update-nvd2-native.bb
index 1901641965..2d23d28c3e 100644
--- a/meta/recipes-core/meta/cve-update-nvd2-native.bb
+++ b/meta/recipes-core/meta/cve-update-nvd2-native.bb
@@ -8,7 +8,6 @@  INHIBIT_DEFAULT_DEPS = "1"
 
 inherit native
 
-deltask do_unpack
 deltask do_patch
 deltask do_configure
 deltask do_compile
@@ -35,7 +34,9 @@  CVE_DB_INCR_UPDATE_AGE_THRES ?= "10368000"
 # Number of attempts for each http query to nvd server before giving up
 CVE_DB_UPDATE_ATTEMPTS ?= "5"
 
-CVE_DB_TEMP_FILE ?= "${CVE_CHECK_DB_DIR}/temp_nvdcve_2.db"
+CVE_CHECK_DB_DLDIR_FILE ?= "${DL_DIR}/CVE_CHECK/${CVE_CHECK_DB_FILENAME}"
+CVE_CHECK_DB_DLDIR_LOCK ?= "${CVE_CHECK_DB_DLDIR_FILE}.lock"
+CVE_CHECK_DB_TEMP_FILE ?= "${CVE_CHECK_DB_FILE}.tmp"
 
 python () {
     if not bb.data.inherits_class("cve-check", d):
@@ -52,9 +53,9 @@  python do_fetch() {
 
     bb.utils.export_proxies(d)
 
-    db_file = d.getVar("CVE_CHECK_DB_FILE")
+    db_file = d.getVar("CVE_CHECK_DB_DLDIR_FILE")
     db_dir = os.path.dirname(db_file)
-    db_tmp_file = d.getVar("CVE_DB_TEMP_FILE")
+    db_tmp_file = d.getVar("CVE_CHECK_DB_TEMP_FILE")
 
     cleanup_db_download(db_file, db_tmp_file)
     # By default let's update the whole database (since time 0)
@@ -77,6 +78,7 @@  python do_fetch() {
         pass
 
     bb.utils.mkdirhier(db_dir)
+    bb.utils.mkdirhier(os.path.dirname(db_tmp_file))
     if os.path.exists(db_file):
         shutil.copy2(db_file, db_tmp_file)
 
@@ -89,10 +91,16 @@  python do_fetch() {
         os.remove(db_tmp_file)
 }
 
-do_fetch[lockfiles] += "${CVE_CHECK_DB_FILE_LOCK}"
+do_fetch[lockfiles] += "${CVE_CHECK_DB_DLDIR_LOCK}"
 do_fetch[file-checksums] = ""
 do_fetch[vardeps] = ""
 
+python do_unpack() {
+    import shutil
+    shutil.copyfile(d.getVar("CVE_CHECK_DB_DLDIR_FILE"), d.getVar("CVE_CHECK_DB_FILE"))
+}
+do_unpack[lockfiles] += "${CVE_CHECK_DB_DLDIR_LOCK} ${CVE_CHECK_DB_FILE_LOCK}"
+
 def cleanup_db_download(db_file, db_tmp_file):
     """
     Cleanup the download space from possible failed downloads