WIP: Massive pacemaker-based refactors #4011
Draft
+3,997
−3,790
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nrwahl2
force-pushed
the
nrwahl2-based
branch
3 times, most recently
from
deaa1a7 to
bc1dd5d
Compare
nrwahl2
force-pushed
the
nrwahl2-based
branch
3 times, most recently
from
852e402 to
5241abc
Compare
It's always set to !preserve_status, and preserve_status is always set to the same value as stand_alone. So we can use !stand_alone in place of discard_status. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The sole call sets it to "cib.xml". Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
They're always set to cib_root. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
cib_archive_sort() sorts directory entries in the reverse order from the one documented. The code still worked as intended, because the while loop iterated in reverse order. This commit sorts directory entries from newest to oldest and then iterates starting at index 0. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
And add Doxygen. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
...and in read_current_cib(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This has nothing to do with reading the CIB from a file, so it should not be part of based_read_cib(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
* Rename function to archive_unusable_file(). * Add Doxygen. * Improve log messages. * Rename new_fd variable to fd. * Allow fd == 0. This should never happen in practice, but it's odd to check (fd < 0) for error and (fd > 0) for success when 0 is a valid file descriptor. * Close fd sooner on success. mkstemp() is the safe and idiomatic way to generate a temp file name, because it avoids race conditions that can occur when generating a file name and then creating a file. However, we don't need the file to be open, and we don't need the fd except for the purpose of closing the file. By separating the (fd < 0) case from the (rename(...) < 0) case, we can be sure that (fd >= 0) when we call close(). * Rename old and new variables to old_file and new_file, respectively. I suspect that the use of "new" may be problematic when compiling as C++. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Previously, in case of a digest mismatch, we renamed the CIB file and the digest file in cib_root to new unique names generated by mkstemp(). Without checking the error logs, the file names did not make it clear that the two files were related, or which one was the CIB file and which one was the digest file. Now, we create a new subdirectory of cib_root using mkdtemp(). Then we move both the CIB file and the digest file to the new subdirectory. They keep the base names "cib.xml" and "cib.xml.sig". It's not necessary for archive_on_digest_mismatch() to take arguments, but this avoids the need to allocate redundant strings for the old paths. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Also add Doxygen and rename to backup_cib_filter(). The trace/debug messages don't seem especially useful, and we recently got rid of similar ones that were commented out of schema_filter(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Also add Doxygen, rename to compare_backup_cibs(), and create a helper function. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Note that the return value is guaranteed not to be NULL, because the createEmptyCib() return value is guaranteed not to be NULL. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
As long as the_cib is a global variable, this function is unnecessary. Also, it was too complicated -- it can be a one-liner, as shown here. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
There are two callers. The argument in startCib() is clearly non-NULL. The argument in cib_process_command() is unsurprisingly less clear. result_cib gets set by cib_perform_op(), and we call activateCibXml() only if we're performing an operation that modifies the CIB, rc is pcmk_rc_ok, and (result_cib != the_cib). result_cib should always be non-NULL in that case. And if it ever is NULL, then this is something I'd like to find out about via an assertion log. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Also include stddef.h for NULL, and rename cib_op_functions to op_functions so that it doesn't look like a public libcib symbol. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It was always NULL. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The XSL transformation pipeline frees its XML argument and replaces it with a new XML tree. libxslt and libxml2 documentation are missing a lot of info, such as whether xsltApplyStylesheet returns a new document or modifies the existing one in place. We can avoid guesswork as follows: * Make a copy of the CIB (name the variable "upgraded"). * Perform the upgrade on "upgraded". * Replace the CIB root element with a copy of "upgraded" (which unlinks and frees the original CIB root element). * Free "upgraded". In this way, our resulting CIB is an upgraded CIB, but it's part of the same document as the pre-upgrade CIB, which has been freed. Specifically, it's the new root of that same document. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
I don't know of any way to enforce this, but if we adhere to this requirement, we can make cib_perform_op() simpler. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Now that we have updated all cib__op_fn_t functions so that they don't change what document the resulting *cib is part of (or unset its tracking flag), this is unnecessary. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The new approach is to set the caller's result_cib pointer to the current CIB instead of to NULL. Then pass &result_cib to cib_perform_op() as its "cib" argument. * If it doesn't make a copy, then it operates on the original CIB directly and then assign the original CIB to working_cib -- just as we were already doing. * If it does make a copy, then it's copying the original CIB, just as we were already doing. * If it makes *cib point somewhere else at the end (in the "done" section), then that changes the value of the caller's result_cib, but it DOES NOT change the value of caller's current CIB pointer (the_cib in pacemaker-based or private->cib_xml in cib_file.c). So the caller still has a reference to the original CIB. The logic is arguably a bit more complicated, but previously we had five CIB pointers (current_cib, result_cib, working_cib, old_versions, top) in cib_perform_op(). Now we only have to deal with four. That feels more manageable and thus worth it IMO. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We were setting *cib = working_cib in the "done" section. There are no early returns, so that always happens before we return. The only real change required here, is that we assign *cib to old_versions before setting *cib to a copy of itself. (Otherwise, old_versions would point to the copy.) We now have only three CIB pointers to deal with in cib_perform_op, not including saved_cib, which is used only in a small scope in order to free an object. We also have some easy opportunities to reduce duplication in an upcoming commit. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The request argument provides the op value for functions that need it. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The request argument provides the options value for functions that need it. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The request argument provides the section value for functions that need it. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
...directly, instead of calling cib__process_apply_patch(). This will make an upcoming commit simpler. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The request argument provides the input value for functions that need it, via cib__get_calldata(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We were passing the input argument just for logging on error. But we're already logging the request, and the request always contains the input (if non-NULL) as the child of a PCMK__XE_CIB_CALLDATA child. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
I'm ambivalent about this, since it adds lines of code, but request contains everything needed by a lot of these helper functions. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We already warn for the same thing in based_process_request() if applicable. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This is already done a few lines prior. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
And drop a fairly useless trace message. cib_dryrun always has to be false there, by the way. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
So that we can use pcmk__xe_get_ll(). This also avoids theoretical overflow risk, if a value is sent as uint64_t and then scanned as long long upon receiving it. That overflow should never happen in practice, as it would require sending a massive number of pings before restarting based. Also improve documentation a bit and add a note of non-understanding. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It might be possible for a new DC to be elected after we send a ping request and before we process a modifying request (which would set ping_modified_since to true and cause the ping reply to be ignored). Add this check just in case. If we're no longer DC, then we don't want to sync our CIB to other nodes. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
These are called only by cib_perform_op()/cib__perform_query(), which assert that *answer is NULL. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It's used for all non-modifying ops, not just queries. This also enables renaming cib_perform_op() to cib__perform_op_rw() next. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Avoid using the public API prefix, and make it clearer that this is not for all CIB ops, but rather the ones that may modify something. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.