-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add remote syslog configuration #14513
Conversation
@@ -17,7 +17,10 @@ if [[ ($NUM_ASIC -gt 1) ]]; then | |||
else | |||
udp_server_ip=$(ip -j -4 addr list lo scope host | jq -r -M '.[0].addr_info[0].local') | |||
fi | |||
hostname=$(hostname) |
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.
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.
it is better to take from linux than DEVICE_METADATA/hostname.
example: DB can be empty if not configured with CLI. hostname can also be dynamically assigned using DHCP.
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.
If DEVICE_METADATA/hostname has value, it should be preferred.
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.
The hostname can be dynamically assigned by DHCP, in that case, DB value may contain the wrong hostname.
Plus DEVICE_METADATA
is a configuration and not a STATE, once data is placed to DEVICE_METADATA it will update the system hostname according to https://github.com/sonic-net/sonic-host-services/blob/bc08806b64002c506b8401eae5d9e1c760651e49/scripts/hostcfgd#L1568
So in my opinion it is better to take it from the hostname
utility as the most appropriate one.
c3e5cbc
to
d2a0298
Compare
@iavraham Can you please update configuration.md to reflect the new schema changes? |
0cb8f8f
to
062545d
Compare
done |
@@ -136,17 +136,6 @@ | |||
} | |||
} | |||
}, | |||
"SYSLOG_SERVER_INVALID_IPADDR_TEST" : { |
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.
Why is this test removed? Is it because syslog server can be a string and hence IP validation cannot be done?
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.
yes
{% set regex = conf.get('filter_regex') -%} | ||
|
||
{% set fmodifier = '!' if filter == 'exclude' else '' %} | ||
{% set device = 'mgmt' if vrf in ['mgmt', 'management'] else 'eth0' %} |
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.
This assumes that syslog server is reachable only through mgmt network or eth0. What if the server is reachable through the data network or OOB mgmt port?
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.
oh, yes. I missed the address
as well. I will fix it.
Good catch!
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.
Updated
f5ed003
to
b6d90a3
Compare
/azp run |
Commenter does not have sufficient privileges for PR 14513 in repo sonic-net/sonic-buildimage |
### Description of PR Summary: Fixes test fail in sonic-net/sonic-buildimage#14513 ### Approach #### What is the motivation for this PR? Align the test according to the new rsyslog server type. Originally only IPv4 and IPv6 addresses were supported. Now hostames are supported as well #### How did you do it? Update the generic updater test to follow new schema #### How did you verify/test it? Run it on t1 setup
@qiluo-msft all comments addressed. Please take a look and if everything looks good to you, please merge that PR |
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.
LGTM
* Add an ability to configure remote syslog servers * Add an initial configuration for remote syslog * Extend YANG module and add unit tests Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
@qiluo-msft updated according to the last comment. Could you please take a look? |
@qiluo-msft Can you please review and signoff? |
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
) cherry-pick: #14513 depends: sonic-net/sonic-utilities#2939 * Add an ability to configure remote syslog servers * Add an initial configuration for remote syslog * Extend YANG module and add unit tests #### Why I did it Adding the following functionality to rsyslog feature: * Configure remote syslog servers: protocol, filter, severity level * Update global syslog configuration: severity level, message format #### How I did it added parameters to syslog server and global configuration. #### How to verify it create syslog server using CLI/adding to Redis-DB verify server is added to file /etc/rsyslog.conf and server is functional. #### Description for the changelog extend rsyslog capabilities, added server and global configuration parameters. #### Link to config_db schema for YANG module changes [sonic-syslog.yang](https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-syslog.yang)
* Add an ability to configure remote syslog servers * Add an initial configuration for remote syslog * Extend YANG module and add unit tests #### Why I did it Adding the following functionality to rsyslog feature: - Configure remote syslog servers: protocol, filter, severity level - Update global syslog configuration: severity level, message format #### How I did it added parameters to syslog server and global configuration. #### How to verify it create syslog server using CLI/adding to Redis-DB verify server is added to file /etc/rsyslog.conf and server is functional. #### Description for the changelog extend rsyslog capabilities, added server and global configuration parameters. #### Link to config_db schema for YANG module changes https://github.com/iavraham/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-syslog.yang
…-net#8668) ### Description of PR Summary: Fixes test fail in sonic-net/sonic-buildimage#14513 ### Approach #### What is the motivation for this PR? Align the test according to the new rsyslog server type. Originally only IPv4 and IPv6 addresses were supported. Now hostames are supported as well #### How did you do it? Update the generic updater test to follow new schema #### How did you verify/test it? Run it on t1 setup
…-net#8668) ### Description of PR Summary: Fixes test fail in sonic-net/sonic-buildimage#14513 ### Approach #### What is the motivation for this PR? Align the test according to the new rsyslog server type. Originally only IPv4 and IPv6 addresses were supported. Now hostames are supported as well #### How did you do it? Update the generic updater test to follow new schema #### How did you verify/test it? Run it on t1 setup
Hard coded |
Why I did it
Adding the following functionality to rsyslog feature:
How I did it
added parameters to syslog server and global configuration.
How to verify it
create syslog server using CLI/adding to Redis-DB
verify server is added to file /etc/rsyslog.conf and server is functional.
Description for the changelog
extend rsyslog capabilities, added server and global configuration parameters.
Link to config_db schema for YANG module changes
https://github.com/iavraham/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-syslog.yang