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

Binding annotations should be preserved alongside Host assignment #4103

Closed
jdef opened this issue Feb 4, 2015 · 5 comments · Fixed by #5005
Closed

Binding annotations should be preserved alongside Host assignment #4103

jdef opened this issue Feb 4, 2015 · 5 comments · Fixed by #5005
Labels
area/api Indicates an issue on api area. area/extensibility priority/backlog Higher priority than priority/awaiting-more-evidence. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.

Comments

@jdef
Copy link
Contributor

jdef commented Feb 4, 2015

Recap of related tangent in #2483 follows.

Use case: I need to implement mesos task reconciliation in the kubernetes-mesos scheduler. If it crashes and then restarts, I want to be able to recover things like taskID, slaveID, offerID, etc. so as to rebuild the internal state of the scheduler. Once state is recovered, I can reconcile the against the tasks that the mesos master knows about.

If I can't use binding annotations for this, then I need to store state some other (super inconvenient) way. Currently the etcd registry assignPod() implementation drops binding annotations on the floor. It should not, and instead they should be preserved for later access.

Proposal: Store binding-related annotations in a new field: Pod.Status.Annotations

  • When setting Pod.Status.Host="{some-host-name}" via assignPod(), binding annotations are copied in from Binding
  • When cloning Pod.Status.Host in UpdatePod() (for an already-scheduled pod), the Pod.Status.Annotations are also cloned.

I don't think /bindings has to be a readable apiserver endpoint, as long as the binding metadata is exposed some other way, as proposed above.

I'd also like to propose that once binding annotations are "bound" to Pod.Status they become read-only. So they're essentially write-once, just like Pod.Status.Host is meant to be.

cc @smarterclayton @bgrant0607 @erictune
xref d2iq-archive/kubernetes-mesos#73

@bgrant0607 bgrant0607 added area/api Indicates an issue on api area. area/extensibility sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Feb 4, 2015
@bgrant0607
Copy link
Member

There is one situation where we probably want to clear Host, actually: in the case that the pod can't actually be materialized on the selected node for some reason, such as #4135.

@davidopp davidopp added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Feb 8, 2015
@jdef
Copy link
Contributor Author

jdef commented Feb 12, 2015

Any objection to a PR implementing my proposal? I'm happy to do the work.

@smarterclayton
Copy link
Contributor

You'll probably want to do it on top of #4248 (or after it lands) since that code path is changing significantly.

On Feb 12, 2015, at 12:18 AM, James DeFelice notifications@github.com wrote:

Any objection to a PR implementing my proposal? I'm happy to do the work.


Reply to this email directly or view it on GitHub.

@bgrant0607
Copy link
Member

Though it is also represented in PodStatus, Host is properly a field in the PodSpec.

Status must be completely reconstructable based on observations. The proposed Pod.Status.Annotations would not be. Furthermore, I don't think we should introduce another annotations field to Pod. I'd prefer to add a podAnnotations field to Binding, and then merge them into the pod. As for clearing the annotations, I'd prefer to simply fail a pod that was rejected by a node so as not to have to deal with that.

Also note the discussion here re. moving binding to a pod subresource: #2726 (comment)

@bgrant0607
Copy link
Member

See also #4375.

akram referenced this issue in akram/kubernetes Apr 7, 2015
clarify resource conflict status, rebase to master
remove ResourceConflict, replace usage with Conflict
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue on api area. area/extensibility priority/backlog Higher priority than priority/awaiting-more-evidence. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants