-
Notifications
You must be signed in to change notification settings - Fork 33
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
fix: add canary_id to job_history and cleanup sync canary function #1169
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -276,8 +276,13 @@ func ScanCanaryConfigs() { | |
var canaryUpdateTimeCache = sync.Map{} | ||
|
||
// TODO: Refactor to use database object instead of kubernetes | ||
func SyncCanaryJob(canary v1.Canary) error { | ||
if !canary.DeletionTimestamp.IsZero() || canary.Spec.GetSchedule() == "@never" { | ||
func SyncCanaryJob(dbCanary pkg.Canary) error { | ||
canary, err := dbCanary.ToV1() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if canary.Spec.GetSchedule() == "@never" { | ||
DeleteCanaryJob(canary.GetPersistedID()) | ||
return nil | ||
} | ||
|
@@ -290,14 +295,10 @@ func SyncCanaryJob(canary v1.Canary) error { | |
} | ||
} | ||
|
||
dbCanary, err := db.GetCanary(canary.GetPersistedID()) | ||
if err != nil { | ||
return err | ||
} | ||
job := CanaryJob{ | ||
Client: Kommons, | ||
Kubernetes: Kubernetes, | ||
Canary: canary, | ||
Canary: *canary, | ||
DBCanary: dbCanary, | ||
LogPass: canary.IsTrace() || canary.IsDebug() || LogPass, | ||
LogFail: canary.IsTrace() || canary.IsDebug() || LogFail, | ||
|
@@ -333,52 +334,25 @@ func SyncCanaryJob(canary v1.Canary) error { | |
func SyncCanaryJobs() { | ||
logger.Debugf("Syncing canary jobs") | ||
|
||
jobHistory := models.NewJobHistory("CanarySync", "canary", "").Start() | ||
_ = db.PersistJobHistory(jobHistory) | ||
defer func() { _ = db.PersistJobHistory(jobHistory.End()) }() | ||
|
||
canaries, err := db.GetAllCanariesForSync() | ||
if err != nil { | ||
logger.Errorf("Failed to get canaries: %v", err) | ||
jobHistory.AddError(err.Error()) | ||
return | ||
} | ||
|
||
for _, c := range canaries { | ||
canary, err := c.ToV1() | ||
if err != nil { | ||
logger.Errorf("Error parsing canary[%s]: %v", c.ID, err) | ||
jobHistory.AddError(err.Error()) | ||
continue | ||
} | ||
|
||
if len(canary.Status.Checks) == 0 && len(canary.Spec.GetAllChecks()) > 0 { | ||
logger.Infof("Persisting %s as it has no checks", canary.Name) | ||
pkgCanary, _, _, err := db.PersistCanary(*canary, canary.Annotations["source"]) | ||
if err != nil { | ||
logger.Errorf("Failed to persist canary %s: %v", canary.Name, err) | ||
jobHistory.AddError(err.Error()) | ||
continue | ||
} | ||
Comment on lines
-355
to
-362
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since these canaries are always fetched from database, no persistence logic is needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yashmehrotra we were also saving checks from the canary spec in here. Now, checks aren't being saved. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. @yashmehrotra I'm not talking about component checks. If you create a canary from the UI then the checks are never saved. |
||
|
||
v1canary, err := pkgCanary.ToV1() | ||
if err != nil { | ||
logger.Errorf("Failed to convert canary to V1 %s: %v", canary.Name, err) | ||
jobHistory.AddError(err.Error()) | ||
continue | ||
} | ||
Comment on lines
-364
to
-369
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. redundant conversion |
||
jobHistory := models.NewJobHistory("CanarySync", "canary", c.ID.String()).Start() | ||
_ = db.PersistJobHistory(jobHistory) | ||
|
||
if err := SyncCanaryJob(*v1canary); err != nil { | ||
logger.Errorf(err.Error()) | ||
jobHistory.AddError(err.Error()) | ||
} | ||
} else if err := SyncCanaryJob(*canary); err != nil { | ||
logger.Errorf(err.Error()) | ||
jobHistory.AddError(err.Error()) | ||
if err := SyncCanaryJob(c); err != nil { | ||
logger.Errorf("Error syncing canary[%s]: %v", c.ID, err.Error()) | ||
_ = db.PersistJobHistory(jobHistory.AddError(err.Error()).End()) | ||
continue | ||
} | ||
jobHistory.IncrSuccess() | ||
_ = db.PersistJobHistory(jobHistory.End()) | ||
} | ||
|
||
jobHistory.IncrSuccess() | ||
logger.Infof("Synced canary jobs %d", len(CanaryScheduler.Entries())) | ||
} | ||
|
||
|
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.
Not required, we are doing this in the SyncCanary func