API changes for client-initiated termination reason #45504

yguo0905 · 2017-05-08T18:04:48Z

What this PR does / why we need it:

This PR contains the API changes for kubernetes/community#541.

Special notes for your reviewer:

The logic that generates and delivers the termination reason in kubectl and kubelet will be in separate PRs.

Release note:

NONE

k8s-reviewable · 2017-05-08T18:04:54Z

This change is

k8s-github-robot · 2017-05-08T18:05:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yguo0905
We suggest the following additional approver: dchen1107

Assign the PR to them by writing /assign @dchen1107 in a comment when ready.

Needs approval from an approver in each of these OWNERS Files:

api/OWNERS
cmd/libs/go2idl/client-gen/OWNERS
federation/OWNERS
pkg/api/OWNERS
pkg/controller/OWNERS
pkg/kubelet/OWNERS
staging/src/k8s.io/apimachinery/pkg/OWNERS
staging/src/k8s.io/apiserver/OWNERS
staging/src/k8s.io/client-go/pkg/api/OWNERS
staging/src/k8s.io/kube-aggregator/OWNERS
test/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

yguo0905 · 2017-05-08T18:05:35Z

@kow3ns

ncdc · 2017-05-22T17:25:20Z

pkg/api/defaulting_test.go

@@ -160,7 +160,7 @@ func TestDefaulting(t *testing.T) {
 				if !expectedChanged || changedOnce {
 					break
 				}
-				if iter > 300 {
+				if iter > 500 {


Why this change?

The test generated 300 objects, which were enough to cover all the fields with default values. Since this change added nested fields to Lifecycle, we now need more permutations to be generated for the test objects in order to ensure that the additional fields with default values are covered. This happened before in #39181. I will create a separate issue for a real fix.

ncdc · 2017-05-22T17:30:23Z

pkg/api/types.go

@@ -1595,6 +1595,45 @@ type Handler struct {
 	TCPSocket *TCPSocketAction
 }

+// DeleteExecAction describes a "run in container" action that will be invoked
+// inside of a container prior to sending the TERM signal to the container's


Note that now that #45236 has merged, the PID namespace is shared by default. I don't know that saying it sends TERM to the container's entrypoint makes sense any more. WDYT?

Can you please elaborate on what the shared PID namespace implies here? How does it relate to the termination procedure?

It means that if you have a pod with multiple containers, the PID namespace is shared among all the containers. Before this change, each container's PID 1 was its entrypoint, and if you killed PID 1 in a single container, that container would die, but the other containers in the pod would be unaffected. With a shared PID namespace, PID 1 is owned by the pod sandbox (is that the right terminology @derekwaynecarr?), and each container's entrypoint has some unique PID other than 1. If you kill PID 1 in the shared PID namespace, you will bring down all the containers for that pod.

Thanks. So the issue here is the "entrypoint"? Removed it in the latest commit.

ncdc · 2017-05-22T17:30:40Z

pkg/api/types.go

@@ -1595,6 +1595,45 @@ type Handler struct {
 	TCPSocket *TCPSocketAction
 }

+// DeleteExecAction describes a "run in container" action that will be invoked


Since the type is DeleteExecAction do you want to say that it describes an "exec in container" action?

The Delete indicates it's an ExecAction plus a deletion reason string. Made it clear in the comment.

ncdc · 2017-05-22T17:31:41Z

pkg/api/types.go

+	ReasonHeader string
+}
+
+// PreStopHandler invokes either a DeleteExecAction, a DeleteHTTPGetAction or a


comma after DeleteHTTPGetAction

ncdc · 2017-05-22T17:31:52Z

pkg/api/types.go

+
+// PreStopHandler invokes either a DeleteExecAction, a DeleteHTTPGetAction or a
+// TCPSocketAction prior to the graceful termination of a Pod. One and only one
+// of the fields should be specified.


s/should/may/

ncdc · 2017-05-22T17:33:39Z

pkg/api/v1/meta.go

@@ -51,6 +51,9 @@ func (meta *ObjectMeta) GetDeletionGracePeriodSeconds() *int64 { return meta.Del
 func (meta *ObjectMeta) SetDeletionGracePeriodSeconds(deletionGracePeriodSeconds *int64) {
 	meta.DeletionGracePeriodSeconds = deletionGracePeriodSeconds
 }
+func (meta *ObjectMeta) GetDeletionReason() string { return "" }


Are these supposed to be left unimplemented?

I think so. The ObjectMeta in pkg/api/v1/meta.go is deprecated and we add the two function here to make it satisfy the interface.

ncdc · 2017-05-22T17:39:19Z

pkg/api/validation/validation.go

+	allErrors := field.ErrorList{}
+	if handler.Exec != nil {
+		if numHandlers > 0 {
+			allErrors = append(allErrors, field.Forbidden(fldPath.Child("exec"), "may not specify more than 1 handler type"))


Not sure how you'd ever hit this

Removed this condition.

ncdc · 2017-05-22T17:48:02Z

pkg/kubelet/pleg/generic.go

@@ -186,8 +186,6 @@ func (g *GenericPLEG) relist() {
 	}

 	timestamp := g.clock.Now()
-	// Update the relist time.


Why did you move this?

I think this is a mistake introduced during rebase.

ncdc · 2017-05-22T17:49:12Z

@derekwaynecarr @yujuhong @Random-Liu

yguo0905 · 2017-05-24T20:16:00Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
@k8s-bot pull-kubernetes-e2e-kops-aws test this
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

ncdc · 2017-07-31T15:47:13Z

/unassign
/assign @derekwaynecarr @yujuhong

k8s-ci-robot · 2017-08-30T00:04:04Z

@yguo0905: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-bazel	`1974104`	link	`/test pull-kubernetes-e2e-gce-bazel`
pull-kubernetes-bazel-build	`1974104`	link	`/test pull-kubernetes-bazel-build`
pull-kubernetes-bazel-test	`1974104`	link	`/test pull-kubernetes-bazel-test`
pull-kubernetes-e2e-kops-aws	`1974104`	link	`/test pull-kubernetes-e2e-kops-aws`
pull-kubernetes-e2e-gce-gpu	`1974104`	link	`/test pull-kubernetes-e2e-gce-gpu`
pull-kubernetes-kubemark-e2e-gce	`1974104`	link	`/test pull-kubernetes-kubemark-e2e-gce`
pull-kubernetes-federation-e2e-gce	`1974104`	link	`/test pull-kubernetes-federation-e2e-gce`
pull-kubernetes-verify	`1974104`	link	`/test pull-kubernetes-verify`
pull-kubernetes-unit	`1974104`	link	`/test pull-kubernetes-unit`
pull-kubernetes-node-e2e	`1974104`	link	`/test pull-kubernetes-node-e2e`
pull-kubernetes-e2e-gce-etcd3	`1974104`	link	`/test pull-kubernetes-e2e-gce-etcd3`

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.

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-github-robot · 2017-09-01T08:03:50Z

@yguo0905 PR needs rebase

dims · 2017-11-16T01:56:18Z

@yguo0905 Do we need this for 1.9? please rebase if so.

k8s-github-robot · 2017-11-18T08:09:39Z

[MILESTONENOTIFIER] Milestone Removed From Pull Request

@dchen1107 @derekwaynecarr @yguo0905 @yujuhong

Important: This pull request was missing labels required for the v1.9 milestone for more than 3 days:

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.
sig owner: Must specify at least one label prefixed with sig/.

Help

fejta-bot · 2018-02-16T08:55:01Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2018-03-18T09:41:49Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

derekwaynecarr · 2018-04-14T13:57:47Z

/do-not-merge
just noting that the community design pr is still not resolved.

fejta-bot · 2018-05-14T14:39:50Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 8, 2017

k8s-github-robot assigned ncdc May 8, 2017

k8s-github-robot assigned dchen1107 May 8, 2017

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 8, 2017

yguo0905 force-pushed the delete-reason-api branch 2 times, most recently from f931492 to b1b6e4d Compare May 9, 2017 00:04

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2017

yguo0905 force-pushed the delete-reason-api branch from b1b6e4d to c1003f2 Compare May 22, 2017 16:47

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2017

ncdc reviewed May 22, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2017

yguo0905 force-pushed the delete-reason-api branch 3 times, most recently from 7516887 to 49b2b83 Compare May 23, 2017 21:12

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2017

calebamiles mentioned this pull request Jun 6, 2017

Client Initiated Pod Termination Reason kubernetes/enhancements#289

Closed

k8s-ci-robot assigned derekwaynecarr and yujuhong and unassigned ncdc Jul 31, 2017

yguo0905 force-pushed the delete-reason-api branch from 49b2b83 to 1974104 Compare August 29, 2017 22:44

yguo0905 added 2 commits August 30, 2017 14:25

API changes for client-initiated termination reason

b056f1c

Generated docs

c82fa55

yguo0905 force-pushed the delete-reason-api branch from 1974104 to c82fa55 Compare August 30, 2017 21:40

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 30, 2017

dchen1107 added this to the v1.9 milestone Sep 5, 2017

k8s-github-robot added the milestone/incomplete-labels label Nov 14, 2017

k8s-github-robot added milestone/removed and removed milestone/incomplete-labels labels Nov 18, 2017

k8s-github-robot removed this from the v1.9 milestone Nov 18, 2017

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 16, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 18, 2018

derekwaynecarr added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2018

k8s-ci-robot closed this May 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API changes for client-initiated termination reason #45504

API changes for client-initiated termination reason #45504

yguo0905 commented May 8, 2017

k8s-reviewable commented May 8, 2017

k8s-github-robot commented May 8, 2017

yguo0905 commented May 8, 2017

ncdc May 22, 2017

yguo0905 May 22, 2017

ncdc May 22, 2017

yguo0905 May 22, 2017

ncdc May 23, 2017

yguo0905 May 24, 2017

ncdc May 22, 2017

yguo0905 May 22, 2017

ncdc May 22, 2017

yguo0905 May 24, 2017

ncdc May 22, 2017

yguo0905 May 24, 2017

ncdc May 22, 2017

yguo0905 May 22, 2017

ncdc May 22, 2017

yguo0905 May 24, 2017

ncdc May 22, 2017

yguo0905 May 22, 2017

ncdc commented May 22, 2017

yguo0905 commented May 24, 2017

ncdc commented Jul 31, 2017

k8s-ci-robot commented Aug 30, 2017

k8s-github-robot commented Sep 1, 2017

dims commented Nov 16, 2017

k8s-github-robot commented Nov 18, 2017

fejta-bot commented Feb 16, 2018

fejta-bot commented Mar 18, 2018

derekwaynecarr commented Apr 14, 2018

fejta-bot commented May 14, 2018

API changes for client-initiated termination reason #45504

API changes for client-initiated termination reason #45504

Conversation

yguo0905 commented May 8, 2017

k8s-reviewable commented May 8, 2017

k8s-github-robot commented May 8, 2017

yguo0905 commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented May 22, 2017

yguo0905 commented May 24, 2017

ncdc commented Jul 31, 2017

k8s-ci-robot commented Aug 30, 2017

k8s-github-robot commented Sep 1, 2017

dims commented Nov 16, 2017

k8s-github-robot commented Nov 18, 2017

fejta-bot commented Feb 16, 2018

fejta-bot commented Mar 18, 2018

derekwaynecarr commented Apr 14, 2018

fejta-bot commented May 14, 2018