Skip to content
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

Phase level progress tracking proposal #1875

Merged
merged 11 commits into from
Sep 13, 2023
Merged

Phase level progress tracking proposal #1875

merged 11 commits into from
Sep 13, 2023

Conversation

PrasadG193
Copy link
Contributor

@PrasadG193 PrasadG193 commented Jan 23, 2023

Change Overview

Proposal for phase level progress tracking for actionset

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • fixes #issue-number

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@github-actions
Copy link
Contributor

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.

@infraq infraq added this to In Progress in Kanister Jan 23, 2023
@PrasadG193 PrasadG193 changed the title Add proposal for phase tracking Phase level tracking proposal Jan 23, 2023
@PrasadG193 PrasadG193 changed the title Phase level tracking proposal Phase level progress tracking proposal Jan 23, 2023
Copy link
Contributor

@ankitjain235 ankitjain235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some minor questions.

design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved
Action can consists of multiple phases. These all phases can take consume different duration for completion based on the operations it performs. All phases for the action can be given equal weight-age and to calculate action progress completion %, average of all phase completion %ge can be calculated

```
% completion of action = sum(% Phase completion of all the phases)/ no. of phases
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we not including the combined result as part of ActionStatus?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have ActionSetStatus.Progress and Phase level Progress, but not ActionStatus since use can only see actionset status. Is that the reason progress of each action is not included?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ActionSet is supposed to perform single action only, the ActionSet progress is the same as Action progress. So by ActionSet.Progress users can get information about Action progress as well.


### Updating actionset status

[TrackActionProgress](https://github.com/kanisterio/kanister/blob/master/pkg/progress/action.go#L47) function is responsible for periodically updating progress for the action in ActionSet resource status.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we deciding the period for updating the progress?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be const period - 5 or 10s

Co-authored-by: Ankit Jain <ankitjain.meone@gmail.com>
Copy link
Contributor

@viveksinghggits viveksinghggits left a 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.

design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved

### Non-datamover Kanister functions

TBD
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it's not decided as of now, to actually implement this we will have to have something here right? For example either hard code or something else, otherwise would we be able to implement this design?

Copy link
Contributor Author

@PrasadG193 PrasadG193 Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. We have discussed a couple of ideas, will add them here. Thanks

Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@github-actions
Copy link
Contributor

This PR is marked as stale due to inactivity. Add a new comment to reactivate it.

@github-actions github-actions bot added the stale label May 24, 2023
@pavannd1 pavannd1 added frozen and removed stale labels May 25, 2023
Copy link
Contributor

@pavannd1 pavannd1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the detailed design doc!
The design is pretty clear and can be implemented. No changes are needed to the design.
Suggested some language updates and wrapped text at 80 characters.

design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved
design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved
design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved
design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved
design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved
design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved
design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved
design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved
design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved
design/phase-level-progress-tracking.md Outdated Show resolved Hide resolved
PrasadG193 and others added 5 commits September 12, 2023 20:41
Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
Kanister automation moved this from In Progress to Reviewer approved Sep 12, 2023
@mergify mergify bot merged commit bc2a874 into master Sep 13, 2023
15 checks passed
Kanister automation moved this from Reviewer approved to Done Sep 13, 2023
@mergify mergify bot deleted the prop-phase-tracking branch September 13, 2023 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants