-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Make replica controllers return current state. #1248
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,7 @@ func (rs *REST) Get(id string) (runtime.Object, error) { | |
if err != nil { | ||
return nil, err | ||
} | ||
rs.fillCurrentState(controller) | ||
return controller, err | ||
} | ||
|
||
|
@@ -104,6 +105,7 @@ func (rs *REST) List(selector labels.Selector) (runtime.Object, error) { | |
filtered := []api.ReplicationController{} | ||
for _, controller := range controllers.Items { | ||
if selector.Matches(labels.Set(controller.Labels)) { | ||
rs.fillCurrentState(&controller) | ||
filtered = append(filtered, controller) | ||
} | ||
} | ||
|
@@ -147,7 +149,11 @@ func (rs *REST) Watch(label, field labels.Selector, resourceVersion uint64) (wat | |
} | ||
return watch.Filter(incoming, func(e watch.Event) (watch.Event, bool) { | ||
repController := e.Object.(*api.ReplicationController) | ||
return e, label.Matches(labels.Set(repController.Labels)) | ||
match := label.Matches(labels.Set(repController.Labels)) | ||
if match { | ||
rs.fillCurrentState(repController) | ||
} | ||
return e, match | ||
}), nil | ||
} | ||
|
||
|
@@ -164,3 +170,15 @@ func (rs *REST) waitForController(ctrl *api.ReplicationController) (runtime.Obje | |
} | ||
return ctrl, nil | ||
} | ||
|
||
func (rs *REST) fillCurrentState(ctrl *api.ReplicationController) error { | ||
if rs.podLister == nil { | ||
return nil | ||
} | ||
list, err := rs.podLister.ListPods(labels.Set(ctrl.DesiredState.ReplicaSelector).AsSelector()) | ||
if err != nil { | ||
return err | ||
} | ||
ctrl.CurrentState.Replicas = len(list.Items) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this pattern where apiserver fills in the current state intended to be permanent? If so, I think we should not store the CurrentState field in etcd. Otherwise, we should have some component that updates the thing that is stored in etcd and apiserver shouldn't do this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like, as implemented here you couldn't watch for this. I think it's totally reasonable to want to watch for this data instead of polling for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really don't want to write transient state into etcd. If we want to have a background poll which updates this in memory, and then we watch that, I'd be ok with that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it leads to lots of unnecessary write load on the data store, and also, it makes compareAndSwap a lot trickier, because suddenly there are writes you care about, and writes you don't care about, or else you have a bunch of unnecessary conflicts. I think its better hygiene to keep etcd/persistent store the domain of desired state, and current state is always dynamically obtained from the source (possibly with a caching layer for performance) |
||
return nil | ||
} |
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.
O(N * M) scaling.
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.
Yep, do we care? This is all pretty local.
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.
I don't think we care at the moment, but stuff like this will keep us from scaling out of the 100's of pods and rep. controllers.