Changes before the presentation at HL2025 #3
Conversation
Alex2ndr
commented
Oct 30, 2025
Alex2ndr
commented
- Add README.md with quick start guide
- Add demo env configuration files and instructions
- Added a conscious error about the absence of JQ in the tools
- Fixed dependencies: Added PyYAML as a required dependency
- Fixed a bug in accessing the timeout parameter
- Fixed typo in evpn-connector config sample
* Add README.md with quick start guide * Add demo env configuration files and instructions * Added a conscious error about the absence of JQ in the tools * Fixed dependencies: Added PyYAML as a required dependency * Fixed a bug in accessing the timeout parameter * Fixed typo in evpn-connector config sample
Alex2ndr
force-pushed
the
fixes_for_demo
branch
from
6ea442f to
fe9103c
Compare
|
/gemini review |
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
|
/gemini review |
![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 significantly improves the project's documentation by adding a comprehensive README and a guide for setting up a demo environment. It also includes several fixes, such as adding a missing dependency, checking for the jq tool, and resolving a bug related to a timeout parameter. My review includes suggestions to improve the clarity and correctness of the new documentation, address a security concern with a hardcoded Sentry DSN in the demo configuration, and enhance the maintainability of the timeout bug fix.
|
|
||
| [sentry] | ||
| enabled = False | ||
| dsn = "http://01b92236b6514577812280415d631354@192.168.220.220:9000/2" |
There was a problem hiding this comment.
The Sentry DSN is hardcoded with what appears to be a real or example credential. It is a security risk to commit secrets to version control, even in demo files. Please replace it with a placeholder and add a comment instructing the user to replace it with their actual DSN.
dsn = "http://<key>@<sentry_host>:<port>/<project_id>"
|
|
||
| [sentry] | ||
| enabled = False | ||
| dsn = "http://01b92236b6514577812280415d631354@192.168.220.220:9000/2" |
There was a problem hiding this comment.
The Sentry DSN is hardcoded with what appears to be a real or example credential. It is a security risk to commit secrets to version control, even in demo files. Please replace it with a placeholder and add a comment instructing the user to replace it with their actual DSN.
dsn = "http://<key>@<sentry_host>:<port>/<project_id>"
|
|
||
| ### Prerequisites | ||
|
|
||
| * **Python 3.8 |
There was a problem hiding this comment.
The Python version requirement is listed as Python 3.8, which is incomplete and could be misinterpreted as only version 3.8 is supported. The badge at the top of the file says python-3.8+, which is more accurate. Please update this line to be consistent and clear.
| * **Python 3.8 | |
| * **Python 3.8+** |
| { | ||
| "cfg_type": "l2", // Config type for L2 connectivity use "l2" | ||
| "mac": "36:e7:a5:7e:0c:81", // MAC address of client | ||
| "ip": "10.0.0.1", // IP address of client | ||
| "vni": 10, // VXLAN segment identifier | ||
| "ofport": 1000, // OpenFlow port number in current OpenvSwitch switch | ||
| "type": "flat", // OpenvSwitch port type. May be "flat" and "vlan" | ||
| "tag": 0, // OpenvSwitch port segment identifier. Ignored on "flat" | ||
| "imp_rt": ["65000:10"], // List of imported BGP Route Targets | ||
| "exp_rt": ["65000:10"] // List of exported BGP Route Targets | ||
| } |
There was a problem hiding this comment.
The JSON example for L2 connectivity contains // style comments. This makes it invalid JSON, and it will be rejected by standard JSON parsers. Please remove the comments to ensure the example is valid and can be copied directly by users. You can add explanations about the fields in the text surrounding the code block.
| { | |
| "cfg_type": "l2", // Config type for L2 connectivity use "l2" | |
| "mac": "36:e7:a5:7e:0c:81", // MAC address of client | |
| "ip": "10.0.0.1", // IP address of client | |
| "vni": 10, // VXLAN segment identifier | |
| "ofport": 1000, // OpenFlow port number in current OpenvSwitch switch | |
| "type": "flat", // OpenvSwitch port type. May be "flat" and "vlan" | |
| "tag": 0, // OpenvSwitch port segment identifier. Ignored on "flat" | |
| "imp_rt": ["65000:10"], // List of imported BGP Route Targets | |
| "exp_rt": ["65000:10"] // List of exported BGP Route Targets | |
| } | |
| { | |
| "cfg_type": "l2", | |
| "mac": "36:e7:a5:7e:0c:81", | |
| "ip": "10.0.0.1", | |
| "vni": 10, | |
| "ofport": 1000, | |
| "type": "flat", | |
| "tag": 0, | |
| "imp_rt": ["65000:10"], | |
| "exp_rt": ["65000:10"] | |
| } |
| { | ||
| "cfg_type": "l3", // Config type for L3 connectivity use "l3" | ||
| "mac": "36:e7:a5:7e:0c:81", // MAC address of client | ||
| "routes": ["10.0.0.1/32"], // List of CIDR prefixes for this client | ||
| "vni": 10, // VXLAN segment identifier | ||
| "ofport": 1000, // OpenFlow port number in current OpenvSwitch switch | ||
| "type": "flat", // OpenvSwitch port type. May be "flat" and "vlan" | ||
| "tag": 0, // OpenvSwitch port segment identifier. Ignored on "flat" | ||
| "imp_rt": ["65000:10"], // List of imported BGP Route Targets | ||
| "exp_rt": ["65000:10"] // List of exported BGP Route Targets | ||
| } |
There was a problem hiding this comment.
The JSON example for L3 connectivity contains // style comments. This makes it invalid JSON, and it will be rejected by standard JSON parsers. Please remove the comments to ensure the example is valid and can be copied directly by users. You can add explanations about the fields in the text surrounding the code block.
| { | |
| "cfg_type": "l3", // Config type for L3 connectivity use "l3" | |
| "mac": "36:e7:a5:7e:0c:81", // MAC address of client | |
| "routes": ["10.0.0.1/32"], // List of CIDR prefixes for this client | |
| "vni": 10, // VXLAN segment identifier | |
| "ofport": 1000, // OpenFlow port number in current OpenvSwitch switch | |
| "type": "flat", // OpenvSwitch port type. May be "flat" and "vlan" | |
| "tag": 0, // OpenvSwitch port segment identifier. Ignored on "flat" | |
| "imp_rt": ["65000:10"], // List of imported BGP Route Targets | |
| "exp_rt": ["65000:10"] // List of exported BGP Route Targets | |
| } | |
| { | |
| "cfg_type": "l3", | |
| "mac": "36:e7:a5:7e:0c:81", | |
| "routes": ["10.0.0.1/32"], | |
| "vni": 10, | |
| "ofport": 1000, | |
| "type": "flat", | |
| "tag": 0, | |
| "imp_rt": ["65000:10"], | |
| "exp_rt": ["65000:10"] | |
| } |
| 2. **Run evpn-connector** | ||
| ```bash | ||
| source evpn/bin/activate | ||
| evpn-connector --config-file ~/evpn/config/evpn_connector.cfg --daemon-configs_dir "/var/lib/evpn_connector/client_configs/" |
There was a problem hiding this comment.
The example command to run the service is confusing. The path to the config file --config-file ~/evpn/config/evpn_connector.cfg is used, but the previous steps do not show how this file is created. Step 2 under "Configuration" instructs to copy the sample config to /etc/evpn_connector/evpn_connector.conf. The command should probably use that path for consistency and to avoid user confusion.
| evpn-connector --config-file ~/evpn/config/evpn_connector.cfg --daemon-configs_dir "/var/lib/evpn_connector/client_configs/" | |
| evpn-connector --config-file /etc/evpn_connector/evpn_connector.conf --daemon-configs_dir "/var/lib/evpn_connector/client_configs/" |
| 6. Install python3.8 for evpn-connector: | ||
| ```bash | ||
| pyenv install 3.8.20 | ||
| pyenv global system 3.8 |
There was a problem hiding this comment.
The command pyenv global system 3.8 is invalid. pyenv global accepts only one version name. To set the Python version installed in the previous step (3.8.20) as the global default for the current user, the command should be pyenv global 3.8.20.
| pyenv global system 3.8 | |
| pyenv global 3.8.20 |
| ``` | ||
| 11. Copy configs for all daemons from repo (for each compute host separately): | ||
| ```bash | ||
| cp -r evpn_connector/docs/source/demo/compute1/* ~/ |
There was a problem hiding this comment.
The path evpn_connector/docs/source/demo/compute1/* seems incorrect. Based on the file structure in this pull request, the demo files are located in docs/demo/, not docs/source/demo/. The source directory component should be removed for the command to work correctly.
| cp -r evpn_connector/docs/source/demo/compute1/* ~/ | |
| cp -r evpn_connector/docs/demo/compute1/* ~/ |
| """ | ||
| if timeout is None: | ||
| timeout = CONF.metrics.obsender_timeout | ||
| timeout = 2.0 |
There was a problem hiding this comment.
This change hardcodes the timeout value to 2.0. While this fixes the immediate bug of accessing a non-existent configuration option, it removes the ability for operators to configure this value. A more maintainable approach would be to add a proper configuration option, for example obsender_timeout in the [victoria_metrics] section, register it in evpn_connector/common/conf_opts.py, and then use it here as CONF.victoria_metrics.obsender_timeout. This would make the timeout configurable again without hardcoding values.
