-
Notifications
You must be signed in to change notification settings - Fork 4.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
Improve deployment scaling behavior #5875
Conversation
This is going to require a lot of review and testing. |
The way this is coded right now, replica counts for failed deployments won't be reconciled until the deployment config controller full resync interval, since there is no update to the deploymentConfig which would trigger a watch event. If that's a problem, we could do something like update an annotation or status of the deploymentConfig when a deployment fails (just some off the cuff examples). |
// If the latest deployment already exists, reconcile replica counts. | ||
if latestDeploymentExists { | ||
// If the latest deployment exists and is still running, there's nothing | ||
// to do. |
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.
We could change the target replicas annotation here so that the rolling deployer shoots for the updated version
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.
The rolling updater code (upstreamed) won't consider mid-flight changes to the target replica count. The complexity involved in making it do so at this point is probably more than we're willing to take on given our push to use upstream deployments (which does support mid-steam scale changes.)
You should drop in a commit that makes the DC scale subresource use this, so we can test how well it works with the HPA |
|
||
// Compute the desired replicas for the deployment. Use the last completed | ||
// deployment's current replica count, or the config template if there is no | ||
// prior completed deployment available. | ||
desiredReplicas := config.Template.ControllerTemplate.Replicas |
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.
yay!:)
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 would like a big fat comment above explaining what has changed since this pretty big.
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.
Did a ton of simplifying refactoring in here, PTAL. Holding off on unit tests until I do some more manual testing.
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 also ripped out the status update code since it'll be reintroduced in a refactored form with #5530
ps. this definitely needs a lot of testing. Will start from today. |
Completely redid the deploymentConfigController unit test. |
3a18a3c
to
8e46900
Compare
If you trigger a fake config change then you would have the config controller be another entrypoint for scaling? I don't like it. I don't like that we have this interval for the dc controller, too:) What is stopping us from having a controller that will do both? Reconciling at intervals and on config changes too? |
Any updates on this? |
I am trying to implement this locally and based on this comment, I think we may want to add a new field in the deploymentConfig that will keep track of all the replicas that ever existed. 1) We would get closer to upstream deployments with this change and 2) we would compute this only in the controller where we already list all the deployments in every dc reconclilation (so no need for origin/pkg/deploy/registry/deployconfig/etcd/etcd.go Lines 243 to 260 in 0c0e452
|
The config controller is still the scaling controller. What I mean is, due to our pod based deployment mechanism, there are times when we want to reconcile the DCs in response to non-DC updates. In this case, when the deployer pod reaches a terminal state, we should reconcile the owning DC. The question is how to trigger that reconcile other than waiting for the resync interval since it's not the DC that changed, but a related resource (the deployer pod). |
I agree that the scale subresource should be updated along with this PR, will add it to my list of TODOs. |
Ah, makes sense now. Agreed that a mocked config change would fit here. On Mon, Nov 16, 2015 at 3:48 PM, Dan Mace notifications@github.com wrote:
|
Agree, but is there any reason that has to be bundled with this PR? |
No reason at all, I just wanted to discuss it. I can work on a separate PR and add it once this PR lands. |
8e46900
to
224d98d
Compare
I would be fine with marking the owning dc with an annotation while marking the deployment as failed. |
config.Details = new(deployapi.DeploymentDetails) | ||
// No deployments are running and the latest deployment doesn't exist, so | ||
// create the new deployment. | ||
deployment, err := deployutil.MakeDeployment(config, c.codec) |
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 utility doesn't propagate the config namespace to the deployment resulting in errors when the controller is trying to create a new deployment.
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.
Good catch, thanks
I have been testing this locally including the fix I commented above and so far it works as expected. It takes the interval but older deployments are scaled down as expected. Still no stress testing though. One question: since the hpa controller now will target the dc template, the only way to scale an older deployment is by manually scaling it, so somebody has to specifically run |
If you tried to scale the old RC manually, the deployment controller would scale it down when reconciling. As a user trying to scale up an old deployment, do you really just want a rollback? |
c3be907
to
bb2c6ff
Compare
Can we think more about this and address it in a followup? |
Should we flesh out better e2e and extended tests separately in #5879? |
[test] |
@openshift/ui-review - want to make sure that the console is using the scaling API endpoint for DCs rather than manipulating RCs directly. |
Evaluated for origin test up to d970f0a |
Testing locally with a new client and so far it works fine. Will test with an older client too. And I still want to have one more look in unit tests. |
latest flake: #6176 |
|
The desired replica count for the new deployment is now driven entirely by Guess we'll have to think more about that. Whack-a-mole continues. On Wed, Dec 2, 2015 at 5:18 PM, Michail Kargakis notifications@github.com
|
If you're rapid-fire wham-banging a scale followed by a deployment using an old client, I don't think there's much we can do to avoid the scenario from @Kargakis. I don't think it's a blocker to moving forward, and should be part of the education/documentation/release notes (tl;dr: upgrade your client). |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7620/) |
I was hoping this wouldn't work on master but it is :) :(
|
@Kargakis why the frown? |
Because we wouldn't really care if we didn't change any behavior i guess |
@Kargakis I'm confused... doesn't your most recent example (from master) show what we want? A scale to 3 immediately followed by a deployment results in 3 replicas. Am I missing something? |
I'm frowning because the same example using an old client and the server changes in this PR doesn't work... at the end of the day |
Since it seems like a pretty rare scenario, I'm certainly fine documenting On Wed, Dec 2, 2015 at 5:43 PM, Andy Goldstein notifications@github.com
|
So the user impact in the case of new server/old client is your quick scale/deployment isn't effective, and you have to run scale once more after the deployment finishes. Accurate? Still seems like a pretty rare event. If you're manually scaling and observe the bug, scale once more. I'd be more concerned if we had this sort of issue with the auto scaler, but that'll be fine. |
I am fine with moving on with this. The design of the controller is more robust than before and I don't perceive the issue I reported as a blocker, supposing we have docs mentioning it. |
Do we need any additional reviews? @smarterclayton @liggitt @deads2k @DirectXMan12 ? |
ObjectMeta: kapi.ObjectMeta{ | ||
Name: dc.Name, | ||
Namespace: dc.Namespace, | ||
CreationTimestamp: dc.CreationTimestamp, |
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 timestamp looks unusual. Why would we be forcing a CreationTimestamp
on an object? Doesn't that happen server-side in FillObjectMetaSystemFields
?
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.
Since Scale objects don't have the normal object lifecycle (they're never persisted, they don't use a strategy, etc), I don't think so.
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.
Actually, who uses this field at all? And if we're going to have it, why is it useful to be the dc.CreationTimestamp?
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.
And if we're going to have it, why is it useful to be the dc.CreationTimestamp?
Because this is a dc subresource and it's supposed to carry information about the main resource. Probably. Already upstream: https://github.com/kubernetes/kubernetes/blob/8c182c2713ea6e1b8ffff1da11e0d802cacd0bd8/pkg/apis/extensions/helpers.go#L75
I'll take one last look-through, but it should be all set. |
@@ -123,68 +114,6 @@ func (c *DeployerPodController) Handle(pod *kapi.Pod) error { | |||
return nil | |||
} | |||
|
|||
func (c *DeployerPodController) cleanupFailedDeployment(deployment *kapi.ReplicationController) error { |
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 is probably obvious if you know the controller better, but let's say I have a DC that has a PodSpec that grabs a NodePort. The post-deploy hook fails. Does this leave me with an running Pod that is claiming that NodePort instead of cleaning it up?
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.
If the deployment fails, the RC is scaled down to 0 eventually no matter what, so there won't be any lingering pods. Make sense?
I think for anybody not very familiar with the code, validating the controller's test cases might be the most productive way to review. Behavioral issues in terms of scenarios are the primary concern at this point. |
@Kargakis please tag this if you agree it's ready. |
LGTM |
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4261/) (Image: devenv-rhel7_2896) |
Evaluated for origin merge up to d970f0a |
BTW the tests you added here are phenomenally clear. Excellent job. |
This change makes the DeploymentConfigController solely responsible for scaling RCs owned by a deployment config which aren't currently being manipulated by a deployer process. RCs owned by the deployment config will be reconciled so that the active deployment matches the config replica count and all other RCs are scaled to 0.
Rationale:
This also adds a variety of new events to provide insight into what the controller is doing.
Fixes #5597
TODO