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

Comments

Projects
None yet
7 participants
@Zuldan

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?

@RobBiddle

This comment has been minimized.

Show comment
Hide comment
@RobBiddle

RobBiddle Aug 17, 2017

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

RobBiddle commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Aug 17, 2017

Collaborator

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.

Collaborator

PlagueHO commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@RobBiddle

RobBiddle 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.

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

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Aug 18, 2017

Collaborator

@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.

Collaborator

PlagueHO commented Aug 18, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Aug 22, 2017

Collaborator

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.

Collaborator

PlagueHO commented Aug 22, 2017

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 and removed question labels Aug 22, 2017

@Zuldan

This comment has been minimized.

Show comment
Hide comment
@Zuldan

Zuldan Aug 22, 2017

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

Zuldan commented Aug 22, 2017

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

@RobBiddle

This comment has been minimized.

Show comment
Hide comment
@RobBiddle

RobBiddle Aug 22, 2017

@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.

RobBiddle commented Aug 22, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Aug 26, 2017

Collaborator

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.

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Aug 26, 2017

Collaborator

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.

Collaborator

PlagueHO commented Aug 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Aug 26, 2017

Collaborator

@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!

Collaborator

PlagueHO commented Aug 26, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@Zuldan

Zuldan Aug 27, 2017

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

Zuldan commented Aug 27, 2017

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

@RobBiddle

This comment has been minimized.

Show comment
Hide comment
@RobBiddle

RobBiddle Aug 27, 2017

@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.

RobBiddle commented Aug 27, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Aug 28, 2017

Collaborator

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?

Collaborator

PlagueHO commented Aug 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Zuldan

Zuldan 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.

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

This comment has been minimized.

Show comment
Hide comment
@hadhh

hadhh Aug 29, 2017

Hi @PlagueHO,

Will string definition allow to define never ending task ?

RepetitionDuration = 'Indefinitely'

hadhh commented Aug 29, 2017

Hi @PlagueHO,

Will string definition allow to define never ending task ?

RepetitionDuration = 'Indefinitely'

@cd83

This comment has been minimized.

Show comment
Hide comment
@cd83

cd83 Aug 29, 2017

@PlagueHO please consider @hadhh 's suggestion

cd83 commented Aug 29, 2017

@PlagueHO please consider @hadhh 's suggestion

@PlagueHO

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Aug 30, 2017

Collaborator

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?

Collaborator

PlagueHO commented Aug 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@hadhh

hadhh 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"

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

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Sep 11, 2017

Collaborator

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.

Collaborator

PlagueHO commented Sep 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@agarstang

agarstang Sep 13, 2017

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.

agarstang commented Sep 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Sep 14, 2017

Collaborator

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.

Collaborator

PlagueHO commented Sep 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Sep 15, 2017

Collaborator

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.

Collaborator

PlagueHO commented Sep 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PlagueHO

PlagueHO Sep 17, 2017

Collaborator

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.

Collaborator

PlagueHO commented Sep 17, 2017

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 PlagueHO closed this in #100 Sep 19, 2017

PlagueHO added a commit that referenced this issue Sep 19, 2017

Merge pull request #100 from PlagueHO/dev
BREAKING CHANGE: xScheduledTask Duration/Interval Fields Changed to Strings - Fixes #85
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment