add resources "delete all" method in each service #68

sivchari · 2021-12-19T17:59:46Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

I added delete all resources of each objects.

Which issue(s) this PR fixes:

Fixes #67

Special notes for your reviewer:

/label tide/merge-method-squash

sanposhiho · 2021-12-20T04:11:45Z

server/di/interface.go

@@ -18,6 +18,7 @@ type PodService interface {
 	List(ctx context.Context) (*corev1.PodList, error)
 	Apply(ctx context.Context, pod *configv1.PodApplyConfiguration) (*corev1.Pod, error)
 	Delete(ctx context.Context, name string) error
+	Deletes(ctx context.Context) error


We cannot guess what Deletes does.
Maybe DeleteAll or Reset is better.

sanposhiho · 2021-12-20T04:12:51Z

node/node.go

+		return xerrors.Errorf("list pods: %w", err)
+	}
+
+	// delete all pods


Why don't you use the ”delete all method” on Pod service?

sanposhiho · 2021-12-20T04:13:17Z

node/node.go

+
+	// delete all nodes
+	if err := s.client.CoreV1().Nodes().DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{}); err != nil {
+		return xerrors.Errorf("delete nodes: %w", err)


Suggested change

return xerrors.Errorf("delete nodes: %w", err)

return xerrors.Errorf("delete all nodes: %w", err)

sanposhiho · 2021-12-20T04:13:37Z

persistentvolume/persistentvolume.go

+// Deletes deletes all persistentVolumes.
+func (s *Service) Deletes(ctx context.Context) error {
+	if err := s.client.CoreV1().PersistentVolumes().DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{}); err != nil {
+		return xerrors.Errorf("delete persistentVolumes: %w", err)


Suggested change

return xerrors.Errorf("delete persistentVolumes: %w", err)

return xerrors.Errorf("delete all persistentVolumes: %w", err)

sanposhiho · 2021-12-20T04:13:44Z

persistentvolumeclaim/persistentvolumeclaim.go

+// Deletes deletes all persistentVolumeClaims.
+func (s *Service) Deletes(ctx context.Context) error {
+	if err := s.client.CoreV1().PersistentVolumeClaims(defaultNamespaceName).DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{}); err != nil {
+		return xerrors.Errorf("delete persistentVolumeClaims: %w", err)


Suggested change

return xerrors.Errorf("delete persistentVolumeClaims: %w", err)

return xerrors.Errorf("delete all persistentVolumeClaims: %w", err)

sanposhiho · 2021-12-20T04:13:52Z

pod/pod.go

+// Deltes deletes all pods.
+func (s *Service) Deletes(ctx context.Context) error {
+	if err := s.client.CoreV1().Pods(defaultNamespaceName).DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{}); err != nil {
+		return fmt.Errorf("delete pods: %w", err)


Suggested change

return fmt.Errorf("delete pods: %w", err)

return fmt.Errorf("delete all pods: %w", err)

sanposhiho · 2021-12-20T04:14:00Z

priorityclass/priorityclass.go

+// Deletes deletes all priority classes.
+func (s *Service) Deletes(ctx context.Context) error {
+	if err := s.client.SchedulingV1().PriorityClasses().DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{}); err != nil {
+		return xerrors.Errorf("delete priorityClasses: %w", err)


Suggested change

return xerrors.Errorf("delete priorityClasses: %w", err)

return xerrors.Errorf("delete all priorityClasses: %w", err)

sanposhiho · 2021-12-20T04:15:02Z

storageclass/storageclass.go

+// Deletes deletes all storageClasss.
+func (s *Service) Deletes(ctx context.Context) error {
+	if err := s.client.StorageV1().StorageClasses().DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{}); err != nil {
+		return xerrors.Errorf("delete storageClasss: %w", err)


Suggested change

return xerrors.Errorf("delete storageClasss: %w", err)

return xerrors.Errorf("delete all storageClasses: %w", err)

sanposhiho · 2021-12-20T04:30:32Z

@sivchari Could you tag #26 in the issue description instead of #67?

sanposhiho · 2021-12-20T04:31:44Z

Let me retitle this to be more appropriate.

/retitle add resources "delete all" method in each service.

sanposhiho · 2021-12-20T08:25:12Z

/retitle add resources "delete all" method in each service

sanposhiho · 2021-12-21T02:05:57Z

storageclass/storageclass.go

+// DeleteAll deletes all storageClassses.
+func (s *Service) DeleteAll(ctx context.Context) error {
+	if err := s.client.StorageV1().StorageClasses().DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{}); err != nil {
+		return xerrors.Errorf("delete all storageClassses: %w", err)


typo (the same typo on the DeleteAll comment.)

Suggested change

return xerrors.Errorf("delete all storageClassses: %w", err)

return xerrors.Errorf("delete all storageClasses: %w", err)

sanposhiho · 2021-12-21T02:06:56Z

node/node.go

+	}
+	if len(pl.Items) == 0 {
+		klog.Info("delete all nodes: no pods to delete")
+		return nil


Why can we return here?

Oh, I mistook.

sanposhiho · 2021-12-21T02:07:18Z

node/node_test.go

@@ -146,3 +146,122 @@ func TestService_Delete(t *testing.T) {
 		})
 	}
 }
+
+func TestService_DeleteAll(t *testing.T) {


sanposhiho · 2021-12-22T03:57:48Z

node/node.go

+		return xerrors.Errorf("list pods: %w", err)
+	}
+	if len(pl.Items) == 0 {
+		klog.Info("delete all nodes: no pods to delete")


I think we don't need to list pod only to log it.

sanposhiho · 2021-12-22T03:57:58Z

node/node.go

+
+	// delete all pods
+	if err := s.podService.DeleteAll(ctx); err != nil {
+		return xerrors.Errorf("delete all nodes: %w", err)


Suggested change

return xerrors.Errorf("delete all nodes: %w", err)

return xerrors.Errorf("delete all pods: %w", err)

sanposhiho · 2021-12-22T04:05:05Z

node/node_test.go

+		wantErr                 bool
+	}{
+		{
+			name: "delete all nodes and pods on nodes",


Ah, Do you want to delete only Pods on node? (= do you want not to delete unscheduled pods?)
Yeah, I think it is better than deleting all nodes and all pods (include unscheduled pods.).

Currently, DeleteAll deletes all nodes and all pods, so, should be fixed.

I'll fixed it.

sanposhiho · 2021-12-22T04:06:35Z

node/node_test.go

+			wantErr: false,
+		},
+		{
+			name: "fail to delete no nodes",


Could you double-check? I think this name is incorrect.

Suggested change

name: "fail to delete no nodes",

name: "fail if deleting all pods returns error",

sivchari · 2022-01-07T16:11:14Z

I fixed DeleteAll method of pod.go.
I update it to delete pod if pod's node name is empty in concurrency.
If someone has better idea than implementation now, please tell me.

sanposhiho · 2022-01-08T09:35:55Z

pod/pod.go

@@ -74,3 +74,39 @@ func (s *Service) Delete(ctx context.Context, name string) error {

 	return nil
 }
+
+// DelteAll deletes all pods.
+func (s *Service) DeleteAll(ctx context.Context) error {


In this DeleteAll method, you should delete all Pods.
But, in DeleteAll of node service, you shouldn't delete all Pods but delete all scheduled Pods.

So,

rename this method to DeleteAllScheduledPod

use DeleteAllScheduledPod from DeleteAll in node service

create new DeleteAll which deletes all pods(= scheduled pods + unscheduled pods.)

Thanks for review.

create new DeleteAll which deletes all pods(= scheduled pods + unscheduled pods.)

This method will be not used now, so I think it's better not to create it.
What do you think ?

This method will be not used now,

No. DeleteAll method is needed to delete all Pods.

You'd like to create "delete all" method in each service now, right?

Deleting all Nodes means "delete all Nodes and delete all scheduled Pods". So, you need to create "DeleteAllScheduledPod" to delete all scheduled Pods in "DeleteAll" in node service.

Deleting all Pods means "delete all pods whether they are scheduled or not." So, you also need to create "DeleteAll" to delete all Pods.

Ah, this PR is scoped to create delete all method in each service, so we should create DeleteAllScheduledPod and DeleteAll, right ?

Deleting all Nodes means "delete all Nodes and delete all scheduled Pods". So, you need to create
"DeleteAllScheduledPod" to delete all scheduled Pods in "DeleteAll" in node service.

Deleting all Pods means "delete all pods whether they are scheduled or not." So, you also need to create
"DeleteAll" to delete all Pods.

I know, thanks.

@sanposhiho

I added DeleteAll method, too. pod service test isn't existed, now.
Shall I add a test? Or do you hope to create different PR because the scope is larger than this PR?

Oh, I've just commented about the test. Yes, please add test for DeleteAllScheduledPod.
#68 (comment)

sanposhiho · 2022-01-08T11:20:01Z

node/node.go

+func (s *Service) DeleteAll(ctx context.Context) error {
+	// delete all pods
+	if err := s.podService.DeleteAllScheduledPod(ctx); err != nil {
+		return xerrors.Errorf("delete all pods: %w", err)


nits:

Suggested change

return xerrors.Errorf("delete all pods: %w", err)

return xerrors.Errorf("delete all scheduled pods: %w", err)

sanposhiho · 2022-01-08T11:21:14Z

node/node_test.go

+			wantErr: false,
+		},
+		{
+			name: "fail if delteing all pods returns error",


Suggested change

name: "fail if delteing all pods returns error",

name: "fail if deleting all pods returns error",

sanposhiho · 2022-01-08T11:22:24Z

node/node_test.go

+		{
+			name: "delete all nodes and pods scheduled on them",
+			preparePodServiceMockFn: func(m *mock_node.MockPodService) {
+				m.EXPECT().List(gomock.Any()).Return(&corev1.PodList{


List method on Pod service is not called.

sanposhiho · 2022-01-08T11:23:03Z

pod/pod.go

@@ -74,3 +73,21 @@ func (s *Service) Delete(ctx context.Context, name string) error {

 	return nil
 }
+
+// DeleteAllPod deletes all pods.


Suggested change

// DeleteAllPod deletes all pods.

// DeleteAll deletes all pods.

sanposhiho · 2022-01-08T11:23:19Z

storageclass/storageclass.go

@@ -62,3 +62,12 @@ func (s *Service) Delete(ctx context.Context, name string) error {

 	return nil
 }
+
+// DeleteAll deletes all storageClassses.


Suggested change

// DeleteAll deletes all storageClassses.

// DeleteAll deletes all storageClasses.

sanposhiho · 2022-01-08T11:27:38Z

pod/pod.go

+}
+
+// DelteAllScheduledPod deletes all scheduled pods.
+func (s *Service) DeleteAllScheduledPod(ctx context.Context) error {


Could you 念のため add a brief test for this method? :)

sivchari · 2022-01-08T11:30:02Z

I'm checking, now. so I'll notify you I ready to review 👍

sanposhiho · 2022-01-08T11:31:00Z

server/di/interface.go

@@ -18,6 +18,8 @@ type PodService interface {
 	List(ctx context.Context) (*corev1.PodList, error)
 	Apply(ctx context.Context, pod *configv1.PodApplyConfiguration) (*corev1.Pod, error)
 	Delete(ctx context.Context, name string) error
+	DeleteAll(ctx context.Context) error
+	DeleteAllScheduledPod(ctx context.Context) error


DeleteAllScheduledPod is not expected to call from handler layer for now, so I think it is not needed to add this.

sanposhiho · 2022-01-08T11:31:44Z

pod/pod.go

+// DelteAllScheduledPod deletes all scheduled pods.
+func (s *Service) DeleteAllScheduledPod(ctx context.Context) error {


nits: DeleteAllScheduledPod → DeleteAllScheduledPods

Suggested change

// DelteAllScheduledPod deletes all scheduled pods.

func (s *Service) DeleteAllScheduledPod(ctx context.Context) error {

// DeleteAllScheduledPods deletes all scheduled pods.

func (s *Service) DeleteAllScheduledPods(ctx context.Context) error {

sanposhiho · 2022-01-08T11:52:14Z

@sivchari

On my second thought, it is better to implement DeleteCollection in pod service and use it from node service.
DeleteCollection method should be like this:

func (s *Service) DeleteCollection(ctx context.Context, listOpts metav1.ListOptions) error {

Yes, we can delete all Pods with this DeleteCollection method, but I think DeleteAll on pod service is still needed because there is no uniformity in using the DeleteAll method to delete the resources of other services, but using this DeleteCollection for Pod.
Of course, DeleteAll should be changed to use DeleteCollection internally to make it dry.

What do you think about this? Could you re-implement like the above if no objections? 🙇

sanposhiho · 2022-01-14T10:25:15Z

/assign

sivchari · 2022-01-20T15:17:15Z

@sanposhiho
I refactored this PR and added pod test.

sanposhiho

Thanks!!! One comment.

sanposhiho · 2022-01-21T09:03:04Z

persistentvolume/persistentvolume.go

@@ -62,3 +62,12 @@ func (s *Service) Delete(ctx context.Context, name string) error {

 	return nil
 }
+
+// DeleteCollection deletes all persistentVolumes.


I think it's better to try to make interfaces of all services same.
Currently, the pod service's DeleteCollection only accepts metav1.ListOptions arg. So, let's add ListOptions to other service's DeleteCollection arg.

Ok, I fixed it.

I know this commit is failed.
I fixed it later.

sanposhiho · 2022-01-22T09:55:00Z

/test pull-kube-scheduler-simulator-backend-unit-test
/test pull-kube-scheduler-simulator-backend-lint

sivchari · 2022-01-24T13:47:38Z

@sanposhiho

I fixed. Please review 🙏

sanposhiho

Thanks ❤️
One comment from me.

sanposhiho · 2022-01-27T11:35:43Z

node/node.go

+		lopts := metav1.ListOptions{
+			FieldSelector: "spec.nodeName=" + n.Name,
+		}
+		if err := s.podService.DeleteCollection(ctx, lopts); err != nil {
+			return xerrors.Errorf("failed to delete pods on node %s: %w", n.Name, err)
+		}
+
+		// delete specific node
+		if err := s.client.CoreV1().Nodes().Delete(ctx, n.Name, metav1.DeleteOptions{}); err != nil {
+			return xerrors.Errorf("delete node: %w", err)
+		}


process these part in parallel with errgroup or waitgroup

Thanks ! I agree with you.

sanposhiho

Thanks.

sanposhiho · 2022-01-29T05:59:56Z

node/node.go

@@ -90,3 +92,33 @@ func (s *Service) Delete(ctx context.Context, name string) error {
 	}
 	return nil
 }
+
+// DeleteCollection deletes all nodes.


One nits:
Please change other services doc as well

Suggested change

// DeleteCollection deletes all nodes.

// DeleteCollection deletes Nodes according to the list options.

// And it also deletes Pods scheduled on those Nodes.

Thanks. I fixed it.

sanposhiho

Looks good. Thanks

/lgtm
/approve

k8s-ci-robot · 2022-01-29T09:29:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sanposhiho, sivchari

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~~ [sanposhiho]

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

sivchari added 2 commits December 20, 2021 02:41

impl delete all resources

7fb47db

update each interface

134ea53

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 19, 2021

k8s-ci-robot requested review from alculquicondor and sanposhiho December 19, 2021 17:59

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 19, 2021

sanposhiho reviewed Dec 20, 2021

View reviewed changes

sanposhiho mentioned this pull request Dec 20, 2021

reset scheduler and all resources #67

Closed

k8s-ci-robot changed the title ~~Feature add resources delete method~~ add resources "delete all" method in each service. Dec 20, 2021

k8s-ci-robot changed the title ~~add resources "delete all" method in each service.~~ add resources "delete all" method in each service Dec 20, 2021

sanposhiho mentioned this pull request Dec 20, 2021

Add reset button to reset all resources and scheduler configuration #26

Closed

sivchari added 3 commits December 21, 2021 01:59

update interface

2ee215f

update signature and log format

17ab3a3

add delete all test of node

e630aa8

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 20, 2021

fix typo

ab2aa35

sivchari force-pushed the feature-add-resources-delete-method branch from 4c70044 to ab2aa35 Compare December 20, 2021 17:15

sanposhiho reviewed Dec 21, 2021

View reviewed changes

fix for review

b89cc50

sanposhiho reviewed Dec 22, 2021

View reviewed changes

sivchari added 2 commits December 22, 2021 15:06

fix subtest name and delete unnecessary code

84f3e29

modify delete all pods method

762bfb0

sanposhiho reviewed Jan 8, 2022

View reviewed changes

k8s-ci-robot assigned sanposhiho Jan 14, 2022

sivchari added 4 commits January 20, 2022 23:14

fix interface

d059edb

fix comment

1a19929

fix node service test

417f8de

add pod test

53bc8dc

sanposhiho reviewed Jan 21, 2022

View reviewed changes

sivchari added 5 commits January 23, 2022 00:53

fix interface

0115af4

fix logic

d65e090

fix test

6e15fe7

add param

703ebd1

fix lint

f1579d0

sanposhiho requested changes Jan 27, 2022

View reviewed changes

sivchari added 2 commits January 28, 2022 01:11

add errgroup

2fa99f9

fix lint

f9fdba5

sanposhiho reviewed Jan 29, 2022

View reviewed changes

fix method description

4373b32

sanposhiho approved these changes Jan 29, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2022

k8s-ci-robot merged commit 6021a87 into kubernetes-sigs:master Jan 29, 2022

	return xerrors.Errorf("delete nodes: %w", err)
	return xerrors.Errorf("delete all nodes: %w", err)

	return xerrors.Errorf("delete persistentVolumes: %w", err)
	return xerrors.Errorf("delete all persistentVolumes: %w", err)

	return fmt.Errorf("delete pods: %w", err)
	return fmt.Errorf("delete all pods: %w", err)

	return xerrors.Errorf("delete priorityClasses: %w", err)
	return xerrors.Errorf("delete all priorityClasses: %w", err)

	return xerrors.Errorf("delete storageClasss: %w", err)
	return xerrors.Errorf("delete all storageClasses: %w", err)

	return xerrors.Errorf("delete all nodes: %w", err)
	return xerrors.Errorf("delete all pods: %w", err)

	name: "fail to delete no nodes",
	name: "fail if deleting all pods returns error",

	return xerrors.Errorf("delete all pods: %w", err)
	return xerrors.Errorf("delete all scheduled pods: %w", err)

	name: "fail if delteing all pods returns error",
	name: "fail if deleting all pods returns error",

	// DeleteAllPod deletes all pods.
	// DeleteAll deletes all pods.

	// DeleteAll deletes all storageClassses.
	// DeleteAll deletes all storageClasses.

		// DelteAllScheduledPod deletes all scheduled pods.
		func (s *Service) DeleteAllScheduledPod(ctx context.Context) error {

	// DeleteCollection deletes all nodes.
	// DeleteCollection deletes Nodes according to the list options.
	// And it also deletes Pods scheduled on those Nodes.

add resources "delete all" method in each service #68

add resources "delete all" method in each service #68

Conversation

sivchari commented Dec 19, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Choose a reason for hiding this comment

sanposhiho Dec 20, 2021 • edited Loading

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

sanposhiho commented Dec 20, 2021

sanposhiho commented Dec 20, 2021 • edited Loading

sanposhiho commented Dec 20, 2021

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

sanposhiho Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivchari commented Jan 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Jan 8, 2022 • edited Loading

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

sanposhiho Jan 8, 2022 • edited Loading

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

sanposhiho Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

sivchari commented Jan 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho commented Jan 8, 2022 • edited Loading

sanposhiho commented Jan 14, 2022

sivchari commented Jan 20, 2022 • edited Loading

sanposhiho left a comment

Choose a reason for hiding this comment

sanposhiho Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho commented Jan 22, 2022

sivchari commented Jan 24, 2022

sanposhiho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 29, 2022

sanposhiho Dec 20, 2021 •

edited

Loading

sanposhiho commented Dec 20, 2021 •

edited

Loading

sanposhiho Dec 22, 2021 •

edited

Loading

sanposhiho Jan 8, 2022 •

edited

Loading

sanposhiho Jan 8, 2022 •

edited

Loading

sanposhiho Jan 8, 2022 •

edited

Loading

sanposhiho commented Jan 8, 2022 •

edited

Loading

sivchari commented Jan 20, 2022 •

edited

Loading

sanposhiho Jan 21, 2022 •

edited

Loading