-
Notifications
You must be signed in to change notification settings - Fork 260
Fix CNS and CNI ETW log interference issue #3692
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix CNS and CNI ETW log interference issue #3692
Conversation
Signed-off-by: Yerlan Baiturinov <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR resolves ETW session conflicts by giving CNS and CNI separate Geneva profile names for logging.
- Change default ETW provider in
ConfigtoACN-Monitoring-CNS - Update CNS logging core to use
ACN-Monitoring-CNS - Update CNI logging core to use
ACN-Monitoring-CNI
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cns/logger/v2/config_windows.go | Default ETW.ProviderName updated to ACN-Monitoring-CNS |
| cns/logger/cnslogger_windows.go | zapetw.New call now uses ACN-Monitoring-CNS |
| cni/log/logger_windows.go | zapetw.New call now uses ACN-Monitoring-CNI |
Comments suppressed due to low confidence (3)
cns/logger/cnslogger_windows.go:22
- There are no unit tests verifying the ETW core uses the new provider name; consider adding tests to assert the correct Geneva profile is set for CNS logging.
etwcore, _, err := zapetw.New("ACN-Monitoring-CNS", etwCNSEventName, encoder, loggingLevel)
cni/log/logger_windows.go:29
- Similar to CNS, CNI logging should have a test to confirm it picks up the
ACN-Monitoring-CNIprovider name; adding a unit test would prevent regressions.
etwcore, _, err := zapetw.New("ACN-Monitoring-CNI", etwCNIEventName, jsonEncoder, loggingLevel)
cns/logger/v2/config_windows.go:22
- The change to default ETW provider names isn't reflected in comments or external docs; update any related documentation to note the new defaults for CNS and CNI.
func (c *Config) normalize() {
Signed-off-by: Yerlan Baiturinov <[email protected]>
|
I'm not sure that this will necessarily solve the root issue. There can be multiple events published by the same provider. I think the deeper issue is in the current naming convention for event names. While event providers can be hyphenated ("-"), the event name cannot (this isn't listed explicitly in any documentation but is a reinforcement mechanism that Geneva does when parsing the config). This isn't an issue with DNC because having a default event of a different name can find an event name that's hyphenated. For whatever reason, this isn't the case when there are multiple events names from the same provider. With this in mind, I think a more appropriate approach would be to try to change the event names (removing the hyphen in "Azure-CNI") before changing the event provider name. |
Signed-off-by: Yerlan Baiturinov <[email protected]>
|
Let's try just changing the event name at first. If that's sufficient, there's no need to change the provider name. |
Signed-off-by: Yerlan Baiturinov <[email protected]>
Resolved |
|
/azp run Azure Container Networking PR |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| maxLogFileSizeInMb = 5 | ||
| maxLogFileCount = 8 | ||
| etwCNIEventName = "Azure-CNI" | ||
| etwCNIEventName = "AzureCNI" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious to know what is this change for?
* fix(log): Fix CNS and CNI ETW log interference issue Signed-off-by: Yerlan Baiturinov <[email protected]> * fix: Docker containers version updated Signed-off-by: Yerlan Baiturinov <[email protected]> * fix: ETW Event Name changed for CNI Signed-off-by: Yerlan Baiturinov <[email protected]> * fix: Revert changing Profilename in the ETW loggers Signed-off-by: Yerlan Baiturinov <[email protected]> --------- Signed-off-by: Yerlan Baiturinov <[email protected]>
Reason for Change:
This PR addresses a logging conflict between CNS and CNI when both components attempt to emit ETW logs using the same Geneva profile name. Previously, both services were configured to use ACN-Monitoring, which caused one of the services (typically CNI) to fail to emit logs due to ETW session conflicts.
Changes
Introduced separate Geneva profile names:
Issue Fixed:
Fixes #3691
Requirements:
Notes: