From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 7A7E31FF164 for ; Fri, 31 Jan 2025 14:42:48 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E824F2DB8C; Fri, 31 Jan 2025 14:42:45 +0100 (CET) Date: Fri, 31 Jan 2025 14:42:11 +0100 From: Wolfgang Bumiller To: Lukas Wagner Message-ID: References: <20250128122520.167796-1-l.wagner@proxmox.com> <20250128122520.167796-9-l.wagner@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20250128122520.167796-9-l.wagner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.067 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [mod.rs] Subject: Re: [pdm-devel] [PATCH proxmox-datacenter-manager 08/15] task cache: fetch every 5mins, requesting only missing tasks X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Cc: pdm-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" On Tue, Jan 28, 2025 at 01:25:13PM +0100, Lukas Wagner wrote: > Instead of purging the cache completely and requesting the entire task > history over the last seven days, we keep the per-remote task history in > the cache and only fetch missing tasks since the last time we fetched > them. > > If a tracked task finishes, we also force to fetch the most recent task > data, since that is the only way right now to get the task status *and* > task endtime. > > Signed-off-by: Lukas Wagner > --- > server/src/remote_tasks/mod.rs | 52 +++++++++++++++------------ > server/src/remote_tasks/task_cache.rs | 25 +++++++++---- > 2 files changed, 49 insertions(+), 28 deletions(-) > > diff --git a/server/src/remote_tasks/mod.rs b/server/src/remote_tasks/mod.rs > index 4a0552c..16c46a5 100644 > --- a/server/src/remote_tasks/mod.rs > +++ b/server/src/remote_tasks/mod.rs > @@ -20,6 +20,8 @@ use task_cache::TaskCache; > > // TODO: Does this number make sense? > const CACHED_TASKS_PER_REMOTE: usize = 2000; > +const REFRESH_EVERY_S: i64 = 300; > +const OVERLAP_S: i64 = 60; > > /// Get tasks for all remotes > // FIXME: filter for privileges > @@ -42,24 +44,32 @@ pub async fn get_tasks(filters: TaskFilters) -> Result, Error> > // a task's endtime, which is only returned by > // /nodes//tasks... > // Room for improvements in the future. > - invalidate_cache_for_finished_tasks(&mut cache); > + let force_refresh_remotes = get_remotes_with_finished_tasks(); > > + let now = proxmox_time::epoch_i64(); > for (remote_name, remote) in &remotes.sections { > - if let Some(tasks) = cache.get_tasks(remote_name.as_str()) { > - // Data in cache is recent enough and has not been invalidated. > - all_tasks.extend(tasks); > - } else { > - let tasks = match fetch_tasks(remote).await { > - Ok(tasks) => tasks, > + let last_fetched = cache.get_last_fetched(remote_name).unwrap_or(0); > + let diff = now - last_fetched; > + > + if diff > REFRESH_EVERY_S || diff < 0 || force_refresh_remotes.contains(remote_name) { > + // Add some overlap so that we for sure fetch every task - duplicates > + // are remove when adding the tasks to the cache. > + let fetch_since = > + (cache.get_most_recent_starttime(remote_name).unwrap_or(0) - OVERLAP_S).max(0); > + > + match fetch_tasks(remote, fetch_since).await { > + Ok(tasks) => { > + cache.add_tasks(remote_name.as_str(), tasks, now); > + } > Err(err) => { > // ignore errors for not reachable remotes > continue; > } > - }; > - cache.add_tasks(remote_name.as_str(), tasks.clone()); > - > - all_tasks.extend(tasks); > + } > } > + > + let tasks = cache.get_tasks(remote_name).unwrap_or_default(); > + all_tasks.extend(tasks); > } > > let mut returned_tasks = add_running_tasks(all_tasks)?; > @@ -125,7 +135,7 @@ pub async fn get_tasks(filters: TaskFilters) -> Result, Error> > } > > /// Fetch tasks (active and finished) from a remote > -async fn fetch_tasks(remote: &Remote) -> Result, Error> { > +async fn fetch_tasks(remote: &Remote, since: i64) -> Result, Error> { > let mut tasks = Vec::new(); > > match remote.ty { > @@ -138,9 +148,8 @@ async fn fetch_tasks(remote: &Remote) -> Result, Error> { > let params = ListTasks { > // Include running tasks > source: Some(ListTasksSource::All), > - // TODO: How much task history do we want? Right now we just hard-code it > - // to 7 days. > - since: Some(proxmox_time::epoch_i64() - 7 * 24 * 60 * 60), > + since: Some(since), > + limit: Some(CACHED_TASKS_PER_REMOTE as u64), > ..Default::default() > }; > > @@ -182,18 +191,17 @@ fn map_tasks(tasks: Vec, remote: &str) -> Result Ok(mapped) > } > > -/// Drops the cached task list of a remote for all finished tasks. > +/// Get list of remotes which have finished tasks. > /// > /// We use this to force a refresh so that we get the full task > /// info (including `endtime`) in the next API call. > -fn invalidate_cache_for_finished_tasks(cache: &mut TaskCache) { > +fn get_remotes_with_finished_tasks() -> HashSet { > let mut finished = FINISHED_FOREIGN_TASKS.write().expect("mutex poisoned"); Already mentioned this off list (since I also had looked at the old code the first time just this week): This could just use let finished = std::mem::take(&mut *FINISHED_...lock().unwrap()); to immediately unlock the mutex and use `into_iter()` instead of `drain()` which should be more efficient. > > - // If a task is finished, we force a refresh for the remote - otherwise > - // we don't get the 'endtime' for the task. > - for task in finished.drain() { > - cache.invalidate_cache_for_remote(task.remote()); > - } > + finished > + .drain() > + .map(|task| task.remote().to_string()) > + .collect() > } > > /// Supplement the list of tasks that we received from the remote with > diff --git a/server/src/remote_tasks/task_cache.rs b/server/src/remote_tasks/task_cache.rs > index 8a98876..e08e3d4 100644 > --- a/server/src/remote_tasks/task_cache.rs > +++ b/server/src/remote_tasks/task_cache.rs > @@ -107,10 +107,11 @@ impl TaskCache { > /// > /// If the total number of stored tasks exceeds `max_tasks_per_remote`, the > /// oldest ones are truncated. > - pub(super) fn add_tasks(&mut self, remote: &str, tasks: Vec) { > + pub(super) fn add_tasks(&mut self, remote: &str, tasks: Vec, last_fetched: i64) { > let entry = self.content.remote_tasks.entry(remote.into()).or_default(); > > entry.tasks = Self::merge_tasks(entry.tasks.clone(), tasks, self.max_tasks_per_remote); `entry.tasks = something(entry.tasks.clone())` seems wasteful, this is also solved by `take()` entry.tasks = Self::merge_tasks(take(&mut entry.tasks), ...); > + entry.last_fetched = last_fetched; > > self.dirty = true; > } > @@ -123,10 +124,21 @@ impl TaskCache { > .map(|entry| entry.tasks.clone()) > } > > - // Invalidate cache for a given remote. > - pub(super) fn invalidate_cache_for_remote(&mut self, remote: &str) { > - self.dirty = true; > - self.content.remote_tasks.remove(remote); > + // Get the `last_fetched` time stamp for a given remote. > + pub(super) fn get_last_fetched(&self, remote: &str) -> Option { > + self.content > + .remote_tasks > + .get(remote) > + .map(|entry| entry.last_fetched) > + } > + > + // Get the `most_recent_starttime` time stamp for a given remote. > + pub(super) fn get_most_recent_starttime(&self, remote: &str) -> Option { > + if let Some(entry) = self.content.remote_tasks.get(remote) { > + Some(entry.tasks.first()?.starttime) > + } else { > + None > + } > } > > // Lock the cache for modification. > @@ -211,6 +223,7 @@ where > /// Per-remote entry in the task cache. > struct TaskCacheEntry { > tasks: Vec, > + last_fetched: i64, > } > > #[derive(Debug, Default, Serialize, Deserialize)] > @@ -267,7 +280,7 @@ mod tests { > tasks.push(make_upid(now - 10 * i, None, None)?); > } > > - cache.add_tasks("some-remote", tasks.clone()); > + cache.add_tasks("some-remote", tasks.clone(), 0); > cache.save()?; > > let cache = TaskCache::new(temp_file.path().into(), options, 50)?; > -- > 2.39.5 _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel