Bug 1939294: Avoid setting metadata.GracePeriodSeconds to zero seconds #2047

timflannagan · 2021-03-16T19:54:30Z

Update any references where we set the metadata.GracePeriodSeconds field
to zero seconds. If we need a Pod to be deleted as quick as possible,
use a single second.

Description of the change:

Motivation for the change:

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

openshift-ci-robot · 2021-03-16T22:30:18Z

@timflannagan: This pull request references Bugzilla bug 1939294, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jianzhangbjz

In response to this:

Bug 1939294: Avoid setting metadata.GracePeriodSeconds to zero seconds

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.

openshift-ci-robot · 2021-03-16T22:30:19Z

@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: jianzhangbjz.

Note that only operator-framework members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@timflannagan: This pull request references Bugzilla bug 1939294, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)

bug target release (4.8.0) matches configured target release for branch (4.8.0)

bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jianzhangbjz

In response to this:

Bug 1939294: Avoid setting metadata.GracePeriodSeconds to zero seconds

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.

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.

timflannagan · 2021-03-16T23:04:09Z

/retest

timflannagan · 2021-03-17T02:17:44Z

Hmm, I searched for that upgrade error in the openshift CI search tool and it looks like a known flake. Going to try and retest again before pinging the monitoring team.

/retest

timflannagan · 2021-03-17T13:54:43Z

/retest

ankitathomas · 2021-03-17T19:35:48Z

/lgtm

Update any references where we set the metadata.GracePeriodSeconds field to zero seconds. If we need a Pod to be deleted as quick as possible, use a single second. Signed-off-by: timflannagan <[email protected]>

timflannagan · 2021-03-17T20:58:05Z

pkg/controller/registry/reconciler/grpc.go

@@ -226,7 +226,7 @@ func (c *GrpcRegistryReconciler) ensurePod(source grpcCatalogSourceDecorator, sa
 			return nil
 		}
 		for _, p := range currentLivePods {
-			if err := c.OpClient.KubernetesInterface().CoreV1().Pods(source.GetNamespace()).Delete(context.TODO(), p.GetName(), *metav1.NewDeleteOptions(0)); err != nil {
+			if err := c.OpClient.KubernetesInterface().CoreV1().Pods(source.GetNamespace()).Delete(context.TODO(), p.GetName(), *metav1.NewDeleteOptions(1)); err != nil {


Probably easiest if I also add a package constant set to 1 and just reference that variable in NewDeleteOptions(...) instead of hardcoding the value of 1. I need to double-check what the default grace period seconds are, and whether we can just pass an empty metav1.DeleteOptions object instead of explicitly setting the grace period every time.

I'm partial to a package scoped constant that is also the zero value for metav1.DeleteOptions: metav1.DeleteOptions{}.

According to the type docs, as long as the GracePeriodSeconds field is nil:

... the default grace period for the specified type will be used.

timflannagan · 2021-03-18T18:41:45Z

/retest

timflannagan · 2021-03-19T14:11:32Z

/retest

timflannagan · 2021-03-22T15:41:33Z

/retest

pkg/controller/registry/reconciler/grpc.go

ecordell · 2021-03-30T17:56:20Z

/approve

openshift-ci-robot · 2021-03-30T17:56:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, timflannagan

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:

~~OWNERS~~ [ecordell]

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

ankitathomas · 2021-03-30T18:28:09Z

/lgtm

openshift-bot · 2021-03-30T21:08:16Z

/retest

Please review the full test history for this PR and help us cut down flakes.

timflannagan · 2021-03-31T01:51:04Z

That last upgrade test looks somewhat concerning, but going to retest anyway just in case a flake was produced.

/retest

openshift-ci-robot · 2021-03-31T03:50:51Z

@timflannagan: All pull requests linked via external trackers have merged:

operator-framework/operator-lifecycle-manager#2047

Bugzilla bug 1939294 has been moved to the MODIFIED state.

In response to this:

Bug 1939294: Avoid setting metadata.GracePeriodSeconds to zero seconds

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.

openshift-ci-robot requested review from ankitathomas and dinhxuanvu March 16, 2021 19:54

timflannagan force-pushed the avoid-graceperiod-delete-zero branch from 23d97ba to 0acd9f3 Compare March 16, 2021 19:56

timflannagan changed the title ~~pkg,test: Avoid setting metadata.GracePeriodSeconds to zero seconds~~ Bug 1939294: Avoid setting metadata.GracePeriodSeconds to zero seconds Mar 16, 2021

openshift-ci-robot added bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Mar 16, 2021

openshift-ci-robot assigned ankitathomas Mar 17, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2021

timflannagan force-pushed the avoid-graceperiod-delete-zero branch from 0acd9f3 to c5a192a Compare March 17, 2021 20:21

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2021

pkg,test: Avoid setting metadata.GracePeriodSeconds to zero seconds

9c1b99b

Update any references where we set the metadata.GracePeriodSeconds field to zero seconds. If we need a Pod to be deleted as quick as possible, use a single second. Signed-off-by: timflannagan <[email protected]>

timflannagan force-pushed the avoid-graceperiod-delete-zero branch from c5a192a to 9c1b99b Compare March 17, 2021 20:41

timflannagan commented Mar 17, 2021

View reviewed changes

njhale reviewed Mar 23, 2021

View reviewed changes

pkg/controller/registry/reconciler/grpc.go Show resolved Hide resolved

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2021

openshift-merge-robot merged commit 2645388 into operator-framework:master Mar 31, 2021

timflannagan deleted the avoid-graceperiod-delete-zero branch March 31, 2021 13:41

perdasilva mentioned this pull request Jan 21, 2022

Remove grace period from csv deletions #2580

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1939294: Avoid setting metadata.GracePeriodSeconds to zero seconds #2047

Bug 1939294: Avoid setting metadata.GracePeriodSeconds to zero seconds #2047

timflannagan commented Mar 16, 2021

openshift-ci-robot commented Mar 16, 2021

openshift-ci-robot commented Mar 16, 2021

timflannagan commented Mar 16, 2021

timflannagan commented Mar 17, 2021

timflannagan commented Mar 17, 2021

ankitathomas commented Mar 17, 2021

timflannagan Mar 17, 2021

njhale Mar 23, 2021

timflannagan commented Mar 18, 2021

timflannagan commented Mar 19, 2021

timflannagan commented Mar 22, 2021

ecordell commented Mar 30, 2021

openshift-ci-robot commented Mar 30, 2021

ankitathomas commented Mar 30, 2021

openshift-bot commented Mar 30, 2021

timflannagan commented Mar 31, 2021

openshift-ci-robot commented Mar 31, 2021

Bug 1939294: Avoid setting metadata.GracePeriodSeconds to zero seconds #2047

Bug 1939294: Avoid setting metadata.GracePeriodSeconds to zero seconds #2047

Conversation

timflannagan commented Mar 16, 2021

openshift-ci-robot commented Mar 16, 2021

openshift-ci-robot commented Mar 16, 2021

timflannagan commented Mar 16, 2021

timflannagan commented Mar 17, 2021

timflannagan commented Mar 17, 2021

ankitathomas commented Mar 17, 2021

timflannagan Mar 17, 2021

Choose a reason for hiding this comment

njhale Mar 23, 2021

Choose a reason for hiding this comment

timflannagan commented Mar 18, 2021

timflannagan commented Mar 19, 2021

timflannagan commented Mar 22, 2021

ecordell commented Mar 30, 2021

openshift-ci-robot commented Mar 30, 2021

ankitathomas commented Mar 30, 2021

openshift-bot commented Mar 30, 2021

timflannagan commented Mar 31, 2021

openshift-ci-robot commented Mar 31, 2021