Archive append support#2611
Open
natoscott wants to merge 12 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds PMI append mode and atom-value handle writes, refactors import value allocation, documents Python usage and typings, introduces high-resolution write/result APIs, updates headers/exports, and adds QA tests exercising create/append/fallback behaviors. ChangesPCP log import append mode, atom-value handle, and documentation updates
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/python/pcp/pmi.py (1)
389-398:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
pmiWriteto handlesec=0whenusecis omitted (avoid passingNonetopmiWrite2).When
sec == 0andusecis omitted (usec=None), the conditionif sec and not usec:doesn’t run, soLIBPCP_IMPORT.pmiWrite2(sec, usec)is called withusec=None. SincepmiWrite2.argtypes = [c_longlong, c_int], this raisesTypeErrorat runtime.Proposed fix
- if sec and not usec: + if usec is None: if isinstance(sec, datetime): sec = float((sec - self._epoch).total_seconds()) if isinstance(sec, float): ts = modf(sec) sec = int(ts[1]) usec = int(ts[0] * 1000000) else: usec = 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/python/pcp/pmi.py` around lines 389 - 398, The bug is that when sec == 0 and usec is None the branch `if sec and not usec:` is skipped and `pmiWrite2(sec, usec)` is called with usec==None causing a TypeError; update the condition to explicitly check for a provided sec (e.g., `if sec is not None and usec is None:`) so the conversion logic that handles datetime (using `self._epoch` and `modf`) and the fallback `usec = 0` are executed for sec==0 as well, ensuring `LIBPCP_IMPORT.pmiWrite2(sec, usec)` is always called with an integer usec.src/libpcp_import/src/import.c (1)
342-354:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
PMI_APPEND | PMI_INHERITor define a hard precedence.This branch still clones the previous context when
PMI_INHERITis set, even after putting the new context into append mode. The append path described forsrc/libpcp_import/src/archive.calso reloads metric and indom metadata from the target archive, so this combination can seed a second, potentially unrelated metadata/handle set before append initialization runs. That risks duplicate descriptors or handles pointing at the wrong metric indexes during append.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp_import/src/import.c` around lines 342 - 354, Detect and handle the conflicting PMI_APPEND | PMI_INHERIT case in the pmiStart path: before setting current->state and before running the inheritance block that uses old_current, check if ((flags & PMI_APPEND) && (flags & PMI_INHERIT)) and either (A) reject the call with a clear error/return (e.g., return PM_ERR with a message) or (B) enforce a hard precedence by clearing one flag (e.g., prefer append: flags &= ~PMI_INHERIT) so the subsequent inheritance block (the code referencing old_current and cloning metadata) is skipped; implement the chosen behavior immediately after flags are received and ensure the rest of the function (current->state, current->archive, and the if ((flags & PMI_INHERIT) && old_current != NULL) block) acts consistently with that decision.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qa/group`:
- Line 2212: Update the test mapping for identifier 1670 in qa/group so the
entry includes the local sweep group; locate the line "1670 libpcp_import" and
change it to include "local" alongside the existing group (e.g., "1670 local
libpcp_import") following the same spacing/order style used by other entries so
the standard local sweep will run this test.
In `@src/libpcp_import/src/archive.c`:
- Around line 195-211: The code currently treats a successful
__pmLogLookupDesc(acp, pmid, &existing) as meaning all metadata is present;
instead, when __pmLogLookupDesc returns 0 you must compare the returned existing
descriptor and its namelist against current->metric[m].desc and namelist to
ensure compatibility and that any new aliases are present; only if descriptors
are identical/compatible and the existing namelist already contains the current
namelist set current->metric[m].meta_done = 1, otherwise call
__pmLogPutDesc(acp, ¤t->metric[m].desc, 1, namelist) (and set *needti and
meta_done appropriately) so new alias entries are persisted and incompatible
re-registrations are detected/handled.
In `@src/libpcp_import/src/import.c`:
- Around line 891-899: pmiPutAtomValueHandle currently only validates the
handle; first check that the atom pointer is not NULL and return a suitable
error via current->last_sts (e.g. PMI_ERR_BADARG) if it is, and if atom->type ==
PM_TYPE_STRING also validate that atom->cp is not NULL (again set and return
current->last_sts = PMI_ERR_BADARG) before calling _pmi_stuff_atomvalue; this
prevents _pmi_stuff_atomvalue from dereferencing atom or doing strlen(atom->cp)
on a NULL pointer.
In `@src/libpcp/src/logutil.c`:
- Around line 411-417: Before trusting the loaded temporal index, validate the
`.index` label using __pmLogChkLabel on lcp->tifp (the same check __pmLogOpen
does) and only call __pmLogLoadIndex if __pmLogChkLabel succeeds; if the check
fails, call __pmFclose on lcp->mdfp and lcp->tifp, set them to NULL, and return
an error (as is done on other failures) so lcp->endtime cannot be seeded from a
stale or mismatched .index. Ensure you reference __pmLogChkLabel,
__pmLogLoadIndex, __pmFclose and lcp->tifp in the change and preserve the
existing cleanup pattern.
---
Outside diff comments:
In `@src/libpcp_import/src/import.c`:
- Around line 342-354: Detect and handle the conflicting PMI_APPEND |
PMI_INHERIT case in the pmiStart path: before setting current->state and before
running the inheritance block that uses old_current, check if ((flags &
PMI_APPEND) && (flags & PMI_INHERIT)) and either (A) reject the call with a
clear error/return (e.g., return PM_ERR with a message) or (B) enforce a hard
precedence by clearing one flag (e.g., prefer append: flags &= ~PMI_INHERIT) so
the subsequent inheritance block (the code referencing old_current and cloning
metadata) is skipped; implement the chosen behavior immediately after flags are
received and ensure the rest of the function (current->state, current->archive,
and the if ((flags & PMI_INHERIT) && old_current != NULL) block) acts
consistently with that decision.
In `@src/python/pcp/pmi.py`:
- Around line 389-398: The bug is that when sec == 0 and usec is None the branch
`if sec and not usec:` is skipped and `pmiWrite2(sec, usec)` is called with
usec==None causing a TypeError; update the condition to explicitly check for a
provided sec (e.g., `if sec is not None and usec is None:`) so the conversion
logic that handles datetime (using `self._epoch` and `modf`) and the fallback
`usec = 0` are executed for sec==0 as well, ensuring
`LIBPCP_IMPORT.pmiWrite2(sec, usec)` is always called with an integer usec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9da0a39b-e55c-4866-b2da-9302dda7cacf
⛔ Files ignored due to path filters (3)
qa/1418.outis excluded by!**/*.outqa/1670.outis excluded by!**/*.outqa/369.outis excluded by!**/*.out
📒 Files selected for processing (32)
man/man3/pmiaddinstance.3man/man3/pmiaddmetric.3man/man3/pmiend.3man/man3/pmigethandle.3man/man3/pmiputlabel.3man/man3/pmiputmark.3man/man3/pmiputresult.3man/man3/pmiputtext.3man/man3/pmiputvalue.3man/man3/pmiputvaluehandle.3man/man3/pmisethostname.3man/man3/pmisettimezone.3man/man3/pmisetversion.3man/man3/pmistart.3man/man3/pmiunits.3man/man3/pmiwrite.3qa/1670qa/groupqa/src/.gitignoreqa/src/GNUlocaldefsqa/src/check_import.cqa/src/check_import_append.csrc/include/pcp/import.hsrc/include/pcp/libpcp.hsrc/libpcp/src/exports.insrc/libpcp/src/logutil.csrc/libpcp_import/src/archive.csrc/libpcp_import/src/exportssrc/libpcp_import/src/import.csrc/libpcp_import/src/private.hsrc/libpcp_import/src/stuff.csrc/python/pcp/pmi.py
natoscott
force-pushed
the
pmi-append-mode
branch
from
ec641ac to
ad79e75
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libpcp/src/logutil.c`:
- Around line 411-418: The early-return after __pmLogChkLabel failure in
__pmLogOpenAppend (which currently calls __pmLogFreeLabel, __pmFclose on
lcp->mdfp and lcp->tifp and returns sts) must be changed to use the shared
cleanup path: remove the direct closes/return and instead jump to a common
failure label that calls __pmLogClose(acp) and logFreeMeta(lcp) (so lcp->label
and lcp->name/meta are freed) before returning sts; apply the same change to the
other early-return branches in __pmLogOpenAppend to ensure all error exits
funnel through the same cleanup sequence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 16472fcd-c22f-433d-89a4-eea88554bcc4
📒 Files selected for processing (1)
src/libpcp/src/logutil.c
natoscott
force-pushed
the
pmi-append-mode
branch
8 times, most recently
from
548a7ec to
b119458
Compare
natoscott
force-pushed
the
pmi-append-mode
branch
from
c181c62 to
2bf08a3
Compare
Rename the second argument of pmiStart() from the implicit boolean
'inherit' to an explicit 'flags' field. Named constants are now
provided in import.h:
PMI_INHERIT (0x1) - existing behaviour: inherit metric definitions
from the current context into the new one
PMI_APPEND (0x2) - open an existing archive for appending instead
of creating a new one
pmiStart() with PMI_APPEND opens the named archive if it already
exists and positions all file handles at end-of-file, loading the
existing metadata so that subsequent pmiWrite() calls append new
records after the last existing record. If the archive does not
yet exist the flag is silently ignored and a new archive is created,
making PMI_APPEND safe for unconditional use in timer-driven
collectors (e.g. sadc) on first invocation.
Add QA test 1693 to exercise the append mode.
Signed-off-by: Nathan Scott <nathans@redhat.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
pmiWrite2(int64_t sec, int usec) and pmiHighResWrite(int64_t sec, int nsec) were already implemented and exported from the shared library but were missing from the public import.h header, making them unusable by callers without forward-declaring them manually. Add declarations for both and deprecate pmiWrite(int, int) with a comment: its 32-bit seconds argument is not Y2038-safe and callers should migrate to pmiHighResWrite() for nanosecond-resolution timestamps or pmiWrite2() for microsecond resolution. Signed-off-by: Nathan Scott <nathans@redhat.com> Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
logutil.c: rework __pmLogOpenAppend() to use __pmLogFindOpen() for volume discovery. Previously we iterated using lcp->numti (temporal index count) as a starting hint; this bypassed the canonical readdir + __pmLogAddVolume path. Now: - __pmLogFindOpen() performs the directory scan, correctly setting lcp->name, lcp->minvol and lcp->maxvol via __pmLogAddVolume. - .meta and .index handles from __pmLogFindOpen (read mode) are used to validate the archive label and load the temporal index for lcp->endtime, then closed and reopened in r+ mode for writing. - The highest data volume (lcp->maxvol) is opened in r+ mode so its label is validated with __pmLogChkLabel before appending, matching the consistency checking done by __pmLogChangeVol in the read path. - acp->ac_flags |= PM_CTXFLAG_LAST_VOLUME signals that we are at the most recent volume, following the __pmLogOpen convention. import.h: expose pmiWrite2, pmiHighResWrite and pmiPutHighResResult as the preferred Y2038-safe interfaces. pmiWrite(int,int) is marked deprecated. pmiwrite.3: document pmiWrite, pmiWrite2 and pmiHighResWrite on one page; note pmiWrite as deprecated and pmiHighResWrite as preferred. pmiputresult.3: document pmiPutResult and pmiPutHighResResult together; note pmiPutHighResResult as the preferred nanosecond-resolution variant. Signed-off-by: Nathan Scott <nathans@redhat.com> Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Add pmiPutAtomValueHandle(int handle, pmAtomValue *atom) as a binary counterpart to pmiPutValueHandle, passing values directly via the pmAtomValue union rather than as strings requiring parsing. Refactor stuff.c to extract _pmi_alloc_vp and _pmi_alloc_valblock static helpers, eliminating the duplicated result/vset/vp setup and pmValueBlock allocation that both _pmi_stuff_value and the new _pmi_stuff_atomvalue share. Update pmiputvaluehandle.3 and exercise the new function in qa/src/check_import.c covering the INSITU path (U32), DPTR path (64-bit), bad-handle and duplicate-value error cases. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Nathan Scott <nathans@redhat.com>
Add a Python SYNOPSIS section to each libpcp_import man page that has a Python binding in src/python/pcp/pmi.py. The Python API wraps the C library via the pmi.pmiLogImport class; methods are called on an instance. Multi-line call signatures use the same continuation-indent style as the existing Perl SYNOPSIS sections. pmiputresult.3 gains a Python-only synopsis (Perl has no binding for pmiPutResult/pmiPutHighResResult). pmisetversion.3 gains both Perl and Python synopses, fixing a pre-existing omission. pmiusecontext.3 and pmierrstr.3 are left without a Python synopsis as pmi.pmiLogImport handles context switching internally and surfaces errors as exceptions. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Nathan Scott <nathans@redhat.com>
Annotate all method signatures in the pmiLogImport class with PEP 484 type hints, compatible with the project's Python 3.6 minimum. Return types reflect actual Python-level values (int status, pmID, pmInDom, pmUnits, None). String parameters are annotated as str since bytes encoding is an internal detail. pmiPutValue/pmiGetHandle inst parameters are Optional[str] to reflect the None-for-singular-metric convention. pmiWrite sec is Union[int, float, datetime] matching its three-way dispatch. Imports Optional and Union from typing. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Nathan Scott <nathans@redhat.com>
pmiPutAtomValueHandle: validate atom is non-NULL before passing to _pmi_stuff_atomvalue, returning PM_ERR_ARG to avoid a segfault on PM_TYPE_STRING metrics where atom->cp would be dereferenced. archive.c _pmi_do_desc: when a descriptor for this PMID already exists in the archive, verify it is compatible before skipping the write. A package upgrade may have corrected an incorrect type, semantics, instance domain or units for a metric, in which case the old archive cannot be extended with new data. Return PM_ERR_LOGCHANGETYPE, PM_ERR_LOGCHANGESEM, PM_ERR_LOGCHANGEINDOM or PM_ERR_LOGCHANGEUNITS as appropriate so the caller has a precise diagnosis. qa/group: add local group to qa/1670 so the standard local sweep picks up the new append-mode coverage. Signed-off-by: Nathan Scott <nathans@redhat.com> Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The condition 'if sec and not usec' fails when sec==0 because 0 is falsy, leaving usec as None and causing pmiWrite2(0, None) to raise TypeError against its c_int argtypes. Change to 'if usec is None' so the datetime/float dispatch and the usec=0 fallback always run when usec was not supplied by the caller. Add a pmiWrite(0) call at the start of qa/src/test_pmi.python to cover this path, and update qa/708.out accordingly. Signed-off-by: Nathan Scott <nathans@redhat.com> Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Add pmiSetVolumeSize(size_t max_bytes, void (*on_rotate)(const char *))
to libpcp_import. After each successful pmiHighResWrite() or pmiWrite()
call, if the current data volume file size meets or exceeds max_bytes the
library closes the current volume, opens the next numbered volume (writing
a new label), and invokes the caller's callback with the path of the
just-closed volume so compression or other post-processing can be arranged.
Passing max_bytes=0 disables rotation (the default).
Guard: pmiSetVolumeSize() rejects a threshold at or below the on-disk
label size for the configured archive version. Accepting a sub-label
threshold would cause a rotation cascade since every new volume's label
immediately exceeds the threshold. The check uses the public pmapi.h
field-width constants to avoid a dependency on the file-static
__pmLabel_v2/v3 types.
Also: replace the magic number 100000 in _pmi_put_result() with the
named constant PMI_FLUSH_INTERVAL throughout archive.c.
Also: pair consecutive int fields in pmi_context (private.h) to eliminate
four bytes of alignment padding before each pointer on 64-bit platforms.
The struct is private to libpcp_import so there is no ABI constraint.
QA: check_volsize.c exercises rotation, callback, size-bound and label-size
guard; wired into qa/1693 (the existing PMI_APPEND test) with a
pmlogcheck step to verify multi-volume archive integrity.
Signed-off-by: Nathan Scott <nathans@redhat.com>
PCP v3 archives have a 'zoneinfo' label field for the full Olson
database timezone identifier (e.g. "Australia/Brisbane"), which gives
tools DST-correct replay without ambiguity. pmiSetTimezone() only
sets the POSIX abbreviated form ("AEST-10") and was incorrectly
clearing zoneinfo when applying it to the label.
Add pmiSetZoneinfo(const char *value) which sets the zoneinfo field
independently, modelled on pmiSetTimezone(). Passing NULL auto-detects
the local Olson name via __pmZoneinfo() (libpcp tz.c), which handles
both the /etc/localtime symlink case and the recursive file-content
matching fallback.
Signed-off-by: Nathan Scott <nathans@redhat.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
natoscott
force-pushed
the
pmi-append-mode
branch
from
2bf08a3 to
42d815a
Compare
Three sources of repeated metadata writes in PMI_APPEND mode: Indoms: check_indom() had no dedup check so every append session re-wrote all indom records regardless of whether membership had changed. For timer-driven collectors like sadc this caused O(N×S) .meta growth where N is the number of processes and S is the number of sessions per day. Fix: compare the registered indom against the most recent entry in hashindom (populated by __pmLogLoadMeta on append open). For v3 archives use pmaDeltaInDom() — the same helper pmlogger uses — to write a compact TYPE_INDOM_DELTA record when membership changes, and update the hash with the full indom afterwards so the next comparison starts from the correct state. For v2 archives use pmaSameInDom() and fall back to full indom writes. Link -lpcp_archive for access to pmaDeltaInDom() and pmaSameInDom(). Help text: _pmi_put_text() reset meta_done=0 each session via pmiStart inherit, causing all help text to be re-written on every append. Fix: check __pmLogLookupText() before writing; skip if content is unchanged. Signed-off-by: Nathan Scott <nathans@redhat.com>
natoscott
force-pushed
the
pmi-append-mode
branch
from
42d815a to
4fc060f
Compare
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
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.
Provides a mechanism whereby libpcp_import can append to an existing PCP archive.
Adds pre-existing PMI interfaces to the pcp/import.h header that were missing. Similar to recent work in MMV, a pmAtomValue interface is also added so tools that have values in binary form already do not pay the cost of string conversions and parsing for every value on every sample.
Additional commits improve the PMI documentation (man page synopsis sections for Python), provide python typing information to the PMI library, and cleanup a QA makefile.