REVIEW ONLY - Track jid in proc file for runners #5

austinpapp · 2019-02-12T15:31:44Z

Added support functions to handle reading proc files master side
Added clean proc dir for master side
dateutil is not always there
async option for ShellTestCase/ShellCase

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

Remove this section if not relevant

New Behavior

Remove this section if not relevant

Tests written?

Yes/No

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

clickthisnick2 · 2019-02-13T14:36:47Z

salt/utils/master.py

+        except Exception as err:
+            # need to add serial exception here
+            # Could not read proc file
+            log.warrning("Issue deserializing data: %s", err)


Suggested change

log.warrning("Issue deserializing data: %s", err)

log.warning("Issue deserializing data: %s", err)

clickthisnick2 · 2019-02-13T14:56:54Z

tests/integration/utils/test_master.py

+from tests.support.unit import skipIf
+from tests.support.case import ShellTestCase
+from tests.support.paths import TMP_ROOT_DIR
+from tests.support.runtests import RUNTIME_VARS


Doesn't look like this is used. Does it need to be imported?

Suggested change

from tests.support.runtests import RUNTIME_VARS

clickthisnick2 · 2019-02-13T15:06:01Z

tests/integration/scheduler/test_skip.py

@@ -6,11 +6,16 @@
 import logging
 import os

-import dateutil.parser as dateutil_parser
+try:
+    import dateutil.parser as dateutil_parser


Just wondering, was the test working before in master? - Couldn't we add to the dev.txt requirements

this is a PR for upstream not salt-core

clickthisnick2 · 2019-02-13T15:13:55Z

tests/integration/utils/test_master.py

+        jid = ret['jid']
+        time.sleep(2)
+        jobs = master.get_running_jobs(DEFAULT_CONFIG)
+        assert any([job['jid'] == jid for job in jobs])


This is nitpicky of me, but this asserts that there could be multiple jobs with the same jid (Which should never happen).

Could we change, to assert that one and only one jobs has the wanted jid:

Suggested change

assert any([job['jid'] == jid for job in jobs])

jids = [job['jid'] for job in jobs]

assert jids.count(jid) == 1

meh. but i can change it.

mattp- · 2019-02-12T19:59:43Z

salt/client/mixins.py

+            # Job has finished or issue found, so let's clean up after ourselves
+            try:
+                os.remove(jid_proc_file)
+            except OSError as err:


you catch ioerror below but only soerror here, why?

well, im not actually reading it so there is no ioerror possible.

mattp- · 2019-02-12T20:00:33Z

salt/utils/master.py

+        try:
+            proc = psutil.Process(pid)
+        except psutil.NoSuchProcess:
+            log.warning("PID %s is no longer running.", pid)


is a warning necessary here. maybe its called in cases where pid being dead/finished isnt an error?

yea i was on the fence with that. technically this should never happen because "things" should clean up after themselves. i didnt want to generate an error bc i thought that was a bit much.

mattp- · 2019-02-12T20:01:17Z

salt/utils/master.py

        return False
+
+    cmdline_file = "/proc/{}/cmdline".format(pid)


you should probably use os.path.join

mattp- · 2019-02-13T15:24:05Z

tests/integration/scheduler/test_skip.py

@@ -6,11 +6,16 @@
 import logging
 import os

-import dateutil.parser as dateutil_parser
+try:
+    import dateutil.parser as dateutil_parser


this is a PR for upstream not salt-core

* Added support functions to handle reading proc files master side * Added clean proc dir for master side * Added dateutil and setproctile to tests.txt * async option for ShellTestCase/ShellCase

…into master_proc_file

also added flaky to the test

* Added support functions to handle reading proc files master side * Added clean proc dir for master side * Added dateutil and setproctile to tests.txt * async option for ShellTestCase/ShellCase

also added flaky to the test

Add event_listen_queue_max_seconds to fix saltstack#53411

Only use the provider conf.d file we are testing

clickthisnick2 reviewed Feb 13, 2019

View reviewed changes

mattp- suggested changes Feb 13, 2019

View reviewed changes

austinpapp force-pushed the master_proc_file branch 5 times, most recently from f8f522a to aeddd21 Compare February 13, 2019 19:25

Track jid in proc file for runners

c466103

* Added support functions to handle reading proc files master side * Added clean proc dir for master side * Added dateutil and setproctile to tests.txt * async option for ShellTestCase/ShellCase

austinpapp force-pushed the master_proc_file branch from e3ea314 to c466103 Compare February 13, 2019 19:47

austinpapp and others added 7 commits February 13, 2019 14:50

Merge branch 'develop' into master_proc_file

d46cc7c

lint

d356667

return byte data

441afea

Merge branch 'develop' into master_proc_file

90a99b8

increasing the sleep for tests

8dae34e

Merge branch 'master_proc_file' of https://github.com/bloomberg/salt …

d8c744f

…into master_proc_file

Attempt to wait but more intelligently

b0a45e9

also added flaky to the test

austinpapp force-pushed the master_proc_file branch from 56c0de2 to b0a45e9 Compare February 19, 2019 20:06

Austin Papp and others added 8 commits February 28, 2019 16:27

Track jid in proc file for runners

ebaf0bb

* Added support functions to handle reading proc files master side * Added clean proc dir for master side * Added dateutil and setproctile to tests.txt * async option for ShellTestCase/ShellCase

lint

d50569c

return byte data

5570f0f

increasing the sleep for tests

e189120

Attempt to wait but more intelligently

0e4e664

also added flaky to the test

Merge branch 'develop' into master_proc_file

c17c672

fix

38a8498

see if we can catch the job earlier

365c607

austinpapp force-pushed the master_proc_file branch from eaa535c to 365c607 Compare February 28, 2019 22:17

clean up debug

a4a257d

Austin Papp and others added 4 commits March 11, 2019 11:10

lint

2ed902e

Merge branch 'develop' into master_proc_file

70d4488

lint

68880b9

Merge branch 'develop' into master_proc_file

63cdd3a

austinpapp merged commit 63cdd3a into develop Mar 18, 2019

sbrennan4 pushed a commit that referenced this pull request Aug 19, 2019

Merge pull request #5 from mphillips81/v2018.3.3-ca

0092325

Add event_listen_queue_max_seconds to fix saltstack#53411

mattp- pushed a commit that referenced this pull request Oct 21, 2019

Merge pull request #5 from Ch3LL/cloud_provider_files

e58b40a

Only use the provider conf.d file we are testing

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REVIEW ONLY - Track jid in proc file for runners #5

REVIEW ONLY - Track jid in proc file for runners #5

austinpapp commented Feb 12, 2019

clickthisnick2 Feb 13, 2019

austinpapp Feb 13, 2019

clickthisnick2 Feb 13, 2019

austinpapp Feb 13, 2019

clickthisnick2 Feb 13, 2019

mattp- Feb 13, 2019

clickthisnick2 Feb 13, 2019

austinpapp Feb 13, 2019

mattp- Feb 12, 2019

austinpapp Feb 13, 2019 •

edited

Loading

mattp- Feb 12, 2019

austinpapp Feb 13, 2019

mattp- Feb 12, 2019

austinpapp Feb 13, 2019

mattp- Feb 13, 2019

	log.warrning("Issue deserializing data: %s", err)
	log.warning("Issue deserializing data: %s", err)

	assert any([job['jid'] == jid for job in jobs])
	jids = [job['jid'] for job in jobs]
	assert jids.count(jid) == 1

REVIEW ONLY - Track jid in proc file for runners #5

REVIEW ONLY - Track jid in proc file for runners #5

Conversation

austinpapp commented Feb 12, 2019

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

New Behavior

Tests written?

Commits signed with GPG?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinpapp Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinpapp Feb 13, 2019 •

edited

Loading