-
Notifications
You must be signed in to change notification settings - Fork 152
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
Progress tracking actionset #1586
Conversation
Signed-off-by: Ivan Sim <ivan.sim@kasten.io>
Signed-off-by: Ivan Sim <ivan.sim@kasten.io>
Signed-off-by: Ivan Sim <ivan.sim@kasten.io>
Signed-off-by: Ivan Sim <ivan.sim@kasten.io>
Signed-off-by: Ivan Sim <ivan.sim@kasten.io>
In the k8s cluster < 1.20 we got into a problem while updating actionset progress. Details can be found [here](#1576). This tries to fix that.
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
"github.com/kanisterio/kanister/pkg/log" | ||
"github.com/kanisterio/kanister/pkg/reconcile" | ||
"github.com/kanisterio/kanister/pkg/validate" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" |
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.
Third-party pkg imports should be in separate group
return updateActionSet(ctx, client, actionSet, progressPercent, now) | ||
} | ||
|
||
func weight(phase *crv1alpha1.BlueprintPhase) float64 { |
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.
[nit] ideally function name should start with a verb. e.g calculateWeight
or getWeight
return weightNormal | ||
} | ||
|
||
func longRunning(phase *crv1alpha1.BlueprintPhase) bool { |
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.
[nit] since the function returns bool - we can rename it to something - containsLongRunningFunc
return nil | ||
} | ||
|
||
func completedOrFailed(actionSet *crv1alpha1.ActionSet) bool { |
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.
[nit] Since the func returns bool, we can rename this to something like - isCompletedOrFailed
"nextUpdateTime": time.Now().Add(pollDuration), | ||
} | ||
log.Error().WithError(err).Print("failed to update phase progress", fields) | ||
continue |
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.
This may run into an infinite loop if the client fails to update action progress every iteration. Instead of continue, should we return error?
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.
The idea here is to ignore intermediate, flakey errors, due to e.g., API server or client-go throttling. The final progress should always be calculated based on the actionset status. If the actionset status completed, progress should always be 100% despite of intermediate errors.
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.
It shouldn't lead to an infinite loop because either ctx
will be done, or the completedOrFailed()
function will return, based on the actionset status.
"namespace": actionSet.GetNamespace(), | ||
"progress": progressPercent, | ||
} | ||
log.Debug().Print("updating action progress", fields) |
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.
We should also add actionset name (and may be phase name as well) to the logs so that we can differentiate between logs in case of multiple statements
if err := reconcile.ActionSet(ctx, client.CrV1alpha1(), actionSet.GetNamespace(), actionSet.GetName(), updateFunc); err != nil { | ||
return err | ||
} | ||
|
||
return nil |
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.
if err := reconcile.ActionSet(ctx, client.CrV1alpha1(), actionSet.GetNamespace(), actionSet.GetName(), updateFunc); err != nil { | |
return err | |
} | |
return nil | |
return reconcile.ActionSet(ctx, client.CrV1alpha1(), actionSet.GetNamespace(), actionSet.GetName(), updateFunc) |
error check can be skipped
@@ -131,6 +132,16 @@ type ActionStatus struct { | |||
DeferPhase Phase `json:"deferPhase,omitempty"` | |||
} | |||
|
|||
// ActionProgress provides information on the progress of an action. | |||
type ActionProgress struct { |
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.
should we also add the name of the current phase in progress?
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.
phase progress is planned for the next iteration. at this level, we just want to communicate the overall progress without bloating the API spec.
- Improve test a bit - Add `omitempty` to another field of `actionProgress`
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.
I believe most of the changes in this PR are a part of the revert and were previously approved. My approval of this PR is purely focused on the intended bug fix. We can follow up on the other review comments in a separate PR.
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.
@viveksinghggits Thanks for working on this 👍. Since the previous PR has been reviewed quite a bit, we should keep changes to this PR to the minimal, unless more bugs are found. If possible, I want to avoid another round of review churns - that last PR took a month (or more?) to merge.
I suggest other important non-bug review items be added to a new GH issue, which we may or may not address when we start the phase progress work.
We should start finding ways to surface these kind of errors sooner in the dev cycle. Reverting PRs is costly and inefficient as they create churns and re-works.
* Update controller to track weighted action progress Signed-off-by: Ivan Sim <ivan.sim@kasten.io> * Revert auto-changes by codegen Signed-off-by: Ivan Sim <ivan.sim@kasten.io> * Address Eugen's feedback Signed-off-by: Ivan Sim <ivan.sim@kasten.io> * Address Pavan's feedback Signed-off-by: Ivan Sim <ivan.sim@kasten.io> * Address Vivek's feedback Signed-off-by: Ivan Sim <ivan.sim@kasten.io> * Fix tests Signed-off-by: Ivan Sim <ivan.sim@kasten.io> * Fix a bug while updating lastTransitionTime in <1.20 cluster In the k8s cluster < 1.20 we got into a problem while updating actionset progress. Details can be found [here](kanisterio#1576). This tries to fix that. * Remove extra newline from codegen.sh * Address review comments - Improve test a bit - Add `omitempty` to another field of `actionProgress` Co-authored-by: Ivan Sim <ivan.sim@kasten.io> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Change Overview
Re-opens #1487, with #1576 fix.
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan
Install Kanister on k8s version 1.19 and 1.21 to check if we are backup and restore an application.