fix(controller): add finish logs for all gc functions#6532
Conversation
All gc functions only had "start to gc ..." logs without corresponding completion logs, making it difficult to determine whether a gc cycle completed successfully or hung. Add "finish to gc ..." logs before each successful return to improve observability and debugging. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
dosubot
bot
added
the
size:S
There was a problem hiding this comment.
Pull request overview
This PR improves controller GC observability by adding “finish” log lines so operators can tell whether each GC function completed (vs. potentially hanging).
Changes:
- Add
finish to gc ...logs on successful completion paths across the GC functions inpkg/controller/gc.go. - Keep
markAndCleanLSPfinish log atklog.V(4)to match its existing start-log verbosity. - Add separate finish logging for the DHCP options portion of
gcLogicalSwitch.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| klog.Infof("finish to gc address set") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The new finish log won’t be emitted on the successful no-op path: this function can return nil early when len(asList) == 0 (line 810-812), after printing the start log. To ensure start/finish pairs (and to match the PR’s intent), add a finish log before that early return or use a deferred finish log that only runs on success.
| klog.Infof("finish to gc security group residual port groups") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The finish log is only reached when there are port groups to delete; on the successful no-op path (len(needToDelPgs) == 0) the function returns nil early (line 857-859) after logging the start message. If the goal is to detect hung GC cycles via start/finish pairs, add a finish log before that early return or use a deferred finish log that only executes on success.
Pull Request Test Coverage Report for Build 23628132626Details
💛 - Coveralls |
Address review feedback: gcAddressSet and gcSecurityGroup had early return paths (when nothing to gc) that were missing finish logs. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Summary
klog.V(4)level formarkAndCleanLSPto match its existing start log level.Test plan
make lintpasses🤖 Generated with Claude Code