logstash_mod makePickle doesn't convert to bytes

[kiemlicz]

What does this PR do?

Adds missing to_bytes conversion in log

What issues does this PR fix or reference?

#51123

Previous Behavior

--- Logging error ---
Traceback (most recent call last):
  File "/usr/lib/python3.6/logging/handlers.py", line 634, in emit
    self.send(s)
  File "/usr/lib/python3.6/logging/handlers.py", line 692, in send
    self.sock.sendto(s, self.address)
TypeError: a bytes-like object is required, not 'str'

New Behavior

No stacktrace, message sent to logstash

Tests written?

No

[Ch3LL]

Can you please add a regression test so we can ensure this does not fail on python3 in the future?

[kiemlicz]

I have Salt installed for development + I've followed:
https://docs.saltstack.com/en/latest/topics/development/tests/index.html#running-test-subsections
Running any test (branch develop), yields:

> python3 tests/runtests.py --unit
Traceback (most recent call last):
  File "tests/runtests.py", line 62, in <module>
    from tests.integration import TestDaemon  # pylint: disable=W0403
  File "/home/stanislaw/projects/open-source/salt/tests/integration/__init__.py", line 36, in <module>
    from tests.support.case import ShellTestCase
  File "/home/stanislaw/projects/open-source/salt/tests/support/case.py", line 35, in <module>
    from tests.support.mixins import AdaptedConfigurationTestCaseMixin, SaltClientTestCaseMixin
  File "/home/stanislaw/projects/open-source/salt/tests/support/mixins.py", line 33, in <module>
    import salt.config
  File "/home/stanislaw/projects/open-source/salt/salt/config/__init__.py", line 27, in <module>
    import salt.utils.network
  File "/home/stanislaw/projects/open-source/salt/salt/utils/network.py", line 24, in <module>
    import salt.utils.win_network
  File "/home/stanislaw/projects/open-source/salt/salt/utils/win_network.py", line 42, in <module>
    USE_WMI = StrictVersion(platform.version()) < StrictVersion('6.2')
  File "/usr/lib/python3.5/distutils/version.py", line 40, in __init__
    self.parse(vstring)
  File "/home/stanislaw/projects/open-source/salt/salt/utils/versions.py", line 36, in parse
    _StrictVersion.parse(self, vstring)
  File "/usr/lib/python3.5/distutils/version.py", line 137, in parse
    raise ValueError("invalid version number '%s'" % vstring)
ValueError: invalid version number '#1 SMP Debian 4.9.110-3+deb9u6 (2018-10-08)'

So... I find that there is this magic _version.py that obviously I need to setup, but I have no luck with properly creating it.
How do I create salt/_version.py locally so that I can proceed?

[Ch3LL]

hmm i havent see that error before. Can you paste the commands you used to install the test requiremenst and what OS are you running from?

[kiemlicz]

Sure
First: It's a develop branch
My OS it's a debian 9.9

> uname -ra
Linux my-hostname-here 4.9.0-8-amd64 #1 SMP Debian 4.9.110-3+deb9u6 (2018-10-08) x86_64 GNU/Linux

In virtualenv, I've performed roughly:

salt_dir > pip3 install pyzmq PyYAML pycrypto msgpack-python jinja2 psutil futures tornado
salt_dir > pip3 install -e .
salt_dir > pip3 install -r requirements/dev_python34.txt
salt_dir > pip3 install -r requirements/pytest.txt
salt_dir > pip3 install -r requirements/tests.txt

[Ch3LL]

I wasn't able to recreate this traceback, but was able to track down thats because it was fixed on develop by this commit: fded9da

Can you rebase with develop and check again?

[kiemlicz]

Thank you, using latest develop works!
I've added simple unit test that tests if the log is successfully pickled, sent and received

Could you please check if the test meets your needs (it's rather simple test)?

Shall I add ZMQLogstashHandler test as well?
However since the error occurs right before hitting the wire I need (?) some ZQM test receiver.
Given I have created one (or there is some other approach I should undertake?), should I move such a test to the integration tests (it adds some runtime overhead)?

[Ch3LL]

thanks for adding that test :) really appreciate it.

Shall I add ZMQLogstashHandler test as well?

yes we want to ensure we have coverage for both changes.

[kiemlicz]

I've added ZMQ test
I must admit though I'm not proud of that test, I couldn't send/receive the very first message,
thus I created 'loopy' way: since I want to test the pickling not the ZMQ

Could you please check?

[Ch3LL]

thanks for taking the time to write that. Looks like there is just one lint error that needs cleaning up

I would also like @dwoz to take a look at that zmq test to ensure everything gets shut down properly so it does not affect other tests.

[kiemlicz]

Yes, I forgot, now it should be fine, thank you

[kiemlicz]

Is it good to go and shall I merge or are there some more checks to be done (I've done the requested changes some time ago)?

[waynew]

From what I gather from this answer on Stack Overflow, the socket should get cleaned up once the test gets disposed. If the tests aren't actually getting disposed then we should explicitly close the zmq socket/context.

[kiemlicz]

I've added close() and term() calls to tearDown. Is it ok?

[waynew]

I just noticed this, but that should be under probably tests/integration/log/handlers/test_logstash_mod.py - probably need a couple of directories with __init__.py in them.

This is setting up actual TCP/IP servers, so it should go under integration tests rather than unit tests.

[kiemlicz]

But the integration tests start the Salt-Master and Minion which:

• vastly increases the runtime
• I don't need Master and Minion running

I understand that these tests are not exactly the 'unit' ones but how do I put them under integration and not start Minion and Master?

[waynew]

We don't yet have our test suites running functional tests - this is something that the Test and Release Working Group, and @s0undt3ch in particular has been working on - which would definitely do what you're after.

---

Sidenote: we just released this SEP as announced in our Office Hours this morning, and we're changing our branch/release strategy.

The quickest way to get this merged in will be to rebase your changes on master, force push to your fork, and edit this PR to point to master instead.

[kiemlicz]

Thanks
So the integration tests are not yet running (in some kind of CI) but eventually they will?

@s0undt3ch Is there a possibility to disable starting salt-master and salt-minion for particular Salt integration test?

@waynew please advice if you still want to move the test under integration even in case there is no possibility to not run salt-master/minion in integration tests (please mind that runtime impact is huge)

[s0undt3ch]

Is there a possibility to disable starting salt-master and salt-minion for particular Salt integration test?

Not currently.

[s0undt3ch]

So the integration tests are not yet running (in some kind of CI) but eventually they will?

They are, we just can't yet control config changes on the running master/minion. It's all or nothing.

Soon, we'll have functional testing which is what's suited to your particular testing scenario.

[kiemlicz]

@s0undt3ch thanks
@waynew please advice if you still want this under integration package (mind the runtime impact)

[waynew]

@kiemlicz I think I'd rather (currently) see this under integration - while it's true that it will take longer to run the suite locally, the server startup only happens once when we do the full test run, so there really isn't extra overhead.

You could put a TODO comment in there that explains that it should be moved to the functional suite, when that becomes available.

[kiemlicz]

Please excuse the delay, I've moved the tests under integration suite. Could you please verify?

[waynew]

Looks like this got out of date with master - I merged the latest changes in and we'll see if the build passes.

[waynew]

Only one failing build, but that was just due to infrastructure. I've restarted py2/amazon1, then this should be good to go 👍

[kiemlicz]

I guess that I won't have permissions to perform the merge and write to the repo
Will someone with the permissions be able to merge this?

[Ch3LL]

@kiemlicz still a couple failing tests and one of them didn't still have their results so restarted the tests. i'll try to monitor this one closely for ya to make sure we get it in soon.

[kiemlicz]

Thank you.
I've checked the test results and I think that I didn't introduce breaking changes.
The master branch timeouts as well

[Ch3LL]

yeah looks like those tests are timing out. I noticed the amazon tests are taking a much longer time to run then previously. I created this issue: #55852 so one of us can investigate why this is occurring but in the meantime a workaround was merged in to increase the timeout to 7 hours so i'll update your branch so the tests can run with this new fix so we makes sure all hte tests run on that OS.