Pod comp stat 2 #192

asm582 · 2022-07-27T16:47:19Z

Completion status is now represented as:

completionstatus: "Complete,Failed"

In the current design services and podgroups are ignored since they do not have completion status. below scenarios were tested:

dmatch01

Can you also add a failure test case for handling non-pod creating generic items? The easiest way is to duplicate one of the new cases and change the service object to include completionstatus. This will ensure that any end user inadvertently adding this field will not force a controller failure. Thanks!

pkg/controller/queuejob/queuejob_controller_ex.go

config/crd/bases/mcad.ibm.com_appwrappers.yaml

dmatch01 · 2022-08-01T12:49:29Z

pkg/controller/queuejob/queuejob_controller_ex.go

+//Set Appwrapper state to complete if all items in Appwrapper
+//are completed
+if awNew.Status.State != arbv1.AppWrapperStateCompleted {
+cc.GetAllObjectsOwned(awNew)
+}
+
+if awNew.Status.State == arbv1.AppWrapperStateRunningHoldCompletion {
+awNew.Status.QueueJobState = arbv1.AppWrapperCondRunningHoldCompletion
+cond := GenerateAppWrapperCondition(arbv1.AppWrapperCondRunningHoldCompletion, v1.ConditionTrue, "SomeItemsCompleted", "")
+awNew.Status.Conditions = append(awNew.Status.Conditions, cond)
+awNew.Status.FilterIgnore = true // Update AppWrapperCondCompleted
+cc.updateEtcd(awNew, "[syncQueueJob] setRunningHoldCompletion")
+}
+
+if awNew.Status.State == arbv1.AppWrapperStateCompleted {
+awNew.Status.QueueJobState = arbv1.AppWrapperCondCompleted
+cond := GenerateAppWrapperCondition(arbv1.AppWrapperCondCompleted, v1.ConditionTrue, "PodsCompleted", "")
+awNew.Status.Conditions = append(awNew.Status.Conditions, cond)
+awNew.Status.FilterIgnore = true // Update AppWrapperCondCompleted
+cc.updateEtcd(awNew, "[syncQueueJob] setCompleted")
+}
+


All aw.Status.State changes are done in manageQueueJob(). Can we move the code to this fn. for consistency? The logic not quite intuitive as there are multiple queues involved thus a quick discussion would likely help here.

An appwrapper is set to running state insyncQueueJob, newly added states are obtained after AW turns running, hence I added these in the same method

.status.state is being updated in the call cc.GetAllObjectsOwned(awNew). All state changes are done in managequeuejob(). Suggest moving all the logic to managequeuejob() to be consistent. The syncQueueJob() only update the .status.queuejobstate field.

dmatch01 · 2022-08-01T13:06:55Z

pkg/controller/queuejob/queuejob_controller_ex.go

+countCompletionRequired := 0
+ignoredItems := 0
+for _, genericItem := range cqj.Spec.AggrResources.GenericItems {
+objectName := genericItem.GenericTemplate


If the generic item does not have the new completionstatus field, can we skip this logic for performance reasons? Please let me know your thoughts.

Inside Appwrapper we are finding items of 3 types of items completionRequired, completedItems and ignoredItems, so we would need to iterate all the items I believe

See comment response below.

pkg/controller/queuejob/queuejob_controller_ex.go

dmatch01 · 2022-08-01T13:20:23Z

pkg/controller/queuejob/queuejob_controller_ex.go

+//ignore service and podgroups since these items do not have status
+if status == "ignore" {
+ignoredItems = ignoredItems + 1
+}


See comment above regarding logic checking of new completionstatus

If we start ignoring items without completionstatus I think none of the ray jobs will complete with the current design.

That's correct. We don't want to presume completion status unless the end user requests the AW generic item to be considered. This allows for end users to add run to completion objects that they specify. If the end user does not want the generic item to be considered for completion we have that flexibility. Please share your thoughts.

ok, I think the above is not entirely true. please consider the case, if AW has job and deployment as two items and only job is tracked, we still set the status as RunningHoldCompletion. So non-compute items are treated differently than the compute items.

dmatch01 · 2022-08-01T13:25:14Z

pkg/controller/queuejobresources/genericresource/genericresource.go

+//service and podgroup has no conditions return ALWAYS true
+//TODO: make it configurable?
+if gvk.GroupKind().Kind == "Service" || gvk.GroupKind().Kind == "PodGroup" {
+return "ignore"
+}
+


This can be removed if generic item does not have the new field completionstatus correct?

I don't think so, service or podgroup does not have completion status. from our initial design the idea was to hold AW in RunningHoldCompletion if any item other than podgroup or service is not complete.

Ok, I see the challenge here. As part of the AW pod phases (pending/running/completed...) are tracked. If there is a check of the pods when setting state, this would be sufficient. Iterating over each generic item that is not tracked would not be necessary. only when all tracked items indicate completionstatus the state should be changed (of which is being done). The change here is that the decision to set state to RunningHoldCompletion or Complete is when all pods associated to AW are in Failed/Completed phase states. These counts are already tracked in the AW and can be referenced from the .status field. Please let me know your thoughts. Thanks!

please refer to this comment #192 (comment)

dmatch01 · 2022-08-01T13:34:51Z

pkg/controller/queuejobresources/genericresource/genericresource.go

+for _, job := range inEtcd.Items {
+completionRequiredBlock := aw.CompletionStatus
+if len(completionRequiredBlock) > 0 {
+conditions := job.Object["status"].(map[string]interface{})["conditions"].([]interface{})


Since this logic is dealing with any object (including custom resources), should probably check to ensure there is a conditions field and likely to occur but also a check for status field

I have seen status field has other objects, for instance, status may have conditions array or loadbalancer object, does it make sense to stick with .status.conditions.type for now?

I'm ok with not adding the logic but a new test case should be added to handle such and event. The test case should create an object without conditions that tracks. For instance, duplicate on of the test cases and add an addition generic item that is a new namespace that tracks (completionstatus). This is a failure test case so the controller should handle this problem gracefully.

dmatch01

There were quite a bit more format changes. If they can be reduced in this PR that would be helpful. Feel free to submit a separate PR with just format change which would be great! Thanks!

dmatch01 · 2022-08-02T12:22:40Z

config/crd/bases/mcad.ibm.com_appwrappers.yaml

+ an item in Appwrapper should be a candidate to complete an appwrapper.
+ this field should contain conditions that make appwrapper complete, for instance :-
+ completionconditions could be "Complete", "Failed" to make appwrapper completes.
+ To compare status we look at level .status.conditions[0].type.


We look at all conditions not just the first one correct? Suggest using status.conditions[].type if that is correct.

Yes, we do look for all conditions.

config/crd/bases/mcad.ibm.com_appwrappers.yaml

deployment/mcad-controller/templates/deployment.yaml

pkg/controller/queuejob/queuejob_controller_ex.go

dmatch01 · 2022-08-02T12:37:23Z

pkg/controller/queuejob/queuejob_controller_ex.go

+//ignore service and podgroups since these items do not have status
+if status == "ignore" {
+ignoredItems = ignoredItems + 1
+}


That's correct. We don't want to presume completion status unless the end user requests the AW generic item to be considered. This allows for end users to add run to completion objects that they specify. If the end user does not want the generic item to be considered for completion we have that flexibility. Please share your thoughts.

dmatch01 · 2022-08-02T12:43:10Z

pkg/controller/queuejob/queuejob_controller_ex.go

+countCompletionRequired := 0
+ignoredItems := 0
+for _, genericItem := range cqj.Spec.AggrResources.GenericItems {
+objectName := genericItem.GenericTemplate


See comment response below.

dmatch01 · 2022-08-02T12:48:50Z

pkg/controller/queuejob/queuejob_controller_ex.go

+//Set Appwrapper state to complete if all items in Appwrapper
+//are completed
+if awNew.Status.State != arbv1.AppWrapperStateCompleted {
+cc.GetAllObjectsOwned(awNew)
+}
+
+if awNew.Status.State == arbv1.AppWrapperStateRunningHoldCompletion {
+awNew.Status.QueueJobState = arbv1.AppWrapperCondRunningHoldCompletion
+cond := GenerateAppWrapperCondition(arbv1.AppWrapperCondRunningHoldCompletion, v1.ConditionTrue, "SomeItemsCompleted", "")
+awNew.Status.Conditions = append(awNew.Status.Conditions, cond)
+awNew.Status.FilterIgnore = true // Update AppWrapperCondCompleted
+cc.updateEtcd(awNew, "[syncQueueJob] setRunningHoldCompletion")
+}
+
+if awNew.Status.State == arbv1.AppWrapperStateCompleted {
+awNew.Status.QueueJobState = arbv1.AppWrapperCondCompleted
+cond := GenerateAppWrapperCondition(arbv1.AppWrapperCondCompleted, v1.ConditionTrue, "PodsCompleted", "")
+awNew.Status.Conditions = append(awNew.Status.Conditions, cond)
+awNew.Status.FilterIgnore = true // Update AppWrapperCondCompleted
+cc.updateEtcd(awNew, "[syncQueueJob] setCompleted")
+}
+


.status.state is being updated in the call cc.GetAllObjectsOwned(awNew). All state changes are done in managequeuejob(). Suggest moving all the logic to managequeuejob() to be consistent. The syncQueueJob() only update the .status.queuejobstate field.

dmatch01 · 2022-08-02T13:06:17Z

pkg/controller/queuejobresources/genericresource/genericresource.go

+//service and podgroup has no conditions return ALWAYS true
+//TODO: make it configurable?
+if gvk.GroupKind().Kind == "Service" || gvk.GroupKind().Kind == "PodGroup" {
+return "ignore"
+}
+


Ok, I see the challenge here. As part of the AW pod phases (pending/running/completed...) are tracked. If there is a check of the pods when setting state, this would be sufficient. Iterating over each generic item that is not tracked would not be necessary. only when all tracked items indicate completionstatus the state should be changed (of which is being done). The change here is that the decision to set state to RunningHoldCompletion or Complete is when all pods associated to AW are in Failed/Completed phase states. These counts are already tracked in the AW and can be referenced from the .status field. Please let me know your thoughts. Thanks!

dmatch01

I just realized this branch is on the multi-cluster repo. Could generate the PR from your fork and delete this branch? Thanks!

asm582 · 2022-08-03T12:08:58Z

I just realized this branch is on the multi-cluster repo. Could generate the PR from your fork and delete this branch? Thanks!
Yes, sure

asm582 · 2022-08-03T12:45:03Z

Closing the PR to create new PR from fork.

asm582 · 2022-08-05T22:21:53Z

Can you also add a failure test case for handling non-pod creating generic items? The easiest way is to duplicate one of the new cases and change the service object to include completionstatus. This will ensure that any end user inadvertently adding this field will not force a controller failure. Thanks!

The controller will not fail, If the user adds completionstatus to the service then AW status will be Complete as in the current design Service and Podgroups are ignored.

pod completion stat 2

45bddb0

asm582 changed the base branch from master to quota-management July 27, 2022 16:48

add missing method

bf06fe7

dmatch01 requested changes Aug 1, 2022

View reviewed changes

asm582 added 2 commits August 1, 2022 18:27

resolve review comment

d2cf4e4

review changes -1

2265d03

dmatch01 self-requested a review August 2, 2022 12:17

dmatch01 requested changes Aug 2, 2022

View reviewed changes

dmatch01 requested changes Aug 3, 2022

View reviewed changes

asm582 added 2 commits August 3, 2022 07:55

remove file path

3b73e9e

fix yaml description

a38079e

asm582 closed this Aug 3, 2022

asm582 deleted the pod_comp_stat_2 branch August 3, 2022 12:45

Uh oh!

Pod comp stat 2 #192

Pod comp stat 2 #192

Uh oh!

Conversation

asm582 commented Jul 27, 2022

dmatch01 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmatch01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmatch01 left a comment

Choose a reason for hiding this comment

asm582 commented Aug 3, 2022

asm582 commented Aug 3, 2022

asm582 commented Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Labels

3 participants

dmatch01 left a comment •

edited

Loading

asm582 commented Aug 5, 2022 •

edited

Loading