Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory issues with pcmk__schedule_actions() #3810

Merged
merged 19 commits into from
Feb 7, 2025
Merged

Fix memory issues with pcmk__schedule_actions() #3810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a90267c
Refactor: various: Use uint32_t for group of pcmk_sim_flags internally
nrwahl2 Jan 24, 2025
ff6e7a9
Refactor: fencer: Set scheduler input, flags, and status explicitly
nrwahl2 Jan 24, 2025
37f7210
Refactor: scheduler: Set scheduler input, flags, and status explicitly
nrwahl2 Jan 24, 2025
625672d
Fix: tools: Avoid crash in crm_simulate --profile
nrwahl2 Jan 24, 2025
8b9caad
Low: libpacemaker: Fix mem leak in pcmk__profile_dir()
nrwahl2 Jan 24, 2025
a1d8655
Refactor: libpacemaker: Create scheduler object in pcmk__profile_dir()
nrwahl2 Jan 24, 2025
481b0dd
Low: libpacemaker: Handle scandir() error in pcmk__profile_dir()
nrwahl2 Jan 24, 2025
64281ae
Refactor: libpacemaker: Best practices for pcmk__profile_dir()
nrwahl2 Jan 24, 2025
9f1edd8
Refactor: libpacemaker: Use scandir() filter in pcmk__profile_dir()
nrwahl2 Jan 24, 2025
3fca1ba
Low: tools: Fix overflow in crm_simulate --repeat
nrwahl2 Jan 25, 2025
767c55f
Refactor: libpacemaker: Pass NULL input in pcmk__simulate()
nrwahl2 Jan 25, 2025
27d519f
Refactor: libpacemaker: Set input and flags explicitly in pcmk__verify()
nrwahl2 Jan 25, 2025
eae8f5b
Refactor: libpacemaker: Don't make unnecessary copy for scheduler input
nrwahl2 Jan 26, 2025
00b82bd
Refactor: tools: update_dataset() passes nulls to pcmk__schedule_actions
nrwahl2 Jan 25, 2025
ae63ed5
Refactor: tools: Set scheduler flags explicitly in wait_till_stable()
nrwahl2 Jan 25, 2025
4a3f90b
Refactor: libpacemaker: Drop unused args from pcmk__schedule_actions()
nrwahl2 Jan 25, 2025
5dbc819
Fix: libpe_status: Make cluster_status() idempotent
nrwahl2 Jan 25, 2025
f280506
Refactor: libpacemaker: Unpack status in pcmk__schedule_actions()
nrwahl2 Jan 25, 2025
b1ac2a5
Feature: libpacemaker: Reset scheduler object in pcmk_simulate()
nrwahl2 Jan 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions daemons/fenced/fenced_scheduler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2009-2024 the Pacemaker project contributors
* Copyright 2009-2025 the Pacemaker project contributors
*
* The version control history for this file may have further details.
*
Expand All @@ -19,6 +19,7 @@
#include <pacemaker-internal.h>
#include <pacemaker-fenced.h>

// fenced_scheduler_run() assumes it's the only place scheduler->input gets set
static pcmk_scheduler_t *scheduler = NULL;

/*!
Expand Down Expand Up @@ -233,19 +234,22 @@ register_if_fencing_device(gpointer data, gpointer user_data)
* \internal
* \brief Run the scheduler for fencer purposes
*
* \param[in] cib Cluster's current CIB
* \param[in] cib CIB to use as scheduler input
*
* \note Scheduler object is reset before returning, but \p cib is not freed.
*/
void
fenced_scheduler_run(xmlNode *cib)
{
CRM_CHECK((cib != NULL) && (scheduler != NULL), return);
CRM_CHECK((cib != NULL) && (scheduler != NULL)
&& (scheduler->input == NULL), return);

if (scheduler->priv->now != NULL) {
crm_time_free(scheduler->priv->now);
scheduler->priv->now = NULL;
}
pcmk__schedule_actions(cib, pcmk__sched_location_only
|pcmk__sched_no_counts, scheduler);
pcmk_reset_scheduler(scheduler);

scheduler->input = cib;
pcmk__set_scheduler_flags(scheduler,
pcmk__sched_location_only|pcmk__sched_no_counts);
pcmk__schedule_actions(scheduler);
g_list_foreach(scheduler->priv->resources, register_if_fencing_device,
NULL);

Expand Down
14 changes: 9 additions & 5 deletions daemons/schedulerd/schedulerd_messages.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2004-2024 the Pacemaker project contributors
* Copyright 2004-2025 the Pacemaker project contributors
*
* The version control history for this file may have further details.
*
Expand Down Expand Up @@ -91,9 +91,14 @@ handle_pecalc_request(pcmk__request_t *request)
}

if (process) {
pcmk__schedule_actions(converted,
pcmk__sched_no_counts
|pcmk__sched_show_utilization, scheduler);
scheduler->input = converted;
pcmk__set_scheduler_flags(scheduler,
pcmk__sched_no_counts
|pcmk__sched_show_utilization);
pcmk__schedule_actions(scheduler);

// Don't free converted as part of scheduler
scheduler->input = NULL;
}

// Get appropriate index into series[] array
Expand Down Expand Up @@ -122,7 +127,6 @@ handle_pecalc_request(pcmk__request_t *request)
crm_trace("Series %s: wrap=%d, seq=%u, pref=%s",
series[series_id].name, series_wrap, seq, value);

scheduler->input = NULL;
reply = pcmk__new_reply(msg, scheduler->priv->graph);

if (reply == NULL) {
Expand Down
6 changes: 3 additions & 3 deletions include/pacemaker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2019-2024 the Pacemaker project contributors
* Copyright 2019-2025 the Pacemaker project contributors
*
* The version control history for this file may have further details.
*
Expand Down Expand Up @@ -32,6 +32,7 @@ extern "C" {
* \brief Modify operation of running a cluster simulation.
*/
enum pcmk_sim_flags {
// @COMPAT Use UINT32_C(1); should not affect behavior
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with UINT32_C() but I see that we are using it in a variety of places already. Should we be using it in the definitions of all these flags enums?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically yeah. It's a macro that makes a fixed-width integer constant basically. I don't think it matters AT ALL. It was something that I suggested as a super-pedantic point on some Pacemaker PR that I halfway reviewed while I was still in support. Ken started using it in a fair number of places after that.

pcmk_sim_none = 0,
pcmk_sim_all_actions = 1 << 0,
pcmk_sim_show_pending = 1 << 1,
Expand Down Expand Up @@ -234,8 +235,7 @@ int pcmk_resource_digests(xmlNodePtr *xml, pcmk_resource_t *rsc,
* \param[in,out] scheduler Scheduler data
* \param[in] injections A structure containing cluster events
* (node up/down, tickets, injected operations)
* \param[in] flags A bitfield of :pcmk_sim_flags to modify
* operation of the simulation
* \param[in] flags Group of <tt>enum pcmk_sim_flags</tt>
* \param[in] section_opts Which portions of the cluster status output
* should be displayed?
* \param[in] use_date Date to set the cluster's time to (may be NULL)
Expand Down
5 changes: 2 additions & 3 deletions include/pcmki/pcmki_scheduler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2014-2024 the Pacemaker project contributors
* Copyright 2014-2025 the Pacemaker project contributors
*
* The version control history for this file may have further details.
*
Expand Down Expand Up @@ -36,8 +36,7 @@ typedef struct {

void pcmk__unpack_constraints(pcmk_scheduler_t *scheduler);

void pcmk__schedule_actions(xmlNode *cib, unsigned long long flags,
pcmk_scheduler_t *scheduler);
void pcmk__schedule_actions(pcmk_scheduler_t *scheduler);

GList *pcmk__copy_node_list(const GList *list, bool reset);

Expand Down
23 changes: 11 additions & 12 deletions include/pcmki/pcmki_simulate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2021-2024 the Pacemaker project contributors
* Copyright 2021-2025 the Pacemaker project contributors
*
* The version control history for this file may have further details.
*
Expand Down Expand Up @@ -28,16 +28,16 @@ extern "C" {
* CIB file in a given directory, printing the profiling timings for
* each.
*
* \note \p scheduler->priv->out must have been set to a valid \p pcmk__output_t
* object before this function is called.
* \param[in,out] out Output object
* \param[in] flags Group of <tt>enum pcmk_sim_flags</tt>
* \param[in] dir Directory full of CIB files to be profiled
* \param[in] repeat Number of times to run on each input file
* \param[in] use_date Date to set the cluster's time to (can be \c NULL)
*
* \param[in] dir A directory full of CIB files to be profiled
* \param[in] repeat Number of times to run on each input file
* \param[in,out] scheduler Scheduler data
* \param[in] use_date The date to set the cluster's time to (may be NULL)
* \return Standard Pacemaker return code
*/
void pcmk__profile_dir(const char *dir, long long repeat,
pcmk_scheduler_t *scheduler, const char *use_date);
int pcmk__profile_dir(pcmk__output_t *out, uint32_t flags, const char *dir,
unsigned int repeat, const char *use_date);

/*!
* \internal
Expand Down Expand Up @@ -67,8 +67,7 @@ enum pcmk__graph_status pcmk__simulate_transition(pcmk_scheduler_t *scheduler,
* \param[in] injections A structure containing cluster events
* (node up/down, tickets, injected operations)
* and related data
* \param[in] flags A bitfield of \p pcmk_sim_flags to modify
* operation of the simulation
* \param[in] flags Group of <tt>enum pcmk_sim_flags</tt>
* \param[in] section_opts Which portions of the cluster status output
* should be displayed?
* \param[in] use_date The date to set the cluster's time to
Expand All @@ -85,7 +84,7 @@ enum pcmk__graph_status pcmk__simulate_transition(pcmk_scheduler_t *scheduler,
* \return Standard Pacemaker return code
*/
int pcmk__simulate(pcmk_scheduler_t *scheduler, pcmk__output_t *out,
const pcmk_injections_t *injections, unsigned int flags,
const pcmk_injections_t *injections, uint32_t flags,
uint32_t section_opts, const char *use_date,
const char *input_file, const char *graph_file,
const char *dot_file);
Expand Down
33 changes: 3 additions & 30 deletions lib/pacemaker/pcmk_scheduler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2004-2024 the Pacemaker project contributors
* Copyright 2004-2025 the Pacemaker project contributors
*
* The version control history for this file may have further details.
*
Expand Down Expand Up @@ -730,43 +730,16 @@ log_unrunnable_actions(const pcmk_scheduler_t *scheduler)
}
}

/*!
* \internal
* \brief Unpack the CIB for scheduling
*
* \param[in,out] cib CIB XML to unpack (may be NULL if already unpacked)
* \param[in] flags Scheduler flags to set in addition to defaults
* \param[in,out] scheduler Scheduler data
*/
static void
unpack_cib(xmlNode *cib, unsigned long long flags, pcmk_scheduler_t *scheduler)
{
if (pcmk_is_set(scheduler->flags, pcmk__sched_have_status)) {
crm_trace("Reusing previously calculated cluster status");
pcmk__set_scheduler_flags(scheduler, flags);
return;
}
pcmk__assert(cib != NULL);
crm_trace("Calculating cluster status");
pcmk_reset_scheduler(scheduler);
pcmk__set_scheduler_flags(scheduler, flags);
scheduler->input = cib;
cluster_status(scheduler); // Sets pcmk__sched_have_status
}

/*!
* \internal
* \brief Run the scheduler for a given CIB
*
* \param[in,out] cib CIB XML to use as scheduler input
* \param[in] flags Scheduler flags to set in addition to defaults
* \param[in,out] scheduler Scheduler data
*/
void
pcmk__schedule_actions(xmlNode *cib, unsigned long long flags,
pcmk_scheduler_t *scheduler)
pcmk__schedule_actions(pcmk_scheduler_t *scheduler)
{
unpack_cib(cib, flags, scheduler);
cluster_status(scheduler);
pcmk__set_assignment_methods(scheduler);
pcmk__apply_node_health(scheduler);
pcmk__unpack_constraints(scheduler);
Expand Down
Loading