warn user about resource being deleted #95544

SaiHarshaK · 2020-10-14T05:38:40Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Detect "kubectl apply" to resources currently being deleted and warn user.

Which issue(s) this PR fixes:
Ref #93363

Special notes for your reviewer:
/cc @apelisse
Not sure if i should use Fixes keyword so used Ref instead.

Does this PR introduce a user-facing change?:

Warns user when executing kubectl apply/diff to resource currently being deleted.

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

k8s-ci-robot · 2020-10-14T05:38:49Z

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

dims · 2020-10-14T10:59:27Z

/ok-to-test

apelisse

Note that this is not going to work for server-side apply, but I don't think we can do it client-side since we don't see the object before. Alternately, especially since it's merely a warning, we could check that on the final/merged/returned object.

apelisse · 2020-10-14T16:35:00Z

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

@@ -538,8 +540,13 @@ See http://k8s.io/docs/reference/using-api/api-concepts/#conflicts`, err)
 		return err
 	}

+	metadata, _ = meta.Accessor(info.Object)


I'm assuming you moved the "Accessor" above (L411) so that you would be able to access it here? so I don't think that is needed, is it?

Actually both were required because it didnt receive the delete timestamp yet on 411. But change is still unncessary so reverting change to that Line

apelisse · 2020-10-14T16:35:49Z

staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go

@@ -74,6 +74,8 @@ var (

 		# Diff file read from stdin
 		cat service.yaml | kubectl diff -f -`))
+
+	warningChangesOnDeletingResource = "Warning: Detected changes to resource %[1]s which is currently being deleted.\n"


It makes me a little sad that we're duplicating this string, is there a good place to share it?

is apply fine? I could just export this by capitalizing
Diff anyway imports apply too

apelisse · 2020-10-14T16:36:26Z

staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go

@@ -548,6 +550,13 @@ func (o *DiffOptions) Run() error {
 				break
 			}
 		}
+
+		metadata, _ := meta.Accessor(info.Object)
+		if metadata != nil && metadata.GetDeletionTimestamp() != nil {


I'd rather check for the error than for empty metadata.

When you diff a resource which hasnt yet applied i think then this throws error.

SaiHarshaK · 2020-10-14T20:51:51Z

/retest

SaiHarshaK · 2020-10-15T07:37:45Z

/retest

apelisse · 2020-10-15T15:54:02Z

What do you think about doing it on the post-merged resource?

SaiHarshaK · 2020-10-15T16:03:59Z

What do you think about doing it on the post-merged resource?

as in warning when there are multiple apply running parallely?

apelisse · 2020-10-15T16:13:32Z

No, my problem is that your current solution doesn't work for server-side apply because we don't get the resource before sending the patch. Also, you have a race-condition if the deletionTimestamp is added between the get and the actual patch is applied. Alternately, you could look at the resource that gets back from the patch, and see if it contains the deletion timestamp.

SaiHarshaK · 2020-10-15T16:26:41Z

For the 1st part does this get the object from server?
As for the 2nd part, yes i think i should do it after the patch is done, ill update that

apelisse · 2020-10-15T16:28:08Z

For the 1st part does this get the object from server?

SSA happens right before and isn't included: https://github.com/kubernetes/kubernetes/blob/52630540538c7c0949f50abe4fe960c7b17398c0/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go#L434-L491

SaiHarshaK · 2020-10-15T16:32:46Z

For the 1st part does this get the object from server?

SSA happens right before and isn't included:

https://github.com/kubernetes/kubernetes/blob/52630540538c7c0949f50abe4fe960c7b17398c0/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go#L434-L491

Oh right, should i insert the warning block here too or some function is preferred?

apelisse · 2020-10-15T16:52:48Z

Oh right, should i insert the warning block here too or some function is preferred?

yeah, that sounds good. And for client-side apply, I would apply it here:
https://github.com/kubernetes/kubernetes/blob/52630540538c7c0949f50abe4fe960c7b17398c0/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go#L564

apelisse

That looks good, I think it's fine now. I'd maybe make a function for the error:

func WarnIfDeleting(obj runtime.Object, stderr io.Write) {
   ...
}

Then you wouldn't have to export the warning string, and you could re-use the same logic throughout.

apelisse · 2020-10-15T19:32:22Z

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

@@ -133,6 +133,8 @@ var (
 		kubectl apply --prune -f manifest.yaml --all --prune-whitelist=core/v1/ConfigMap`))

 	warningNoLastAppliedConfigAnnotation = "Warning: resource %[1]s is missing the %[2]s annotation which is required by %[3]s apply. %[3]s apply should only be used on resources created declaratively by either %[3]s create --save-config or %[3]s apply. The missing annotation will be patched automatically.\n"
+	// WarningChangesOnDeletingResource is for warning about the resource currently being deleted on which a kubectl operation is run.


That comment is not useful. If it was, then it would probably mean that the message needs to be improved ;-)

apelisse · 2020-10-15T19:36:12Z

/assign @seans3

apelisse · 2020-10-16T16:52:17Z

Looks great to me, thanks!
/approve

I'll let @seans3 lgtm.

SaiHarshaK · 2020-10-16T17:37:03Z

/retest

soltysh

/lgtm

soltysh · 2020-11-05T14:43:33Z

/priority backlog
/triage accepted

k8s-ci-robot · 2020-11-05T14:43:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, SaiHarshaK, soltysh

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:

~~staging/src/k8s.io/kubectl/OWNERS~~ [apelisse,soltysh]

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

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 14, 2020

k8s-ci-robot requested a review from apelisse October 14, 2020 05:38

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 14, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 14, 2020

SaiHarshaK force-pushed the warn-race branch from 6a2e4b8 to f93b78f Compare October 14, 2020 12:27

apelisse reviewed Oct 14, 2020

View reviewed changes

SaiHarshaK force-pushed the warn-race branch from f93b78f to 5263054 Compare October 14, 2020 19:59

SaiHarshaK requested a review from apelisse October 15, 2020 08:32

SaiHarshaK force-pushed the warn-race branch from 5263054 to ca50cfd Compare October 15, 2020 17:15

apelisse reviewed Oct 15, 2020

View reviewed changes

k8s-ci-robot assigned seans3 Oct 15, 2020

warn user about resource being deleted

7b0ef88

SaiHarshaK force-pushed the warn-race branch from ca50cfd to 7b0ef88 Compare October 16, 2020 16:43

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2020

soltysh approved these changes Nov 5, 2020

View reviewed changes

k8s-ci-robot assigned soltysh Nov 5, 2020

k8s-ci-robot merged commit 374989e into kubernetes:master Nov 5, 2020

k8s-ci-robot added this to the v1.20 milestone Nov 5, 2020

github-actions bot mentioned this pull request Nov 10, 2020

Week Ending November 8, 2020 dev-obs/actus#265

Open

apelisse mentioned this pull request Jan 11, 2021

Detect "kubectl apply" to resources currently being deleted #93363

Closed

nicks mentioned this pull request Apr 20, 2023

Full update does not re create PVCs tilt-dev/tilt#6048

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

warn user about resource being deleted #95544

warn user about resource being deleted #95544

SaiHarshaK commented Oct 14, 2020 •

edited

Loading

k8s-ci-robot commented Oct 14, 2020

dims commented Oct 14, 2020

apelisse left a comment

apelisse Oct 14, 2020

SaiHarshaK Oct 14, 2020

apelisse Oct 14, 2020

SaiHarshaK Oct 14, 2020

apelisse Oct 14, 2020

SaiHarshaK Oct 14, 2020

SaiHarshaK commented Oct 14, 2020

SaiHarshaK commented Oct 15, 2020

apelisse commented Oct 15, 2020

SaiHarshaK commented Oct 15, 2020 •

edited

Loading

apelisse commented Oct 15, 2020

SaiHarshaK commented Oct 15, 2020

apelisse commented Oct 15, 2020

SaiHarshaK commented Oct 15, 2020 •

edited

Loading

apelisse commented Oct 15, 2020

apelisse left a comment •

edited

Loading

apelisse Oct 15, 2020

apelisse commented Oct 15, 2020

apelisse commented Oct 16, 2020

SaiHarshaK commented Oct 16, 2020

soltysh left a comment

soltysh commented Nov 5, 2020

k8s-ci-robot commented Nov 5, 2020

warn user about resource being deleted #95544

warn user about resource being deleted #95544

Conversation

SaiHarshaK commented Oct 14, 2020 • edited Loading

k8s-ci-robot commented Oct 14, 2020

dims commented Oct 14, 2020

apelisse left a comment

Choose a reason for hiding this comment

apelisse Oct 14, 2020

Choose a reason for hiding this comment

SaiHarshaK Oct 14, 2020

Choose a reason for hiding this comment

apelisse Oct 14, 2020

Choose a reason for hiding this comment

SaiHarshaK Oct 14, 2020

Choose a reason for hiding this comment

apelisse Oct 14, 2020

Choose a reason for hiding this comment

SaiHarshaK Oct 14, 2020

Choose a reason for hiding this comment

SaiHarshaK commented Oct 14, 2020

SaiHarshaK commented Oct 15, 2020

apelisse commented Oct 15, 2020

SaiHarshaK commented Oct 15, 2020 • edited Loading

apelisse commented Oct 15, 2020

SaiHarshaK commented Oct 15, 2020

apelisse commented Oct 15, 2020

SaiHarshaK commented Oct 15, 2020 • edited Loading

apelisse commented Oct 15, 2020

apelisse left a comment • edited Loading

Choose a reason for hiding this comment

apelisse Oct 15, 2020

Choose a reason for hiding this comment

apelisse commented Oct 15, 2020

apelisse commented Oct 16, 2020

SaiHarshaK commented Oct 16, 2020

soltysh left a comment

Choose a reason for hiding this comment

soltysh commented Nov 5, 2020

k8s-ci-robot commented Nov 5, 2020

SaiHarshaK commented Oct 14, 2020 •

edited

Loading

SaiHarshaK commented Oct 15, 2020 •

edited

Loading

SaiHarshaK commented Oct 15, 2020 •

edited

Loading

apelisse left a comment •

edited

Loading