Welcome @huutomerkki!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

Hi @huutomerkki. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

thanks for your PR. The two obvious questions are: how can we make sure this change doesn't break existing flows (e.g. do we have sufficient test coverage?) ? since this change is admittedly partially addressing a problem, how can we assess the gains?

/triage accepted
/priority important-longterm

/ok-to-test

thanks for your PR. The two obvious questions are: how can we make sure this change doesn't break existing flows (e.g. do we have sufficient test coverage?) ? since this change is admittedly partially addressing a problem, how can we assess the gains?

Thanks for checking it out and sorry for the delay in answering.

For the first question, for a flow to be affected, it has to use the order in which Kubelet tries to start the pods for something and make assumptions based on it. This order is not promised to be the actual starting order of pods, so I would assume there is rather limited amount of valid use cases for this. If there is some obvious one I'm missing, I'd be more than happy to write tests to cover it, same goes for editing the test cases that exist to cover this change if viewed necessary.

For assessing the gains of this change; I'll try to get my hands on some data that I can share. Without it it's hard to assess the gains, but I'd like to point out that this change is aimed for larger and heavier applications, and there most likely is not any noticeable benefit in applications consisting of just a handful of pods. This change assures that the order in which pods are tried to start is always optimal, which might not be too visible in action. If nothing else, it should make the restart process and times more uniform and predictable.

@ffromani do you have any other concerns on this patch ?

/retest

For the first question, for a flow to be affected, it has to use the order in which Kubelet tries to start the pods for something and make assumptions based on it. This order is not promised to be the actual starting order of pods, so I would assume there is rather limited amount of valid use cases for this. If there is some obvious one I'm missing, I'd be more than happy to write tests to cover it, same goes for editing the test cases that exist to cover this change if viewed necessary.

For assessing the gains of this change; I'll try to get my hands on some data that I can share. Without it it's hard to assess the gains, but I'd like to point out that this change is aimed for larger and heavier applications, and there most likely is not any noticeable benefit in applications consisting of just a handful of pods. This change assures that the order in which pods are tried to start is always optimal, which might not be too visible in action. If nothing else, it should make the restart process and times more uniform and predictable.

Hey! do we have updates about the gains this PR brings? While I'm generally in favor of this PR, it admittedly partially addresses a problem touching a critical and not-strongly-tested kubelet flow, so having solid gains will be a big plus towards the merge.

Other than that, I think we should consider adding e2e coverage. We can start with minimal non-regression coverage, i for myself would call that sufficient.

In overall idea looks ok, but generic question: for 2 pods with same priority, should it be still sorted by creation time?

I want to make sure to reflect one point we discussed at SIG Node today. With this change, if kubelet restart happens at precise moment when a new Pod is scheduled, we will have a big change in behavior - instead of eviction message we will get a pod admission failure. This change of behavior will be confusing.

To mitigate this risk, can you please write down all conditions need to be met for this to happen. Is it probable? Is it likely to happen in specific circumstances (e.g. in a deployment one pod is restarting kubelet to apply a new config and it results in a more critical pod scheduled).

Maybe the right fix is to change ordering for all pods that were running before and keep the new ones to the end. Maybe some other solution like this will help.

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: huutomerkki
Once this PR has been reviewed and has the lgtm label, please assign tallclair for approval. For more information see the Kubernetes Code Review Process.

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

Adding the work I've done so far here as I changing org. Had some issues with rebasing so there's some accidental labels on here, feel free to remove if you know how.

Current status of the e2e test is that the pods are not being restarted, would love to have another set of eyes to tell me why?

In overall idea looks ok, but generic question: for 2 pods with same priority, should it be still sorted by creation time?

I see no reason to use other method to determine that, as 2 pods with same priorities should be interchangeable in starting order.

I want to make sure to reflect one point we discussed at SIG Node today. With this change, if kubelet restart happens at precise moment when a new Pod is scheduled, we will have a big change in behavior - instead of eviction message we will get a pod admission failure. This change of behavior will be confusing.

To mitigate this risk, can you please write down all conditions need to be met for this to happen. Is it probable? Is it likely to happen in specific circumstances (e.g. in a deployment one pod is restarting kubelet to apply a new config and it results in a more critical pod scheduled).

Maybe the right fix is to change ordering for all pods that were running before and keep the new ones to the end. Maybe some other solution like this will help.

Can you elaborate on this? I don't understand why the behaviour would change.

@huutomerkki: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.