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

scheduling-framework.md: update Reserve and Unreserve descriptions #22035

Closed
wants to merge 193 commits into from

Conversation

adtac
Copy link
Member

@adtac adtac commented Jun 24, 2020

This doc update depends on the following API changes:

tl;dr: Reserve and Unreserve plugins have been merged into one plugin called a Reserve plugin. These plugins must implement both extension points. Also, since reserve extension points may fail, after kubernetes/kubernetes#92391, we will trigger the unreserve extension point in the event of such failures. The extension points are no longer merely informational; they can have state changes + side effects such as allocating resources.

/assign @alculquicondor
/cc @ahg-g
/sig scheduling
/hold

Enrique Medina Montenegro and others added 30 commits June 12, 2019 16:56
Add warning to avoid sharing security groups between multiple services in the `service.beta.kubernetes.io/aws-load-balancer-security-groups` annotation
Signed-off-by: Weiping Cai <weiping.cai@daocloud.io>
…s/docs/concepts_workloads_controllers/jobs-run_and_replicaset
Some words in spanish weren't translated correctly.
--dry-run is deprecated

`--restart=Never` flag is unnecessary to create a pod from 1.18
imagePullPolicy when defined without a value defaults to Always not to IfNotPresent
Update docs for new container- Makefile targets
update to right styling for VolumeSnapshotClass concept
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Jun 24, 2020
@netlify
Copy link

netlify bot commented Jun 24, 2020

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 81b50a6

https://deploy-preview-22035--kubernetes-io-master-staging.netlify.app


The reserve extension point happens before the scheduler actually binds a Pod to
a node and its uses are three-fold. First, it serves as a hook to allocate
resources in the designated node that the scheduled Pod might need. Second, it
Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be used to allocate resources. Such actions are usually costly and should be done in PreBind.

Copy link
Member

Choose a reason for hiding this comment

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

We should also preserve the phrasing "Plugins which maintain runtime state (aka "stateful plugins") should use this extension point"

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the volume binding plugin an exception then? I'm now confused as to whether reserve plugins are purely informational or if they can have side effects other than cycle state changes.

Copy link
Member

Choose a reason for hiding this comment

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

Volume binding plugin only updates its cache. It doesn't bind the volumes. That's done in PreBind.

serves as an informational plugin which maintain runtime state (aka "stateful
plugins") to be notified by the scheduler when resources on a node are being
reserved for a given Pod. Third, it exists to prevent race conditions while the
scheduler waits for the bind to succeed. This is the penultimate step in the
Copy link
Member

Choose a reason for hiding this comment

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

Why do you say penultimate?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's also a permit extension point in the scheduling cycle -- reserve is not the last step.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or is permit a part of the binding cycle? The flow chart in this doc says it's a part of the scheduling cycle, but your other comment says it's a part of the binding cycle.

Copy link
Member

@alculquicondor alculquicondor Jun 24, 2020

Choose a reason for hiding this comment

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

Oh, I forgot about kubernetes/kubernetes#88199. Please make sure the doc for Permit is up to date. I believe we did update it.


The reserve extension point may fail or succeed. If it fails, the
[Unreserve](#unreserve) extension point is triggered. Alternatively, if it
succeeds, the Pod is deemed to be in the reserved state, at which point,
Copy link
Member

Choose a reason for hiding this comment

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

We are actually packing information pertaining to the other extension points here.

It should be more like:
"If it succeeds, the binding routine is initiated, where Permit is the first extension point to run."

@sftim
Copy link
Contributor

sftim commented Jun 24, 2020

Hi @adtac

It looks to me as if you wanted to target docs for the next release? If that's right, you should revise this pull request and choose dev-1.19 as the target branch.

/hold
until clarified

@alculquicondor
Copy link
Member

Oops, missed that. Yes, this should target dev-1.19

@sftim
Copy link
Contributor

sftim commented Jun 24, 2020

/milestone 1.19

@k8s-ci-robot k8s-ci-robot added this to the 1.19 milestone Jun 24, 2020
@adtac adtac changed the base branch from master to dev-1.19 June 24, 2020 18:58
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 24, 2020
@k8sio-netlify-preview-bot
Copy link
Collaborator

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit cbdd710

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5ef3a2913ef4cf000877947c

@k8s-ci-robot k8s-ci-robot added language/es Issues or PRs related to Spanish language language/fr Issues or PRs related to French language language/id Issues or PRs related to Indonesian language labels Jun 24, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign bradtopol
You can assign the PR to them by writing /assign @bradtopol in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added language/it Issues or PRs related to Italian language language/ja Issues or PRs related to Japanese language language/ko Issues or PRs related to Korean language language/pl Issues or PRs related to Polish language language/ru Issues or PRs related to Russian language language/zh Issues or PRs related to Chinese language labels Jun 24, 2020
@adtac adtac closed this Jun 24, 2020
@adtac adtac deleted the adtac/reserve branch June 24, 2020 19:02
@adtac adtac restored the adtac/reserve branch June 24, 2020 19:02
@adtac
Copy link
Member Author

adtac commented Jun 24, 2020

welp, github just autoclosed the PR :(

I'll create a new one with the correct target branch I guess

@alculquicondor
Copy link
Member

/reopen

@k8s-ci-robot
Copy link
Contributor

@alculquicondor: Failed to re-open PR: state cannot be changed. The adtac/reserve branch has been deleted.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. language/en Issues or PRs related to English language language/es Issues or PRs related to Spanish language language/fr Issues or PRs related to French language language/id Issues or PRs related to Indonesian language language/it Issues or PRs related to Italian language language/ja Issues or PRs related to Japanese language language/ko Issues or PRs related to Korean language language/pl Issues or PRs related to Polish language language/ru Issues or PRs related to Russian language language/zh Issues or PRs related to Chinese language sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet