Conversation
…ources.xml and bump chart.yaml version
There was a problem hiding this comment.
Pull Request Overview
This PR adds environment variable configuration support for OpenNMS database connection pool settings in the initialization script. Previously, most connection pool parameters were hardcoded; now they can be configured via environment variables.
Key Changes:
- Added environment variables for database connection pool configuration (idle timeout, login timeout, min pool, max pool, max size)
- Applied these configurable parameters to all three database connection configurations in opennms-datasources.xml
- Bumped chart version from 1.1.20 to 1.1.21
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| horizon/scripts/onms-core-init.sh | Added environment variable defaults and applied them to database connection pool configurations in opennms-datasources.xml |
| horizon/Chart.yaml | Incremented chart version to reflect the configuration changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
horizon/scripts/onms-core-init.sh
Outdated
| # Defaults | ||
| OPENNMS_DATABASE_CONNECTION_IDLETIMEOUT="${OPENNMS_DATABASE_CONNECTION_IDLETIMEOUT:-600}" | ||
| OPENNMS_DATABASE_CONNECTION_LOGINTIMEOUT="${OPENNMS_DATABASE_CONNECTION_LOGINTIMEOUT:-3}" | ||
| OPENNMS_DATABASE_CONNECTION_MINPOOL="${OPENNMS_DATABASE_CONNECTION_MAXSIZE:-25}" |
There was a problem hiding this comment.
The MINPOOL variable is incorrectly using MAXSIZE as its source. It should use OPENNMS_DATABASE_CONNECTION_MINPOOL instead: OPENNMS_DATABASE_CONNECTION_MINPOOL=\"${OPENNMS_DATABASE_CONNECTION_MINPOOL:-25}\"
| OPENNMS_DATABASE_CONNECTION_MINPOOL="${OPENNMS_DATABASE_CONNECTION_MAXSIZE:-25}" | |
| OPENNMS_DATABASE_CONNECTION_MINPOOL="${OPENNMS_DATABASE_CONNECTION_MINPOOL:-25}" |
…E_CONNECTION_MINPOOL
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
horizon/scripts/onms-core-init.sh
Outdated
| maxPool="10" | ||
| maxSize="50" /> | ||
| <connection-pool idleTimeout="${OPENNMS_DATABASE_CONNECTION_IDLETIMEOUT}" | ||
| minPool="${OPENNMS_DATABASE_CONNECTION_MINPOOL}" |
There was a problem hiding this comment.
The minPool values for template1 and postgres datasources are now set to 25 (from the default), but were previously hardcoded to 0. This changes the behavior of these connection pools and may cause resource consumption issues. Consider using a separate environment variable for these datasources or documenting this breaking change.
horizon/scripts/onms-core-init.sh
Outdated
| maxPool="10" | ||
| maxSize="50" /> | ||
| <connection-pool idleTimeout="${OPENNMS_DATABASE_CONNECTION_IDLETIMEOUT}" | ||
| minPool="${OPENNMS_DATABASE_CONNECTION_MINPOOL}" |
There was a problem hiding this comment.
The minPool values for template1 and postgres datasources are now set to 25 (from the default), but were previously hardcoded to 0. This changes the behavior of these connection pools and may cause resource consumption issues. Consider using a separate environment variable for these datasources or documenting this breaking change.
…om the onms-core-init script
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| EOF | ||
| if $USE_UPDATED_DATASOURCE; then | ||
| cat <<EOF >> ${CONFIG_DIR_OVERLAY}/opennms-datasources.xml | ||
| <jdbc-data-source name="opennms-admin" | ||
| database-name="template1" | ||
| class-name="org.postgresql.Driver" | ||
| url="jdbc:postgresql://${POSTGRES_HOST}:${POSTGRES_PORT}/template1?sslmode=${POSTGRES_SSL_MODE}&sslfactory=${POSTGRES_SSL_FACTORY}" | ||
| user-name="${POSTGRES_USER}" | ||
| password="${POSTGRES_PASSWORD}"> | ||
| <connection-pool idleTimeout="600" | ||
| minPool="0" | ||
| maxPool="10" | ||
| maxSize="50" /> | ||
| </jdbc-data-source> | ||
|
|
||
| <jdbc-data-source name="opennms-monitor" | ||
| database-name="postgres" | ||
| class-name="org.postgresql.Driver" | ||
| url="jdbc:postgresql://${POSTGRES_HOST}:${POSTGRES_PORT}/postgres?sslmode=${POSTGRES_SSL_MODE}&sslfactory=${POSTGRES_SSL_FACTORY}" | ||
| user-name="${POSTGRES_USER}" | ||
| password="${POSTGRES_PASSWORD}"> | ||
| <connection-pool idleTimeout="600" | ||
| minPool="0" | ||
| maxPool="10" | ||
| maxSize="50" /> | ||
| </jdbc-data-source> | ||
| </datasource-configuration> | ||
| EOF | ||
| else | ||
| cat <<EOF >> ${CONFIG_DIR_OVERLAY}/opennms-datasources.xml | ||
| <jdbc-data-source name="opennms-admin" | ||
| database-name="template1" | ||
| class-name="org.postgresql.Driver" | ||
| url="jdbc:postgresql://${POSTGRES_HOST}:${POSTGRES_PORT}/template1?sslmode=${POSTGRES_SSL_MODE}&sslfactory=${POSTGRES_SSL_FACTORY}" | ||
| user-name="${POSTGRES_USER}" | ||
| password="${POSTGRES_PASSWORD}"/> | ||
| </datasource-configuration> | ||
| EOF | ||
| fi | ||
|
|
||
| # Enable storeByGroup to improve performance |
There was a problem hiding this comment.
The removal of the datasource configuration logic leaves no clear indication of how opennms-datasources.xml should now be configured. Consider adding a comment explaining the new approach for configuring database connections, or verify that this configuration is now handled elsewhere (e.g., via environment variables, a ConfigMap, or a different initialization mechanism).
2 participants
…ources.xml