feat(backup): Add backup and scheduler system #49
Conversation
Implement complete backup and scheduled task functionality: - Add scheduler package with cron-based task execution - Add backup package for deployment backup/restore - Add API endpoints for managing backups and scheduled tasks - Extend ServiceMetadata model with backup specifications - Update WordPress and Laravel templates with backup definitions Backup features: - Backup compose files, env files, and metadata - Export container data using docker cp - Dump MySQL/PostgreSQL databases - Pre/post backup hooks for maintenance mode - Retention-based cleanup Scheduler features: - Cron expression support for task scheduling - Backup and command task types - Task execution history tracking - On-demand task execution Signed-off-by: nfebe <fenn25.fn@gmail.com>
Code Review SummaryThis pull request introduces comprehensive backup and scheduling functionalities to the Flatrun agent, including API endpoints, a job tracker, and an executor for tasks. It significantly enhances the agent's capabilities for managing deployments and their data lifecycle. The changes involve new files for API handlers, backup management, scheduler logic, and related tests. 🚀 Key Improvements
💡 Minor Suggestions
🚨 Critical Issues
|
🔍 Code Review💡 1. **internal/api/server.go** (Lines 801-808) - SECURITYThe Suggested Code: Current Code: func generateRandomPassword(length int) string {
const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
b := make([]byte, length)
for i := range b {
b[i] = charset[time.Now().UnixNano()%int64(len(charset))]
time.Sleep(time.Nanosecond)
}
return string(b)
}💡 2. **internal/backup/manager.go** (Lines 313-316) - SECURITYPassing MySQL password directly in the Suggested Code: Current Code: cmd := exec.CommandContext(ctx, "docker", args...)
output, err := cmd.Output()
if err != nil {
return "", fmt.Errorf("mysqldump failed: %w", err)
}💡 3. **internal/backup/manager.go** (Line 386) - SECURITYThe Suggested Code: Current Code: cmd := exec.CommandContext(hookCtx, "docker", "exec", containerName, "sh", "-c", hook.Command)💡 4. **internal/scheduler/executor.go** (Line 76) - SECURITYSimilar to backup hooks, the command executed via Suggested Code: Current Code: cmd := exec.CommandContext(cmdCtx, "docker", "exec", containerName, "sh", "-c", config.Command)💡 5. **internal/api/server.go** (Lines 154-163) - REFACTORThe initialization of the Suggested Code: Current Code: if backupManager != nil {
executor := scheduler.NewExecutor(backupManager, manager)
schedulerManager, err := scheduler.NewManager(cfg.DeploymentsPath, executor)
if err != nil {
log.Printf("Warning: Failed to initialize scheduler manager: %v", err)
} else {
s.schedulerManager = schedulerManager
s.schedulerManager.Start()
}
}💡 6. **internal/api/backup_handlers.go** (Lines 12-16) - REFACTORThe repeated Suggested Code: Current Code: func (s *Server) listBackups(c *gin.Context) {
if s.backupManager == nil {
c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Backup manager not enabled"})
return
}💡 7. **internal/backup/manager.go** (Line 44) - IMPROVEMENTThe Suggested Code: Current Code: backupID := fmt.Sprintf("%s_%s", deploymentName, time.Now().Format("20060102_150405"))💡 8. **internal/scheduler/scheduler.go** (Lines 119-120) - PERFORMANCEThe Suggested Code: Current Code: ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute)
defer cancel()Verdict: APPROVE Posted as a comment because posting a review failed. |
- Add JobTracker for managing async backup/restore jobs - Refactor backup creation to return job ID immediately - Add RestoreBackup functionality with async execution - Add job status endpoints (GET /backups/jobs, GET /backups/jobs/:id) - Update API handlers to use StartBackupJob/StartRestoreJob
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
internal/backup/jobs.go
Outdated
| return jobID | ||
| } | ||
|
|
||
| func (m *Manager) StartRestoreJob(ctx context.Context, req *RestoreBackupRequest) string { |
There was a problem hiding this comment.
Similar to StartBackupJob, StartRestoreJob takes ctx context.Context but uses context.Background() for the actual restore operation. This is inconsistent with the function signature. It's either explicitly not needed in the signature, or should be used for the underlying restore logic.
| func (m *Manager) StartRestoreJob(ctx context.Context, req *RestoreBackupRequest) string { | |
| func (m *Manager) StartRestoreJob(req *RestoreBackupRequest) string { |
| req = backup.RestoreBackupRequest{} | ||
| } |
There was a problem hiding this comment.
If c.ShouldBindJSON(&req) fails, the req struct is completely reinitialized. This means if any JSON fields were successfully bound before the error (e.g., RestoreData), they would be reset to their zero values. It's generally better to handle binding errors more granularly or check for Content-Length before attempting JSON binding if the body is optional.
| req = backup.RestoreBackupRequest{} | |
| } | |
| var req backup.RestoreBackupRequest | |
| if c.Request.ContentLength > 0 { | |
| if err := c.ShouldBindJSON(&req); err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | |
| return | |
| } | |
| } | |
| req.BackupID = backupID |
| if s.schedulerManager == nil { | ||
| c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Scheduler not enabled"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
Similar to backup handlers, the repeated check for s.schedulerManager == nil can be refactored into a middleware to reduce code duplication and improve handler readability.
| if s.schedulerManager == nil { | |
| c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Scheduler not enabled"}) | |
| return | |
| } | |
| func schedulerManagerRequired(s *Server) gin.HandlerFunc { | |
| return func(c *gin.Context) { | |
| if s.schedulerManager == nil { | |
| c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Scheduler not enabled"}) | |
| c.Abort() | |
| return | |
| } | |
| c.Next() | |
| } | |
| } | |
| // Then in setupRoutes: | |
| // protectedScheduler := protected.Group("/scheduler").Use(schedulerManagerRequired(s)) | |
| // protectedScheduler.GET("/tasks", s.listScheduledTasks) |
Add comprehensive tests for: - JobTracker operations (create, get, update, list, cleanup) - Manager backup operations (create, list, get, delete, cleanup) - Async job handling Signed-off-by: nfebe <fenn25.fn@gmail.com>
nfebe
force-pushed
the
feat/backup-and-scheduler
branch
from
140dc2d to
6bd1778
Compare
| if s.backupManager == nil { | ||
| c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Backup manager not enabled"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
This check for s.backupManager == nil is repeated at the beginning of almost every handler in this file. It would be more idiomatic and cleaner to extract this into a Gin middleware function. This reduces boilerplate and centralizes the logic.
| if s.backupManager == nil { | |
| c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Backup manager not enabled"}) | |
| return | |
| } | |
| func requireBackupManager(s *Server) gin.HandlerFunc {return func(c *gin.Context) {if s.backupManager == nil {c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Backup manager not enabled"})c.Abort()return}c.Next()}} |
| if s.schedulerManager == nil { | ||
| c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Scheduler not enabled"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
Similar to the backupManager checks, the s.schedulerManager == nil check is repeated in all scheduler handlers. This should also be refactored into a middleware to improve code quality and reduce duplication.
| if s.schedulerManager == nil { | |
| c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Scheduler not enabled"}) | |
| return | |
| } | |
| func requireSchedulerManager(s *Server) gin.HandlerFunc {return func(c *gin.Context) {if s.schedulerManager == nil {c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Scheduler not enabled"})c.Abort()return}c.Next()}} |
| c.JSON(http.StatusBadRequest, gin.H{"error": "Deployment not found: " + req.DeploymentName}) | ||
| return |
There was a problem hiding this comment.
Returning req.DeploymentName directly in the error message for a Deployment not found can be an information disclosure risk, especially if deployment names are sensitive. It's better to keep error messages generic for StatusNotFound to avoid leaking internal system details.
| c.JSON(http.StatusBadRequest, gin.H{"error": "Deployment not found: " + req.DeploymentName}) | |
| return | |
| c.JSON(http.StatusNotFound, gin.H{"error": "Deployment not found"}) |
Implement complete backup and scheduled task functionality:
Backup features:
Scheduler features: