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.

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

Convert to using libpacemaker for managing tickets #144

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

clumens
Copy link

@clumens clumens commented Jun 25, 2024

I've tested this a little bit several weeks ago, but obviously it needs to be more thoroughly tested again. I'd love to come up with a way to do this automatically, because it's kind of a pain to set up a test for it. Suggestions welcome.

For the moment, this is just a draft PR to see if I'm on the right track.

@clumens clumens requested a review from jfriesse June 25, 2024 14:04
src/main.c Outdated
@@ -1627,6 +1630,10 @@ int main(int argc, char *argv[], char *envp[])
enum qb_log_target_slot i;
#endif

#ifdef LIBPACEMAKER
crm_xml_init();
Copy link
Author

Choose a reason for hiding this comment

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

We've deprecated crm_xml_init in pacemaker. I'm not sure what the replacement for external users is at the moment, if any. We'll have to think of something, though. Looking into it.

Copy link
Author

Choose a reason for hiding this comment

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

We're going to need to take care of https://projects.clusterlabs.org/T840 before this can be worked on.

Copy link
Member

@jfriesse jfriesse left a comment

Choose a reason for hiding this comment

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

This is review of draft so no ack/nack yet. It seems to work just fine so I guess general direction is good.

I think it might make sense to rename pacemaker.c to pcmk.c so h and c file name matches.

booth-cfg-nameattribute is not set.

Pacemaker already includes a public pacemaker.h which we are going to
start using, and gcc doesn't like two header files with the same name in
different locations.

And rename pacemaker.c to pcmk.c for consistency.
This is required to initialize various internal pacemaker XML structures
and schemas before they are used the first time.  Without this, calling
into libpacemaker will result in tracebacks.
libpacemaker provides a public API for managing tickets.  A first step
towards using this API is just the setting and removing code, which is
pretty straightforward.  We can ignore the XML result portion of the API
because in these cases it doesn't contain anything more useful than just
the return code.
Similar to the previous commit, we can use the new libpacemaker API to
also handle atomically modifying a ticket.
While this function isn't currently used, we might as well convert
everything to use libpacemaker in case it finds a use in the future.  It
also provides a good example of how to do this same thing elsewhere in
future commits.
This function is now slightly different.  Instead of going all the way
from reading a file pointer to loading attributes, it now stops before
loading the attributes and returns an XML document.  The attribute
loading happens afterwards in the caller.
The function itself is very simple.  The rest of the work is in
save_attributes, where it needs to understand both the libpacemaker XML
result as well as the result of calling crm_ticket directly.  It's still
pretty simple, though - just digging around in the XML to set a node
pointer correctly.

Fixes ClusterLabs#136
@clumens
Copy link
Author

clumens commented Oct 1, 2024

Rebased to take care of file conflicts and get rid of the couple initial rpm/build patches that were merged elsewhere. We are still working on the libpacemaker initialization stuff, so this is still not ready to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants