-
Notifications
You must be signed in to change notification settings - Fork 157
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
Add ExecutionProgress() method to function interface to report exec progress #2023
Conversation
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. |
37e30eb
to
5146900
Compare
5146900
to
e2ea4ef
Compare
c8f8520
to
defe297
Compare
defe297
to
2eadae1
Compare
Signed-off-by: Prasad Ghangal <[email protected]>
2eadae1
to
3402512
Compare
output, err = p.Exec(ctx, *bp, action.Name, *tp) | ||
doneProgressTrack() |
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.
do we need to call it again even after calling it from defer
?
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.
Yes, the doneProgressTrack()
needs to be called as soon as Exec()
is done. The defer()
gets called once all the phase Exec is done and action is completed (or failed). When the next phase execution starts during interaction in this function, the older phase's Progress() starts sending completion percent as 0. So before running next phase, the goroutine for current phase needs to be closed. Let me know if this makes sense or not.
The other question that I had was, how are we going to handle the Kopia related function. Some functions are already, in so they are going to break right? |
@viveksinghggits I've already rebased the PR with master which has kopia based function. I've implemented |
Signed-off-by: Prasad Ghangal <[email protected]>
Signed-off-by: Prasad Ghangal <[email protected]>
Signed-off-by: Prasad Ghangal <[email protected]>
eed913b
to
35c5578
Compare
Signed-off-by: Prasad Ghangal <[email protected]>
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.
looks good to me almost, just some small questions.
if actionSet.Status.Actions[i].Phases[j].Name != phaseName { | ||
continue | ||
} | ||
if actionSet.Status.Actions[i].Phases[j].State == crv1alpha1.StatePending || | ||
actionSet.Status.Actions[i].Phases[j].State == crv1alpha1.StateFailed { | ||
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.
should we combine these two statements? I am ok with this change if you think it's more readable.
@pavannd1 could you please take a look? |
Co-authored-by: Pavan Navarathna <[email protected]>
Co-authored-by: Pavan Navarathna <[email protected]>
Co-authored-by: Pavan Navarathna <[email protected]>
Co-authored-by: Pavan Navarathna <[email protected]>
Signed-off-by: Prasad Ghangal <[email protected]>
Added test case scenario when executing multiple actions with the same blueprint in the PR's test plan. @pavannd1 @viveksinghggits PTAL |
I had already looked into the PR so I am going to skip 🍏 , but just wanted to make sure that the issue that we talked about where two actions are running in parallel is not going to cause any issues with |
@viveksinghggits I've filed - #2270 |
@pavannd1 could you please re-review the 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.
LGTM. Feel free to merge once CI passes.
* Implement ExecutionProgress method on Kanister functions Signed-off-by: Prasad Ghangal <[email protected]> * Set LastTransitionTime in ExecutionProgress return Signed-off-by: Prasad Ghangal <[email protected]> * Update pkg/function/backup_data.go Co-authored-by: Pavan Navarathna <[email protected]> --------- Signed-off-by: Prasad Ghangal <[email protected]> Co-authored-by: Pavan Navarathna <[email protected]>
Signed-off-by: Prasad Ghangal <[email protected]>
Signed-off-by: Prasad Ghangal <[email protected]>
Signed-off-by: Prasad Ghangal <[email protected]>
…rogress (#2023) * Add phase level progress fields in actionset object Signed-off-by: Prasad Ghangal <[email protected]> * Change weight based progress calc with func implementation Signed-off-by: Prasad Ghangal <[email protected]> * Refactor and add unit tests for phase progress Signed-off-by: Prasad Ghangal <[email protected]> * Update generated deepcopy methods Signed-off-by: Prasad Ghangal <[email protected]> * Fix import cycle issue in test Signed-off-by: Prasad Ghangal <[email protected]> * Update pkg/apis/cr/v1alpha1/types.go Co-authored-by: Pavan Navarathna <[email protected]> * Update pkg/apis/cr/v1alpha1/types.go Co-authored-by: Pavan Navarathna <[email protected]> * Update pkg/apis/cr/v1alpha1/types.go Co-authored-by: Pavan Navarathna <[email protected]> * Update pkg/apis/cr/v1alpha1/types.go Co-authored-by: Pavan Navarathna <[email protected]> * Use correct action index while updating action phase state Signed-off-by: Prasad Ghangal <[email protected]> * Implement ExecutionProgress method on Kanister functions (#2117) * Implement ExecutionProgress method on Kanister functions Signed-off-by: Prasad Ghangal <[email protected]> * Set LastTransitionTime in ExecutionProgress return Signed-off-by: Prasad Ghangal <[email protected]> * Update pkg/function/backup_data.go Co-authored-by: Pavan Navarathna <[email protected]> --------- Signed-off-by: Prasad Ghangal <[email protected]> Co-authored-by: Pavan Navarathna <[email protected]> * Implement ExecProgress on test function Signed-off-by: Prasad Ghangal <[email protected]> * Remove unused mockPhase in test Signed-off-by: Prasad Ghangal <[email protected]> * Remove unnecessary whitespace in updateActionProgress func Signed-off-by: Prasad Ghangal <[email protected]> --------- Signed-off-by: Prasad Ghangal <[email protected]> Signed-off-by: Prasad Ghangal <[email protected]> Co-authored-by: Pavan Navarathna <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Change Overview
This PR adds ExecProgress() method to Kanister function interface to report the function exec progress.
This PR replaces the weight based progress calculation logic with function-specific implementation.
By default, all functions report progressPercent as 0 during exec and 100 once the execution is completed.
Ideally, each function should have a way to get stats of execution progress. Which can be added as a followup
The design has been discussed here: #1875
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan
Sample output of actionSet status of each phase completion status
TC:1 Blueprint action consists of 3 phases and all phases succeeds
1. First phase completed
2. Second phase completed
3. Third phase completed
TC:2 Blueprint consists of 3 phases, first phases succeeds and 2nd one fails
1. First Phase started
2. First phase completed and Second phase started
3. Second phase failed
TC:3 Blueprint with actions having multiple phases. And actionset running multiple action
1. First phase if each action is running
2. First phase of second action is completed
3. First phase of both the action is completed and 2nd phase is running
4. 2nd phase of second action is completed
5. All the actions are completed