sonic/config_generator: align syslog and mgmt_port values with schema#2260
Open
sonic/config_generator: align syslog and mgmt_port values with schema#2260
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="osism/tasks/conductor/sonic/config_generator.py" line_range="2096-2100" />
<code_context>
proto = device.config_context.get("_segment_log_server_proto", "udp")
severity = device.config_context.get("_segment_log_server_severity", "info")
vrf = device.config_context.get("_segment_log_server_vrf", "mgmt")
+ proto = proto.lower()
+ severity = severity.lower()
+ severity = _SYSLOG_SEVERITY_ALIASES.get(severity, severity)
config["SYSLOG_SERVER"] = {}
for host in hosts:
</code_context>
<issue_to_address>
**suggestion:** Consider trimming whitespace when normalizing severity to avoid subtle misconfigurations.
Currently, leading/trailing whitespace in values from `config_context` (e.g. `' INFO '`) will prevent matching in `_SYSLOG_SEVERITY_ALIASES` and result in `' info '` being written. Using `severity = severity.strip().lower()` before the alias lookup would make this more tolerant of minor formatting issues in the context data.
```suggestion
severity = device.config_context.get("_segment_log_server_severity", "info")
vrf = device.config_context.get("_segment_log_server_vrf", "mgmt")
proto = proto.lower()
severity = severity.strip().lower()
severity = _SYSLOG_SEVERITY_ALIASES.get(severity, severity)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Three values produced by config generation did not match the SONiC schema
enforced by the new Pydantic validator:
- MGMT_PORT.eth0.autoneg in the base config_db.json was "true"; the
schema requires "on"|"off".
- _add_log_server_configuration uppercased the syslog protocol ("UDP"),
but the schema requires lowercase "tcp"|"udp".
- _add_log_server_configuration passed the severity through unchanged,
so common syslog aliases like "warning" or "critical" failed against
the SONiC-accepted set (none, debug, info, notice, warn, error, crit).
Lowercase the protocol, lowercase and alias-map the severity
(warning -> warn, informational -> info, err -> error, critical -> crit),
fix the autoneg value in the base config, and update the existing tests
to match.
AI-assisted: Claude Code
Signed-off-by: Christian Berendt <berendt@osism.tech>
berendt
added a commit
that referenced
this pull request
May 5, 2026
_add_bgp_configurations wrote "admin_status": "true" for every BGP_NEIGHBOR_AF row it generated: ipv4_unicast, ipv6_unicast and l2vpn_evpn keys for both interfaces and port channels, plus the VLAN peer ipv4_unicast row. The schema enforced by the new Pydantic validator (BgpNeighborAfListRow.admin_status) is Literal["up", "down"], so validation rejected every generated row with "Input should be 'up' or 'down'". Same fix pattern as #2260. Replace the literal "true" with "up" at all seven BGP call sites, and centralise the schema-allowed values as ADMIN_STATUS_UP / ADMIN_STATUS_DOWN constants in sonic/constants.py so future drift fails at import time rather than at validation. Switch the remaining admin_status producers (MGMT_INTERFACE, INTERFACE incl. breakout ports, VLAN, VLAN_INTERFACE, LOOPBACK, and the PORTCHANNEL row built in sonic/interface.py) to the same constants for consistency. AI-assisted: Claude Code Signed-off-by: Christian Berendt <berendt@osism.tech>
Member
Author
|
Depends on #2258 |
osfrickler
reviewed
May 5, 2026
| "informational": "info", | ||
| "warning": "warn", | ||
| "err": "error", | ||
| "critical": "crit", |
Member
There was a problem hiding this comment.
why did you omit the other three? at least debug sounds like it could be useful in certain cases
| proto = device.config_context.get("_segment_log_server_proto", "udp") | ||
| severity = device.config_context.get("_segment_log_server_severity", "info") | ||
| vrf = device.config_context.get("_segment_log_server_vrf", "mgmt") | ||
| proto = proto.strip().lower() |
Member
There was a problem hiding this comment.
this is uppercase in the generated config, I would prefer to stick to that
| "eth0": { | ||
| "admin_status": "up", | ||
| "autoneg": "true", | ||
| "autoneg": "on", |
Member
There was a problem hiding this comment.
again, I'd prefer to stick to how the config is generated and amend the scheme if needed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three values produced by config generation did not match the SONiC schema enforced by the new Pydantic validator:
Lowercase the protocol, lowercase and alias-map the severity (warning -> warn, informational -> info, err -> error, critical -> crit), fix the autoneg value in the base config, and update the existing tests to match.
AI-assisted: Claude Code