license: implement vCPU audit background writer for self-hosted clusters#170859
Conversation
|
😎 Merged successfully - details. |
|
This change is |
sadaf-crl
force-pushed
the
CC-35136
branch
5 times, most recently
from
f01bd1e to
77a504e
Compare
|
Detected infrastructure failure (matched: self-hosted runner lost communication with the server). Automatically rerunning failed jobs. (run link) |
sadaf-crl
requested review from
a team,
bghal,
rahulcrl and
vishalv1994
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
AI Review: Potential Issue(s) DetectedAn inline comment has been added to the relevant line with details and a suggested fix. Summary: In If helpful: add |
github-actions
Bot
added
the
o-AI-Review-Potential-Issue-Detected
| // vcpuAuditData contains the data needed to write a vCPU audit record. | ||
| type vcpuAuditData struct { | ||
| // licenseID is the unique identifier for the license. | ||
| // Nil if no license is installed. |
There was a problem hiding this comment.
nit: This is set to no-license (and not Nil as mentioned in the comment)
There was a problem hiding this comment.
Removed the comment as it was not meaningful here
|
|
||
| for { | ||
| select { | ||
| case <-ticker.C: |
There was a problem hiding this comment.
IIUC, the first record will be written only after an hour after start - is that intended, since we would like to avoid gaps in consumption tracking?
There was a problem hiding this comment.
Good catch, it should write as soon as the node starts to avoid losing that hour's consumption. Updated it.
| // TODO(sadaf-crl): Replace log.Infof with actual SQL INSERT when | ||
| // system.vcpu_hours_audit table is available. | ||
| func (e *Enforcer) writeVCPUAuditRecord( | ||
| ctx context.Context, nodeID roachpb.NodeID, hourTimestamp time.Time, |
There was a problem hiding this comment.
Can we remove nodeID as a param here since Enforcer already has it?
| hourTimestamp = hourTimestamp.Truncate(time.Hour) | ||
|
|
||
| // TODO(sadaf-crl): Replace this log statement with SQL INSERT: | ||
| // INSERT INTO system.vcpu_hours_audit (node_id, license_id, hour_timestamp, num_vcpu) |
There was a problem hiding this comment.
This is not an immediate concern but when we implement the INSERT logic, we might need to guard against duplicate rows since (node_id, license_id, period_start) is the primary key and we round off against the hour. For example, we might have the license change logic and shutdown logic kicking off writeVCPUAuditRecord around the same time and trying to write the same value.
There was a problem hiding this comment.
Noted. Will handle this in upcoming PR where INSERT logic is implemented
| // maybeWriteOnLicenseRotation checks if the license ID changed and triggers | ||
| // an immediate vCPU audit write to prevent gaps in consumption tracking. | ||
| // This is called from RefreshForLicenseChange when the license is updated. | ||
| func (e *Enforcer) maybeWriteOnLicenseRotation(ctx context.Context, newLicenseID []byte) { |
There was a problem hiding this comment.
This function name is slightly misleading because it unconditionally mutates currentLicenseID, even if it doesn't always write the audit record. Can we rename it to updateLicenseIDAndMaybeWrite (preferable) or add a comment on the store line stating that caching happens on every call?
There was a problem hiding this comment.
Updated to updateLicenseIDAndMaybeWrite
|
|
||
| // Wait for at least 2 ticks and verify writes actually fired. | ||
| require.Eventually(t, func() bool { | ||
| return writeCount.Load() >= 2 |
There was a problem hiding this comment.
Should this be >= 3 now that we have the write on startup?
self-hosted clusters Add background ticker infrastructure for tracking vCPU consumption in self-hosted clusters. A background goroutine fires hourly to collect vCPU data from status.GetVCPUs() and prepare it for writing to system.vcpu_hours_audit (table creation in follow-up). The writer detects license rotation and triggers an immediate write to prevent gaps in consumption tracking. It handles graceful shutdown with a 5-second timeout for a final audit record write. The actual SQL INSERT logic is stubbed with log.Dev.Infof() until the system table is created in a follow-up commit. Epic: CC-35515 Release note: None
|
TFTR @rahulcrl @suj-krishnan |
|
/trunk merge |
4 participants
Add background ticker infrastructure for tracking vCPU consumption in self-hosted clusters.
A background goroutine fires hourly to collect vCPU data from status.GetVCPUs() and prepare it for writing to system.vcpu_hours_audit (table creation in follow-up).
The writer detects license rotation and triggers an immediate write to prevent gaps in consumption tracking. It handles graceful shutdown with a 5-second timeout for a final audit record write.
The actual SQL INSERT logic is stubbed with log.Dev.Infof() until the system table is created in a follow-up commit. (#170328)
Epic: CC-35515
Release note: None