fix(monitoring): create duplicated pipelines #225
Conversation
WalkthroughThe pull request introduces multiple changes across several files, primarily enhancing the functionality of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
controllers/greptimedbcluster/deployers/monitoring.go(4 hunks)controllers/greptimedbstandalone/deployer.go(0 hunks)go.mod(2 hunks)
💤 Files with no reviewable changes (1)
- controllers/greptimedbstandalone/deployer.go
🔇 Additional comments (2)
go.mod (2)
16-16: LGTM!
The prometheus client dependency change from indirect to direct with the same version is appropriate for the monitoring-related changes.
9-9: Verify the MySQL driver version.
The addition of mysql driver v1.8.1 for database interactions needs verification for stability and security concerns.
✅ Verification successful
MySQL driver v1.8.1 is secure and up-to-date
The verification shows that:
- v1.8.1 is the latest available version of the mysql driver
- No security vulnerabilities were found in the GitHub Security Advisory Database
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and latest versions of the mysql driver
# Check for latest version on pkg.go.dev
curl -s "https://pkg.go.dev/github.com/go-sql-driver/mysql?tab=versions" | grep -o 'v[0-9]\+\.[0-9]\+\.[0-9]\+'
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/go-sql-driver/mysql") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1344
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/e2e/testdata/resources/cluster/enable-monitoring/cluster.yamlis excluded by!tests/e2e/testdata/**
📒 Files selected for processing (1)
tests/e2e/setup/create-cluster.sh(2 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/e2e/setup/diagnostic-cluster.sh (1)
29-32: LGTM! Consider enhancing error handling and output formatting.The function correctly retrieves GreptimeDB resources. Consider these improvements:
- Add error handling in case CRDs aren't installed
- Add output formatting for better readability
function dump_greptime_resources() { + echo "===> Dumping GreptimeDB Cluster resources" + if ! kubectl api-resources | grep -q "greptimedbclusters.greptime.io"; then + echo "Warning: GreptimeDB Cluster CRD not found" + return + fi kubectl get greptimedbclusters.greptime.io -A + echo "===> Dumping GreptimeDB Standalone resources" + if ! kubectl api-resources | grep -q "greptimedbstandalones.greptime.io"; then + echo "Warning: GreptimeDB Standalone CRD not found" + return + fi kubectl get greptimedbstandalones.greptime.io -A }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
tests/e2e/helper/helper.go(1 hunks)tests/e2e/setup/diagnostic-cluster.sh(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/helper/helper.go
🔇 Additional comments (1)
tests/e2e/setup/diagnostic-cluster.sh (1)
48-48: LGTM! Logical placement of the new function call.
The new function is appropriately placed in the diagnostic sequence, between basic information gathering and event collection.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
controllers/greptimedbcluster/deployers/monitoring.go (2)
98-111: Improve error message formattingThe error message uses single quotes which is inconsistent with Go conventions. Go typically uses double quotes for strings.
Apply this diff:
- klog.Errorf("failed to get pipeline for standalone, err: '%v'", err) + klog.Errorf("failed to get pipeline for standalone, err: %v", err) - klog.Errorf("failed to create pipeline for standalone, err: '%v'", err) + klog.Errorf("failed to create pipeline for standalone, err: %v", err)
221-227: Improve error handling for table existence checkThe current error check for table existence is fragile as it relies on string matching. Consider using a more robust approach.
Apply this diff:
- rows, err := db.QueryContext(ctx, "SELECT 1 FROM pipelines LIMIT 1") - if err != nil { - if strings.Contains(err.Error(), "TableNotFound") { - return "", nil - } - return "", err - } + var exists bool + err = db.QueryRowContext(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM information_schema.tables + WHERE table_schema = ? AND table_name = 'pipelines' + )`, "greptime_private").Scan(&exists) + if err != nil { + return "", fmt.Errorf("failed to check table existence: %v", err) + } + if !exists { + return "", nil + }🧰 Tools
🪛 GitHub Check: Build the project
[failure] 221-221:
unreachable code🪛 GitHub Check: Run unit tests
[failure] 221-221:
unreachable code
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
controllers/greptimedbcluster/deployers/monitoring.go(4 hunks)
🧰 Additional context used
🪛 GitHub Check: Build the project
controllers/greptimedbcluster/deployers/monitoring.go
[failure] 221-221:
unreachable code
🪛 GitHub Check: Run unit tests
controllers/greptimedbcluster/deployers/monitoring.go
[failure] 221-221:
unreachable code
🔇 Additional comments (2)
controllers/greptimedbcluster/deployers/monitoring.go (2)
20-20: LGTM: Required imports added for database interaction
The addition of database/sql and github.com/go-sql-driver/mysql imports is appropriate for the new database interaction functionality.
Also applies to: 30-30
221-230:
Fix resource management for database rows
The rows objects should be properly managed using defer to ensure they're always closed, even in error cases.
Apply this diff:
rows, err := db.QueryContext(ctx, "SELECT 1 FROM pipelines LIMIT 1")
if err != nil {
if strings.Contains(err.Error(), "TableNotFound") {
return "", nil
}
return "", err
}
+ defer rows.Close()
- if rows != nil {
- rows.Close()
- }
pipelineName := common.LogsPipelineName(cluster.Namespace, cluster.Name)
rows, err = db.QueryContext(ctx, "SELECT pipeline FROM pipelines WHERE name = ? LIMIT 1", pipelineName)
if err != nil {
return "", err
}
+ defer rows.Close()
if rows.Next() {
var pipeline string
if err := rows.Scan(&pipeline); err != nil {
return "", err
}
return pipeline, nil
}
-
- if rows != nil {
- rows.Close()
- }Also applies to: 233-249
🧰 Tools
🪛 GitHub Check: Build the project
[failure] 221-221:
unreachable code
🪛 GitHub Check: Run unit tests
[failure] 221-221:
unreachable code
zyy17
force-pushed
the
fix/duplicate-pipeline
branch
2 times, most recently
from
7656ec8 to
a153da1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
controllers/greptimedbcluster/deployers/monitoring.go (1)
98-111: Enhance error handling and loggingWhile the logic flow is correct, consider these improvements:
- Add more context to error messages
- Use structured logging with fields
- pipeline, err := d.getPipeline(ctx, cluster) + pipeline, err := d.getPipeline(ctx, cluster) if err != nil { - klog.Errorf("failed to get pipeline for standalone, err: '%v'", err) + klog.ErrorS(err, "failed to get pipeline", + "cluster", klog.KObj(cluster), + "namespace", cluster.Namespace) return false, err } if pipeline == "" { - klog.Infof("create pipeline '%s' for standalone monitoring", common.LogsPipelineName(cluster.Namespace, cluster.Name)) + pipelineName := common.LogsPipelineName(cluster.Namespace, cluster.Name) + klog.InfoS("creating pipeline", + "pipeline", pipelineName, + "cluster", klog.KObj(cluster)) if err := d.createPipeline(cluster); err != nil { - klog.Errorf("failed to create pipeline for standalone, err: '%v'", err) + klog.ErrorS(err, "failed to create pipeline", + "pipeline", pipelineName, + "cluster", klog.KObj(cluster)) return false, err } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
controllers/greptimedbcluster/deployers/monitoring.go(4 hunks)tests/e2e/setup/create-cluster.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/setup/create-cluster.sh
🔇 Additional comments (4)
controllers/greptimedbcluster/deployers/monitoring.go (4)
20-20: LGTM: Required imports added for database functionality
The new imports for database/sql and mysql driver are appropriate for the added database interaction functionality.
Also applies to: 30-30
218-227:
Fix resource leak in table existence check
The current implementation may leak database resources. Use defer immediately after successful query.
rows, err := db.QueryContext(ctx, "SELECT 1 FROM pipelines LIMIT 1")
if err != nil {
if strings.Contains(err.Error(), "TableNotFound") {
return "", nil
}
return "", err
}
+defer rows.Close()
-if rows != nil {
- rows.Close()
-}230-246:
Fix resource leak in pipeline query
Similar to the table check, this query also needs proper resource cleanup.
rows, err = db.QueryContext(ctx, "SELECT pipeline FROM pipelines WHERE name = ? LIMIT 1", pipelineName)
if err != nil {
return "", err
}
+defer rows.Close()
if rows.Next() {
var pipeline string
if err := rows.Scan(&pipeline); err != nil {
return "", err
}
return pipeline, nil
}
-if rows != nil {
- rows.Close()
-}211-216: 🛠️ Refactor suggestion
Add connection pool configuration
The database connection should be configured with proper pool settings.
db, err := sql.Open("mysql", cfg.FormatDSN())
if err != nil {
return "", err
}
defer db.Close()
+
+// Configure connection pool
+db.SetMaxOpenConns(defaultMaxOpenConns)
+db.SetMaxIdleConns(defaultMaxIdleConns)
+db.SetConnMaxLifetime(defaultConnMaxLifetime)
+
+// Test connection
+if err := db.PingContext(ctx); err != nil {
+ return "", fmt.Errorf("failed to connect to database: %w", err)
+}Likely invalid or redundant comment.
zyy17
force-pushed
the
fix/duplicate-pipeline
branch
from
a153da1 to
f16e49e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
controllers/greptimedbcluster/deployers/monitoring.go (1)
234-237: Enhance error handling for row scanningAdd more context to the scan error to help with debugging.
Apply this diff:
if rows.Next() { var pipeline string if err := rows.Scan(&pipeline); err != nil { - return "", err + return "", fmt.Errorf("failed to scan pipeline for '%s': %w", pipelineName, err) } return pipeline, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
controllers/greptimedbcluster/deployers/monitoring.go(4 hunks)tests/e2e/setup/create-cluster.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/setup/create-cluster.sh
🔇 Additional comments (5)
controllers/greptimedbcluster/deployers/monitoring.go (5)
20-20: LGTM: Required imports added for database operations
The new imports for database/sql and github.com/go-sql-driver/mysql are appropriate for the database operations introduced.
Also applies to: 30-30
204-210: 🛠️ Refactor suggestion
Make database configuration more configurable
The database configuration values are hardcoded. Consider:
- Moving configuration to CRD spec or environment variables
- Adding connection pool settings
211-216: 🛠️ Refactor suggestion
Add connection pooling and testing
The database connection should be configured with proper connection pooling settings and tested before use.
233-241:
Fix resource leak in rows handling
The defer rows.Close() statement after rows.Next() is ineffective as it comes after a return statement. Move it immediately after the query.
Apply this diff:
rows, err = db.QueryContext(ctx, "SELECT pipeline FROM pipelines WHERE name = ? LIMIT 1", pipelineName)
if err != nil {
return "", err
}
+defer rows.Close()
if rows.Next() {
var pipeline string
if err := rows.Scan(&pipeline); err != nil {
return "", err
}
return pipeline, nil
}
-defer rows.Close()
return "", nilLikely invalid or redundant comment.
98-111: Consider handling concurrent pipeline creation
While the logic to check before creating is good, there's a potential race condition if multiple instances check simultaneously and find no pipeline exists. Consider adding a distributed locking mechanism or handling the case where pipeline creation fails due to it already existing.
Let's check if there are any existing distributed locking mechanisms in use:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
Documentation