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

win_nssm: Data line 'System.Collections.Hashtable' is not in 'name=value' format. #25265

Closed
5f6b3fb8 opened this issue Jun 1, 2017 · 28 comments · Fixed by #44755
Closed

win_nssm: Data line 'System.Collections.Hashtable' is not in 'name=value' format. #25265

5f6b3fb8 opened this issue Jun 1, 2017 · 28 comments · Fixed by #44755
Labels
affects_2.3 This issue/PR affects Ansible v2.3 bug This issue/PR relates to a bug. module This issue/PR relates to a module. support:core This issue/PR relates to code supported by the Ansible Engineering Team. windows Windows community

Comments

@5f6b3fb8
Copy link

5f6b3fb8 commented Jun 1, 2017

ISSUE TYPE
  • Bug Report
COMPONENT NAME
  • win_nssm
ANSIBLE VERSION
ansible 2.3.0.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.6 (default, Oct 26 2016, 20:30:19) [GCC 4.8.4]
CONFIGURATION

No settings changed in ansible.cfg, everything commented.
No ANSIBLE_ environment variables set

OS / ENVIRONMENT

Control machine: Windows 10 (14393.1066) with Linux Subsystem for Windows.
Managed machine: Windows Server 2016 (14393.1198)

SUMMARY

Every attempt to use app_parameters results in
{'_ansible_no_log': False, '_ansible_parsed': True, u'changed': False, u'failed': True, u'msg': u"Data line 'System.Collections.Hashtable' is not in 'name=value' format. "}

STEPS TO REPRODUCE
- name: Create jenkins slave service 
  win_nssm:
    name: jenkins_slave
    application: C:\ProgramData\Oracle\Java\javapath\java.exe
    app_parameters:
      -jar: "{{ jenkins_home }}\\swarm-client-3.4.jar" 

The following example from the documentation also fails (only modification is the application which actually exists on my system).

- win_nssm:
    name: foo
    application: C:\ProgramData\Oracle\Java\javapath\java.exe
    app_parameters:
      bar: true
EXPECTED RESULTS

Expected the service to be created, but failed to start because of incomplete parameter list.

ACTUAL RESULTS
root@BOT0001D:/mnt/c/src/ads/ansible# ansible-playbook site.yml -i inventories/skyd -vvvv
Using /etc/ansible/ansible.cfg as config file
statically included: /mnt/c/src/ads/ansible/roles/deploy/tasks/_jenkins-slave.yml
Loading callback plugin default of type stdout, v2.0 from /usr/lib/python2.7/dist-packages/ansible/plugins/callback/__init__.pyc

PLAYBOOK: site.yml *****************************************************************************************************************************************************
1 plays in site.yml

PLAY [deploy] **********************************************************************************************************************************************************

TASK [Gathering Facts] *************************************************************************************************************************************************
Using module file /usr/lib/python2.7/dist-packages/ansible/modules/windows/setup.ps1
<172.19.0.56> ESTABLISH WINRM CONNECTION FOR USER: Administrator on PORT 5986 TO 172.19.0.56
EXEC (via pipeline wrapper)
ok: [172.19.0.56]
META: ran handlers

TASK [deploy : Create workspace] ***************************************************************************************************************************************
task path: /mnt/c/src/ads/ansible/roles/deploy/tasks/_jenkins-slave.yml:2
Using module file /usr/lib/python2.7/dist-packages/ansible/modules/windows/win_file.ps1
<172.19.0.56> ESTABLISH WINRM CONNECTION FOR USER: Administrator on PORT 5986 TO 172.19.0.56
EXEC (via pipeline wrapper)
ok: [172.19.0.56] => {
    "changed": false
}

TASK [deploy : Download swarm client] **********************************************************************************************************************************
task path: /mnt/c/src/ads/ansible/roles/deploy/tasks/_jenkins-slave.yml:7
Using module file /usr/lib/python2.7/dist-packages/ansible/modules/windows/win_get_url.ps1
<172.19.0.56> ESTABLISH WINRM CONNECTION FOR USER: Administrator on PORT 5986 TO 172.19.0.56
EXEC (via pipeline wrapper)
changed: [172.19.0.56] => {
    "changed": true,
    "win_get_url": {
        "dest": "c:\\jenkins_slave\\swarm-client-3.4.jar",
        "url": "https://repo.jenkins-ci.org/releases/org/jenkins-ci/plugins/swarm-client/3.4/swarm-client-3.4.jar"
    }
}

TASK [deploy : Create jenkins slave service] ***************************************************************************************************************************
task path: /mnt/c/src/ads/ansible/roles/deploy/tasks/_jenkins-slave.yml:12
Using module file /usr/lib/python2.7/dist-packages/ansible/modules/windows/win_nssm.ps1
<172.19.0.56> ESTABLISH WINRM CONNECTION FOR USER: Administrator on PORT 5986 TO 172.19.0.56
EXEC (via pipeline wrapper)
fatal: [172.19.0.56]: FAILED! => {
    "changed": false,
    "failed": true,
    "msg": "Data line 'System.Collections.Hashtable' is not in 'name=value' format. "
}
Debugger invoked
(debug) p task.args
{u'app_parameters': {u'-jar': u'{{ jenkins_home }}\\swarm-client-3.4.jar'},
 u'application': u'C:\\ProgramData\\Oracle\\Java\\javapath\\java.exe',
 u'name': u'jenkins_slave'}
(debug) p vars['jenkins_home']
u'c:\\jenkins_slave'
(debug)

Any help appreciated.

@ansibot
Copy link
Contributor

ansibot commented Jun 1, 2017

@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 bug_report module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. windows Windows community labels Jun 1, 2017
@jhawkesworth
Copy link
Contributor

I suggest trying to use app_parameters_free_form it seems to be more forgiving than app parameters.

Not really a fix but I have used win_ regedit to put the app parameters in place, then just used a minimal win_nssm configuration to set up the rest of the service.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jun 1, 2017
@5f6b3fb8
Copy link
Author

5f6b3fb8 commented Jun 1, 2017

Thanks, @jhawkesworth . I ended up doing that as a workaround. It seems the value of $appParameters in the function ParseAppParameters, is literally just 'System.Collections.Hashtable'. Subsequently, the ConvertFrom-StringData cmdlet is giving me the name=value error.

@5f6b3fb8
Copy link
Author

5f6b3fb8 commented Jun 1, 2017

So I don't have this 100% working; it's just a matter of fine tuning the parameters for my specific application, but the app_parameters are now parsing.

Right out of the gate, this does not work in win_nssm.ps1 $appParameters = Get-AnsibleParam -obj $params -name "app_parameters" -type "str". I think the assumption here is that the value passed to $appParameters will be powershell code like @{ "key" = "value" }, but instead app_parameters is a hashtable and getting the string value of that is just the name of the type (which is useless).

So I changed the initial assignment of $appParameters from
$appParameters = Get-AnsibleParam -obj $params -name "app_parameters" -type "str"
to
$appParameters = $params.app_parameters

Because of this change, there is no point in parsing the app parameters, just use the hashtable directly. So I commented out:
$appParametersHash = ParseAppParameters -appParameters $appParameters
and changed $appParametersHash.GetEnumerator() to $appParameters.GetEnumerator()

Finally, I noticed an issue with the singleLineParams concatenation, where a subsequent key would follow directly after the previous value without a space. So I added a space after $val""
"$singleLineParams = $singleLineParams + "$key ""$val"" "

I'm awful at pull requests. If someone needs to me work on one I'll give it a try, but I'm hoping I provided enough info to fix the issues.

@trondhindenes
Copy link
Contributor

that's funny, I've used it extensively without problems (apart from the stuff fixed with free_form). I'll take a closer look at this. Thanks for the code, sohuld be quick enough to create a PR from it

@5f6b3fb8
Copy link
Author

5f6b3fb8 commented Jun 2, 2017

Maybe Get-AnsibleParam changed? I'm sure the win_nssm.ps1 would work as-is if that cmdlet returned the app_parameters as @{ "key" = "value" } but all it's returning for me is System.Collections.Hashtable

Even with Get-AnsibleParam fixed, I still think the key/value concat needs to be fixed. It might work with some apps, but my scenario needs a space between switches.

@jhawkesworth
Copy link
Contributor

Yeah it did change. optional parameter typing was added during 2.3. -type "str" must have been added erroneously.

@trondhindenes
Copy link
Contributor

Yup, looks like this commit may have changed too much stuff: ff1efb2#diff-5ba1945628b98dfd2ff1c1893772b39a
@dagwieers , care to have a look?

@dagwieers
Copy link
Contributor

Right, the parameter became a string, and it should become a dictionary (with support for key=value in old syntax). Support for dictionaries is not implemented for Windows modules, it's on my list of things to do.

I will remove the -type "str" for this parameter. Sorry for the inconvenience.

@dagwieers
Copy link
Contributor

dagwieers commented Jun 5, 2017

So I fixed the app_parameters parameter in win_nssm, it now has -type="dict" which is unimplemented (so that is the same as before when it did not have that statement).

I noticed that dependencies is in fact a list, and we should probably simplify win_nssm to understand -type "list".

And more importantly, we should add integration tests making use of these options, so we can fail on regressions. See our plan at: https://public.etherpad-mozilla.org/p/Ansible_Windows_Community_Plan

The win_nssm module also lacks check-mode support.

@trondhindenes
Copy link
Contributor

trondhindenes commented Jun 5, 2017

Pretty sure we're talking about the same thing but just in case:
I've added a PR which adds a "psobject" to the list of possible param types. I've tested this against the win_nssm module and it works. imho, there are two "basic constructs" in Powershell objects: psobject and Hashtable. A hashtable is a key+value pair list, and a psobject is more akin to the python dict (its just an object with a random list of attributes). The reason I named it "psobject" is to make it easier for Powershell developers (ie non-python devs) to understand the model, as a "dict" is not well-known type in Powershell lingo. Open to suggestions.

@dagwieers
Copy link
Contributor

dagwieers commented Jun 5, 2017

I would make both one and the same thing. But I would not make it a PSObject. I dont't see a compelling reason (but could be persuaded).

@trondhindenes
Copy link
Contributor

My reasoning is the difference in how they are transformed into a string object (as win_nssm clearly shows, although I'll agree that the implementation is a bit clunky). A hashtable serialized into a string will simply be the string "System.Collections.Hashtable". A string-serialized psobject translates into something like "@{name=trond}" (for a psobject with a single "name" property). I'm pretty sure it would be a good idea to have "type parsers" for both types in powershell.ps1, but we should be careful to treat them as the same, because they're not.

@dagwieers
Copy link
Contributor

@trondhindenes We will have to support what win_nssm supported earlier. (which is a YAML and a key=value pair if I understand it correctly). I still don't see what a PSObject brings us, so I would simply stick with -type="dict", but let's discuss it in a WWG meeting.

@trondhindenes
Copy link
Contributor

Totally agree. Just for clarity: If you supply a "key/value-type" parameter (as of Ansible 2.3.0) and don't specify the type with "Get-AnsibleParameter", then the resulting type is a Hashtable. Win_nssm expects a serialized psobject instead, which is why it's choking (and how Ansible was working prior to 2.3.0). One might argue that it would be better to refactor win_nssm (and I'm all for it), but in the meantime it's my opinion that we just need to get this working. I'm not sure if we're discussing the same thing, because like I mentioned above "dict" is really not a basic type available in PowerShell.
Short story, #25356 would allow us to:

  1. get win_mssn working without a major refactor
  2. Allow powershell devs to have a "psobject" type available without having to parse it inside each module needing it
  3. Help Powershell devs by using "powershell lingo" instead of python-lingo. (If someone told me to create a "dict" before I started dabbling in python, I'd have no idea what that meant :-) )

@jhawkesworth
Copy link
Contributor

Hi

I sort of think we need to have dict and psobject, and not just so there's something familiar for both 'people coming from powershell' and 'people coming from python'. Why? because they are (slightly) different things.

My understanding is that powershell hashtable is a basic key-value structure, with its main purpose being storing and retrieving values by their key as fast as possible. A psobject is a more object-oriented thing with at least 2 extra qualities that a raw hashtable (and I think a python dict) don't have.

The first thing you get with a psobject is ability to add methods into the object (not sure how useful that is in the context of ansible modules) and the second is optional ability to preserve the order in which keys were added. (Just to add yet another language to the table, this would be analogous to a java LinkedHashSet).

I think for win_nssm (as it stands) we need the insertion order preserved (otherwise those args could get deserialised in a random order).

If anyone has capacity for refactoring win_nssm I would do my best to review it.

@dagwieers
Copy link
Contributor

@jhawkesworth Good point, we discussed it briefly yesterday, but I really wanted to defer a decision to the next meeting (which has more favorable hours for @trondhindenes and others).

Related to command-line arguments and preserved order, that is why for python lists are being used for the same purpose, or it remains a string (depending on the exact syntax). Although in theory you could use an OrderedDict, but that is not implemented as a type (I have a feature request open for this).

@dagwieers
Copy link
Contributor

@jhawkesworth The name "dict" or "dictionaries" is not very specific to Python, it's been called associative array, hash, map (e.g. in YAML) or mappings, key-table, or tables and by Powershell hashtable. https://en.wikipedia.org/wiki/Associative_array

BTW If we plan to add ordered dictionaries, I would propose to call this type "omap" (YAML). And would then map exactly with the !!omap typing in YAML.

PS It makes more sense to me to migrate over time to the YAML type names, rather than the Python type names. A lot or similar, and we could add the YAML types http://yaml.org/type/ Maybe something to discuss on a Core meeting ?

@afunix
Copy link
Contributor

afunix commented Jun 17, 2017

Any chance this one will be fixed in 2.3?
I see different pull request on this matter but all of them for 2.4

@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Jun 29, 2017
@jhawkesworth
Copy link
Contributor

At 19:00 UTC on Tuesday 19 September 2017 this ticket will be processed during the second Windows Sprint,
https://github.com/ansible/community/wiki/Windows:-sprints
we would appreciate if you could join the Sprint to help out with understanding/fixing/closing this issue.

@jhawkesworth
Copy link
Contributor

Discussed at the sprint, @dagwieers will attempt to refactor this module.

@ansibot
Copy link
Contributor

ansibot commented Oct 5, 2017

@ansibot
Copy link
Contributor

ansibot commented Oct 29, 2017

cc @themiwi
click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Nov 23, 2017

@ansibot
Copy link
Contributor

ansibot commented Jan 10, 2018

@ansibot ansibot added bug This issue/PR relates to a bug. and removed bug_report labels Mar 1, 2018
@cukal
Copy link
Contributor

cukal commented Jun 19, 2018

Ran into this problem in 2.5.5

@ksubileau
Copy link
Contributor

I also encountered the same issue in version 2.5.4

ksubileau added a commit to ksubileau/ansible that referenced this issue Aug 27, 2018
These tests highlight several issues with this module:
 * Service not started when state=started
 * Errors with app_parameters (see ansible#25265)
 * Exception when passing several dependencies separated by comma as specified in doc
ksubileau added a commit to ksubileau/ansible that referenced this issue Aug 27, 2018
ksubileau added a commit to ksubileau/ansible that referenced this issue Aug 28, 2018
These tests highlight several issues with this module:
 * Service not started when state=started
 * Errors with app_parameters (see ansible#25265)
 * Exception when passing several dependencies separated by comma as specified in doc
ksubileau added a commit to ksubileau/ansible that referenced this issue Aug 28, 2018
@ansibot ansibot added support:core This issue/PR relates to code supported by the Ansible Engineering Team. and removed support:community This issue/PR relates to code supported by the Ansible community. labels Sep 14, 2018
jborean93 pushed a commit that referenced this issue Sep 27, 2018
* win_nssm: add failing tests for issue #44079

* win_nssm: use Run-Command instead of Invoke-Expression to prevent interpretation issue

Fix #44079

* win_nssm: add more failing tests

These tests highlight several issues with this module:
 * Service not started when state=started
 * Errors with app_parameters (see #25265)
 * Exception when passing several dependencies separated by comma as specified in doc

* win_nssm: fix service not started when state=started

Nssm status returns a multiline output that doesn't match any of the strict patterns in the switch statement.

* win_nssm: fix incorrect separator in doc for service dependencies

The dependencies parameter works with space as separator, but not with comma as shown in the documentation

* win_nssm: fix error with app_parameters parameter

Fix #25265

* win_nssm: add idempotence tests

* win_nssm: fix several idempotence issues and misbehaviors

Add missing space between arguments when app_parameters contains several keys.
Use Argv-ToString and Escape-Argument to improve arguments handling (parameters with quotes, backslashes or spaces).

* win_nssm: test parameters with spaces, quotes or backslashes

* win_nssm: restore comma as separator for service dependencies

Revert commit ddd4b4b

* win_nssm: restore support of string as dict form for app_parameters and remove support of literal YAML dict

* win_nssm: wrong variable in tests
ksubileau added a commit to ksubileau/ansible that referenced this issue Sep 29, 2018
These tests highlight several issues with this module:
 * Service not started when state=started
 * Errors with app_parameters (see ansible#25265)
 * Exception when passing several dependencies separated by comma as specified in doc

(cherry picked from commit e50234b)
ksubileau added a commit to ksubileau/ansible that referenced this issue Sep 29, 2018
ksubileau added a commit to ksubileau/ansible that referenced this issue Sep 29, 2018
These tests highlight several issues with this module:
 * Service not started when state=started
 * Errors with app_parameters (see ansible#25265)
 * Exception when passing several dependencies separated by comma as specified in doc

(cherry picked from commit e50234b)
ksubileau added a commit to ksubileau/ansible that referenced this issue Sep 29, 2018
abadger pushed a commit that referenced this issue Oct 9, 2018
These tests highlight several issues with this module:
 * Service not started when state=started
 * Errors with app_parameters (see #25265)
 * Exception when passing several dependencies separated by comma as specified in doc

(cherry picked from commit e50234b)
abadger pushed a commit that referenced this issue Oct 9, 2018
mattclay pushed a commit to mattclay/ansible that referenced this issue Oct 17, 2018
These tests highlight several issues with this module:
 * Service not started when state=started
 * Errors with app_parameters (see ansible#25265)
 * Exception when passing several dependencies separated by comma as specified in doc

(cherry picked from commit e50234b)
mattclay pushed a commit to mattclay/ansible that referenced this issue Oct 17, 2018
mattclay added a commit that referenced this issue Oct 17, 2018
* win_nssm: add failing tests for issue #44079

(cherry picked from commit a5d1241)

* win_nssm: add more failing tests

These tests highlight several issues with this module:
 * Service not started when state=started
 * Errors with app_parameters (see #25265)
 * Exception when passing several dependencies separated by comma as specified in doc

(cherry picked from commit e50234b)

* win_nssm: use Run-Command instead of Invoke-Expression to prevent interpretation issue

Fix #44079

(cherry picked from commit 20a0d90)

* win_nssm: fix service not started when state=started

Nssm status returns a multiline output that doesn't match any of the strict patterns in the switch statement.

(cherry picked from commit 8180a7c)

* win_nssm: fix incorrect separator in doc for service dependencies

The dependencies parameter works with space as separator, but not with comma as shown in the documentation

(cherry picked from commit ddd4b4b)

* win_nssm: fix error with app_parameters parameter

Fix #25265

(cherry picked from commit aba0d48)

* win_nssm: add idempotence tests

(cherry picked from commit 46a5e4f)

* win_nssm: fix several idempotence issues and misbehaviors

Add missing space between arguments when app_parameters contains several keys.
Use Argv-ToString and Escape-Argument to improve arguments handling (parameters with quotes, backslashes or spaces).

(cherry picked from commit 933a409)

* win_nssm: test parameters with spaces, quotes or backslashes

(cherry picked from commit 51843a7)

* win_nssm: restore comma as separator for service dependencies

Revert commit ddd4b4b

(cherry picked from commit ead882b)

* win_nssm: restore support of string as dict form for app_parameters and remove support of literal YAML dict

(cherry picked from commit 8628552)

* win_nssm: wrong variable in tests

(cherry picked from commit 9b9c839)

* win_nssm: add changelog fragment
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.3 This issue/PR affects Ansible v2.3 bug This issue/PR relates to a bug. module This issue/PR relates to a module. support:core This issue/PR relates to code supported by the Ansible Engineering Team. windows Windows community
Projects
None yet
8 participants