-
Notifications
You must be signed in to change notification settings - Fork 209
Adds dapr scheduler
.
#1559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Adds dapr scheduler
.
#1559
Conversation
See dapr/cli#1559 Signed-off-by: joshvanl <me@joshvanl.dev>
Docs: TODO ```bash $ dapr scheduler Scheduler management commands. Use -k to target a Kubernetes Dapr cluster. Usage: dapr scheduler [command] Aliases: scheduler, sched Available Commands: delete Delete one of more jobs from scheduler." Job names are formatted by their type, app ID, then identifier. Actor reminders require the actor type, actor ID, then reminder name, separated by /. Workflow reminders require the app ID, instance ID, then reminder name, separated by /. Accepts multiple names. delete-all Delete all scheduled jobs in the specified namespace of a particular filter. Accepts a single key as an argument. Deletes all jobs which match the filter key. export Export all jobs and actor reminders to a binary file, including the tracked count. get Get a scheduled app job or actor reminder in Scheduler. Job names are formatted by their type, app ID, then identifier. Actor reminders require the actor type, actor ID, then reminder name, separated by /. Workflow reminders require the app ID, instance ID, then reminder name, separated by /. Activity reminders require the app ID, activity ID, separated by /. Accepts multiple names. import Import all jobs and actor reminders from a binary file generated by 'dapr scheduler export'. list List scheduled jobs in Scheduler. Flags: -h, --help help for scheduler -k, --kubernetes Perform scheduler command on a Kubernetes Dapr cluster -n, --namespace string Namespace of the Dapr application (default "default") --scheduler-namespace string Kubernetes namespace where the scheduler is deployed, only relevant if --kubernetes is set (default "dapr-system") Global Flags: --log-as-json Log output in JSON format --runtime-path string The path to the dapr runtime installation directory Use "dapr scheduler [command] --help" for more information about a command. ``` ``` $ dapr scheduler delete --help Delete one of more jobs from scheduler." Job names are formatted by their type, app ID, then identifier. Actor reminders require the actor type, actor ID, then reminder name, separated by /. Workflow reminders require the app ID, instance ID, then reminder name, separated by /. Accepts multiple names. Usage: dapr scheduler delete [flags] Aliases: delete, d, del Examples: dapr scheduler delete app/my-app-id/my-job-name dapr scheduler delete actor/my-actor-type/my-actor-id/my-reminder-name dapr scheduler delete workflow/my-app-id/my-instance-id/my-workflow-reminder-name Flags: -h, --help help for delete Global Flags: -k, --kubernetes Perform scheduler command on a Kubernetes Dapr cluster --log-as-json Log output in JSON format -n, --namespace string Namespace of the Dapr application (default "default") --runtime-path string The path to the dapr runtime installation directory --scheduler-namespace string Kubernetes namespace where the scheduler is deployed, only relevant if --kubernetes is set (default "dapr-system") ``` ``` $ dapr scheduler delete-all --help Delete all scheduled jobs in the specified namespace of a particular filter. Accepts a single key as an argument. Deletes all jobs which match the filter key. Usage: dapr scheduler delete-all [flags] Aliases: delete-all, da, delall Examples: dapr scheduler delete-all all dapr scheduler delete-all app dapr scheduler delete-all app/my-app-id dapr scheduler delete-all actor/my-actor-type dapr scheduler delete-all actor/my-actor-type/my-actor-id dapr scheduler delete-all workflow dapr scheduler delete-all workflow/my-app-id dapr scheduler delete-all workflow/my-app-id/my-workflow-id Flags: -h, --help help for delete-all Global Flags: -k, --kubernetes Perform scheduler command on a Kubernetes Dapr cluster --log-as-json Log output in JSON format -n, --namespace string Namespace of the Dapr application (default "default") --runtime-path string The path to the dapr runtime installation directory --scheduler-namespace string Kubernetes namespace where the scheduler is deployed, only relevant if --kubernetes is set (default "dapr-system") ``` ``` $ dapr scheduler export --help Export jobs and actor reminders which are scheduled in Scheduler. Can later be imported using 'dapr scheduler import'. dapr scheduler export -o output.bin Usage: dapr scheduler export [flags] Flags: -h, --help help for export -o, --output-file string Output binary file to export jobs and actor reminders to. Global Flags: -k, --kubernetes Perform scheduler command on a Kubernetes Dapr cluster --log-as-json Log output in JSON format -n, --namespace string Namespace of the Dapr application (default "default") --runtime-path string The path to the dapr runtime installation directory --scheduler-namespace string Kubernetes namespace where the scheduler is deployed, only relevant if --kubernetes is set (default "dapr-system") ``` ``` $ dapr scheduler get --help Get a scheduled app job or actor reminder in Scheduler. Job names are formatted by their type, app ID, then identifier. Actor reminders require the actor type, actor ID, then reminder name, separated by /. Workflow reminders require the app ID, instance ID, then reminder name, separated by /. Activity reminders require the app ID, activity ID, separated by /. Accepts multiple names. Usage: dapr scheduler get [flags] Aliases: get, g, ge Examples: dapr scheduler get app/my-app-id/my-job-name dapr scheduler get actor/my-actor-type/my-actor-id/my-reminder-name dapr scheduler get workflow/my-app-id/my-instance-id/my-workflow-reminder-name dapr scheduler get activity/my-app-id/xyz::0::1 Flags: -h, --help help for get -o, --output string Output format. One of short, wide, yaml, json (default "short") Global Flags: -k, --kubernetes Perform scheduler command on a Kubernetes Dapr cluster --log-as-json Log output in JSON format -n, --namespace string Namespace of the Dapr application (default "default") --runtime-path string The path to the dapr runtime installation directory --scheduler-namespace string Kubernetes namespace where the scheduler is deployed, only relevant if --kubernetes is set (default "dapr-system") ``` ``` $ dapr scheduler import --help Import jobs and actor reminders to Scheduler from a binary file generated by 'dapr scheduler export'. dapr scheduler import -f export.bin Usage: dapr scheduler import [flags] Flags: -h, --help help for import -f, --input-file string Input file to import jobs and actor reminders from. Global Flags: -k, --kubernetes Perform scheduler command on a Kubernetes Dapr cluster --log-as-json Log output in JSON format -n, --namespace string Namespace of the Dapr application (default "default") --runtime-path string The path to the dapr runtime installation directory --scheduler-namespace string Kubernetes namespace where the scheduler is deployed, only relevant if --kubernetes is set (default "dapr-system") ``` ``` $ dapr scheduler list --help List scheduled jobs in Scheduler. Usage: dapr scheduler list [flags] Flags: --filter string Filter jobs by type. Supported values are all, app, actor, workflow, activity (default "all") -h, --help help for list -o, --output string Output format. One of short, wide, yaml, json (default "short") Global Flags: -k, --kubernetes Perform scheduler command on a Kubernetes Dapr cluster --log-as-json Log output in JSON format -n, --namespace string Namespace of the Dapr application (default "default") --runtime-path string The path to the dapr runtime installation directory --scheduler-namespace string Kubernetes namespace where the scheduler is deployed, only relevant if --kubernetes is set (default "dapr-system") ``` Signed-off-by: joshvanl <me@joshvanl.dev>
Adds uninstall as start test funcs Increase e2e test timeout to 30m Prints output of scheduler run log Increases timeout for waiting for list output Set scheduler & placement host address to 127.0.0.1 in tests Adds error log output to scheduler cmd run tests Hard code gRPC port Skip scheduler tests in slim mode Adds registration log lines Change app port Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: joshvanl <me@joshvanl.dev>
if _, err := os.Stat(opts.TargetFile); !errors.Is(err, os.ErrNotExist) { | ||
if err == nil { | ||
return fmt.Errorf("file '%s' already exists", opts.TargetFile) | ||
} | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience most CLI don't check for this and just overwrite the file if it exists, should we do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have also seen this way tbf- I would rather be safer than not.
Co-authored-by: Albert Callarisa <albert@acroca.com> Signed-off-by: Josh van Leeuwen <me@joshvanl.dev>
Co-authored-by: Albert Callarisa <albert@acroca.com> Signed-off-by: Josh van Leeuwen <me@joshvanl.dev>
Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: joshvanl <me@joshvanl.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly lgtm, just a few nits
if len(resp.Responses) == 0 || resp.Responses[0].GetResponseDeleteRange().Deleted == 0 { | ||
return fmt.Errorf("no job with key '%s' found in namespace '%s'", key, opts.DaprNamespace) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to error here? If the command receives multiple keys and one has no jobs why we do not continue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you try the command out, it ends up being a nice error string output which I think makes more sense in this scenario.
case "all": | ||
if len(split) != 1 { | ||
return fmt.Errorf("invalid key format: %s", key) | ||
} | ||
paths = []string{ | ||
fmt.Sprintf("dapr/jobs/app||%s||", opts.DaprNamespace), | ||
fmt.Sprintf("dapr/jobs/actorreminder||%s||", opts.DaprNamespace), | ||
fmt.Sprintf("dapr/counters/app||%s||", opts.DaprNamespace), | ||
fmt.Sprintf("dapr/counters/actorreminder||%s||", opts.DaprNamespace), | ||
} | ||
case "app": | ||
switch len(split) { | ||
case 1: | ||
paths = []string{ | ||
fmt.Sprintf("dapr/jobs/app||%s||", opts.DaprNamespace), | ||
fmt.Sprintf("dapr/counters/app||%s||", opts.DaprNamespace), | ||
} | ||
case 2: | ||
paths = []string{ | ||
fmt.Sprintf("dapr/jobs/app||%s||%s||", opts.DaprNamespace, split[1]), | ||
fmt.Sprintf("dapr/counters/app||%s||%s||", opts.DaprNamespace, split[1]), | ||
} | ||
default: | ||
return fmt.Errorf("invalid key format: %s", key) | ||
} | ||
|
||
case "actor": | ||
switch len(split) { | ||
case 2: | ||
paths = []string{ | ||
fmt.Sprintf("dapr/jobs/actorreminder||%s||%s||", opts.DaprNamespace, split[1]), | ||
fmt.Sprintf("dapr/counters/actorreminder||%s||%s||", opts.DaprNamespace, split[1]), | ||
} | ||
case 3: | ||
paths = []string{ | ||
fmt.Sprintf("dapr/jobs/actorreminder||%s||%s||%s||", opts.DaprNamespace, split[1], split[2]), | ||
fmt.Sprintf("dapr/counters/actorreminder||%s||%s||%s||", opts.DaprNamespace, split[1], split[2]), | ||
} | ||
default: | ||
return fmt.Errorf("invalid key format: %s", key) | ||
} | ||
|
||
case "workflow": | ||
switch len(split) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rather prefer this command to be more specific instead of building paths by length. So maybe something like
dapr scheduler delete-all --type all
dapr scheduler delete-all --type app
dapr scheduler delete-all --type app --app-id my-app-id
dapr scheduler delete-all --type actor --actor-type my-actor-type
dapr scheduler delete-all --type actor --actor-type my-actor-type --actor-id my-actor-id
dapr scheduler delete-all --type workflow
dapr scheduler delete-all --type workflow --app-id my-app-id
dapr scheduler delete-all --type workflow --app-id my-app-id --workflow-id my-workflow-id
what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This becomes incredibly verbose, and you end up with lots of flags which are mutually exclusive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will add path auto completion (tabbing) eventually but best we get this PR in and released ASAP.
dapr scheduler delete-all all | ||
dapr scheduler delete-all app | ||
dapr scheduler delete-all app/my-app-id | ||
dapr scheduler delete-all actor/my-actor-type | ||
dapr scheduler delete-all actor/my-actor-type/my-actor-id | ||
dapr scheduler delete-all workflow | ||
dapr scheduler delete-all workflow/my-app-id | ||
dapr scheduler delete-all workflow/my-app-id/my-workflow-id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this command run as dry-run
and only print the number of records to delete and run it with --execute
when you are sure that is the number or reminder you expect to delete? Probably the same for delete
command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but then cli becomes pretty annoying to use. I would rather have a --dry-run
flag.
if kvs := resp.Responses[1].GetResponseRange().Kvs; len(kvs) > 0 { | ||
if err = proto.Unmarshal(kvs[0].Value, &storedC); err != nil { | ||
return nil, err | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if len(kvs) > 0 we only Unmarshal kvs[0], can it be more than 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
sort.SliceStable(list, func(i, j int) bool { | ||
if list[i].Namespace == list[j].Namespace { | ||
if list[i].Begin.Equal(list[j].Begin) { | ||
return list[i].Name < list[j].Name | ||
} | ||
return list[i].Begin.Before(list[j].Begin) | ||
} | ||
return list[i].Namespace < list[j].Namespace | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort.SliceStable(list, func(i, j int) bool { | |
if list[i].Namespace == list[j].Namespace { | |
if list[i].Begin.Equal(list[j].Begin) { | |
return list[i].Name < list[j].Name | |
} | |
return list[i].Begin.Before(list[j].Begin) | |
} | |
return list[i].Namespace < list[j].Namespace | |
}) | |
sort.SliceStable(list, func(i, j int) bool { | |
if list[i].Namespace < list[j].Namespace { | |
return true | |
} | |
if list[i].Begin.Before(list[j].Begin) { | |
return true | |
} | |
return list[i].Name < list[j].Name | |
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ups, you are right 👍 , I did not like the nested ifs
Signed-off-by: joshvanl <me@joshvanl.dev>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1559 +/- ##
==========================================
- Coverage 21.11% 18.17% -2.95%
==========================================
Files 44 53 +9
Lines 5285 6141 +856
==========================================
Hits 1116 1116
- Misses 4083 4939 +856
Partials 86 86 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still wondering if the delete-all
can be improved with a diff design, but outside of that am fine with this to get user feedback on. See my comments here about it.
For the sake of getting this out there and tested and more user feedback I am fine with getting this merged as-is and iterating on it going forward based on feedback.
Docs: dapr/docs#4909