Replace TTL-cached Koji sessions with a private singleton and in-process Kerberos - #55
Conversation
…ess Kerberos. get_buildsys() and its cachetools TTL are gone. call_koji() now owns a private _bsys, renews the TGT via python-gssapi only when remaining lifetime is under 55 minutes (and only when --krb5-keytab-file is set), recreates the Koji session as part of that renew unit, and runs acquire/login/RPC work in worker threads. Without a keytab, an existing TGT from $KRB5CCNAME or the system default ccache is used. GSSAPI "Already logged in" is treated as success. Call sites use method names or helpers that receive the session as the first argument. Daemon/run.sh/local_test gain --krb5-keytab-file and --krb5-keytab-principal; the background kinit and koji hello loops are removed. Add connection unit tests and document the new auth path. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughKerberos authentication is moved into the process with optional keytab-based TGT acquisition and renewal. Koji requests now share centralized session handling, while daemon startup, container scripts, documentation, dependencies, and connection tests are updated accordingly. ChangesKerberos and Koji integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Launcher
participant Daemon
participant Kerberos
participant Koji
Launcher->>Daemon: pass keytab and principal options
Daemon->>Kerberos: resolve principal and configure keytab
Daemon->>Koji: issue call_koji request
Koji->>Kerberos: ensure valid TGT and GSSAPI login
Kerberos->>Koji: dispatch authenticated method
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
elnbuildsync/kojihelpers/connection.py (2)
179-193: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider closing the previous
ClientSessionwhen replacing it.Each renewal replaces
_bsyswithout logging out or closing the old session's underlyingrequests.Session, so connections accumulate over the process lifetime. Closing the superseded session (after the new one is successfully created, and only once it's no longer referenced by an in-flight call) would keep socket usage bounded.🤖 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 `@elnbuildsync/kojihelpers/connection.py` around lines 179 - 193, Update _recreate_bsys_sync to retain the previous _bsys, create and assign the replacement ClientSession successfully, then close the superseded session’s underlying requests session once it is no longer in use. Preserve the existing failure behavior by leaving the previous session intact if initialization fails, and avoid closing the newly created session.
223-257: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-RPC ccache read runs on the reactor thread.
_ensure_tgtis awaited by everycall_koji, and_tgt_lifetime_seconds()performs a synchronous credential-store read inline. With aKCM:/KEYRING:cache that's an IPC round-trip on the event loop for each RPC, and batching/multicall paths issue many. Caching the computed expiry (e.g. storemonotonic()deadline after each successful read/renewal and only re-read when it's near) would remove that from the hot path.Note the concurrent-renewal hazard here shares the missing-lock root cause flagged on Lines 196-220.
🤖 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 `@elnbuildsync/kojihelpers/connection.py` around lines 223 - 257, Update _ensure_tgt and the TGT state managed alongside _tgt_lifetime_seconds to cache the computed monotonic expiry after successful credential reads or renewals, returning from the cache while the ticket remains safely above TGT_RENEW_THRESHOLD_SECONDS and re-reading only when near expiry. Ensure cache updates and refresh decisions are protected by the existing concurrent-renewal synchronization so parallel call_koji requests do not perform duplicate reads or renewals.tests/test_koji_connection.py (3)
254-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
__wrapped__.__wrapped__unwrapping couples tests to the decorator stack.Both tests reach the body by peeling exactly two
__wrapped__levels, so any change to the retry decorators oncall_kojisilently breaks these (including the single-decorator consolidation suggested onelnbuildsync/kojihelpers/connection.pyLines 330-351). Extracting the body into a named coroutine thatcall_kojidecorates, or using tenacity'sretry_with(stop=stop_after_attempt(1)), would decouple them.Otherwise the dispatch coverage here is good — leaving
_invoke_koji_syncunpatched exercises the real string-vs-callable branch.🤖 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 `@tests/test_koji_connection.py` around lines 254 - 298, Decouple test_call_koji_string_method and test_call_koji_callable_receives_bsys from the decorator stack by invoking a stable named coroutine containing call_koji’s dispatch body, or by using tenacity’s retry_with with a single-attempt policy. Remove the hard-coded __wrapped__.__wrapped__ access while preserving the existing real _invoke_koji_sync string-versus-callable coverage and assertions.
60-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the other
_tgt_lifetime_secondsbranches.Only the
KRB5CCNAME-set path is exercised. The subtler behaviors are untested:lifetime is NonereturningTGT_RENEW_THRESHOLD_SECONDS(i.e. "never renew"), an exception returning0, and the no-KRB5CCNAMEcall being made without astorekwarg. Those three drive the whole renewal decision in_ensure_tgt.🤖 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 `@tests/test_koji_connection.py` around lines 60 - 80, Expand TestTgtLifetime with coverage for the remaining _tgt_lifetime_seconds branches: return TGT_RENEW_THRESHOLD_SECONDS when credentials.lifetime is None, return 0 when credential acquisition raises, and omit the store keyword when KRB5CCNAME is unset. Assert each result and verify the Credentials call arguments, while preserving the existing configured-principal behavior.
228-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese predicate assertions don't cover composed retry behavior.
Both predicates individually reject auth errors, but the two stacked
@retrydecorators oncall_kojimean the effective retry decision is their composition — see the finding onelnbuildsync/kojihelpers/connection.pyLines 330-351, where a non-retryable 4xx is still retried by the other loop. A test asserting thatcall_kojiitself does not retry onKerberosAuthErrorand on a 403RequestExceptionwould catch that.🤖 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 `@tests/test_koji_connection.py` around lines 228 - 230, Add a composed-behavior test for call_koji that invokes it with KerberosAuthError and a 403 RequestException, asserting each is propagated without retry and the underlying request is attempted only once. Keep the existing predicate assertions, but exercise the stacked retry decorators through call_koji rather than testing only _retry_transient_koji_exception and _retry_koji_request_exception.
🤖 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 `@elnbuildsync/kojihelpers/connection.py`:
- Around line 330-351: Collapse the stacked retry decorators on call_koji in
elnbuildsync/kojihelpers/connection.py lines 330-351 into one decorator with a
single retry predicate and a genuine 60-second bound. In
_retry_transient_koji_exception at lines 290-322, replace the deny-list with an
allow-list of genuinely transient exception types and route RequestException
through the existing status-code logic, preserving fail-fast behavior for other
4xx responses and koji.GenericError, AttributeError, and TypeError.
- Around line 196-220: Update _renew_tgt_and_bsys_once to acquire _auth_lock
around the entire TGT acquisition, _bsys recreation, and rollback sequence. Keep
the existing exception handling and restoration of old_bsys inside that lock,
ensuring renewal cannot interleave with _invoke_koji_sync or concurrent
renewals.
- Around line 146-176: Update _store_creds to always persist acquired
credentials using the default ccache when KRB5CCNAME is unset, rather than
skipping storage when _ccache_store() returns no explicit store. Stop swallowing
store_cred_into() failures: convert them to KerberosAuthError and propagate the
failure to _acquire_tgt_sync(), while preserving the _krb_creds assignment only
after successful persistence.
In `@run.sh`:
- Line 80: Replace stale “keytab kinit” terminology with “keytab-based TGT
acquisition” wording in the option descriptions at run.sh lines 80-80 and
tests/local_test_daemon.sh lines 96-96, and update the corresponding README.md
lines 369-371 phrasing consistently.
In `@tests/test_koji_connection.py`:
- Around line 1-30: Normalize the SPDX header on the `SPDX-License-Identifier`
line by replacing the literal tab after the colon with standard spacing before
`GPL-3.0-or-later`, without changing the license value.
---
Nitpick comments:
In `@elnbuildsync/kojihelpers/connection.py`:
- Around line 179-193: Update _recreate_bsys_sync to retain the previous _bsys,
create and assign the replacement ClientSession successfully, then close the
superseded session’s underlying requests session once it is no longer in use.
Preserve the existing failure behavior by leaving the previous session intact if
initialization fails, and avoid closing the newly created session.
- Around line 223-257: Update _ensure_tgt and the TGT state managed alongside
_tgt_lifetime_seconds to cache the computed monotonic expiry after successful
credential reads or renewals, returning from the cache while the ticket remains
safely above TGT_RENEW_THRESHOLD_SECONDS and re-reading only when near expiry.
Ensure cache updates and refresh decisions are protected by the existing
concurrent-renewal synchronization so parallel call_koji requests do not perform
duplicate reads or renewals.
In `@tests/test_koji_connection.py`:
- Around line 254-298: Decouple test_call_koji_string_method and
test_call_koji_callable_receives_bsys from the decorator stack by invoking a
stable named coroutine containing call_koji’s dispatch body, or by using
tenacity’s retry_with with a single-attempt policy. Remove the hard-coded
__wrapped__.__wrapped__ access while preserving the existing real
_invoke_koji_sync string-versus-callable coverage and assertions.
- Around line 60-80: Expand TestTgtLifetime with coverage for the remaining
_tgt_lifetime_seconds branches: return TGT_RENEW_THRESHOLD_SECONDS when
credentials.lifetime is None, return 0 when credential acquisition raises, and
omit the store keyword when KRB5CCNAME is unset. Assert each result and verify
the Credentials call arguments, while preserving the existing
configured-principal behavior.
- Around line 228-230: Add a composed-behavior test for call_koji that invokes
it with KerberosAuthError and a 403 RequestException, asserting each is
propagated without retry and the underlying request is attempted only once. Keep
the existing predicate assertions, but exercise the stacked retry decorators
through call_koji rather than testing only _retry_transient_koji_exception and
_retry_koji_request_exception.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df63e984-c974-4f77-93b7-d393a169021d
📒 Files selected for processing (13)
README.mdelnbuildsync/batching.pyelnbuildsync/cleanup.pyelnbuildsync/daemon.pyelnbuildsync/kojihelpers/builds.pyelnbuildsync/kojihelpers/connection.pyelnbuildsync/kojihelpers/errors.pyelnbuildsync/kojihelpers/tags.pyelnbuildsync/status.pyrequirements.txtrun.shtests/local_test_daemon.shtests/test_koji_connection.py
1 participant
This addresses the issues we've seen where failure to secure a TGT results in a busy-loop that eventually results in OpenShift deeming the pod frozen (correctly) and kills it. I still don't know why the automatic TGT renewal loop in bash wasn't working, but this patch moves the TGT acquisition into the EBS itself, so we can log it properly. It is also meant to guarantee that the acquisition can never block the mainloop (and thus affect the heartbeat requests). If the problem with TGT acquisition continues, at least we'll have a log of what is actually failing.
AI-assisted description
get_buildsys() and its cachetools TTL are gone. call_koji() now owns a private _bsys, renews the TGT via python-gssapi when remaining lifetime is under 55 minutes (and only when --krb5-keytab-file is set), recreates the Koji session as part of that renew unit, and runs acquire/login/RPC work in worker threads. Without a keytab, an existing TGT from $KRB5CCNAME or the system default ccache is used. GSSAPI "Already logged in" is treated as success.
Call sites use method names or helpers that receive the session as the first argument. Daemon/run.sh/local_test gain --krb5-keytab-file and --krb5-keytab-principal; the background kinit and koji hello loops are removed. Add connection unit tests and document the new auth path.
Assisted-by: Cursor 3.12.29
Summary by CodeRabbit
New Features
Documentation
Bug Fixes