Skip to content
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

xScheduleTask - Time Zone issues #85

Closed
Zuldan opened this issue Jul 18, 2017 · 23 comments · Fixed by #100
Closed

xScheduleTask - Time Zone issues #85

Zuldan opened this issue Jul 18, 2017 · 23 comments · Fixed by #100
Assignees
Labels
bug The issue is a bug. in progress The issue is being actively worked on by someone.

Comments

@Zuldan
Copy link

Zuldan commented Jul 18, 2017

@PlagueHO I've started converting all my cScheduledTask (totally different resource) configurations to xScheduledTask and noticed some time zone issues. The configuration is being built in Australia but the servers are in New Zealand.

I need a task to run every 15 minutes for 23 hours a day. So I used the following config.

RepeatInterval     = '00:15:00'
RepetitionDuration = '23:00:00'

However, I get the following issue.

Invoke-CimMethod : PowerShell DSC resource MSFT_xScheduledTask  failed to execute Set-TargetResource functionality with error message: Repetition duration 01:00:00 is less than repetition interval 02:15:00. Please set RepeatInterval to a value lower or 
equal to RepetitionDuration

If I do something like this...

RepeatInterval     = [datetime]::Today.AddMinutes(15)
RepetitionDuration = '20:00:00'

...then the scheduled task is set to start 12:00AM, repeat every 02:15:00 for a duration of 22:00:00.

I think one solution would be to add a property to the resource called 'UseLocalTime' this would resolve the issue without having a breaking change.

Another question is how do I set the duration to Indefinitely?

@PlagueHO PlagueHO added help wanted The issue is up for grabs for anyone in the community. question The issue is a question. labels Jul 22, 2017
@RobBiddle
Copy link

This is definitely a major bug in the 2.0.0.0 implementaion of xScheduledTask
I too am generating MOFs for systems in multiple time zones and this issue completely breaks the xScheduledTask resource, it can't be used as-is in a multi timezone scenario.

Either a switch to use the local timezone needs to be added as mentioned by Zuldan, or the resource parameters need to be changed to [String] or [Int] values and interpreted during the DSC run on the client

@PlagueHO
Copy link
Member

This definitely sounds like a high priority problem. I was planning on doing some more work on this module this weekend, so ill make this a top priority. Thank you both for raising this.

@RobBiddle
Copy link

RobBiddle commented Aug 18, 2017

I'm sure you already have a good idea how to fix this, but in the off-chance that this is helpful, here's some things I noticed:

It looks like the New-ScheduledTaskSettingsSet cmdlet actually expects [System.TimeSpan] objects, whereas this DSC resource is using [System.DateTime] for its own parameters related to delay/duration/interval.

It looks like the only parameter that will need to be [DateTime] is the -AT parameter for New-ScheduledTaskTrigger, so the StartTime value for specifying the DSC resource configuration of a task is probably the only thing that needs to be adjusted to account for differing time zones and then the [TimeSpan] objects should work for all of the various delay/duration/interval parameters without needing to change a lot of code.

@PlagueHO
Copy link
Member

@RobBiddle - that is great info, and +++ helpful, thank you! I'll make this change tonight (it's Saturday AM here)/tomorrow. If you're up for running it through a couple of tests after I've completed the change/unit tests/integration tests etc that would be very helpful.

@PlagueHO
Copy link
Member

Sorry I haven't completed this one. I'm working on it this week - just been a bit of a lot on at work so haven't had the time to put into it yet. Definitely this week though.

@PlagueHO PlagueHO added bug The issue is a bug. and removed question The issue is a question. labels Aug 22, 2017
@Zuldan
Copy link
Author

Zuldan commented Aug 22, 2017

@PlagueHO no worries. Appreciate you looking at this issue.

@RobBiddle
Copy link

@PlagueHO Sorry I wasn't available over the weekend; I was out of town for the solar eclipse. I'd be happy to test your changes when they're ready.

@PlagueHO PlagueHO added high priority The issue or PR should be resolved first. It is of less priority than the label 'Blocking Release'. in progress The issue is being actively worked on by someone. and removed help wanted The issue is up for grabs for anyone in the community. labels Aug 25, 2017
@PlagueHO
Copy link
Member

PlagueHO commented Aug 26, 2017

Being a big supporter of TDD I've managed to create some failing tests for this scenario so I should be able to implement one of the suggested solutions. I am leaning towards @Zuldan 's solution of adding the UseLocalTime parameter because of it not requiring a breaking change. Will post an update soon.

@PlagueHO
Copy link
Member

Actually, the UseLocalTime method won't work unfortunately 😢.

This is because although the timezone info is stored for each of the Interval/Duration parameters, the DSC LCM handles the conversion of the MOF datetimes back into parameters that are passed into resource. However, this conversion is always performed using local time and as far as I can tell there isn't a way to control that behavior.

So what I'm going to have to do is convert all these parameters over to strings. So a much bigger job. Sorry guys.

@PlagueHO
Copy link
Member

@Zuldan , @RobBiddle - I've finished the changes to convert this over to use string parameters for all Interval/Duration type parameters and everything seems to be working correctly. I'll submit the PR tomorrow, but in the meantime you can find the new version here:
https://github.com/PlagueHO/xComputerManagement/tree/Issue-85

If you fancy testing this out and make sure it solves the problems you were having, that would be very helpful!

@Zuldan
Copy link
Author

Zuldan commented Aug 27, 2017

@PlagueHO great work. I'll give this a run in the lab tomorrow.

@RobBiddle
Copy link

@PlagueHO I just tested the fix and it looks to be working. Thanks for fixing this!

Not sure if you want to bother trying this, but I think that only the StartTime would need to use a [String] type. As long as the StatTime value ends up correct (i.e. absolute value) then the rest of the interval/duration parameters should work using the proper [System.TimeSpan] type. It makes no difference in terms of functionality, but at least the code would be straying less from proper type constraints, which may be better for you down the road.

@PlagueHO
Copy link
Member

Hi @RobBiddle - You raise a good point.

I'm of two minds about which is the best approach:
On one hand it feels odd to use a DateTime as a Timespan object and a String as a DataTime, but on the other hand it would require less parameter type changes. But either way we'll end up with a breaking change.

I did increase the unit test coverage and refactored some code to reduce reuse with this change, but I could copy these changes across if we decided to try changing the StartTime to a String.

The other problem is I'm going to be a bit strapped for time to work on this stuff till the 7th of September (due to day job commitments), but if you're OK to wait till then I could try the other method.

@Zuldan - do you have any preference here?

@Zuldan
Copy link
Author

Zuldan commented Aug 29, 2017

@PlagueHO no preference here on my side. Happy to go with whatever you guys decide. I did manage to test the code yesterday in the lab and can confirm it's working fine.

@hadhh
Copy link

hadhh commented Aug 29, 2017

Hi @PlagueHO,

Will string definition allow to define never ending task ?

RepetitionDuration = 'Indefinitely'

@cd83
Copy link

cd83 commented Aug 29, 2017

@PlagueHO please consider @hadhh 's suggestion

@PlagueHO
Copy link
Member

Hi @hadhh and @cd83 - It does look like this would be easier to implement if RepetitionDuration was a string parameter. And it shouldn't be too difficult either. This would obviously push us to sticking with the string parameters for timespan - @RobBiddle - do you have any thoughts/requirements around this?

@hadhh
Copy link

hadhh commented Sep 6, 2017

@PlagueHO a workaround can be done for never never ending task with timespan

RepetitionDuration = [timespan]::MaxValue.ToString()

based on code: #98

However, it doesn't work fine as the "Test phase" detects task as not in desired state and trigger a "Set phase"

@PlagueHO
Copy link
Member

Perhaps a solution would be to allow:
RepetitionDuration = 'indefinite'

I should be able to include this in this PR if everyone is happy with this approach. Still would like to hear back from @RobBiddle if he's OK with the 'Timespans as Strings' approach rather than the 'At as String' approach.

@agarstang
Copy link

This really causes an issue for us as we can't create a scheduled task that repeats indefinitely (or at least for a large amount of time).

This is because it is using the "TimeOfDay" property which allows us to only fit the duration within a 24 hr window.

The "Once" task type is the only type that repeats properly using an interval.

@PlagueHO
Copy link
Member

Thanks @agarstang - I think this confirms that the right choice was to use strings for the TimeStamp. Sorry @RobBiddle - didn't hear back, but I hope this is OK. @Zuldan has confirmed that the TimeStamp as strings method fixes the issues.

So I'll get the support for Indefinite repetition duration completed this weekend. Sorry it has taken so long.

@PlagueHO
Copy link
Member

Just doing final testing on this change. I went with @hadhh 's format of:
RepetitionDuration = 'Indefinitely'

Because this matched the text you see in the TaskScheduler window for a task with an indefinite duration.

My current inflight PR will include this fix.

@PlagueHO
Copy link
Member

I think this is now completed. I'm waiting on a review before merging, but you can try my version here: https://github.com/PlagueHO/xComputerManagement

Any feed back is appreciated.

This one took a lot longer than I'd planned because there was some spaghetti code due to the differences between WS 2012/R2 and WS 2016 that needed to be refactored.

PlagueHO added a commit that referenced this issue Sep 19, 2017
BREAKING CHANGE: xScheduledTask Duration/Interval Fields Changed to Strings - Fixes #85
@joeyaiello joeyaiello removed the high priority The issue or PR should be resolved first. It is of less priority than the label 'Blocking Release'. label Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug. in progress The issue is being actively worked on by someone.
Projects
None yet
7 participants