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

pod restart when init containers are changed #82567

Open
wants to merge 1 commit into
base: master
from

Conversation

@sofat1989
Copy link

commented Sep 11, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
A user updates the Pod specification, causing the init container image to change. Any changes to the init container image restarts the Pod. App container image changes only restart the app container.

Which issue(s) this PR fixes:

Fixes #
#82523
Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Welcome @sofat1989!

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. 😃

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Hi @sofat1989. 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.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sofat1989
To complete the pull request process, please assign tallclair
You can assign the PR to them by writing /assign @tallclair in a comment when ready.

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sofat1989 sofat1989 force-pushed the sofat1989:fixinitmaster branch from fcca083 to 045b8da Sep 11, 2019

@k8s-ci-robot k8s-ci-robot requested review from krmayankk and yujuhong Sep 11, 2019

@sofat1989 sofat1989 force-pushed the sofat1989:fixinitmaster branch 3 times, most recently from 33ec511 to eb461aa Sep 11, 2019

@mattjmcnaughton
Copy link
Contributor

left a comment

Thanks for your pr :)

I will give this a full look tomorrow, but as a jumping off point, could I ask you to add unit test coverage so we can be sure that this will continue to work going forward?

@krmayankk

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@kubernetes/sig-apps-pr-reviews for this review as well
/ok-to-test

expectedHash, actualHash, changed := containerChanged(container, status)
if changed {
klog.V(4).Infof("Init container spec hash changed (%d vs %d).", actualHash, expectedHash)
return nil, nil, false, true

This comment has been minimized.

Copy link
@krmayankk

krmayankk Sep 12, 2019

Contributor

should this logic live inside findNextInitContainerToRun ?

This comment has been minimized.

Copy link
@sofat1989

sofat1989 Sep 16, 2019

Author

leaving this logic here is to re-use the for loop inside findNextInitContainerToRun. I suppose that findNextInitContainerToRun is to compute next actions for init containers. So i think it is fine that we have hash checking here.

@krmayankk

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Agree we need a UT. Before that I am not sure this is the desired behavior . CC'ing some people to get their review.

/assign @Random-Liu @tallclair

@sofat1989 sofat1989 force-pushed the sofat1989:fixinitmaster branch from eb461aa to 592cb60 Sep 16, 2019

@sofat1989

This comment has been minimized.

Copy link
Author

commented Sep 16, 2019

@mattjmcnaughton @krmayankk Thanks for reviewing. I add one unit test. When the restart policy of the pod is Always, if the init container's image is changed, the we will get killPod which is true.

@sofat1989

This comment has been minimized.

Copy link
Author

commented Sep 17, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.