(MODULES-6845) Allow provider to read / modify V2 tasks

[Iristyle]

No description provided.

[Iristyle]

Triggered adhoc at https://jenkins-master-prod-1.delivery.puppetlabs.net/view/All/job/forge-windows_puppetlabs-scheduled_task_init-manual-parameters_adhoc/9/ FAILED

[glennsarti]

Not sure where this leaves the provider as it's no longer an adapter

[glennsarti]

So now that we can show V2 tasks, does mean we should be enabling child folders, not just the root folder?

[Iristyle]

Yeah probably... not sure if that's worth a separate ticket or not.

[glennsarti]

I have a few issues with this PR. The TaskScheduler2V1Task was meant to mimic the V1 provider so we maintained backwards compatibility. This PR breaks that contract as it now returns V2, V1 and AT tasks for enum. This is also borne out in the spec tests where it runs the sames tests over the provider for both the original implementation and the V2 adapter.

And because that contract is broken I expect spec tests to be failing.

I would've thought it would make more sense to;

1. Copy the contents from scheduled_task/taskscheduler_api2.rb into win32_taskscheduler.rb as we know this is compatible

2. Copy the spec/unit/puppet/provider/scheduled_task/win32_taskscheduler_spec.rb file to spec/unit/puppet/provider/scheduled_task/taskscheduler_api2_spec.rb

3. Remove references to PuppetX::PuppetLabs::ScheduledTask::TaskScheduler2V1Task from spec/unit/puppet/provider/scheduled_task/win32_taskscheduler_spec.rb

4. Remove references to Win32::TaskScheduler from spec/unit/puppet/provider/scheduled_task/taskscheduler_api2_spec.rb

This means now the V1 style provider still uses the V2 API for it's underlying work and can be used for backwards compatiblity and that the V2 provider can
be developed further with more "stuff"

[Iristyle]

Thanks for the feedback @glennsarti - I'm still working on a more comprehensive explanation of the thought process here, because it's a little complicated.

In the interim, here are a few bits about where we were coming from:

• Don't change win32_taskscheduler at all, so that we completely maintain backwards compat against V1 APIs / be more risk averse. This is the part where we have diverged the most in ideas, as you want to keep V1 compat by using the V2 APIs instead because you're concerned V1 APIs shouldn't be used at all. I'm not convinced they've gone anywhere or they can't be used, but then there's this post on SO which seems to make that case. We were coming from the angle that we know this provider will be yanked at some point in the not-too-distant-future, so it didn't seem like it made sense to make changes to it. That said, I'm not wed to this at all - just want to understand tradeoffs fully for either approach.

• Our ideas around contracts are different. From my perspective our only contract to end users is via the attributes on the type, and we can't change those in a way that breaks backwards compat regardless. What is returned / queried should be immaterial as long as it conforms to the same basic structure (if we rip apart tests, I suspect this will be harder to enforce - having tests shared amongst providers helps us prove this). There is no real contract between type and provider aside from the generic T&P contract for all Puppet providers and a couple of methods on the provider. Beyond that, it's all internal details. (I'm glossing over the details a bit because there are some subtle contracts like class of Exception, structures within the trigger hash, etc - but my statement mostly holds true)

• I think the taskscheduler_api2 provider has to return all compatibility levels - otherwise users will not be able to manage existing V1 tasks with the new default provider (this is one of the other reasons I didn't see the merit in redoing win32_taskscheduler). I think this is one of the only bits that is non-negotiable - just want to make sure we're on the same page.

• Now that Trigger code has been pulled out, it shows that there is almost nothing different between TaskSchedulerV1Task and TaskScheduler2Task. It makes us wonder why we need both, and it seemed nice if we could ditch one. But if we do the win32_taskscheduler rejiggering, we'll obviously need the adapter or something like it to remain (it might be inlinable into the provider)

• We want to completely trash the V1 internal representation as it can't roundtrip things properly, and is an unnecessary conversion (haven't filed ticket for that yet)

• @michaeltlombardi added code today to constraint provider specific attributes like compatibility which will come in handy later - (MODULES-6526) Confine compatibility to a feature #30

Anyhow - this was quick off the head stuff - let's chat in person soon.

[glennsarti]

Yeah I think we're mostly on the same page. Just need to define the direction of the older win32_taskscheduler.rb file as that determines some other things.

Don't change win32_taskscheduler at all, so that we completely maintain backwards compat against V1 APIs / be more risk averse. This is the part where we have diverged the most in ideas, as you want to keep V1 compat by using the V2 APIs instead because you're concerned V1 APIs shouldn't be used at all.

In addition to that point, I would like to ditch the V1 API calls and use the V2 API calls instead because;

Pros:

• V2 API is easier to reason about, maintain and debug
• We can take advantage of the all the trigger refactor that we've done
• We've done a lot of work to make the V2API <-> V1 Task adapter, and we know it works, and have tests to prove it. So it's low risk to use it instead of the V1 APIs
• We have to always carry the win32_taskscheduler.rb file otherwise the puppet-agent version will be loaded, so why not use a "better" version of it
• We can still "code freeze" the V1 Task adapter code, as we would, if we used the V1 API call style.

Cons:

• There is a risk that the Task adapter is not 100% the same as the original code. Personally I feel the risk is WAY lower than the risk/effort to debug/fix/maintain the V1 API calls.
• Requires work to inline the V1Task class into the V1 provider

Our ideas around contracts are different. ...

More than likely. I used an overloaded word.

I think the taskscheduler_api2 provider has to return all compatibility levels - otherwise users will not be able to manage existing V1 tasks with the new default provider

100% agree

Now that Trigger code has been pulled out, it shows that there is almost nothing different between TaskSchedulerV1Task and TaskScheduler2Task. It makes us wonder why we need both, and it seemed nice if we could ditch one. But if we do the win32_taskscheduler rejiggering, we'll obviously need the adapter or something like it to remain (it might be inlinable into the provider)

In my mind the fact that they're almost the same right now is expected. All we've really done so far is all "tech debt"/backend work. Now that that's all been done we're adding features and this where they'll being to diverge.

We want to completely trash the V1 internal representation as it can't roundtrip things properly ...

I assume you're talking V1 API here

@michaeltlombardi added code today to constraint provider specific attributes like compatibility which will come in handy later

Saw that. +1

... let's chat in person soon.

Agreed!

[Iristyle]

In addition to that point, I would like to ditch the V1 API calls and use the V2 API calls instead because;

Pros:

• V2 API is easier to reason about, maintain and debug

I think the APIs are about the same after the Trigger code was refactored out. However, it wasn't factored out of the existing win32_taskscheduler, so that is certainly more difficult to maintain. I do think it's easier to work on top of one set of APIs rather than two.

We can take advantage of the all the trigger refactor that we've done

+1

• We've done a lot of work to make the V2API <-> V1 Task adapter, and we know it works, and have tests to prove it. So it's low risk to use it instead of the V1 APIs

It turns out that most of the work here is in the Trigger nonsense, which caused the adapter to go on a 300-400 line diet. But I agree it would be nice to leverage the work invested in the adapter.

• We have to always carry the win32_taskscheduler.rb file otherwise the puppet-agent version will be loaded, so why not use a "better" version of it

Yes, we will have to carry this until the point at which Puppet itself trashes it to make sure that it is not the default provider.

We can still "code freeze" the V1 Task adapter code, as we would, if we used the V1 API call style.
Cons:

There is a risk that the Task adapter is not 100% the same as the original code. Personally I feel the risk is WAY lower than the risk/effort to debug/fix/maintain the V1 API calls.

My thinking here is that we don't touch it either way anyhow. If a user has a problem with what's "in the box" for Puppet, then they must install the module, at which point they get a new default provider. Any "fixes" they need go against api2. A user would have to have the module and opt in to the old provider to use that code path, which seems unlikely, and hopefully never happens.

With any luck, we never touch win32_taskscheduler provider again.

• Requires work to inline the V1Task class into the V1 provider

That was just me thinking about further into the future. Not something that necessarily needs to be done.

Our ideas around contracts are different. ...

More than likely. I used an overloaded word.

The methods on the provider that the type calls are validate_trigger, trigger_inysnc? and user_insync?. As long as we have those (in addition to the other things I mentioned), we at least have the same API. That said, now that we know the win32_taskscheduler in this module overrides one in core, I think we have more latitude to make changes there as well.

Now that Trigger code has been pulled out, it shows that there is almost nothing different between TaskSchedulerV1Task and TaskScheduler2Task. It makes us wonder why we need both, and it seemed nice if we could ditch one. But if we do the win32_taskscheduler rejiggering, we'll obviously need the adapter or something like it to remain (it might be inlinable into the provider)

In my mind the fact that they're almost the same right now is expected. All we've really done so far is all "tech debt"/backend work. Now that that's all been done we're adding features and this where they'll being to diverge.

We're not sure yet, but we suspect much of the divergent behavior is going to end up in the factored out Trigger helpers, rather than in the providers... which is what had us thinking about this. We can always revisit at a later date when we do some additional cleanup.

We want to completely trash the V1 internal representation as it can't roundtrip things properly ...

I assume you're talking V1 API here

Not ... exactly.

I mean some of the Trigger helper code and our "internal representation" of triggers. Right now, we do some things that were appropriate at initial implementation, but have become clearly unnecessary now after refactoring.

• As a provider fetches instances, it calls into the adapter
• The adapter reads ITrigger instances (V2 API), converting them to the V1-esque internal representation using Trigger::V1.from_iTrigger (to remain consistent with how they're returned from the old win32_taskscheduler)
  • V1-esque because that structure still has to be mapped to the FFI struct and vice versa (see Puppet core code for populate_trigger and populate_hash_from_trigger)
  • For saving through V2 API, we use append_v1trigger, translating what was at first a V2 ITrigger (currently a V1-esque struct) back to a V2 ITrigger
• The provider immediately translates those to the manifest hash structure in trigger

For ingress manifest hash definitions, provider.trigger_inysnc? is called from the type to do a comparison, which funnels the heavy-lifting to triggers_same? which compares some manifest hash fields directly, but additionally converts them to temporary copies of the V1-esque representation to compare as well.

I would like to see the V1-esque representation removed completely (in addition to cleaning up comparison and prefetch logic), which we've now realized cannot be done without migrating win32_taskscheduler provider to V2 APIs. IMHO, that's the most compelling reason to get rid of a provider built on the old APIs, because even if new provider doesn't use V1-esque hashes as an intermediate representation, we won't be able to remove the extra Trigger conversion code as the old provider would still need it.

So to summarize, I think we're going to migrate the code from api2 into win32_taskscheduler as you've suggested, and will put up a PR as such. I still think that tests are going to have to overlap to ensure the old manifest hash structures still flow through the new provider properly. But I think we can tackle that in more detail later. We still want to make sure that the new provider is a superset of the old provider (even though we plan to eventually rip the old one out).

[Iristyle]

@michaeltlombardi I resumed your PR at #34 here... however, there are a few things not working right (as of 2nd to last commit 0a0228c).

• ensure => absent is not removing my test task
• Get message like Notice: /Stage[main]/Main/Scheduled_task[test]/ensure: created when trying to modify existing task
• idempotency is busted... I get a message about task compat level change, which I'll need to look at

I suspect this may have to do with the .job extension, but not sure yet.

I've been messing around with this basic manifest and puppet apply

scheduled_task { 'test':
  ensure        => 'present',
  command       => 'c:\\windows\\system32\\notepad.exe',
  # compatibility => 2,
  enabled       => 'true',
  trigger       => [
  {
    'schedule' => 'once',
    # 'start_date' => '1899-12-30',
    'start_date' => '2018-04-30',
    'start_time' => '00:00',
    'minutes_interval' => 0,
    'minutes_duration' => 0,
  }],
  # provider => 'win32_taskscheduler'
}

[michaeltlombardi]

RE: Compatibility, as you discovered, it's because the V2 helper creates the task with a compat level of 2. Unless the compat level is specified, however, it will try to default back to 1, preventing idempotency. This means we created a situation in which not specifying compatibility at all will create an initially non-idempotent task, although this will resolved after two runs.

To fix this, we'll need to have it to be able to pass through the compat level to new_work_item, updating the taskscheduler_api2 provider's create call as well.

• create in taskscheduler_api2 will need to pass the compatibility level
• initialize in taskscheduler2_task will need to accept the compatibility level and send it on to new_work_item
• new_work_item in taskscheduler2_task will need to accept the compatibility level and send it on to set_compatibility instead of hard-coding passing V2 compatibility.

[Iristyle]

@michaeltlombardi / @glennsarti ready for another review - almost there, but not quite.

[Iristyle]

@glennsarti we went through each commit individually and are satisfied with how this seems to be working (including in-place upgrading / downgrading compatibility level)

Want to give this one last pass and merge?

Thanks!

[glennsarti]

LGTM

[Iristyle]

Ad-hoc finished through https://jenkins-master-prod-1.delivery.puppetlabs.net/view/modules/view/windows/view/_adhoc/view/scheduled_task/job/forge-windows_puppetlabs-scheduled_task_i18n-ruby_adhoc/11/