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

Add salt-api in windows installer #49949

Conversation

Projects
None yet
5 participants
@rares-pop
Copy link
Contributor

commented Oct 9, 2018

Add salt-api in Windows installer

Signed-off-by: Rares POP rares.pop@ni.com

What does this PR do?

This added salt-api support on Windows:
#48001
This change adds it in the installer too.

Previous Behavior

salt-api wasn't in the Windows installer

Tests written?

No

Commits signed with GPG?

Yes

Add salt-api in windows installer
Signed-off-by: Rares POP <rares.pop@ni.com>

@rallytime rallytime requested a review from twangboy Oct 9, 2018

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2018

Before we merge this, I would like to have a discussion about the level at which we intend to support this going forward. @rares-pop since you've been working on this, do you believe it's production ready? Do we have adequate test coverage? Or, should we mark this as an experimental feature for Neon?

@rares-pop

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

@cachedout - I tested a very specific case, using the cherrypy backend with eauth: auto. That worked pretty good - I created jobs with no parameters, with parameters, targeting both single and multiple minions. I didn't do stress testing, and I ran no salt tests on windows.
More than that, what I tested was on 2018.3, and while debugging, I hit a path where it was looking for a hardcoded path for PAM that didn't work on Windows (that's why I went with eauth - auto, which is enough for our use-cases).
Most of the netapi salt integrations tests are around login. I can try them tomorrow and see what works and what not, but if pam doesn't work, the tests will render useless.

Before saying about the state (production vs experimental), I'd like to have some more stress tests:

  • firing one or two jobs every 0.5 seconds and have that running for few hours
  • firing hundreds/thousands of parallel jobs and see nothing gets corrupted

I can do that tomorrow.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2018

@rares-pop Sounds like a good plan. I'll look forward to hearing the results of your testing tomorrow. Thank you for your quick response!

@dwoz dwoz self-requested a review Oct 9, 2018

@rares-pop

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

@cachedout - sorry for the late reply. Yesterday is was all meetings..

Here is a summary of what I did, and let's choose together between 'production ready' and 'experimental'.

  • as I was saying, the configuration I tested with is kind of limited:
    config here
    I didn't manage to get 'pam' working on windows, that's why the above configuration
  • I did test single function jobs, multi function jobs, with and without arguments targeted to both single and multiple minions. Everything is working.
  • I did create several stress tests, and I got up to numbers like ~10.000 jobs working (both local and local_async clients), and everything worked.
    code here
  • SSE works

My impressions:

  • feature/configuration-wise, it's experimental (not a lot of options work)
  • stability wise, is pretty rock solid

However - I've noticed a memory leak pulled down my whole system after ~10.000 jobs. I don't know yet if it's salt-api, salt-master or a salt-minion I was running on the windows machine. The second minion was ubuntu and had no problems whatsoever. It's a leak we saw on LinuxRT but never seen it reproduced with a vanilla version of salt. So if the windows behavior is reproducible, I'll open an issue.

@rares-pop

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

LE: the leak does reproduce on windows pretty easily (and on LinuxRT). This happens when they are registered to Windows masters. However, it doesn't happen on the ubuntu minion (even though it's registered to the same Windows master).
windows_salt_minion_leak
windows_minion_leak_2

This is just by running the above script and when it hits ~2000 jobs

@dwoz

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

We have some existing tests that historically have not passed on windows and are not yet in the whitelist.txt.

https://github.com/saltstack/salt/tree/develop/tests/integration/netapi/rest_cherrypy

@rares-pop

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

@cachedout, @dwoz - I'm not sure if I'm requested to do any further work. Should I run the windows tests? Or something else?

@dwoz

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

@rares-pop Sounds like we should mark this as experimental for now. I'll add the netapi tests to whitelist.txt so they start running.

@dwoz

dwoz approved these changes Oct 16, 2018

@rares-pop

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

@dwoz - sounds good! Let me know if you need help with the tests or something. BTW, I didn't get the PAM working on Windows.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Oct 16, 2018

@rares-pop Sorry for being a bit late to the discussion. I agree with what you've written here. @twangboy and @dwoz where would you like to see the caveats for this functionality documented?

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

@dwoz and @twangboy bump ^ :)

@twangboy

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

@rares-pop Please add a note in the following location that salt-api using cherrypy is experimental and has limitations, ie: PAM Authentication:

https://github.com/saltstack/salt/blob/develop/salt/netapi/rest_cherrypy/app.py

Please add anything else you find applicable.

Additionally, I ran the scripts you posted on one of My VM's and didn't see the memory leak you're seeing. I was working directly off one of our branches though... I'll try a few more things to see if I can replicate that.

@rares-pop

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

@twangboy - salt-api is experimental only on Windows, right? I mean, I assume it works on other platforms. Should I add a 'windows support' category, and add details there?

The memory leak I see running those scripts are reproducing on Oxygen. I haven't tried them on 'develop'.

@twangboy

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

@rares-pop Yeah, it's experimental on Windows. That sounds fine.

I'm running it on another machine and salt-api isn't starting on Py2... Did you try Py2? If not, that may be another limitation.

I'm trying the script on 2018.3 now... I'll try Fluorine after that...

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

I think a note for the Neon release notes would be good to add as well.

@rares-pop

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

@twangboy - I only tried with py3. I never tried any salt stuff on py2 on windows. BTW - the leak doesn't reproduce through local salt-calls. Just by creating jobs on the master and have them dispatched and executed by the windows minion.

@rallytime - I'll come up with something and then refine it as you guys review it. I glanced through that file but I didn't find a concrete block to populate right now, but I'll do it tomorrow morning (it's close to 11pm here :) ).

rares-pop added some commits Oct 25, 2018

Add salt-api limited support notes on Windows
Add some notes inside the cherrypy netapi module
detailing the limited support and its experimental
status on Windows
Add Neon release notes.

Signed-off-by: Rares POP <rares.pop@ni.com>
Merge branch 'dev/iepopr/add-saltapi-in-windows-installer' of github.…
…com:rares-pop/salt into dev/iepopr/add-saltapi-in-windows-installer
@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

@dwoz and @twangboy is this ready to go in now?

@twangboy

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

@rallytime Looks good

@rallytime rallytime merged commit 9b216d4 into saltstack:develop Nov 1, 2018

11 checks passed

WIP Ready for review
Details
codeclimate All good!
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.