NCronJob-Dev / NCronJob

A Job Scheduler sitting on top of IHostedService in dotnet.
https://docs.ncronjob.dev/
MIT License
155 stars 11 forks source link

Runtime modification of scheduled jobs #83

Closed KorsG closed 4 months ago

KorsG commented 5 months ago

Is your feature request related to a problem? Please describe. In the process of examining the possibility to use this library as a replacement for Hangfire in many of our solutions, we miss the option to do runtime modifications of the scheduled jobs.

In our current setup we have bound job config to the "IConfiguration" system, so its possible to edit e.g. an appsettings.json file with options like CronExpression, Enabled/Disabled, Timezone etc., for each job, which automatically reloads during runtime and adds, updates, removes the job to the Hangfire scheduler, without having to restart the host (win-service), which can be very cumbersome in many on-prem deployments.

With this option the job config could be loaded from a remote path/url/database and automatically be applied.

Describe the solution you'd like A way to add/update/remove scheduled jobs at runtime. Possibly via (I)JobRegistry. We would need to fetch a specific JobDefinition via its JobName, so it should also be possible to provide a "custom" JobName when registering Jobs.

Describe alternatives you've considered None

Additional context None

Let me know if I can provide further details ๐Ÿ˜Š

linkdotnet commented 5 months ago

Thanks for the idea!

Given a job that has multiple CRON expressions, would you like to control every single of them or just all of them? So given something like:

Services.AddNCronJob(p => p.AddJob<MyJob>(j => j.WithCronExpression("1 0 * * *).And.WithCronExpression("11 1 * * * ")));

Would you like to be able to update/remove all of them together or one by one?

On top of my head, we can "ask" for a jobname - internally we already have something like that, so it is a matter of exposing. If a JobName is not set, we automatically use the typename.

That said, we might want to offer something like this:

public interface IJobRegistry
{
  public void EnableJob(string jobName); // Important for anonymous jobs as well
  public void EnableJob<TJob>() where TJob : IJob;
  public void EnableJob(Type jobType);

  public void DisableJob(string jobName); // Important for anonymous jobs as well
  public void DisableJob<TJob>() where TJob : IJob;
  public void DisableJob(Type jobType);

  public void AddJob(Action<NCronJobOptionBuilder> builder);

  public void RemoveJob(string jobName);
  public void RemoveJob<TJob>() where TJob : IJob;
  public void RemoveJob(Type jobType);
}

I am not sure what exactly the use case for updating would be? Do you want to change the name/CRON expression at runtime?

That brings me to another point: The code should be the single source of truth - so after restarting the service, we are back to square one. So whatever is written inside your builder.Services.AddNCronJob(...) is the truth.

linkdotnet commented 5 months ago

Another "limitation" that comes to my mind, when you want to add jobs dynamically (if they derive from IJob): They have to live inside the container, so that we can resolve dependencies.

With our minimal API, that isn't a requirement, as we resolve the delegate at runtime anyway.

linkdotnet commented 5 months ago

I have an open PR, that adds the following API surface:

/// <summary>
/// Gives the ability to add, delete or adjust jobs at runtime.
/// </summary>
public interface IRuntimeJobRegistry
{
    /// <summary>
    /// Gives the ability to add a job.
    /// </summary>
    void AddJob(Action<NCronJobOptionBuilder> jobBuilder);

    /// <summary>
    /// Removes the job with the given name.
    /// </summary>
    /// <param name="jobName">The name of the job to remove.</param>
    /// <remarks>If the given job is not found, no exception is thrown.</remarks>
    void RemoveJob(string jobName);

    /// <summary>
    /// Removes all jobs of the given type.
    /// </summary>
    void RemoveJob<TJob>() where TJob : IJob;

    /// <summary>
    /// Removes all jobs of the given type.
    /// </summary>
    void RemoveJob(Type type);
}

Also, you have the ability to add names for a job! So you can call: AddJob<MyJob>(p => p.WithCronExpression("* * * * *").WithName("My Job Name"));

Currently we don't enforce uniqueness in job names.

My question would be: Do you need Disable / Enable on said interface? And further: Do you need to disable jobs inside AddNCronJob? Or broader: What would the gap be?

As an MVP, you could leverage that to meet your needs. You could always just remove a CRON job and re-add with the new parameters.

Maybe we could add something like:

UpdateJob(string job, Action<UpdateBuilder> update);
// Alternatively
UpdateJob(string job, string cronExpression); // Is by job name enough?
UpdateJob(string job, object? parameter);

While this covers the minimum, it might be good enough in 80% of use cases.

KorsG commented 5 months ago

Hi Steven and thank you so much for looking into this - truly awesome! ๐Ÿ˜Ž

1

Given a job that has multiple CRON expressions, would you like to control every single of them or just all of them? So given something like:

Services.AddNCronJob(p => p.AddJob<MyJob>(j => j.WithCronExpression("1 0 * * *).And.WithCronExpression("11 1 * * * ")));

Would you like to be able to update/remove all of them together or one by one?

I would like to control each one, but I feel that's solved by the new IRuntimeJobRegistry.

2

I am not sure what exactly the use case for updating would be? Do you want to change the name/CRON expression at runtime?

Yes, our main use-case for this is actually changing the CRON expression at runtime. To disable jobs in our current Hangfire solution we actually just use the following "Never" CRON " 31 2 *", which is valid, but never fires because its the 31st of February.

3

My question would be: Do you need Disable / Enable on said interface? And further: Do you need to disable jobs inside AddNCronJob? Or broader: What would the gap be?

I don't think we will need a specific Enable/Disable method as we can just the the "Never" CRON, which also makes it possible to add "disabled" jobs inside AddNCronJob if needed.

4

Maybe we could add something like:

UpdateJob(string job, Action<UpdateBuilder> update);
// Alternatively
UpdateJob(string job, string cronExpression); // Is by job name enough?
UpdateJob(string job, object? parameter);

Currently we only need to be able to update the CRON so UpdateJob(string job, string cronExpression); would be enough as an MVP in our case - maybe with a TimeZone overload. I feel that the UpdateBuilder could be a great addition at a later point if needed - maybe it could even support updating multiple CRON expressions/JobOptions.

We could of course just remove and re-add the jobs instead of updating, but in most cases we actually register all jobs in startup to "define" the jobs with Notification handlers etc., so its much "easier" to just change the CRON instead of removing/adding the whole job definition.

5

I would also like to request the option to iterate (CRON) jobs in the IRuntimeJobRegistry, because when the "config" changes, we want to iterate jobs in the registry and remove any that may have been removed from the config. That would also be beneficial for diagnostics and simple UI scenarios.

6

I also found the need for an overload in the NCronJobOptionsBuilder when adding jobs via the JobOptionBuilder: currently there is this: public IStartupStage<T> AddJob<T>(Action<JobOptionBuilder>? options = null) but I need the following because I am constructing my own options based on config: public IStartupStage<T> AddJob<T>(JobOptionBuilder options)

It's not a must, but would definitely help.


Let me know what you think and thanks again!

linkdotnet commented 5 months ago

Good points overall. I do think many of them are achievable. I am inclined to release a small preview-release from the feature branch for you to get your hands on to gather some further insights. Let me know if this would workout for you.

To your fifth point: Allowing to iterate over Cron jobs. I am a bit reluctant to expose our internal model, as that binds us to keep it stable. That said, what would be a minimal version for you to work with?

For example something along the line like:

string? GetCronExpression(string jobName);
// Or something like this
(string? CronExpression, TimeZoneInfo? timeZone) GetCronExpression(string jobName);

Regarding number 6: public IStartupStage<T> AddJob<T>(JobOptionBuilder options). While JobOptionBuilder is public a lot of things aren't. Well you could still chain it like inside the action - my question is what scenario is easier or which scenario would enable you that API surface?

linkdotnet commented 5 months ago

Another assumption I have and maybe you could challenge me here: Job names have to be unique. That said: Adding two jobs with the same name should raise an exception.

The alternative is more like a "tag" where this doesn't have to be unique, but calling something like RemoveJob("Foo") and Foo is mapped to multiple jobs will delete all of them.

I don't want to prime anyone, but I am much in favor of having a unique job name and throw an exception if that isn't the case.

linkdotnet commented 5 months ago

I created this release for you: 2.7.5-preview.

Here the most important bit:

/// <summary>
/// Gives the ability to add, delete or adjust jobs at runtime.
/// </summary>
public interface IRuntimeJobRegistry
{
    /// <summary>
    /// Gives the ability to add a job.
    /// </summary>
    void AddJob(Action<NCronJobOptionBuilder> jobBuilder);

    /// <summary>
    /// Removes the job with the given name.
    /// </summary>
    /// <param name="jobName">The name of the job to remove.</param>
    /// <remarks>If the given job is not found, no exception is thrown.</remarks>
    void RemoveJob(string jobName);

    /// <summary>
    /// Removes all jobs of the given type.
    /// </summary>
    void RemoveJob<TJob>() where TJob : IJob;

    /// <summary>
    /// Removes all jobs of the given type.
    /// </summary>
    void RemoveJob(Type type);

    /// <summary>
    /// Updates the schedule of a given job by its name.
    /// </summary>
    /// <param name="jobName">The name of the job.</param>
    /// <param name="cronExpression">The new cron expression for the job.</param>
    /// <param name="timeZoneInfo">An optional timezone to use for the cron expression.If not provided, UTC is used.</param>
    /// <remarks>
    /// If the given job is not found, an exception is thrown.
    /// Furthermore, all current planned executions of that job are canceled and rescheduled with the new cron expression.
    /// </remarks>
    void UpdateSchedule(string jobName, string cronExpression, TimeZoneInfo? timeZoneInfo = null);

    /// <summary>
    /// Updates the parameter of a given job by its name.
    /// </summary>
    /// <param name="jobName">The name of the job.</param>
    /// <param name="parameter">The new parameter that will be passed into the job.</param>
    /// <remarks>
    /// If the given job is not found, an exception is thrown.
    /// Furthermore, all current planned executions of that job are canceled and rescheduled with the new parameter.
    /// </remarks>
    void UpdateParameter(string jobName, object? parameter);

    /// <summary>
    /// Retrieves the schedule of a given job by its name. If the job is not found, the out parameters are set to null.
    /// </summary>
    /// <param name="jobName">The given job name.</param>
    /// <param name="cronExpression">The associated cron expression. If the job has none, or couldn't be found this will be <c>null</c>.</param>
    /// <param name="timeZoneInfo">The associated time zone. If the job has none, or couldn't be found this will be <c>null</c>.</param>
    /// <returns>Returns <c>true</c> if the job was found, otherwise <c>false</c>.</returns>
    bool TryGetSchedule(string jobName, out string? cronExpression, out TimeZoneInfo? timeZoneInfo);
}

The readme for this: https://github.com/NCronJob-Dev/NCronJob/blob/dynamic-jobs/docs/advanced/dynamic-job-control.md

Keep in mind that this version isn't final and the point above is not included (basically adding multiple jobs with the same name will lead to undefined behavior).

CC @falvarez1

KorsG commented 5 months ago

Looks great! ๐Ÿ‘

1

To your fifth point: Allowing to iterate over Cron jobs. I am a bit reluctant to expose our internal model, as that binds us to keep it stable. That said, what would be a minimal version for you to work with?

I understand the increased complexity about having to expose this, but maybe it could just be a very simple representation to begin with?

I created a branch to give the latest changes a spin where I created a simple GetJobs method in the RuntimeJobRegistry and a simple usage example:

Usage: https://github.com/KorsG/NCronJob/blob/cb6b58eba6eb4493429d00d1015a9693ae488bf3/sample/DynamicSample/ScheduledJobsService.cs#L51

Implementation: https://github.com/KorsG/NCronJob/blob/cb6b58eba6eb4493429d00d1015a9693ae488bf3/src/NCronJob/Registry/RuntimeJobRegistry.cs#L177

It only returns the most basic info, and should probably return a type instead of a tuple - maybe RuntimeJob?

The only "issue" I currently see with it, is that it returns the Cronos parsed CronExpression and not the "input" CronExpression, which makes it difficult to do a comparison, to check if it has changed: https://github.com/KorsG/NCronJob/blob/cb6b58eba6eb4493429d00d1015a9693ae488bf3/sample/DynamicSample/ScheduledJobsService.cs#L58

If you agree, we could store the "input" Cron in the JobDefinition alongside the Cronos parsed version, and the return the "input" Cron instead?

2

Another assumption I have and maybe you could challenge me here: Job names have to be unique. That said: Adding two jobs with the same name should raise an exception.

The alternative is more like a "tag" where this doesn't have to be unique, but calling something like RemoveJob("Foo") and Foo is mapped to multiple jobs will delete all of them.

I don't want to prime anyone, but I am much in favor of having a unique job name and throw an exception if that isn't the case.

I agree 100% and are also in favor of unique JobNames ๐Ÿ‘

3

Regarding number 6: public IStartupStage AddJob(JobOptionBuilder options). While JobOptionBuilder is public a lot of things aren't. Well you could still chain it like inside the action - my question is what scenario is easier or which scenario would enable you that API surface?

I have a small scenario here: https://github.com/KorsG/NCronJob/blob/cb6b58eba6eb4493429d00d1015a9693ae488bf3/sample/DynamicSample/ScheduledJobsConfigurationExtensions.cs#L46

But its easily achievable without the overload, so you can just disregard the request ๐Ÿ™‚

linkdotnet commented 5 months ago

We could offer a GetAllSchedules method that returns basically those 3 information:

  1. Job Name (if any is present)
  2. Cron Schedule
  3. TimeZoneInfo

The only "issue" I currently see with it, is that it returns the Cronos parsed CronExpression and not the "input" CronExpression,

If you take job.CronExpression?.ToString() you should have the original cron expression, not? That is what we are utilizing in TryGetSchedule.

But my take-away is the the current API is good "enough" once we add the GetAllSchedules function.

Edit: Cronos.ToString() will basically reverse engineer the input string from the saved cron expression. So there might be a chance that they might be different. I suspect if you have additional whitespace a string comparison will fail:

string cron = "* *  * * *";
CronExpression cronExpression = CronExpression.Parse(cron);
var isEqual = cronExpression.ToString() == cron; // This will fail
linkdotnet commented 5 months ago

@falvarez1 if we offer a GetAllSchedules I am not sure if we should name the returning type Schedule!?

IReadOnlyCollection<Schedule> GetAllSchedules();

This might clash with internal things at some point. UserSchedule/ ScheduleInformation sounds also shady.

linkdotnet commented 5 months ago

I have a small scenario here:

As far as I can tell you are only accessing public information, which makes me believe that your scenario is totally possible in your user code as an extension.

KorsG commented 5 months ago

But my take-away is the the current API is good "enough" once we add the GetAllSchedules function.

Agreed ๐Ÿ™‚

If you take job.CronExpression?.ToString() you should have the original cron expression, not? That is what we are utilizing in TryGetSchedule.

Here's an example of how the Cron becomes different after being parsed into the Cronos CronExpression and using ToString() on that: image

TryGetSchedule should behave similarly, if you agree that it should be changed to return the input Cron.

linkdotnet commented 5 months ago

But my take-away is the the current API is good "enough" once we add the GetAllSchedules function.

Agreed ๐Ÿ™‚

If you take job.CronExpression?.ToString() you should have the original cron expression, not? That is what we are utilizing in TryGetSchedule.

Here's an example of how the Cron becomes different after being parsed into the Cronos CronExpression and using ToString() on that: image

TryGetSchedule should behave similarly, if you agree that it should be changed to return the input Cron.

Ahh thanks for the hint. Yes that is ugly. I will have a look.

linkdotnet commented 5 months ago

Now, there is a 2.7.6 prerelease that should address all of your suggestions:

  1. GetAllRecurringJobs that retrieves you exactly that list with Jobname, type and so on.
  2. The CRON Expresson returned (also for Try) will be the user provided on
KorsG commented 5 months ago

Fantastic! I really appreciate all the effort you put into this ๐Ÿ™‚

harrison314 commented 5 months ago

Hello, I'm trying nuget version 2.7.6-preview, because in my application I need to add cron jobs only in runtime (same job type, different parameter and cron expression).

But call IRuntimeJobRegistry.AddJob has no efect.

string cronExpression = "*/2 * * * *";
string name = "CronJob_1";

runtimeJobRegistry.AddJob(builder =>
{
            builder.AddJob<CronTriggerJob>(opt => opt.WithCronExpression(cronExpression)
                .WithName(name)
                .WithParameter(name));
});

        var collection =  this.runtimeJobRegistry.GetAllRecurringJobs(); //collection is empty and CronTriggerJob can not executed.
linkdotnet commented 5 months ago

Hmmm, troublesome :D @harrison314 Thanks for reporting. I will have a look into it.

linkdotnet commented 5 months ago

But good that it's still in preview :D

linkdotnet commented 5 months ago

@harrison314 Can you show me your AddNCronJob part in the Program.cs?

harrison314 commented 5 months ago

@harrison314 Can you show me your AddNCronJob part in the Program.cs?

 builder.Services.AddTransient<CronTriggerJob>(); // Is it necessary to register a Job in DI?
 builder.Services.AddNCronJob();

I also tried registering a dummy job at startup, but it behaves the same.

linkdotnet commented 5 months ago

You can and should utilize the AddNCronJob function itself.

builder.Services.AddNCronJob(s => s.AddJob<CronTriggerJob>());

We have an open PR that would even lighten up that requirement. But the general flow is that you don't have to register those jobs yourself. Let the library do the dirty work.

Indepdently there seems to be a bug with GetAllRecurringJobs.

linkdotnet commented 5 months ago

That said - in your case you might not even need the runtime feature at all. Or is there a reason not to do your call inside AddNCronJob?

harrison314 commented 5 months ago

Or is there a reason not to do your call inside AddNCronJob?

Yes, there is. I implement alerting/scheduling tasks for a my prototype timeseries database. The database user defines the JS code, cron expression and task name only in the runtime via the REST API. I want every user to be able to have an unlimited number of sheduling tasks.

linkdotnet commented 5 months ago

But independent of that, there was really a bug and a missing bit here. I will push a fix and make a new pre-release.

linkdotnet commented 5 months ago

Or is there a reason not to do your call inside AddNCronJob?

Yes, there is. I implement alerting/scheduling tasks for a my prototype timeseries database. The database user defines the JS code, cron expression and task name only in the runtime via the REST API. I want every user to be able to have an unlimited number of sheduling tasks.

Yeah that is a perfectly fine case.

In the current state I advise to register the job like this:

builder.Services.AddNCronJob(b => b.AddJob<CronTriggerJob>());

// Later in your code:
registry.AddJob(s => s.AddJob<CronTriggerJob>(t => t.WithCronExpression("* * * * *").WithName("My Name")));

I will make a nuget deployment soon - there should be a 2.7.7-preview available in a few minutes. Thanks for reporting!

harrison314 commented 5 months ago

Thanks, I'll try and give feedback.

linkdotnet commented 5 months ago

2.7.7-preview: https://www.nuget.org/packages/NCronJob/2.7.7-preview

As a heads up, we are working on a dashboard and a webAPI to enable your case to be out of the box. But no ETA at this moment as we have to tackle some other fundamental things first (better observability and so on).

harrison314 commented 5 months ago

2.7.7-preview

Problem still continues.

I used the equivalent of this code:

builder.Services.AddNCronJob(b => b.AddJob<CronTriggerJob>());
....
runtimeJobRegistry.AddJob(s => s.AddJob<CronTriggerJob>(t => t.WithCronExpression("* * * * *").WithName("My Name")));

I also connect watch from runtimeJobRegistry after call AddJob:

obrรกzok

If it helps, tomorrow I can make an app to reproduce the problem using the Minimal API.

linkdotnet commented 5 months ago

Oh damn I do think I know why. On it

linkdotnet commented 5 months ago

2.7.8: https://www.nuget.org/packages/NCronJob/2.7.8-preview

harrison314 commented 5 months ago

Thanks, version 2.7.8 fixed it. Adding multiple jobs and deleting them in runtime works correctly.

linkdotnet commented 4 months ago

Sorry for the long delay. We did some other stuff in parallel, which needed a complete rewrite of this feature. We aim to merge the PR soon and hand out a release.

There is a 2.7.9-preview package available. In theory it isn't anything new, but it would be awesome if someone could try it out and report back.