events: migrate cronfederatedhpa to events.k8s.io recorder#7465
events: migrate cronfederatedhpa to events.k8s.io recorder#7465Tej-Katika wants to merge 4 commits into
Conversation
Switch the cronfederatedhpa controller, its handler, and its scaling job from the deprecated record.EventRecorder to the events.k8s.io/v1 EventRecorder via Manager.GetEventRecorder, and establish the parallel EventAction* / EventReason* constant convention in pkg/events for the rest of the migration to follow. Reason strings are kept byte-for-byte identical so consumers watching for "StartRuleFailed", "UpdateCronFederatedHPAFailed", "ScaleFailed", or "UpdateStatusFailed" continue to match. Action values are new and operation-scoped, matching upstream Kubernetes where action describes what the controller did to the regarding object. The ScaleFailed event now passes a synthetic ObjectReference built from cronFHPA.Spec.ScaleTargetRef as the related object, so consumers can link the failure to the workload that was being scaled. The four remaining emissions naturally have no secondary object and pass nil. A behavioral test in cronfederatedhpa_events_test.go captures every field passed to Eventf, including action and related which the upstream events.FakeRecorder drops from its channel, and guards the reason strings against accidental drift. No RBAC changes are included; karmada-controller-manager has no explicit ClusterRole granting events permission against karmada-apiserver in the deploy artifacts. RBAC will be revisited in the cleanup PR after all controllers are migrated. Signed-off-by: Tejashwar Reddy Katika <tejashwar1029@gmail.com>
karmada-bot
added
kind/cleanup
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
karmada-bot
requested review from
XiShanYongYe-Chang,
chaunceyjiang and
whitewindmills
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request serves as a proof-of-concept for migrating controllers from the deprecated core/v1 EventRecorder to the events.k8s.io/v1 API. By focusing on the cronfederatedhpa controller, the changes establish a robust, reusable pattern for event emission that includes per-operation action constants and improved context for event consumers. The design ensures backward compatibility for existing event reason strings while providing a scalable framework for future controller migrations across the codebase. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
karmada-bot
added
the
size/L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrated the CronFederatedHPA controller to the Kubernetes events.k8s.io/v1 API, replacing the deprecated EventRecorder and introducing structured event reasons and actions, supported by new unit tests. Feedback identified the unsafe use of err.Error() as a format string in Eventf calls, recommending the use of a literal "%v" instead to prevent potential formatting errors. Additionally, the reviewer pointed out style guide violations concerning the missing compile-time interface compliance check for the controller and the multi-line signature of the NewCronFederatedHPAJob function.
| if !helper.IsCronFederatedHPARuleSuspend(rule) { | ||
| if err := c.CronHandler.CreateCronJobForExecutor(cronFHPA, rule); err != nil { | ||
| c.EventRecorder.Event(cronFHPA, corev1.EventTypeWarning, "StartRuleFailed", err.Error()) | ||
| c.EventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonStartCronFederatedHPARuleFailed, events.EventActionStartCronFederatedHPARule, err.Error()) |
There was a problem hiding this comment.
Using err.Error() directly as the format string in Eventf is unsafe because the error message might contain format specifiers (like %) which would cause fmt.Sprintf to fail or produce corrupted output. It is safer to use a literal format string like "%v" and pass the error as an argument.
| c.EventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonStartCronFederatedHPARuleFailed, events.EventActionStartCronFederatedHPARule, err.Error()) | |
| c.EventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonStartCronFederatedHPARuleFailed, events.EventActionStartCronFederatedHPARule, "%v", err) |
|
|
||
| if err := c.updateRuleHistory(ctx, cronFHPA, rule); err != nil { | ||
| c.EventRecorder.Event(cronFHPA, corev1.EventTypeWarning, "UpdateCronFederatedHPAFailed", err.Error()) | ||
| c.EventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonUpdateCronFederatedHPAFailed, events.EventActionUpdateCronFederatedHPA, err.Error()) |
There was a problem hiding this comment.
Using err.Error() directly as the format string in Eventf is unsafe. Please use a literal format string and pass the error as an argument to avoid issues with unexpected format specifiers in the error message.
| c.EventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonUpdateCronFederatedHPAFailed, events.EventActionUpdateCronFederatedHPA, err.Error()) | |
| c.EventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonUpdateCronFederatedHPAFailed, events.EventActionUpdateCronFederatedHPA, "%v", err) |
| } | ||
| if err := c.Client.Status().Update(ctx, cronFHPA); err != nil { | ||
| c.EventRecorder.Event(cronFHPA, corev1.EventTypeWarning, "UpdateCronFederatedHPAFailed", err.Error()) | ||
| c.EventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonUpdateCronFederatedHPAFailed, events.EventActionUpdateCronFederatedHPA, err.Error()) |
There was a problem hiding this comment.
Using err.Error() directly as the format string in Eventf is unsafe. Please use a literal format string and pass the error as an argument.
| c.EventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonUpdateCronFederatedHPAFailed, events.EventActionUpdateCronFederatedHPA, err.Error()) | |
| c.EventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonUpdateCronFederatedHPAFailed, events.EventActionUpdateCronFederatedHPA, "%v", err) |
| } | ||
| if err != nil { | ||
| c.eventRecorder.Event(cronFHPA, corev1.EventTypeWarning, "UpdateStatusFailed", err.Error()) | ||
| c.eventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonUpdateCronFederatedHPAStatusFailed, events.EventActionUpdateCronFederatedHPAStatus, err.Error()) |
There was a problem hiding this comment.
Using err.Error() directly as the format string in Eventf is unsafe. Please use a literal format string like "%v" and pass the error as an argument to ensure robust event message generation.
| c.eventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonUpdateCronFederatedHPAStatusFailed, events.EventActionUpdateCronFederatedHPAStatus, err.Error()) | |
| c.eventRecorder.Eventf(cronFHPA, nil, corev1.EventTypeWarning, events.EventReasonUpdateCronFederatedHPAStatusFailed, events.EventActionUpdateCronFederatedHPAStatus, "%v", err) |
| // CronFHPAController is used to operate CronFederatedHPA. | ||
| type CronFHPAController struct { |
There was a problem hiding this comment.
According to the repository style guide, any struct that explicitly implements an interface must include a compile-time interface compliance check.
// Check if our CronFHPAController implements necessary interface
var _ controllerruntime.Reconciler = &CronFHPAController{}
// CronFHPAController is used to operate CronFederatedHPA.References
- Any struct that explicitly implements an interface must include a compile-time interface compliance check using the pattern: var _ InterfaceName = &StructName{}. (link)
| func NewCronFederatedHPAJob(client client.Client, eventRecorder clientgoevents.EventRecorder, scheduler *gocron.Scheduler, | ||
| cronFHPA *autoscalingv1alpha1.CronFederatedHPA, rule autoscalingv1alpha1.CronFederatedHPARule) *ScalingJob { |
There was a problem hiding this comment.
The repository style guide specifies that function signatures should preferably be written on a single line, including the parameter list and return types.
func NewCronFederatedHPAJob(client client.Client, eventRecorder clientgoevents.EventRecorder, scheduler *gocron.Scheduler, cronFHPA *autoscalingv1alpha1.CronFederatedHPA, rule autoscalingv1alpha1.CronFederatedHPARule) *ScalingJob {References
- Function signatures should preferably be written on a single line, including the parameter list and return types. (link)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7465 +/- ##
==========================================
+ Coverage 41.90% 42.01% +0.10%
==========================================
Files 879 879
Lines 54326 54444 +118
==========================================
+ Hits 22766 22872 +106
- Misses 29833 29849 +16
+ Partials 1727 1723 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Replace err.Error() passed as the 'note' format string with the safe
"%v", err pattern at the four Eventf call sites that emit a bare error
message. The new events.k8s.io/v1 Eventf signature treats 'note' as a
printf format string, unlike the deprecated record.EventRecorder.Event
where the message was used verbatim. An error containing format verbs
(e.g. %v from a wrapped error) or a raw % (e.g. URL-encoded path in an
HTTP error) would otherwise be reinterpreted by fmt.Sprintf and produce
garbled output. A new TestEventfFormatStringSafety regression test pins
this property using a hostile error message.
Also:
- Add the compile-time interface compliance check
var _ reconcile.Reconciler = &CronFHPAController{} per the repository
style guide.
- Collapse the NewCronFederatedHPAJob signature to a single line per
the repository style guide.
Signed-off-by: Tejashwar Reddy Katika <tejashwar1029@gmail.com>
- reorder imports: clientgoevents before klog/v2
- use any instead of interface{} in capturingRecorder.Eventf
Signed-off-by: Tejashwar Reddy Katika <tejashwar1029@gmail.com>
- 34 action constants covering every current EventReason* site - consolidated into one block at the end of events.go - the 4 cronfederatedhpa actions added inline by 862e028 move here Signed-off-by: Tejashwar Reddy Katika <tejashwar1029@gmail.com>
|
Action table per the design discussed in #7251. The constants are in commit 52f543764. Action namespace (34 constants)Paired actions (cover Failed/Succeed reason pairs)
Singleton actions (reasons with no paired success/failure variant)
CronFederatedHPA (this PoC slice)
Naming convention
Out of scope, flagged for follow-up13 literal-string reasons are currently emitted without
Two pre-existing reason-name vs reason-value mismatches in
These emit identical wire strings across different operations. Worth a dedicated cleanup PR; not safe to change inside the events-API migration since the reason string is the externally-visible surface. |
|
cc @XiShanYongYe-Chang Please take a look. Thanks! |
|
Thanks for your hard work @Tej-Katika |
|
/assign |
|
Thanks @Tej-Katika Your analysis is very detailed, and I agree with your approach. Perhaps we could create a separate issue for this analysis, and track it to 7521, and then I will further invite other interested people to review it. What do you think? |
|
Thanks for taking a look @XiShanYongYe-Chang — happy the namespace shape works for you. Splitting the analysis out into its own issue under #7251 makes sense to me. The action table will be referenced by every per-controller follow-up, so it's worth having a dedicated place rather than being buried in a PR comment. I'll open the issue today with:
One question on sequencing: should I keep #7465 in draft until the design issue gets reviewer feedback, or move it out of draft now so the implementation review can run in parallel with the design discussion? Either works for me — happy to follow whichever you prefer. |
|
The current PR should be a subtask of the split issue. We can start implementing it step by step once others have no objections. |
|
Sounds good. I'll keep #7465 in draft as a subtask of the new design issue and move it forward once the design lands without objections. Opening the issue today and will ping you when it's up. |
4 participants
Part of #7527.
What this PR does
Proof-of-concept for the migration tracked in #7251. Moves one
controller — cronfederatedhpa — end-to-end from the deprecated
record.EventRecorder(core/v1 Events) to the newevents.EventRecorder(events.k8s.io/v1), and introduces theEventAction*constant pattern inpkg/eventsthat the remainingcontrollers will follow.
The intent is to validate the design before opening per-controller
follow-ups. If reviewers approve the pattern here, subsequent PRs
become mechanical applications of it.
Why cronfederatedhpa
Smallest vertically self-contained slice in the codebase: 1
acquisition site in cmd/controller-manager, 1 in the controller
itself (via
NewCronHandler), 5 emission sites across the controllerand the scaling job, no shared recorders with other controllers.
Design choices worth a look
Per-operation actions, not generic verbs. Four
EventAction*constants (
StartCronFederatedHPARule,UpdateCronFederatedHPA,ScaleCronFederatedHPA,UpdateCronFederatedHPAStatus) rather thana small set of CRUD-style verbs. Matches upstream Kubernetes — see
staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/whereactions are operation-specific (
SNICertificateReload, etc.). Goingcoarser would shrink the constant surface but lose information that
reportingControllerdoesn't already carry.Reason strings preserved byte-for-byte. "StartRuleFailed",
"UpdateCronFederatedHPAFailed", "ScaleFailed", "UpdateStatusFailed"
remain exactly as before. Constants are introduced for them so
future PRs can reference them, but no wire-format change for any
external consumer keyed on these strings. A new test
(
TestEventConstantsPreserveWireFormat) guards against accidentaldrift.
relatedis non-nil where it's meaningful, nil otherwise. TheScaleFailedevent now passes a synthetic*corev1.ObjectReferencebuilt from
cronFHPA.Spec.ScaleTargetRefso a consumer can link theevent to the workload that was being scaled. The other four
emissions (StartRuleFailed, UpdateCronFederatedHPAFailed twice,
UpdateStatusFailed) are about the CronFederatedHPA itself with no
meaningful secondary object, and consistently pass nil — matching
@XiShanYongYe-Chang's guidance in the issue thread.
No RBAC changes. No explicit ClusterRole in the deploy artifacts
grants karmada-controller-manager
eventspermission againstkarmada-apiserver — it authenticates via TLS and presumably has
broad access. Adding speculative
events.k8s.ioRBAC here would bechurn for an unverified problem. If events fail to emit at runtime
in a real deployment, RBAC files will be updated in the final
cleanup PR after all controllers have been migrated. Happy to add
an RBAC update here if reviewers prefer the conservative path.
Behavioral test coverage
cronfederatedhpa_events_test.goadds three focused tests:TestTargetReferenceFor— verifies therelatedobject is builtcorrectly from
ScaleTargetRef.TestEventRecorderEmitsAllFields— captures every field passedto
Eventf(includingactionandrelated, which the upstreamevents.FakeRecorderdrops from its channel) using a customrecorder, and asserts the full contract end-to-end.
TestEventConstantsPreserveWireFormat— wire-format guard forthe four reason strings.
Scope
The other 37
GetEventRecorderForsites and their//nolint:staticcheckmarkers are intentionally untouched in thisPR.
Open questions for reviewers
verbs — does the per-operation choice match what you have in
mind for the wider migration?
events.k8s.iopermission in this PR even though I cannot pointto a specific ClusterRole that needs it?
ScaleSucceeded) be added infollow-up PRs? Not added here because the existing scale paths
return nil even on no-op (target replicas already match), which
would surface false-positive events. Distinguishing actual-change
from no-change is a separate refactor.
Verification
go build ./...go vet ./...go test ./pkg/controllers/cronfederatedhpa/...(3 new tests + all existing pass)on master unrelated to this change
(
pkg/util/validation/TestValidateCrdsTarBall,operator/pkg/tasks/init/TestRunUnpack,pkg/karmadactl/cmdinit/utils/TestListFiles)./kind cleanup