Clean up old dashboard files on upgrade of local builds and static builds. #21370

Ferroin · 2025-12-01T13:05:22Z

Summary

This adds a manifest file to the root of the web directory to keep track of what files were installed by the current version of the Netdata Agent. On upgrade, static builds and local builds will remove any files in that directory that are not in the manifest file.

Test Plan

Requires manual testing to ensure that only files that are supposed to be removed are removed.

Additional Information

Fixes: #21327

This will be used to clean up old files from the web/v3 directory after an update/reinstall for a static build or local build.

cubic-dev-ai

No issues found across 3 files

Copilot

Pull request overview

This PR adds a manifest file (.MANIFEST) to track installed dashboard files and automatically removes obsolete files during upgrades for both static builds and local builds.

Key changes:

CMake generates a .MANIFEST file listing all installed dashboard files
Install scripts use the manifest to identify and remove old dashboard files that are no longer part of the current version
Cleanup occurs after new files are installed to ensure only truly obsolete files are removed

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.

File	Description
`packaging/cmake/Modules/NetdataDashboard.cmake`	Generates `.MANIFEST` file containing list of dashboard files to be installed; adds manifest to CMake install rules
`packaging/makeself/install-or-update.sh`	Implements cleanup logic for static builds using the manifest to remove old dashboard files
`netdata-installer.sh`	Implements cleanup logic for local builds using the manifest to remove old dashboard files

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

netdata-installer.sh

packaging/makeself/install-or-update.sh

netdata-installer.sh

packaging/makeself/install-or-update.sh

Copilot · 2025-12-02T13:13:13Z

packaging/cmake/Modules/NetdataDashboard.cmake

+    file(GLOB files
+         LIST_DIRECTORIES FALSE
+         RELATIVE "${dashboard_src_prefix}"
+         "${dashboard_src_prefix}/${dir}/*")


Both the install rules and manifest generation only handle one level of files within subdirectories (using ${dashboard_src_prefix}/${dir}/*). If the dashboard contains nested subdirectories (e.g., static/css/fonts/), those nested files won't be installed or tracked in the manifest.

This appears to be a limitation of the current implementation. If the dashboard structure is guaranteed to be flat (no nested subdirectories), this is fine. Otherwise, both the install rules and manifest generation should use GLOB_RECURSE or handle nested directories explicitly.

Verify that the dashboard structure doesn't contain nested subdirectories, or update the code:

file(GLOB_RECURSE files LIST_DIRECTORIES FALSE RELATIVE "${dashboard_src_prefix}" "${dashboard_src_prefix}/${dir}/**")

@Ferroin, we still have this suggestion.

This is a known tradeoff taken based on discussion with the frontend team. Actually recursively listing all the files while still making CMake’s install directives work correctly is absurdly complicated to do right, and we have a general guarantee from the frontend team that the dashboard file layout is not going to change significantly from what is currently.

nested
e.g. static/css/fonts/

This actually happened with GIF files (added and removed). I don't remember the exact directory, but there was some nesting.

Correction to my earlier statement, Copilot is wrong here, and I forgot about the specifics of how the subdirlist() function that’s being used to generate the list of directories to process works (it has been more than a year since I wrote this code...).

That function generates a recursive list of all subdirectories of the specified base directory, so it does in fact pick up everything under ${dashboard_src_prefix}.

See https://github.com/netdata/netdata/blob/master/packaging/cmake/Modules/NetdataUtil.cmake#L202 for the actual code.

The strange looking split of processing all the directories one by one instead of doing a recursive glob is because CMake‘s install() command does not preserve the relative paths of the files listed for it (so install(foo/bar DESTINATION baz) will install the file bar directly into the directory baz without creating a directory baz/foo to install it into) and does not take directory trees (each file has to be listed for installation).

Copilot · 2025-12-02T13:13:13Z

packaging/cmake/Modules/NetdataDashboard.cmake

+
+  message(STATUS "  Generating manifest for dashboard code")
+  list(APPEND manifest
+    .MANIFEST


The manifest includes .MANIFEST itself on line 85, but this creates a potential issue: the manifest file won't exist in the source directory when it's being generated, so it shouldn't be listed as a file to preserve.

While self-referencing prevents the manifest from being deleted during cleanup, it's semantically incorrect since the manifest describes the installed dashboard files, not itself. The cleanup logic already reads the manifest file, so it's implicitly protected.

Consider removing .MANIFEST from the manifest list.

Suggested change

.MANIFEST

@Ferroin, we also have this,

The cleanup logic already reads the manifest file, so it's implicitly protected.

This is blatantly wrong, there’s no special protections given to the manifest file by virtue of it being read by the cleanup logic.

packaging/makeself/install-or-update.sh

netdata-installer.sh

packaging/cmake/Modules/NetdataDashboard.cmake

thiagoftsm

PR ran as expected after local building and generation of static installer to install on CentOS 7. LGTM!

ilyam8 · 2025-12-03T08:16:15Z

This is a known tradeoff taken based on discussion with the frontend team. Actually recursively listing all the files while still making CMake’s install directives work correctly is absurdly complicated to do right

If this version has known "tradeoffs"... why don't we make the logic easier and just remove the /usr/share/netdata/web/v3/ directory?

If the dashboard contains nested subdirectories (e.g., static/css/fonts/), those nested files won't be installed or tracked in the manifest.

It does contain. We added and removed 500 MB of gifs. This PR will not remove them, right?

ilyam8 · 2025-12-03T08:18:13Z

@thiagoftsm

ran as expected

What do you mean by that? What did you test?

Ferroin · 2025-12-03T12:09:10Z

This is a known tradeoff taken based on discussion with the frontend team. Actually recursively listing all the files while still making CMake’s install directives work correctly is absurdly complicated to do right

If this version has known "tradeoffs"... why don't we make the logic easier and just remove the /usr/share/netdata/web/v3/ directory?

Because the approach I’m taking here works pretty much independently of how the user chooses to update things (the only case it doesn’t work in is building by hand without netdata-installer.sh).

The static build can’t run things before the files are extracted and put in place, so we need to track what files are installed and remove only those that aren’t part of the current version.

If the dashboard contains nested subdirectories (e.g., static/css/fonts/), those nested files won't be installed or tracked in the manifest.

It does contain. We added and removed 500 MB of gifs. This PR will not remove them, right?

The logic is that anything not in the manifest for the newly installed version gets removed, so yes, it will remove them.

That said, if they got installed in the first place, they would be in the manifest for the versions that they were installed in. The logic being used to generate the list of files for the manifest is literally identical to that used to generate the installation directives for the dashboard files.

packaging/makeself/install-or-update.sh

ilyam8 · 2025-12-03T17:45:19Z

Converting to draft to make it non-measurable - I found some issues while testing, give me some time.

ilyam8 · 2025-12-03T18:53:14Z

@Ferroin, the original cleanup logic was functionally broken. I don't know what @thiagoftsm was testing 🤷‍♂️. @thiagoftsm, if you don't understand the change, please ask or don't review/approve.

Two issues caused grep to match all files:

.MANIFEST contains relative paths, while find -print0 produced
absolute paths, leading to unintended substring matches.
A blank line in .MANIFEST created an empty pattern, causing
grep -Ff to match every file.

As a result, grep -vzFf filtered out the entire file list and xargs
invoked rm with no operands.

This change replaces the pipeline with a correct, POSIX-safe loop that:

converts NUL-separated paths to newline-separated ones,
strips the NETDATA_WEB_DIR prefix to match manifest entries,
performs exact whole-line comparisons with grep -Fxq,
removes only files not present in .MANIFEST.

The cleanup now works as intended and handles all filenames safely.

netdata-installer.sh

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

packaging/makeself/install-or-update.sh

netdata-installer.sh

packaging/makeself/install-or-update.sh

netdata-installer.sh

packaging/makeself/install-or-update.sh

netdata-installer.sh

packaging/makeself/install-or-update.sh

netdata-installer.sh

Ferroin · 2025-12-03T19:10:01Z

The cleanup now works as intended and handles all filenames safely.

Handles all current filenames.

There’s a reason this was using null terminated records instead of newline terminated records, that’s the only character that is guaranteed to not be in a filename on all systems.

Separately, there are a shit ton of files being processed here, which is exactly why I structured this as one pipeline instead of a long form loop that invokes multiple commands for every single file.

Ferroin · 2025-12-03T19:10:26Z

Oh the irony, Copilot pointing out exactly what I was as I was writing my comment...

…fixed himself.

Ferroin · 2025-12-03T19:18:02Z

Force pushed to fix the code properly without wasting resources and correctly accounting for the fact that a newline is actually a valid character in filenames on most UNIX-like systems.

ilyam8 · 2025-12-03T19:20:08Z

Separately, there are a shit ton of files being processed here, which is exactly why I structured this as one pipeline instead of a long form loop that invokes multiple commands for every single file.

Consider testing your changes before marking them ready for review.

ilyam8 · 2025-12-08T09:41:39Z

packaging/makeself/install-or-update.sh

+
+if [ -r "${NETDATA_WEB_DIR}/.MANIFEST" ]; then
+  old_pwd="$(pwd)"
+  cd "${NETDATA_WEB_DIR}" && remove_files="$(find "." -type f -print0 | grep -vzF "" | grep -vzFf "${NETDATA_WEB_DIR}/.MANIFEST")"


grep -vzF ""

This filters everything because every string contains the empty string. What are you trying to do here? Can you please test before pushing commits?

Ferroin added 3 commits December 1, 2025 07:36

Add manifest file to track agent dashboard files on install.

6c6b805

This will be used to clean up old files from the web/v3 directory after an update/reinstall for a static build or local build.

Add dashboard file cleanup in netdata-installer.sh

82234bd

Add dashboard file cleanup in static builds.

676bb3d

github-actions bot added the area/packaging Packaging and operating systems support label Dec 1, 2025

Ferroin marked this pull request as ready for review December 2, 2025 12:15

Ferroin requested review from a team and vkalintiris as code owners December 2, 2025 12:15

Ferroin requested a review from a team December 2, 2025 12:15

cubic-dev-ai bot reviewed Dec 2, 2025

View reviewed changes

ilyam8 requested a review from Copilot December 2, 2025 13:04

Copilot started reviewing on behalf of ilyam8 December 2, 2025 13:04 View session

Copilot finished reviewing on behalf of ilyam8 December 2, 2025 13:09

Copilot AI reviewed Dec 2, 2025

View reviewed changes

Ferroin added 3 commits December 2, 2025 08:48

Make matching more robust.

a6fcbbd

Tidy up manifest contents.

c19612c

Fix incorrect operator ordering in CMake.

e2ebc22

thiagoftsm previously approved these changes Dec 3, 2025

View reviewed changes

ilyam8 reviewed Dec 3, 2025

View reviewed changes

packaging/makeself/install-or-update.sh Show resolved Hide resolved

ilyam8 marked this pull request as draft December 3, 2025 17:44

ilyam8 dismissed thiagoftsm’s stale review via 2540afa December 3, 2025 18:50

ilyam8 reviewed Dec 3, 2025

View reviewed changes

netdata-installer.sh Outdated Show resolved Hide resolved

ilyam8 requested a review from Copilot December 3, 2025 18:55

Copilot started reviewing on behalf of ilyam8 December 3, 2025 18:55 View session

Copilot finished reviewing on behalf of ilyam8 December 3, 2025 18:59

Copilot AI reviewed Dec 3, 2025

View reviewed changes

Correctly fix issues that Ilya failed to report and then incorrectly …

0c54177

…fixed himself.

Ferroin force-pushed the dashboard-manifest branch from de2d1eb to 0c54177 Compare December 3, 2025 19:17

ilyam8 reviewed Dec 8, 2025

View reviewed changes

Clean up old dashboard files on upgrade of local builds and static builds. #21370

Are you sure you want to change the base?

Clean up old dashboard files on upgrade of local builds and static builds. #21370

Uh oh!

Conversation

Ferroin commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Additional Information

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

thiagoftsm Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Ferroin Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

ilyam8 Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ferroin Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

thiagoftsm Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Ferroin Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thiagoftsm left a comment

Choose a reason for hiding this comment

Uh oh!

ilyam8 commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilyam8 commented Dec 3, 2025

Uh oh!

Ferroin commented Dec 3, 2025

Uh oh!

Uh oh!

ilyam8 commented Dec 3, 2025

Uh oh!

ilyam8 commented Dec 3, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ferroin commented Dec 1, 2025 •

edited

Loading

ilyam8 Dec 2, 2025 •

edited

Loading

ilyam8 commented Dec 3, 2025 •

edited

Loading

ilyam8 commented Dec 3, 2025 •

edited

Loading

ilyam8 Dec 8, 2025 •

edited

Loading