Add reset service #105

sivchari · 2022-02-11T17:08:49Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

I added reset service to reset resources.

Which issue(s) this PR fixes:

Fixes #26

Special notes for your reviewer:

/label tide/merge-method-squash

k8s-ci-robot · 2022-02-11T17:08:58Z

Hi @sivchari. Thanks for your PR.

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

sivchari · 2022-02-11T17:09:40Z

/assign @sanposhiho

sanposhiho · 2022-02-12T14:30:59Z

/assign
/ok-to-test
/retitle Add reset service

sanposhiho

Thanks!

sanposhiho · 2022-02-12T14:32:05Z

server/handler/schedulerconfig.go

@@ -39,12 +39,3 @@ func (h *SchedulerConfigHandler) ApplySchedulerConfig(c echo.Context) error {

 	return c.NoContent(http.StatusAccepted)
 }
-
-func (h *SchedulerConfigHandler) ResetScheduler(c echo.Context) error {


sanposhiho · 2022-02-12T14:36:09Z

reset/reset.go

+type NodeService interface {
+	DeleteCollection(ctx context.Context, lopts metav1.ListOptions) error
+}
+
+type PersistentVolumeService interface {
+	DeleteCollection(ctx context.Context, lopts metav1.ListOptions) error
+}
+
+type PersistentVolumeClaimService interface {
+	DeleteCollection(ctx context.Context, lopts metav1.ListOptions) error
+}
+
+type StorageClassService interface {
+	DeleteCollection(ctx context.Context, lopts metav1.ListOptions) error
+}
+
+type PriorityClassService interface {
+	DeleteCollection(ctx context.Context, lopts metav1.ListOptions) error
+}


They all have the same interface. So, we can use just one interface (name will be like DeleteService?) for better abstraction.
That way, when new resource is supported in the future, the changes to the Reset service will be small.

sanposhiho · 2022-02-12T14:37:53Z

reset/reset.go

+
+// Reset cleans up all resources and scheduler configuration.
+func (s *Service) Reset(ctx context.Context) error {
+	lopts := metav1.ListOptions{


Looks not needed. Just passing emptyListOpts to all services should be enough.

sanposhiho · 2022-02-12T14:39:21Z

@sivchari
Could you add the brief doc to this api.md?
https://github.com/kubernetes-sigs/kube-scheduler-simulator/blob/master/docs/api.md

/kind api-change

sanposhiho · 2022-02-12T14:42:37Z

reset/reset.go

+type Service struct {
+	client       clientset.Interface
+	nodeService  NodeService
+	pvService    PersistentVolumeService
+	pvcService   PersistentVolumeClaimService
+	scSerivce    StorageClassService
+	pcService    PriorityClassService
+	schedService SchedulerService
+}


With this change, the field will be like this:

Suggested change

type Service struct {

client clientset.Interface

nodeService NodeService

pvService PersistentVolumeService

pvcService PersistentVolumeClaimService

scSerivce StorageClassService

pcService PriorityClassService

schedService SchedulerService

}

type Service struct {

client clientset.Interface

// deleteServices has the all services for each resource.

// key: service name.

deleteServices map[string]DeleteService

schedService SchedulerService

}

sivchari · 2022-02-12T14:52:52Z

Thank !

sanposhiho · 2022-02-12T16:14:37Z

reset/reset.go

+	emptyListOpts := metav1.ListOptions{}
+	for _, ds := range s.deleteServices {
+		if err := ds.DeleteCollection(ctx, emptyListOpts); err != nil {
+			return err


Please wrap error with deleteServices's key. Like:

xerrors.Errorf("delete all %s: %w", key, err)

sanposhiho · 2022-02-12T16:15:06Z

reset/reset.go

+	// We need emptyListOpts to satisfy interface.
+	emptyListOpts := metav1.ListOptions{}
+	for _, ds := range s.deleteServices {
+		if err := ds.DeleteCollection(ctx, emptyListOpts); err != nil {


Do it in parallel with error group

sanposhiho · 2022-02-12T16:16:17Z

reset/reset.go

+	// We need emptyListOpts to satisfy interface.
+	emptyListOpts := metav1.ListOptions{}


I think we can just remove this emptyListOpts.

sanposhiho · 2022-02-12T16:16:26Z

reset/reset.go

+			return err
+		}
+	}
+	return s.schedService.ResetScheduler()


Please wrap error as well

sanposhiho · 2022-02-12T16:17:01Z

server/di/di.go

@@ -46,6 +48,15 @@ func NewDIContainer(client clientset.Interface, restclientCfg *restclient.Config

 	c.priorityClassService = priorityclass.NewPriorityClassService(client)
 	c.exportService = export.NewExportService(client, c.podService, c.nodeService, c.pvService, c.pvcService, c.storageClassService, c.priorityClassService, c.schedulerService)
+
+	servicem := map[string]reset.DeleteService{
+		"node": c.nodeService,


You miss pod service maybe?

sanposhiho · 2022-02-12T16:18:03Z

server/di/di.go

@@ -46,6 +48,15 @@ func NewDIContainer(client clientset.Interface, restclientCfg *restclient.Config

 	c.priorityClassService = priorityclass.NewPriorityClassService(client)
 	c.exportService = export.NewExportService(client, c.podService, c.nodeService, c.pvService, c.pvcService, c.storageClassService, c.priorityClassService, c.schedulerService)
+
+	servicem := map[string]reset.DeleteService{


nits:

Suggested change

servicem := map[string]reset.DeleteService{

deleteServices := map[string]reset.DeleteService{

sivchari · 2022-02-13T14:24:20Z

I fixed it :)

sanposhiho

Sorry, one more nits 🙏 Looks good otherwise.

sanposhiho · 2022-02-14T03:34:30Z

server/di/di.go

+		"pv":   c.pvService,
+		"pvc":  c.pvcService,
+		"sc":   c.storageClassService,
+		"pc":   c.priorityClassService,


Could you change it to use the full names of resources for better loggings? (like "Persistent volume")

sanposhiho

sorry, "one more nits" again.. 🤦‍♂️

sanposhiho · 2022-02-14T06:22:07Z

docs/api.md


-restart scheduler with default configuration.
+clean up all resources and scheduler configuration.


Suggested change

clean up all resources and scheduler configuration.

clean up all resources and restore the initial scheduler configuration.

(If you didn't pass the initial scheduler configuration via `KUBE_SCHEDULER_CONFIG_PATH`, the default scheduler configuration will be restored.)

Ok, I fixed it.

sanposhiho

Thanks!

/lgtm
/approve

k8s-ci-robot · 2022-02-14T06:28:15Z

[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 8 commits February 12, 2022 02:02

fix conflict

09a784d

delete handler method

a9ae71c

fix conflict

6fc9f75

add mock

cb7c8f3

delete client

9e60188

delet mock

bdc1b02

delete test

ea0fc99

fix

72e40e0

k8s-ci-robot requested review from adtac and alculquicondor February 11, 2022 17:09

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 11, 2022

k8s-ci-robot assigned sanposhiho Feb 11, 2022

sivchari added 3 commits February 12, 2022 02:19

fix lint

d83424f

bit fix

de8aa0d

sort import

7b12920

k8s-ci-robot changed the title ~~Feature add reset method~~ Add reset service Feb 12, 2022

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 Feb 12, 2022

sanposhiho reviewed Feb 12, 2022

View reviewed changes

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Feb 12, 2022

sanposhiho reviewed Feb 12, 2022

View reviewed changes

sivchari added 3 commits February 13, 2022 00:08

fix

bc4209a

fix

50f06ab

fix

ea71e1d

sanposhiho reviewed Feb 13, 2022

View reviewed changes

fix review point

ca9bd21

sanposhiho reviewed Feb 14, 2022

View reviewed changes

fix keyname for better logging

12b2c14

sanposhiho reviewed Feb 14, 2022

View reviewed changes

fix docs

9bb8b34

sanposhiho approved these changes Feb 14, 2022

View reviewed changes

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

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

k8s-ci-robot merged commit fc420bc into kubernetes-sigs:master Feb 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reset service #105

Add reset service #105

sivchari commented Feb 11, 2022

k8s-ci-robot commented Feb 11, 2022

sivchari commented Feb 11, 2022

sanposhiho commented Feb 12, 2022

sanposhiho left a comment

sanposhiho Feb 12, 2022

sanposhiho Feb 12, 2022 •

edited

Loading

sanposhiho Feb 12, 2022

sanposhiho commented Feb 12, 2022

sanposhiho Feb 12, 2022

sivchari commented Feb 12, 2022

sanposhiho Feb 12, 2022 •

edited

Loading

sanposhiho Feb 12, 2022

sanposhiho Feb 12, 2022

sanposhiho Feb 12, 2022

sanposhiho Feb 12, 2022

sanposhiho Feb 12, 2022 •

edited

Loading

sivchari commented Feb 13, 2022

sanposhiho left a comment

sanposhiho Feb 14, 2022

sanposhiho left a comment •

edited

Loading

sanposhiho Feb 14, 2022

sivchari Feb 14, 2022

sanposhiho left a comment

k8s-ci-robot commented Feb 14, 2022

		// We need emptyListOpts to satisfy interface.
		emptyListOpts := metav1.ListOptions{}

	servicem := map[string]reset.DeleteService{
	deleteServices := map[string]reset.DeleteService{


		restart scheduler with default configuration.
		clean up all resources and scheduler configuration.

	clean up all resources and scheduler configuration.
	clean up all resources and restore the initial scheduler configuration.
	(If you didn't pass the initial scheduler configuration via `KUBE_SCHEDULER_CONFIG_PATH`, the default scheduler configuration will be restored.)

Add reset service #105

Add reset service #105

Conversation

sivchari commented Feb 11, 2022

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:

k8s-ci-robot commented Feb 11, 2022

sivchari commented Feb 11, 2022

sanposhiho commented Feb 12, 2022

sanposhiho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho commented Feb 12, 2022

Choose a reason for hiding this comment

sivchari commented Feb 12, 2022

sanposhiho Feb 12, 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 Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

sivchari commented Feb 13, 2022

sanposhiho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho left a comment • edited Loading

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 Feb 14, 2022

sanposhiho Feb 12, 2022 •

edited

Loading

sanposhiho Feb 12, 2022 •

edited

Loading

sanposhiho Feb 12, 2022 •

edited

Loading

sanposhiho left a comment •

edited

Loading