Adding termination grace period to Deployment, RC, RCSet, and Job #82170

vivekbagade · 2019-08-30T11:00:34Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Adding graceful termination to the test framework API

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Adding TerminationGracePeriodSeconds to the test framework API

k8s-ci-robot · 2019-08-30T11:00:37Z

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.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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.

neolit123 · 2019-08-30T11:06:18Z

test/utils/runners.go

@@ -321,7 +322,8 @@ func (config *DeploymentConfig) create() error {
 					Annotations: config.Annotations,
 				},
 				Spec: v1.PodSpec{
-					Affinity: config.Affinity,
+					Affinity:                      config.Affinity,
+					TerminationGracePeriodSeconds: func(i int) *int64 { x := int64(math.Max(float64(i), 1)); return &x }(config.TerminationGracePeriodSeconds),


i think it's better to not use an anonymous function and instead define it in the runners.go level for all create() methods to use.

Shouldn't the default be whatever the k8s default is and not 1?
Why do we even need any function (other than Max)? Maybe we could use https://github.com/kubernetes/utils/blob/master/integer/integer.go#L20?

@neolit123 Thanks for the quick review. Made the change.

@MaciekPytel The default that was being used for RCConfig was 1, so kept that.
The file already uses math.Max for ints so kept that too.

mwielgus · 2019-08-30T11:53:12Z

test/utils/runners.go

@@ -569,7 +573,6 @@ func (config *RCConfig) create() error {
 	if config.DNSPolicy == nil {
 		config.DNSPolicy = &dnsDefault
 	}
-	one := int64(1)


Where is this default now?

Added a default field now.

mwielgus · 2019-08-30T11:54:30Z

test/utils/runners.go

@@ -1537,6 +1540,11 @@ func attachConfigMaps(template *v1.PodTemplateSpec, configMapNames []string) {
 	template.Spec.Containers[0].VolumeMounts = mounts
 }

+func (config *RCConfig) getTerminationGracePeriodSeconds() *int64 {
+	x := int64(math.Max(float64(config.TerminationGracePeriodSeconds), 1))


What if someone sets it explicitly to 0?

fejta-bot · 2019-08-30T12:01:26Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

mwielgus

/lgtm
/approve

vivekbagade · 2019-08-30T12:54:32Z

/remove-kind api-change

liggitt · 2019-08-30T12:59:52Z

/approve

k8s-ci-robot · 2019-08-30T13:00:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mwielgus, vivekbagade

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [liggitt]

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

vivekbagade · 2019-08-30T13:22:53Z

/retest

vivekbagade · 2019-08-30T13:51:38Z

/retest

johnSchnake · 2019-09-01T17:53:55Z

test/utils/runners.go

@@ -1537,6 +1541,13 @@ func attachConfigMaps(template *v1.PodTemplateSpec, configMapNames []string) {
 	template.Spec.Containers[0].VolumeMounts = mounts
 }

+func (config *RCConfig) getTerminationGracePeriodSeconds(defaultGrace *int64) *int64 {


Why not just have the application logic at the use sites apply whatever logic they want? Do we have similar "get default value in certain situations" methods for all the other values?

In what situations does the default change? Why not use the same default throughout and make it a constant?

I'm not against having a method like this, but it seems slightly unnecessary and maybe even a bit confusing. If I was creating an object, I wouldn't know what the default should be and would expect that to just be set if there was a helper method like this. Otherwise this method is just a specific use case of

func firstPositiveInt64Val(vals ...*int64) *int64{ for _,v := range vals{ if v!=nil && *v>0 {return v} } }

It seems this has maybe already been talked about by other comments and is otherwise lgtm/approved. Disregard if everyone is happy with it, but had just gotten around to review.

k8s-ci-robot requested review from johnSchnake and neolit123 August 30, 2019 11:02

neolit123 reviewed Aug 30, 2019

View reviewed changes

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 30, 2019

mwielgus reviewed Aug 30, 2019

View reviewed changes

k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Aug 30, 2019

Adding termination grace period to Deployment, RC, RCSet, and Job

6392b69

mwielgus approved these changes Aug 30, 2019

View reviewed changes

k8s-ci-robot assigned mwielgus Aug 30, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2019

k8s-ci-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 30, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2019

johnSchnake reviewed Sep 1, 2019

View reviewed changes

k8s-ci-robot merged commit e3b5062 into kubernetes:master Sep 11, 2019

k8s-ci-robot added this to the v1.17 milestone Sep 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding termination grace period to Deployment, RC, RCSet, and Job #82170

Adding termination grace period to Deployment, RC, RCSet, and Job #82170

vivekbagade commented Aug 30, 2019 •

edited

k8s-ci-robot commented Aug 30, 2019

neolit123 Aug 30, 2019

MaciekPytel Aug 30, 2019

vivekbagade Aug 30, 2019

mwielgus Aug 30, 2019

vivekbagade Aug 30, 2019

mwielgus Aug 30, 2019

vivekbagade Aug 30, 2019

fejta-bot commented Aug 30, 2019

mwielgus left a comment

vivekbagade commented Aug 30, 2019

liggitt commented Aug 30, 2019

k8s-ci-robot commented Aug 30, 2019

vivekbagade commented Aug 30, 2019

vivekbagade commented Aug 30, 2019

johnSchnake Sep 1, 2019

johnSchnake Sep 1, 2019

Adding termination grace period to Deployment, RC, RCSet, and Job #82170

Adding termination grace period to Deployment, RC, RCSet, and Job #82170

Conversation

vivekbagade commented Aug 30, 2019 • edited

k8s-ci-robot commented Aug 30, 2019

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

fejta-bot commented Aug 30, 2019

mwielgus left a comment

Choose a reason for hiding this comment

vivekbagade commented Aug 30, 2019

liggitt commented Aug 30, 2019

k8s-ci-robot commented Aug 30, 2019

vivekbagade commented Aug 30, 2019

vivekbagade commented Aug 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivekbagade commented Aug 30, 2019 •

edited