-
Notifications
You must be signed in to change notification settings - Fork 65
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
YUNIKORN-462: Streamline core to shim update on allocation change #58
Conversation
In addition to changes discussed offline, have also made changes for synchronous release allocations in a separate flow as I thought of not disturbing the other use cases (trigger points) which are ok with async communication. This kind of synchronous release allocations is required only for processAllocationReleases() method because call to ForgotPod() happens only in this flow through ReSyncSchedulerCache plugin. Once overall approach makes sense, will need to work on unit tests |
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.
not sure about the AllocationRelease message changes.
scheduler-interface-spec.md
Outdated
// AllocationKey from AllocationAsk | ||
string allocationkey = 6; |
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 should already be covered in the message as the UUID. Otherwise we currently would not the possibility to release a pod that is not allocated yet (which is what an ask is).
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.
AssumePod/ForgetPod expects allocationKey to do its cache add/remove operation. Currently, ReSyncSchedulerCache plugin wraps AllocationKey and few more fields under AssumedAllocation/ForgotAllocation message from core side and sends to shim which eventually passes to AssumePod/ForgetPod methods. Now, with the removal of ReSyncSchedulerCache plugin, in this PR, added AllocationKey in AllocationRelease message as AllocationRelease is being passed to shim (as AllocationResponse) through event processing.
scheduler-interface-spec.md
Outdated
// update cache, default is false | ||
bool updateCache = 7; |
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.
Can you explain why we would ever not want to update the cache. Would that not cause issues with an out of sync cache?
The cache should be idempotent and update when needed:
- delete can never fail: if the entry does not exist the result is the same as a successful delete
- for creates and updates we should do a create OR update call with the end result being a consistent cache representing the correct object.
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.
context#processAllocationReleases is the only place where ReSyncSchedulerCache plugin is used to call ForgetPod in shim side for cache updates in addition to sending RMReleaseAllocationEvent event. RMReleaseAllocationEvent uses AllocationRelease message. AllocationRelease message is being used in few more places (for ex, context#handleRMUpdateApplicationEvent, node decommission etc) in addition to context#processAllocationReleases in core side. Hence, to help shim whether to do cache operation or not while processing UpdateAllocation callback method, used "updateCache" field
Sorry that it has taken so long to get back to you :-( We have one change in mind remove the
First new allocations: the message we send is a RMNewAllocationsEvent with all allocations that are new. That event is currently async. That is why we first we call a sync cache update with partial info. The change is that the event will become a sync call. We have enough information in the current Allocations array. This is part of the event we send and we can also call the assume of the pod inside the cache on the shim side by pulling that key from the allocation. We always call assume pod for every new allocation. Simple change on the SI side: we can remove the On the remove side we have 4 locations where we send a RMReleaseAllocationEvent. Only in one location, as you pointed out, we also call the sync of the cache. The sync of the cache triggers the forget of an assumed pod. Only looking at the path that sends events back. The cache sync is part of one of these calls.
Simple change on the SI side: we can remove the The core sends the events synchronously. The shim collapses the assume call into the event processing for new allocations, returns as soon as possible forking of the long running tasks. The shim collapses the forget call into the event processing for the remove. If there is any special cases to not forget the assumption of a removed pod then the shim must implement it. The core should not be the one that decides this. After this we also need to completely remove the |
4274a45
to
62bb2e8
Compare
Thanks for your detailed explanation and giving me a clear picture. Have taken care of SI and Core accordingly. As discussed, once YUNIKORN-876 goes in, we can work on the shim. Summary is, We should call clear the cache on shim side for all these 4 calls. Since we need to clear cache in all 4 calls, then probably we should make all these calls sync? |
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
What is this PR for?
SI changes to remove ReSyncSchedulerCache plugin and added new fields in AllocationRelease message
What type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-462
How should this be tested?
Screenshots (if appropriate)
Questions: