[pbs-devel] [PATCH proxmox-backup v3] close #4723: updated gc view in the ui

[pbs-devel] [PATCH proxmox-backup v3] close #4723: updated gc view in the ui

[Gabriel Goller]

Updated the GC overview in the datastore page. This enables
us to see:
 - schedule
 - State (of last run)
 - Duration (of last run)
 - Last Run
 - Next Run
 - Pending Chunks (of last run)
 - Removed Chunks (of last run)

Added `ObjectGrid` for GCView, moved to different file. Added
endpoint `gc_info` that returns all the necessary config and
last run stats.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---

update v3:
 - ui: removed `required` attribute on items to get the sorting right
 - made `pending_chunks` and `removed_chunks` options, so that they
   are not shown when no gc run exists

update v2:
 - skip serializing if value is `None`
 - return just the schedule if `upid` doesn't exist (means no gc has been
   run)
 - ui: removed default values on timestamps
 - ui: removed flex and minHeight properties

 pbs-api-types/src/datastore.rs |  38 +++++++++++
 src/api2/admin/datastore.rs    |  98 +++++++++++++++++++++++++---
 www/Makefile                   |   1 +
 www/Utils.js                   |  12 ++--
 www/config/GCView.js           | 116 +++++++++++++++++++++++++++++++++
 www/datastore/PruneAndGC.js    |  91 +-------------------------
 6 files changed, 253 insertions(+), 103 deletions(-)
 mode change 100644 => 100755 src/api2/admin/datastore.rs
 create mode 100644 www/config/GCView.js

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 73c4890e..55caa963 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -1250,6 +1250,44 @@ pub struct GarbageCollectionStatus {
     pub still_bad: usize,
 }
 
+#[api(
+    properties: {
+        "last-run-upid": {
+            optional: true,
+            type: UPID,
+        },
+    },
+)]
+#[derive(Clone, Default, Serialize, Deserialize, PartialEq)]
+#[serde(rename_all = "kebab-case")]
+/// Garbage Collection general info
+pub struct GarbageCollectionInfo {
+    /// upid of the last run gc job
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub last_run_upid: Option<String>,
+    /// Number of removed chunks
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub removed_chunks: Option<usize>,
+    /// Number of pending chunks
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub pending_chunks: Option<usize>,
+    /// Schedule  of the gc job
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub schedule: Option<String>,
+    /// Time of the next gc run
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub next_run: Option<i64>,
+    /// Endtime of the last gc run
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub last_run_endtime: Option<i64>,
+    /// State of the last gc run
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub last_run_state: Option<String>,
+    /// Duration of last gc run
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub duration: Option<i64>,
+}
+
 #[api(
     properties: {
         "gc-status": {
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
old mode 100644
new mode 100755
index a95031e7..1127bd95
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -10,6 +10,7 @@ use anyhow::{bail, format_err, Error};
 use futures::*;
 use hyper::http::request::Parts;
 use hyper::{header, Body, Response, StatusCode};
+use proxmox_time::CalendarEvent;
 use serde::Deserialize;
 use serde_json::{json, Value};
 use tokio_stream::wrappers::ReceiverStream;
@@ -33,13 +34,14 @@ use pxar::EntryKind;
 
 use pbs_api_types::{
     print_ns_and_snapshot, print_store_and_ns, Authid, BackupContent, BackupNamespace, BackupType,
-    Counts, CryptMode, DataStoreListItem, DataStoreStatus, GarbageCollectionStatus, GroupListItem,
-    KeepOptions, Operation, PruneJobOptions, RRDMode, RRDTimeFrame, SnapshotListItem,
-    SnapshotVerifyState, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA,
-    BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA,
-    MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
-    PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY,
-    UPID_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
+    Counts, CryptMode, DataStoreConfig, DataStoreListItem, DataStoreStatus, GarbageCollectionInfo,
+    GarbageCollectionStatus, GroupListItem, JobScheduleStatus, KeepOptions, Operation,
+    PruneJobOptions, RRDMode, RRDTimeFrame, SnapshotListItem, SnapshotVerifyState,
+    BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
+    BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DEPTH,
+    NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY,
+    PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, UPID, UPID_SCHEMA,
+    VERIFICATION_OUTDATED_AFTER_SCHEMA,
 };
 use pbs_client::pxar::{create_tar, create_zip};
 use pbs_config::CachedUserInfo;
@@ -67,7 +69,7 @@ use crate::backup::{
     ListAccessibleBackupGroups, NS_PRIVS_OK,
 };
 
-use crate::server::jobstate::Job;
+use crate::server::jobstate::{compute_schedule_status, Job, JobState};
 
 const GROUP_NOTES_FILE_NAME: &str = "notes";
 
@@ -1199,6 +1201,82 @@ pub fn garbage_collection_status(
     Ok(status)
 }
 
+#[api(
+    input: {
+        properties: {
+            store: {
+                schema: DATASTORE_SCHEMA,
+            },
+        },
+    },
+    returns: {
+        type: GarbageCollectionInfo,
+    },
+    access: {
+        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),
+    },
+)]
+/// Garbage collection status.
+pub fn garbage_collection_info(
+    store: String,
+    _info: &ApiMethod,
+    _rpcenv: &mut dyn RpcEnvironment,
+) -> Result<GarbageCollectionInfo, Error> {
+    let (config, _) = pbs_config::datastore::config()?;
+    let store_config: DataStoreConfig = config.lookup("datastore", &store)?;
+
+    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Read))?;
+    let status = datastore.last_gc_status();
+
+    let upid = match status.upid {
+        Some(upid) => upid,
+        None => {
+            return Ok(GarbageCollectionInfo {
+                schedule: store_config.gc_schedule,
+                ..Default::default()
+            })
+        }
+    };
+    let last_state = JobState::load("garbage_collection", &store)
+        .map_err(|err| format_err!("could not open statefile for {}: {}", upid, err))
+        .ok();
+
+    let mut computed_schedule: JobScheduleStatus = JobScheduleStatus::default();
+    if let Some(state) = last_state {
+        computed_schedule = compute_schedule_status(&state, Some(&upid)).unwrap();
+    }
+
+    // calculate next event
+    if let Some(schedule) = &store_config.gc_schedule {
+        if let (Ok(event), Some(last_run)) = (
+            schedule.parse::<CalendarEvent>(),
+            computed_schedule.last_run_endtime,
+        ) {
+            if let Ok(next_event) = event.compute_next_event(last_run) {
+                computed_schedule.next_run = next_event;
+            }
+        }
+    }
+
+    let mut duration = None;
+    if let (Ok(upid), Some(endtime)) = (upid.parse::<UPID>(), computed_schedule.last_run_endtime) {
+        duration = Some(endtime - upid.starttime);
+    }
+
+    let info = GarbageCollectionInfo {
+        last_run_upid: Some(upid),
+        removed_chunks: Some(status.removed_chunks),
+        pending_chunks: Some(status.pending_chunks),
+        schedule: store_config.gc_schedule,
+        next_run: computed_schedule.next_run,
+        last_run_endtime: computed_schedule.last_run_endtime,
+        last_run_state: computed_schedule.last_run_state,
+        duration,
+    };
+
+    Ok(info)
+}
+
 #[api(
     returns: {
         description: "List the accessible datastores.",
@@ -2265,6 +2343,10 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
             .get(&API_METHOD_GARBAGE_COLLECTION_STATUS)
             .post(&API_METHOD_START_GARBAGE_COLLECTION),
     ),
+    (
+        "gc_info",
+        &Router::new().get(&API_METHOD_GARBAGE_COLLECTION_INFO),
+    ),
     (
         "group-notes",
         &Router::new()
diff --git a/www/Makefile b/www/Makefile
index 04c12b31..b67fd73f 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -62,6 +62,7 @@ JSSRC=							\
 	config/SyncView.js				\
 	config/VerifyView.js				\
 	config/PruneView.js				\
+	config/GCView.js				\
 	config/WebauthnView.js				\
 	config/CertificateView.js			\
 	config/NodeOptionView.js			\
diff --git a/www/Utils.js b/www/Utils.js
index 2eca600e..66293b17 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -199,14 +199,14 @@ Ext.define('PBS.Utils', {
 	return fingerprint.substring(0, 23);
     },
 
-    render_task_status: function(value, metadata, record) {
-	if (!record.data['last-run-upid']) {
-	    return '-';
+    render_task_status: function(value, metadata, record, rowIndex, colIndex, store) {
+	if (!record.data['last-run-upid'] && !store.getById('last-run-upid')?.data.value) {
+            return '-';
 	}
 
-	if (!record.data['last-run-endtime']) {
-	    metadata.tdCls = 'x-grid-row-loading';
-	    return '';
+	if (!record.data['last-run-endtime'] && !store.getById('last-run-endtime')?.data.value) {
+            metadata.tdCls = 'x-grid-row-loading';
+            return '';
 	}
 
 	let parsed = Proxmox.Utils.parse_task_status(value);
diff --git a/www/config/GCView.js b/www/config/GCView.js
new file mode 100644
index 00000000..1a79bb01
--- /dev/null
+++ b/www/config/GCView.js
@@ -0,0 +1,116 @@
+Ext.define('PBS.Datastore.GCOptions', {
+    extend: 'Proxmox.grid.ObjectGrid',
+    alias: 'widget.pbsGCJobView',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    onlineHelp: 'maintenance_pruning',
+
+    cbindData: function(initial) {
+        let me = this;
+
+        me.datastore = encodeURIComponent(me.datastore);
+        me.url = `/api2/json/admin/datastore/${me.datastore}/gc_info`;
+        me.editorConfig = {
+            url: `/api2/extjs/config/datastore/${me.datastore}`,
+        };
+        return {};
+    },
+
+    controller: {
+        xclass: 'Ext.app.ViewController',
+
+        edit: function() {
+            this.getView().run_editor();
+        },
+
+        garbageCollect: function() {
+            let me = this;
+            let view = me.getView();
+            Proxmox.Utils.API2Request({
+                url: `/admin/datastore/${view.datastore}/gc`,
+                method: 'POST',
+                failure: function(response) {
+                    Ext.Msg.alert(gettext('Error'), response.htmlStatus);
+                },
+                success: function(response, options) {
+                    Ext.create('Proxmox.window.TaskViewer', {
+                        upid: response.result.data,
+                    }).show();
+                },
+            });
+        },
+    },
+
+    tbar: [
+        {
+            xtype: 'proxmoxButton',
+            text: gettext('Edit'),
+            enableFn: (rec) => rec.id === 'schedule',
+            disabled: true,
+            handler: 'edit',
+        },
+        '-',
+        {
+            xtype: 'proxmoxButton',
+            text: gettext('Start Garbage Collection'),
+            selModel: null,
+            handler: 'garbageCollect',
+        },
+    ],
+
+    listeners: {
+        activate: function() { this.rstore.startUpdate(); },
+        destroy: function() { this.rstore.stopUpdate(); },
+        deactivate: function() { this.rstore.stopUpdate(); },
+        itemdblclick: 'edit',
+    },
+
+    rows: {
+        "schedule": {
+            required: true,
+            defaultValue: Proxmox.Utils.NoneText,
+            header: gettext('Schedule'),
+            editor: {
+                xtype: 'proxmoxWindowEdit',
+                title: gettext('GC Schedule'),
+                onlineHelp: 'maintenance_gc',
+                items: {
+                    xtype: 'pbsCalendarEvent',
+                    name: 'gc-schedule',
+                    fieldLabel: gettext("GC Schedule"),
+                    emptyText: Proxmox.Utils.NoneText,
+                    deleteEmpty: true,
+                },
+            },
+        },
+        "last-run-state": {
+            header: gettext('State'),
+            renderer: PBS.Utils.render_task_status,
+            disabled: true,
+        },
+        "duration": {
+            header: gettext('Duration'),
+            renderer: Proxmox.Utils.render_duration,
+            disabled: true,
+        },
+        "last-run-endtime": {
+            header: gettext('Last Run'),
+            renderer: PBS.Utils.render_optional_timestamp,
+            disabled: true,
+        },
+        "next-run": {
+            header: gettext('Next Run'),
+            renderer: PBS.Utils.render_next_task_run,
+            disabled: true,
+        },
+        "pending-chunks": {
+            header: gettext('Pending Chunks'),
+            disabled: true,
+        },
+        "removed-chunks": {
+            header: gettext('Removed Chunks'),
+            disabled: true,
+        },
+        "last-run-upid": { visible: false },
+    },
+});
diff --git a/www/datastore/PruneAndGC.js b/www/datastore/PruneAndGC.js
index aab98dad..3b9878a3 100644
--- a/www/datastore/PruneAndGC.js
+++ b/www/datastore/PruneAndGC.js
@@ -1,88 +1,3 @@
-Ext.define('PBS.Datastore.GCOptions', {
-    extend: 'Proxmox.grid.ObjectGrid',
-    alias: 'widget.pbsDatastoreGCOpts',
-    mixins: ['Proxmox.Mixin.CBind'],
-
-    onlineHelp: 'maintenance_pruning',
-
-    cbindData: function(initial) {
-	let me = this;
-
-	me.datastore = encodeURIComponent(me.datastore);
-	me.url = `/api2/json/config/datastore/${me.datastore}`;
-	me.editorConfig = {
-	    url: `/api2/extjs/config/datastore/${me.datastore}`,
-	};
-	return {};
-    },
-
-    controller: {
-	xclass: 'Ext.app.ViewController',
-
-	edit: function() { this.getView().run_editor(); },
-
-	garbageCollect: function() {
-	    let me = this;
-	    let view = me.getView();
-	    Proxmox.Utils.API2Request({
-		url: `/admin/datastore/${view.datastore}/gc`,
-		method: 'POST',
-		failure: function(response) {
-		    Ext.Msg.alert(gettext('Error'), response.htmlStatus);
-		},
-		success: function(response, options) {
-		    Ext.create('Proxmox.window.TaskViewer', {
-			upid: response.result.data,
-		    }).show();
-		},
-	    });
-	},
-    },
-
-    tbar: [
-	{
-	    xtype: 'proxmoxButton',
-	    text: gettext('Edit'),
-	    disabled: true,
-	    handler: 'edit',
-	},
-	'-',
-	{
-	    xtype: 'proxmoxButton',
-	    text: gettext('Start Garbage Collection'),
-	    selModel: null,
-	    handler: 'garbageCollect',
-	},
-    ],
-
-    listeners: {
-	activate: function() { this.rstore.startUpdate(); },
-	destroy: function() { this.rstore.stopUpdate(); },
-	deactivate: function() { this.rstore.stopUpdate(); },
-	itemdblclick: 'edit',
-    },
-
-    rows: {
-	"gc-schedule": {
-	    required: true,
-	    defaultValue: Proxmox.Utils.NoneText,
-	    header: gettext('Garbage Collection Schedule'),
-	    editor: {
-		xtype: 'proxmoxWindowEdit',
-		title: gettext('GC Schedule'),
-		onlineHelp: 'maintenance_gc',
-		items: {
-		    xtype: 'pbsCalendarEvent',
-		    name: 'gc-schedule',
-		    fieldLabel: gettext("GC Schedule"),
-		    emptyText: Proxmox.Utils.noneText,
-		    deleteEmpty: true,
-		},
-	    },
-	},
-    },
-});
-
 Ext.define('PBS.Datastore.PruneAndGC', {
     extend: 'Ext.panel.Panel',
     alias: 'widget.pbsDatastorePruneAndGC',
@@ -99,9 +14,9 @@ Ext.define('PBS.Datastore.PruneAndGC', {
     },
     items: [
 	{
-	    xtype: 'pbsDatastoreGCOpts',
+	    xtype: 'pbsGCJobView',
 	    title: gettext('Garbage Collection'),
-	    itemId: 'datastore-gc',
+	    itemId: 'datastore-gc-jobs',
 	    nodename: 'localhost',
 	    cbind: {
 		datastore: '{datastore}',
@@ -111,8 +26,6 @@ Ext.define('PBS.Datastore.PruneAndGC', {
 	    xtype: 'pbsPruneJobView',
 	    nodename: 'localhost',
 	    itemId: 'datastore-prune-jobs',
-	    flex: 1,
-	    minHeight: 200,
 	    cbind: {
 		datastore: '{datastore}',
 	    },
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup v3] close #4723: updated gc view in the ui

[Thomas Lamprecht]

Am 22/09/2023 um 11:13 schrieb Stefan Lendl:
> Handling of GC tasks that never ran works great now. GUI alignment looks good.

Thanks for testing, but two general things:

- use reply-all and ensure your mail user agent is not borked so that it
  includes the in-reply-to header important for making mail threads
  work.

- trim the context in your replies to important stuff. E.g., the mail
  I'm replying to has about 1060 lines, of which a few dozens are
  relevant at most.

- can you post a "Tested-by: Full Name <email>" trailer, I try to amend
  them to the commit message and thus keep track of such information (if
  somebody in the future has questions about this, and the author is
  then currently not available, they could ask you if you got some idea
  about whatever their question is)


> Am 20/09/2023 um 19:02 schrieb Gabriel Goller:
>> Added `ObjectGrid` for GCView, moved to different file. Added
>> endpoint `gc_info` that returns all the necessary config and
>> last run stats.
> 

Sorry to only notice this on v3, but can you please split this patch
into:
- new API
- moving GC view out to own file (with as little semantic change as
  possible)
- adding new UI

In general, it can be fine to do API and UI in one patch if they are
quite coupled, and it's a relatively small and targeted feature. But, if
there's existing UI code it still can be nicer to split this so one can
differ easier between file-movement changes and actual code changes
(git's color-moved isn't IME that helpful)


[...]

>> +    // calculate next event
>> +    if let Some(schedule) = &store_config.gc_schedule {
>> +        if let (Ok(event), Some(last_run)) = (
>> +            schedule.parse::<CalendarEvent>(),
>> +            computed_schedule.last_run_endtime,
>> +        ) {
>> +            if let Ok(next_event) = event.compute_next_event(last_run) {
>> +                computed_schedule.next_run = next_event;
>> +            }
>> +        }
>> +    }
> 
> 
> If GC never ran but is scheduled, it would also be nice to show the next
> scheduled run.
> This might be usefull if people had not GC configured, then create a job
> to further indicate that the job is scheduled correctly.
> 
> 

I did not check it out closely, but how is the situation handled if it's
currently running, returning that under "last_run_upid" would be
probably confusing, but without >UPID one cannot allow one to open the
log easily, or do we want to have this strictly for last- and next-run?

Re: [pbs-devel] [PATCH proxmox-backup v3] close #4723: updated gc view in the ui

[Gabriel Goller]

>> Am 20/09/2023 um 19:02 schrieb Gabriel Goller:
>>> Added `ObjectGrid` for GCView, moved to different file. Added
>>> endpoint `gc_info` that returns all the necessary config and
>>> last run stats.
> Sorry to only notice this on v3, but can you please split this patch
> into:
> - new API
> - moving GC view out to own file (with as little semantic change as
>    possible)
> - adding new UI
>
> In general, it can be fine to do API and UI in one patch if they are
> quite coupled, and it's a relatively small and targeted feature. But, if
> there's existing UI code it still can be nicer to split this so one can
> differ easier between file-movement changes and actual code changes
> (git's color-moved isn't IME that helpful)
So I should submit one patch with the new api endpoint, another one
with moving the gc view ouf of the file (which I will have to create because
the PrunceAndGCView makes one api request for both tables and uses the
same store) and another one for the new ui?
> [...]
>
>>> +    // calculate next event
>>> +    if let Some(schedule) = &store_config.gc_schedule {
>>> +        if let (Ok(event), Some(last_run)) = (
>>> +            schedule.parse::<CalendarEvent>(),
>>> +            computed_schedule.last_run_endtime,
>>> +        ) {
>>> +            if let Ok(next_event) = event.compute_next_event(last_run) {
>>> +                computed_schedule.next_run = next_event;
>>> +            }
>>> +        }
>>> +    }
>>
>> If GC never ran but is scheduled, it would also be nice to show the next
>> scheduled run.
>> This might be usefull if people had not GC configured, then create a job
>> to further indicate that the job is scheduled correctly.
>>
>>
> I did not check it out closely, but how is the situation handled if it's
> currently running, returning that under "last_run_upid" would be
> probably confusing, but without >UPID one cannot allow one to open the
> log easily, or do we want to have this strictly for last- and next-run?
Showing the log while the gc is running was never possible (except when you
manually start it from the ui, then it opens automatically) but we could 
implement
a "Show Log" button as we have the `upid`...

When the gc job is currently running, we display a spinner on the status 
row. To find
out if the gc job is currently running, we simply check if the `upid` 
exists and the
`last_run_endtime` does not.

Re: [pbs-devel] [PATCH proxmox-backup v3] close #4723: updated gc view in the ui

[Thomas Lamprecht]

Am 22/09/2023 um 12:45 schrieb Gabriel Goller:
> Am 22/09/2023 um 11:36 schrieb Thomas Lamprecht:
>> In general, it can be fine to do API and UI in one patch if they are
>> quite coupled, and it's a relatively small and targeted feature. But, if
>> there's existing UI code it still can be nicer to split this so one can
>> differ easier between file-movement changes and actual code changes
>> (git's color-moved isn't IME that helpful)
> So I should submit one patch with the new api endpoint, another one
> with moving the gc view ouf of the file (which I will have to create because
> the PrunceAndGCView makes one api request for both tables and uses the
> same store) and another one for the new ui?

Hmm, forgot that this is using a shared store currently. So yeah, that
make such a split rather busy work..
I'd then maybe just split UI and API into two separate patches, but not
so hard feelings here.

>> [...]
>>
>>>> +    // calculate next event
>>>> +    if let Some(schedule) = &store_config.gc_schedule {
>>>> +        if let (Ok(event), Some(last_run)) = (
>>>> +            schedule.parse::<CalendarEvent>(),
>>>> +            computed_schedule.last_run_endtime,
>>>> +        ) {
>>>> +            if let Ok(next_event) = event.compute_next_event(last_run) {
>>>> +                computed_schedule.next_run = next_event;
>>>> +            }
>>>> +        }
>>>> +    }
>>>
>>> If GC never ran but is scheduled, it would also be nice to show the next
>>> scheduled run.
>>> This might be usefull if people had not GC configured, then create a job
>>> to further indicate that the job is scheduled correctly.
>>>
>>>
>> I did not check it out closely, but how is the situation handled if it's
>> currently running, returning that under "last_run_upid" would be
>> probably confusing, but without >UPID one cannot allow one to open the
>> log easily, or do we want to have this strictly for last- and next-run?
> Showing the log while the gc is running was never possible (except when you
> manually start it from the ui, then it opens automatically) but we could implement
> a "Show Log" button as we have the `upid`...
>

Yeah, I know that it wasn't possible, but seeing the last state wasn't
either until your patch ;-)
I'm just thinking about what a user might expect to see if there's a
currently running GC operation.

> When the gc job is currently running, we display a spinner on the status row. To find
> out if the gc job is currently running, we simply check if the `upid` exists and the
> `last_run_endtime` does not.

In the UI of the Proxmox VE replication framework there's also a spinner
if there's currently running a log, but if I press the available "Log"
button there, I then get the task-viewer of the current log, which
has its merits.  And, FWIW, GC often runs much longer than a
delta-replication.

FWIW, you could always set the UPID and use the availability of the
"last-run-endtime" property as information for if the task is finished
or still running.

Re: [pbs-devel] [PATCH proxmox-backup v3] close #4723: updated gc view in the ui

[Gabriel Goller]

On 9/22/23 13:49, Thomas Lamprecht wrote:

> Am 22/09/2023 um 12:45 schrieb Gabriel Goller:
>> Am 22/09/2023 um 11:36 schrieb Thomas Lamprecht:
>>> In general, it can be fine to do API and UI in one patch if they are
>>> quite coupled, and it's a relatively small and targeted feature. But, if
>>> there's existing UI code it still can be nicer to split this so one can
>>> differ easier between file-movement changes and actual code changes
>>> (git's color-moved isn't IME that helpful)
>> So I should submit one patch with the new api endpoint, another one
>> with moving the gc view ouf of the file (which I will have to create because
>> the PrunceAndGCView makes one api request for both tables and uses the
>> same store) and another one for the new ui?
> Hmm, forgot that this is using a shared store currently. So yeah, that
> make such a split rather busy work..
> I'd then maybe just split UI and API into two separate patches, but not
> so hard feelings here.
ok
>>> [...]
>>>
>>>>> +    // calculate next event
>>>>> +    if let Some(schedule) = &store_config.gc_schedule {
>>>>> +        if let (Ok(event), Some(last_run)) = (
>>>>> +            schedule.parse::<CalendarEvent>(),
>>>>> +            computed_schedule.last_run_endtime,
>>>>> +        ) {
>>>>> +            if let Ok(next_event) = event.compute_next_event(last_run) {
>>>>> +                computed_schedule.next_run = next_event;
>>>>> +            }
>>>>> +        }
>>>>> +    }
>>>> If GC never ran but is scheduled, it would also be nice to show the next
>>>> scheduled run.
>>>> This might be usefull if people had not GC configured, then create a job
>>>> to further indicate that the job is scheduled correctly.
>>>>
>>>>
>>> I did not check it out closely, but how is the situation handled if it's
>>> currently running, returning that under "last_run_upid" would be
>>> probably confusing, but without >UPID one cannot allow one to open the
>>> log easily, or do we want to have this strictly for last- and next-run?
>> Showing the log while the gc is running was never possible (except when you
>> manually start it from the ui, then it opens automatically) but we could implement
>> a "Show Log" button as we have the `upid`...
>>
> Yeah, I know that it wasn't possible, but seeing the last state wasn't
> either until your patch ;-)
> I'm just thinking about what a user might expect to see if there's a
> currently running GC operation.
>
>> When the gc job is currently running, we display a spinner on the status row. To find
>> out if the gc job is currently running, we simply check if the `upid` exists and the
>> `last_run_endtime` does not.
> In the UI of the Proxmox VE replication framework there's also a spinner
> if there's currently running a log, but if I press the available "Log"
> button there, I then get the task-viewer of the current log, which
> has its merits.  And, FWIW, GC often runs much longer than a
> delta-replication.
Implemented a "Show Task Log" button.
> FWIW, you could always set the UPID and use the availability of the
> "last-run-endtime" property as information for if the task is finished
> or still running.
>
Sent a new patch.