Add --prune & --prune-all options to delete the unreferenced revisions #1217

Kaustubh-pande · 2021-02-10T15:39:20Z

Description

As per the review comment #317

Changes

Add --prune option to remove all unreferenced revisions for a given service
Add --prune-all option to remove all the unreferenced revisions

Reference

Fixes #317

knative-prow-robot

@Kaustubh-pande: 2 warnings.

In response to this:

Description

As per the review comment #317

Changes

Add --all option to delete all the revisions

Reference

Fixes #317

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.

pkg/kn/commands/revision/delete_mock_test.go

knative-prow-robot · 2021-02-10T15:39:31Z

Hi @Kaustubh-pande. Thanks for your PR.

I'm waiting for a knative 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.

maximilien

Nice. Thanks for contribution.
/ok-to-test
/lgtm
/hold to allow others a chance to review

Kaustubh-pande · 2021-02-11T04:34:47Z

/retest

navidshaikh

@rhuss : Rethinking on this command, if user deletes all the revisions, the one (or may be multiple) receiving the traffic will be recreated. Should we have this option then ? (as it tries to change the desire state)
Instead, we may consider kn revision delete --unused or kn revision delete --prune to inspect the revisions not receiving the traffic and delete them. wdyt?

pkg/kn/commands/revision/delete.go

lib/test/revision.go

test/e2e/revision_test.go

Kaustubh-pande · 2021-02-15T12:19:35Z

@rhuss : Rethinking on this command, if user deletes all the revisions, the one (or may be multiple) receiving the traffic will be recreated. Should we have this option then ? (as it tries to change the desire state)
Instead, we may consider kn revision delete --unused or kn revision delete --prune to inspect the revisions not receiving the traffic and delete them. wdyt?

@navidshaikh, @rhuss I think kn revision delete --prune is more suitable here, can I rename --all to --prune?

rhuss · 2021-02-15T14:23:48Z

I agree that we should not unconditionally delete all revisions, as this can (a) lead to an inconsistent state (e.g. when a revision is part of a traffic split) and (b) one of the revision would be recreated anyway.

So my suggestion is to add either a kn revision prune command or an option kn revision delete --prune so that all unreferenced revisions are removed. IMO this is the only reasonable use case.

Kaustubh-pande · 2021-02-17T08:05:03Z

/retest

CHANGELOG.adoc

docs/cmd/kn_revision_delete.md

pkg/kn/commands/revision/delete.go

rhuss · 2021-02-23T07:35:24Z

I don't know by heart but is this PR about delete all revisions in a namespace or only revisions for a specific service ? I think it really should be about deleting revisions for a given service as I think this is the main use case.

So, from UX pov I see the following options that I would love to see

kn revision delete --prune myservice (so taking a service name as an argument)
kn revision delete --prune-all (extra option to avoid accidentally pruning everything when a service name is forgotten. also I think the option space is not very polluted for kn revision delete so I think having two options is ok)

sorry for reiterating on this, but I think this is important.

lib/test/revision.go

pkg/kn/commands/revision/delete.go

rhuss · 2021-03-08T07:43:32Z

@Kaustubh-pande @navidshaikh @dsimansk what is left for this PR to get merged ? (sorry, have not followed up the latest changes of the PR)

Kaustubh-pande · 2021-03-08T09:12:41Z

@Kaustubh-pande @navidshaikh @dsimansk what is left for this PR to get merged ? (sorry, have not followed up the latest changes of the PR)

sorry, I was off for a couple of days, I will push a commit today.

rhuss · 2021-03-08T09:54:29Z

/retest

Kaustubh-pande · 2021-03-09T10:52:41Z

/retest

Kaustubh-pande · 2021-03-09T11:40:47Z

/retest

pkg/kn/commands/revision/delete.go

…sions

knative-metrics-robot · 2021-03-09T14:38:36Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/revision/delete.go	84.0%	90.6%	6.6

Kaustubh-pande · 2021-03-09T15:10:30Z

/retest

dsimansk · 2021-03-09T20:07:54Z

/lgtm
/hold
Adding hold for @navidshaikh and others to take a look.

navidshaikh

/lgtm
/unhold
suggested a few nits which can be fixed in a follow up, thanks!

navidshaikh · 2021-03-10T05:52:05Z

lib/test/revision.go

@@ -139,3 +139,17 @@ func RevisionListWithService(r *KnRunResultCollector, serviceNames ...string) {
 		}
 	}
 }
+
+// RevisionDeleteWithPruneOption verifies removeing all unreferenced revisions for a given service in sync mode


Suggested change

// RevisionDeleteWithPruneOption verifies removeing all unreferenced revisions for a given service in sync mode

// RevisionDeleteWithPruneOption verifies removing all unreferenced revisions for a given service in sync mode

navidshaikh · 2021-03-10T05:52:34Z

lib/test/revision.go

+	r.AssertNoError(out)
+}
+
+// RevisionDeleteWithPruneAllOption verifies removeing all unreferenced revision in sync mode


Suggested change

// RevisionDeleteWithPruneAllOption verifies removeing all unreferenced revision in sync mode

// RevisionDeleteWithPruneAllOption verifies removing all unreferenced revision in sync mode

navidshaikh · 2021-03-12T15:23:36Z

pkg/kn/commands/revision/delete.go

+					fmt.Fprintf(cmd.OutOrStdout(), "No unreferenced revisions found.\n")
+					return nil
+				}
+			}


we can create the client first and then replace the multiple if/else with switch

navidshaikh · 2021-03-12T15:32:07Z

test/e2e/revision_test.go

@@ -59,6 +59,25 @@ func TestRevision(t *testing.T) {
 	nonexistRevision := "hello-nonexist"
 	test.RevisionMultipleDelete(r, existRevision1, existRevision2, nonexistRevision)

+	t.Log("update hello service and increase revision count to 4")


Suggested change

t.Log("update hello service and increase revision count to 4")

t.Log("update hello service and increase revision count")

since the revisions are being deleted in above tests

navidshaikh · 2021-03-12T15:32:43Z

test/e2e/revision_test.go

+	unRefRevision := test.FindRevisionByGeneration(r, "hello", 3)
+	test.RevisionDeleteWithPruneOption(r, "hello", unRefRevision)
+
+	t.Log("update hello service and increase revision count to 5")


Suggested change

t.Log("update hello service and increase revision count to 5")

t.Log("update hello service and increase revision count")

knative-prow-robot · 2021-03-12T15:34:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, navidshaikh

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~~ [maximilien,navidshaikh]

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

navidshaikh · 2021-03-15T08:09:11Z

/test pull-knative-client-integration-tests-latest-release

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 10, 2021

knative-prow-robot requested review from cppforlife and maximilien February 10, 2021 15:39

knative-prow-robot reviewed Feb 10, 2021

View reviewed changes

pkg/kn/commands/revision/delete_mock_test.go Outdated Show resolved Hide resolved

pkg/kn/commands/revision/delete_mock_test.go Outdated Show resolved Hide resolved

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 10, 2021

Kaustubh-pande force-pushed the multiple-name-support branch from 3d97334 to 26c1697 Compare February 10, 2021 16:30

maximilien approved these changes Feb 10, 2021

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 10, 2021

knative-prow-robot assigned maximilien Feb 10, 2021

navidshaikh requested changes Feb 11, 2021

View reviewed changes

pkg/kn/commands/revision/delete.go Outdated Show resolved Hide resolved

pkg/kn/commands/revision/delete.go Outdated Show resolved Hide resolved

lib/test/revision.go Outdated Show resolved Hide resolved

test/e2e/revision_test.go Outdated Show resolved Hide resolved

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2021

Kaustubh-pande changed the title ~~Add --all option to delete all the revisions~~ Add --prune option to remove all the unreferenced revisions Feb 17, 2021

Kaustubh-pande force-pushed the multiple-name-support branch from ed452c1 to c97596a Compare February 17, 2021 06:41

Kaustubh-pande requested a review from navidshaikh February 19, 2021 13:33

navidshaikh requested changes Feb 22, 2021

View reviewed changes

CHANGELOG.adoc Show resolved Hide resolved

docs/cmd/kn_revision_delete.md Outdated Show resolved Hide resolved

pkg/kn/commands/revision/delete.go Outdated Show resolved Hide resolved

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2021

knative deleted a comment from knative-prow-robot Feb 23, 2021

Kaustubh-pande force-pushed the multiple-name-support branch from c97596a to f327b64 Compare February 26, 2021 22:34

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2021

dsimansk reviewed Mar 1, 2021

View reviewed changes

lib/test/revision.go Show resolved Hide resolved

Kaustubh-pande force-pushed the multiple-name-support branch from 4abc429 to 7d50aba Compare March 2, 2021 11:51

navidshaikh requested changes Mar 2, 2021

View reviewed changes