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 |
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
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
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] > -=-=-=-=-=-=-=-=-=-=-=- >
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
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
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
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 > >
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 --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