Refactor ExportService and ReplicateExistingClusterService #300

196Ikuchil · 2023-04-29T04:00:40Z

What type of PR is this?

/area simulator
/kind cleanup

What this PR does / why we need it:

The name of ExportService and ReplicateExisitngClusterService didn't represent the function well.
Renamed these.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Mainly, renamed as

ExportService → ShapshotService
- Export → Snap
- Import → Load
ReplicateExistingClusterService → ClusterResourceImportService

/label tide/merge-method-squash

sanposhiho · 2023-04-29T08:22:30Z

simulator/clusterresourceimporter/importer.go

+// ImportClusterResources gets resources from the target cluster via exportService
+// and then apply those resources to the simulator.
+// Note: this method doesn't handle scheduler configuration.
+// If you want to use their scheduler configuration, you need to set config of `KUBE_SCHEDULER_CONFIG_PATH`.


the configuration via env vers is deprecated; so describe this with the scheduler config file

you probably forgot it.

@sanposhiho
Sorry..🙏
How about this?
afd265f

sanposhiho · 2023-04-29T08:23:51Z

simulator/server/di/di.go

 }

 // NewDIContainer initializes Container.
 // It initializes all service and puts to Container.
-// If externalImportEnabled is false, the simulator will not use externalClient and will not create ReplicateExistingClusterService.
+// If externalImportEnabled is false, the simulator will not use externalClient and will not create ImportClusterResourceService.


to be easier to read.

Suggested change

// If externalImportEnabled is false, the simulator will not use externalClient and will not create ImportClusterResourceService.

// Only when externalImportEnabled is true, the simulator uses externalClient and creates ImportClusterResourceService.

sanposhiho · 2023-04-29T08:24:33Z

simulator/server/di/interface.go

-	Export(ctx context.Context, opts ...export.Option) (*export.ResourcesForExport, error)
-	Import(ctx context.Context, resources *export.ResourcesForImport, opts ...export.Option) error
-	IgnoreErr() export.Option
+type SnapshotService interface {


describe it's related to export/import feature.

sanposhiho · 2023-04-29T08:26:00Z

simulator/server/server.go

+	v1.GET("/export", exportHandler.Save)
+	v1.POST("/import", exportHandler.Load)


I prefer Snap instead of Save.
Save can be mistakenly interpreted as "take a snapshot file and save it to somewhere, or save all resources into kube-apiserver".

sanposhiho · 2023-04-29T08:27:39Z

simulator/server/server.go

@@ -33,7 +33,7 @@ func NewSimulatorServer(cfg *config.Config, dic *di.Container) *SimulatorServer

 	// initialize each handler
 	schedulercfgHandler := handler.NewSchedulerConfigHandler(dic.SchedulerService())
-	exportHandler := handler.NewExportHandler(dic.ExportService())
+	exportHandler := handler.NewSnapshotHandler(dic.ExportService())


rename: exportHandler -> snapshotHandler

sanposhiho · 2023-04-29T08:33:56Z

simulator/snapshot/snapshot.go

@@ -52,8 +52,8 @@ type ResourcesForExport struct {
 	Namespaces      []corev1.Namespace                   `json:"namespaces"`
 }

-// ResourcesForImport denotes all resources and scheduler configuration for import.
-type ResourcesForImport struct {
+// ResourcesForLoad denotes indicates all resources and scheduler configuration to be loaded.


Suggested change

// ResourcesForLoad denotes indicates all resources and scheduler configuration to be loaded.

// ResourcesForLoad indicates all resources and scheduler configuration to be loaded.

196Ikuchil · 2023-04-29T15:35:59Z

@sanposhiho
Thanks 👍 Fixed.

sanposhiho · 2023-04-30T03:57:02Z

simulator/snapshot/snapshot.go

 	schedulerService     SchedulerService
 }

-// ResourcesForExport denotes all resources and scheduler configuration for export.
-type ResourcesForExport struct {
+// ResourcesForSnap indicates all resources and scheduler configuration to be saved.


nit

Suggested change

// ResourcesForSnap indicates all resources and scheduler configuration to be saved.

// ResourcesForSnap indicates all resources and scheduler configuration to be snapped.

sanposhiho · 2023-04-30T03:57:37Z

simulator/snapshot/snapshot.go

+				return xerrors.Errorf("restart scheduler with Loaded configuration: %w", err)
 			}
-			klog.Info("The scheduler configuration hasn't been imported because of an external scheduler is enabled.")
+			klog.Info("The scheduler configuration hasn't been Loaded because of an external scheduler is enabled.")


nits: Loaded -> loaded

196Ikuchil · 2023-04-30T05:14:04Z

@sanposhiho
Thanks🙏

sanposhiho · 2023-04-30T05:22:53Z

You missed this
https://github.com/kubernetes-sigs/kube-scheduler-simulator/pull/300/files#r1181016673

sanposhiho

/lgtm
/approve

Thanks!

k8s-ci-robot · 2023-05-01T03:01:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 196Ikuchil, sanposhiho

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

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

196Ikuchil added 2 commits April 29, 2023 11:34

fix:export service

2c07b9e

refactor:replicateexistingcluster and di

242cb0c

k8s-ci-robot added area/simulator Issues or PRs related to the simulator. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. labels Apr 29, 2023

k8s-ci-robot requested a review from alculquicondor April 29, 2023 04:00

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Apr 29, 2023

k8s-ci-robot requested a review from sanposhiho April 29, 2023 04:00

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 29, 2023

sanposhiho reviewed Apr 29, 2023

View reviewed changes

196Ikuchil added 3 commits April 30, 2023 00:24

for review

e6e0360

fix:test case name

eaa403e

refactor:rename Save to Snap

5f7f56f

sanposhiho reviewed Apr 30, 2023

View reviewed changes

fix some nits

d915cb4

fix:comment

afd265f

sanposhiho approved these changes May 1, 2023

View reviewed changes

k8s-ci-robot assigned sanposhiho May 1, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2023

k8s-ci-robot merged commit f982f54 into kubernetes-sigs:master May 1, 2023

196Ikuchil mentioned this pull request May 6, 2023

fix: update the documentation to run the simulator by pulling images from the registry #298

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ExportService and ReplicateExistingClusterService #300

Refactor ExportService and ReplicateExistingClusterService #300

196Ikuchil commented Apr 29, 2023 •

edited

Loading

sanposhiho Apr 29, 2023

sanposhiho Apr 30, 2023

196Ikuchil Apr 30, 2023

sanposhiho Apr 29, 2023

sanposhiho Apr 29, 2023

sanposhiho Apr 29, 2023

sanposhiho Apr 29, 2023

sanposhiho Apr 29, 2023

196Ikuchil commented Apr 29, 2023

sanposhiho Apr 30, 2023

sanposhiho Apr 30, 2023

196Ikuchil commented Apr 30, 2023

sanposhiho commented Apr 30, 2023

sanposhiho left a comment

k8s-ci-robot commented May 1, 2023

	// If externalImportEnabled is false, the simulator will not use externalClient and will not create ImportClusterResourceService.
	// Only when externalImportEnabled is true, the simulator uses externalClient and creates ImportClusterResourceService.

		v1.GET("/export", exportHandler.Save)
		v1.POST("/import", exportHandler.Load)

	// ResourcesForLoad denotes indicates all resources and scheduler configuration to be loaded.
	// ResourcesForLoad indicates all resources and scheduler configuration to be loaded.

	// ResourcesForSnap indicates all resources and scheduler configuration to be saved.
	// ResourcesForSnap indicates all resources and scheduler configuration to be snapped.

Refactor ExportService and ReplicateExistingClusterService #300

Refactor ExportService and ReplicateExistingClusterService #300

Conversation

196Ikuchil commented Apr 29, 2023 • edited Loading

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

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

196Ikuchil commented Apr 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

196Ikuchil commented Apr 30, 2023

sanposhiho commented Apr 30, 2023

sanposhiho left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 1, 2023

196Ikuchil commented Apr 29, 2023 •

edited

Loading