starred/ProxmoxVE

Fork 0

mirror of https://github.com/community-scripts/ProxmoxVE.git synced 2026-04-25 17:26:03 +03:00

[PR #6730] [MERGED] Fix/thinpool detection as it allows to delete active thinpool with different name than "data" #6351

New issue

Closed

opened 2026-02-26 15:31:38 +03:00 by kerem · 0 comments

kerem commented

2026-02-26 15:31:38 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/community-scripts/ProxmoxVE/pull/6730
Author: @onethree7
Created: 8/10/2025
Status: ✅ Merged
Merged: 8/10/2025
Merged by: @tremor021

Base: main ← Head: fix/thinpool-detection

📝 Commits (3)

304b39c Fix: exclude all thin-pools dynamically instead of only 'data'
5d917a8 Fix: dynamically exclude all thin pools (not just 'data') from orphan check
e01140c Fix: dynamically exclude all thin pools (not just 'data') from orphan check

📊 Changes

1 file changed (+9 additions, -2 deletions)

View changed files

📝 tools/pve/clean-orphaned-lvm.sh (+9 -2)

📄 Description

✍️ Description

Script would allow user to choose to delete an active thinpool with volumes under it:
-> Issue, system thinpool called "data" is excluded here
if [[ "$lv" == "data" || "$lv" == "root" || "$lv" == "swap" || "$lv" =~ ^osd-block- ]]; then

But if custom thinpools exist these will be offered up for deletion by the script:
Example:

Fix is avoiding all thinpools, now does not allow the user to delete custom thinpool.

I´ve tested functionality after the fix,

lvs -o lv_name,vg_name,lv_size,data_percent,metadata_percent,lv_health_status                                                                                                                                                                                                                                                                                        46m
 lvcreate -V10M -T pve/data -n test-orphan

-> still lable to delete orphan lvm afterwards, and thin-pools are excluded.

Its questionable if actually deleting a thinpool would work if it still has volumes, mine would have open mounts and cmd would fail, but eventually users could be in a state where the cmd would actually succeed and introduce dl? I dont want to test deleting a thinpool.

/dev/nvme0n1p3 (Physical Partition)
└── pve (VG)
├── data (Thin Pool LV) <<<< protected before fix
│ ├── data_tdata (Thin Pool Data LV)
│ └── data_tmeta (Thin Pool Metadata LV)
├── vm-100-disk-0 (Thin LV inside 'data')

/dev/sda (Physical Disk)
└── 4TBSSD (VG)
├── thinpool (Thin Pool LV) <<<< not protected before fix, but after.
│ ├── thinpool_tdata (Thin Pool Data LV)
│ └── thinpool_tmeta (Thin Pool Metadata LV)
├── vm-100-disk-0 (Thin LV inside 'thinpool')

fix is using this logic:

lvs --noheadings -o lv_name,vg_name,lv_size,seg_type --separator ' ' 2>/dev/null | awk '{print $1, $2, $3, $4}'
thinpool 4TBSSD 3.60t thin-pool
                                     ^^^^^                                 
data pve <1.63t thin-pool
                           ^^^^^

Link: #

✅ Prerequisites (X in brackets)

[x ] Self-review completed – Code follows project standards.
Tested thoroughly – Changes work as expected.
[x ] No security risks – No hardcoded secrets, unnecessary privilege escalations, or permission issues.

🛠️ Type of Change (X in brackets)

🐞 Bug fix – Resolves an issue without breaking functionality.
✨ New feature – Adds new, non-breaking functionality.
💥 Breaking change – Alters existing functionality in a way that may require updates.
🆕 New script – A fully functional and tested script or script set.
🌍 Website update – Changes to website-related JSON files or metadata.
🔧 Refactoring / Code Cleanup – Improves readability or maintainability without changing functionality.
📝 Documentation update – Changes to README, AppName.md, CONTRIBUTING.md, or other docs.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/community-scripts/ProxmoxVE/pull/6730 **Author:** [@onethree7](https://github.com/onethree7) **Created:** 8/10/2025 **Status:** ✅ Merged **Merged:** 8/10/2025 **Merged by:** [@tremor021](https://github.com/tremor021) **Base:** `main` ← **Head:** `fix/thinpool-detection` --- ### 📝 Commits (3) - [`304b39c`](https://github.com/community-scripts/ProxmoxVE/commit/304b39c7e9d7452a1ee479bf52776053c9dc8f1f) Fix: exclude all thin-pools dynamically instead of only 'data' - [`5d917a8`](https://github.com/community-scripts/ProxmoxVE/commit/5d917a8d25145eb3132ccb464e11ade06fa209a3) Fix: dynamically exclude all thin pools (not just 'data') from orphan check - [`e01140c`](https://github.com/community-scripts/ProxmoxVE/commit/e01140c3de104ebac06a0950a1932f51b48b8d83) Fix: dynamically exclude all thin pools (not just 'data') from orphan check ### 📊 Changes **1 file changed** (+9 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `tools/pve/clean-orphaned-lvm.sh` (+9 -2) </details> ### 📄 Description ## ✍️ Description Script would allow user to choose to delete an active thinpool with volumes under it: -> Issue, system thinpool called "data" is excluded here ` if [[ "$lv" == "data" || "$lv" == "root" || "$lv" == "swap" || "$lv" =~ ^osd-block- ]]; then` But if custom thin**pools** exist these will be offered up for deletion by the script: Example: <img width="492" height="143" alt="image" src="https://github.com/user-attachments/assets/363e9b4d-6d39-469f-8c6b-5d1227927f3a" /> Fix is avoiding all thinpools, now does not allow the user to delete custom thinpool. <img width="343" height="125" alt="image" src="https://github.com/user-attachments/assets/1994bbe7-2a3a-4c47-a987-bd7ca0fa6773" /> I´ve tested functionality after the fix, ``` lvs -o lv_name,vg_name,lv_size,data_percent,metadata_percent,lv_health_status 46m lvcreate -V10M -T pve/data -n test-orphan ``` -> still lable to delete orphan lvm afterwards, and thin-pools are excluded. Its questionable if actually deleting a thinpool would work if it still has volumes, mine would have open mounts and cmd would fail, but eventually users could be in a state where the cmd would actually succeed and introduce dl? I dont want to test deleting a thinpool. /dev/nvme0n1p3 (Physical Partition) └── pve (VG) ├── data (Thin Pool LV) <<<< protected before fix │ ├── data_tdata (Thin Pool Data LV) │ └── data_tmeta (Thin Pool Metadata LV) ├── vm-100-disk-0 (Thin LV inside 'data') /dev/sda (Physical Disk) └── 4TBSSD (VG) ├── thinpool (Thin Pool LV) <<<< not protected before fix, but after. │ ├── thinpool_tdata (Thin Pool Data LV) │ └── thinpool_tmeta (Thin Pool Metadata LV) ├── vm-100-disk-0 (Thin LV inside 'thinpool') fix is using this logic: ``` lvs --noheadings -o lv_name,vg_name,lv_size,seg_type --separator ' ' 2>/dev/null | awk '{print $1, $2, $3, $4}' thinpool 4TBSSD 3.60t thin-pool ^^^^^ data pve <1.63t thin-pool ^^^^^ ``` ## 🔗 Related PR / Issue Link: # ## ✅ Prerequisites (**X** in brackets) - [x ] **Self-review completed** – Code follows project standards. - [x] **Tested thoroughly** – Changes work as expected. - [x ] **No security risks** – No hardcoded secrets, unnecessary privilege escalations, or permission issues. --- ## 🛠️ Type of Change (**X** in brackets) - [x] 🐞 **Bug fix** – Resolves an issue without breaking functionality. - [ ] ✨ **New feature** – Adds new, non-breaking functionality. - [ ] 💥 **Breaking change** – Alters existing functionality in a way that may require updates. - [ ] 🆕 **New script** – A fully functional and tested script or script set. - [ ] 🌍 **Website update** – Changes to website-related JSON files or metadata. - [ ] 🔧 **Refactoring / Code Cleanup** – Improves readability or maintainability without changing functionality. - [ ] 📝 **Documentation update** – Changes to `README`, `AppName.md`, `CONTRIBUTING.md`, or other docs. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

kerem

2026-02-26 15:31:38 +03:00

closed this issue
added the
pull-request
label

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

starred/ProxmoxVE#6351

No description provided.

Rows
Columns