Wait for old pods to terminate before proceeding to Recreate #11917
Conversation
|
[test] |
| } | ||
| return deletionsNeeded == 0, nil | ||
| } | ||
| // TODO: Timeout should be config.Spec.Strategy.RecreateParams.TimeoutSeconds - (time.Now - deployerPodCreationTime) |
There was a problem hiding this comment.
Opened kubernetes/kubernetes#36813 for this. There is another occurence in the deployer code where we need the creation time of the deployer pod (when we set activeDeadlineSeconds for hooks).
| // Watch from the resource version of the list and wait for all pods to be deleted | ||
| // before proceeding with the Recreate strategy. | ||
| options.ResourceVersion = podList.ResourceVersion | ||
| w, err := s.podClient.Pods(from.Namespace).Watch(options) |
There was a problem hiding this comment.
the watch will handle just 1000 events then it drops
There was a problem hiding this comment.
Unless you run 1000+ pods in one deployment, the deployer scales them down, the replication controller reaches rc.spec.replicas == rc.status.replicas == 0 (which means all of them have been marked for deletion) but >1000 pods are not deleted yet.
There was a problem hiding this comment.
1000 watch events across all pods in all namespaces (or fewer if the watch cache is configured for a smaller number of events, or if the watch cache is disabled across all resources in all namespaces)... if dropping this watch fails the deployment, you need to handle re-establishment
There was a problem hiding this comment.
This watch runs every time for a single deployment.
There was a problem hiding this comment.
And watches only the pods of that replication controller
There was a problem hiding this comment.
doesn't matter, 1000 event limit still applies across all pods, even when the watch is filtered down. what happens if the watch gets dropped here?
There was a problem hiding this comment.
The rollout fails. We can ignore watch errors I guess (fallback to the old behavior).
| return deletionsNeeded == 0, nil | ||
| } | ||
| // TODO: Timeout should be config.Spec.Strategy.RecreateParams.TimeoutSeconds - (time.Now - deployerPodCreationTime) | ||
| config, err := deployutil.DecodeDeploymentConfig(from, s.decoder) |
There was a problem hiding this comment.
just pass the timeout in the function, no need to decode twice?
openshift-bot
added
needs-rebase
| eventClient: client.Core(), | ||
| getUpdateAcceptor: func(timeout time.Duration, minReadySeconds int32) strat.UpdateAcceptor { | ||
| return stratsupport.NewAcceptNewlyObservedReadyPods(out, client.Core(), timeout, AcceptorInterval, minReadySeconds) | ||
| }, | ||
| scaler: scaler, | ||
| decoder: decoder, | ||
| hookExecutor: stratsupport.NewHookExecutor(client.Core(), tagClient, client.Core(), os.Stdout, decoder), | ||
| // TODO: Should be config.Spec.Strategy.RecreateParams.TimeoutSeconds - (time.Now - deployerPodCreationTime) |
There was a problem hiding this comment.
do we have follow up issue created for this?
| defer w.Stop() | ||
| // Observe as many deletions as the remaining pods and then return. | ||
| deletionsNeeded := len(podList.Items) | ||
| condition := func(event watch.Event) (bool, error) { |
There was a problem hiding this comment.
do we need to verify the object in the event?
There was a problem hiding this comment.
I don't think so, we should always get pods back.
| } | ||
| return deletionsNeeded == 0, nil | ||
| } | ||
| // TODO: Timeout should be timeout - (time.Now - deployerPodStartTime) |
There was a problem hiding this comment.
issue pls and assigne to me :-)
There was a problem hiding this comment.
Basically can you handle both of these as part of #12154?
|
@Kargakis rebase and LGTM |
openshift-bot
added
the
needs-rebase
openshift-bot
removed
the
needs-rebase
|
#12157 [test] |
|
flake is #12157 |
|
[merge] |
|
yum [merge] |
|
#8571 [merge] |
|
#8571 [merge] |
|
#8502 [merge] |
|
#8502 [merge] |
|
[test] |
|
Evaluated for origin test up to 2a434d1 |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12285/) (Base Commit: 9ee2ff6) |
|
#10988 [merge] |
|
Evaluated for origin merge up to 2a434d1 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12318/) (Base Commit: eb304fd) (Image: devenv-rhel7_5531) |
@mfojtik should fix https://bugzilla.redhat.com/show_bug.cgi?id=1369644
cc: @smarterclayton opted for this instead of having a TerminatingReplicas field in the replication controller.
Upstream equivalent PR is kubernetes/kubernetes#36748