Skip to content

pod comp status - 3 #201

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Oct 13, 2022
Merged

pod comp status - 3 #201

merged 16 commits into from
Oct 13, 2022

Conversation

@asm582
Copy link
Member

@asm582 asm582 commented Aug 4, 2022

Manually tested:

Screen Shot 2022-08-04 at 4 59 51 PM

  • Formatting fixes in yaml files
  • Moved state change logic to manageQueueJob
@asm582 asm582 requested a review from dmatch01 August 4, 2022 21:06
dmatch01
dmatch01 previously requested changes Aug 5, 2022
View reviewed changes
Copy link
Collaborator

@dmatch01 dmatch01 left a comment

Choose a reason for hiding this comment

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

The managequeuejob() is where all state changes should occur but I'm not seeing it in the function. Also, please add suggested test cases to PR. Thanks!
@asm582
Copy link
Member Author

asm582 commented Aug 5, 2022

The managequeuejob() is where all state changes should occur but I'm not seeing it in the function. Also, please add suggested test cases to PR. Thanks!

I added the state change function in manageQueueJob, sharing the below snippet:

//set appwrapper status to Complete or RunningHoldCompletion	if qj.Status.CanRun && qj.Status.State != arbv1.AppWrapperStateCompleted {	qj.Status.State = cc.GetAllObjectsOwned(qj)	}
@asm582
Copy link
Member Author

asm582 commented Aug 7, 2022

  • Added completion status logic at the top in manageQueue
  • Added test cases
@asm582 asm582 requested a review from dmatch01 August 8, 2022 16:01
@asm582 asm582 mentioned this pull request Aug 10, 2022
Closed
@asm582 asm582 dismissed dmatch01’s stale review September 24, 2022 16:19

Review was done over call and we changed the design

@asm582
Copy link
Member Author

asm582 commented Sep 24, 2022

@dmatch01 Thanks for the review, All test cases pass now
dmatch01
dmatch01 requested changes Sep 27, 2022
View reviewed changes
Copy link
Collaborator

@dmatch01 dmatch01 left a comment

Choose a reason for hiding this comment

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

Thank for the latest changes. Please see addition suggestion related to performance and additional test case needed.
pkg/controller/queuejob/queuejob_controller_ex.go
Comment on lines +1789 to +1791
if qj.Status.CanRun && qj.Status.State != arbv1.AppWrapperStateActive &&
qj.Status.State != arbv1.AppWrapperStateCompleted &&
qj.Status.State != arbv1.AppWrapperStateRunningHoldCompletion {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This condition will never happen due to the first if statement added above. Please confirm my logic...
Copy link
Member Author

Choose a reason for hiding this comment

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

This condition does happen as confirmed by the test cases, it does set AppwrapperRunnable in the conditions block:

Conditions: Last Transition Micro Time: 2022-09-26T23:24:37.074294Z Last Update Micro Time: 2022-09-26T23:24:37.074294Z Status: True Type: Init Last Transition Micro Time: 2022-09-26T23:24:37.074605Z Last Update Micro Time: 2022-09-26T23:24:37.074604Z Reason: AwaitingHeadOfLine Status: True Type: Queueing Last Transition Micro Time: 2022-09-26T23:24:37.085727Z Last Update Micro Time: 2022-09-26T23:24:37.085726Z Reason: FrontOfQueue. Status: True Type: HeadOfLine Last Transition Micro Time: 2022-09-26T23:24:37.184950Z Last Update Micro Time: 2022-09-26T23:24:37.184950Z Reason: AppWrapperRunnable Status: True Type: Dispatched Last Transition Micro Time: 2022-09-26T23:24:39.074060Z Last Update Micro Time: 2022-09-26T23:24:39.074060Z Reason: PodsRunning Status: True Type: Running Last Transition Micro Time: 2022-09-26T23:24:44.100438Z Last Update Micro Time: 2022-09-26T23:24:44.100438Z Reason: PodsCompleted Status: True Type: Completed
@asm582 asm582 requested a review from dmatch01 September 27, 2022 20:16
dmatch01
dmatch01 previously requested changes Sep 28, 2022
View reviewed changes
@asm582 asm582 requested a review from dmatch01 October 2, 2022 00:51
@asm582 asm582 dismissed dmatch01’s stale review October 4, 2022 01:55

Changes were done for the review

dmatch01
dmatch01 requested changes Oct 6, 2022
View reviewed changes
Copy link
Collaborator

@dmatch01 dmatch01 left a comment

Choose a reason for hiding this comment

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

@asm582 This PR is very close to merging. See a couple of minor suggestions.
@asm582 asm582 requested a review from dmatch01 October 7, 2022 16:35
dmatch01
dmatch01 approved these changes Oct 12, 2022
View reviewed changes
Copy link
Collaborator

@dmatch01 dmatch01 left a comment

Choose a reason for hiding this comment

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

Please open issue for if statement comment.
pkg/controller/queuejob/queuejob_controller_ex.go Outdated
Comment on lines 609 to 610
if len(genericItem.CompletionStatus) > 0 {
countCompletionRequired = countCompletionRequired + 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Closing, outdated comment.
@dmatch01
Copy link
Collaborator

LGTM, great!
@dmatch01 dmatch01 merged commit 5b74921 into project-codeflare:quota-management Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants

@asm582 @dmatch01