Skip to content

SEP-17: Job acknowledgement events #55820

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

Closed
wants to merge 6 commits into from

Conversation

lukasraska
Copy link
Contributor

@lukasraska lukasraska commented Jan 8, 2020

What does this PR do?

Implements new event type salt/job/<JID>/ack/<MID> that is fired everytime minion receives job for execution.

What issues does this PR fix or reference?

saltstack/salt-enhancement-proposals#23

Tests written?

Yes, but unit tests only. I've prepared integration tests as well, but they are too flaky to be used steadily (they use multithreaded Queue and it doesn't play well with these tests on various platforms).

Commits signed with GPG?

Yes

TODO:

  • Update release notes before release (not done, since not sure when will it be merged)

@lukasraska
Copy link
Contributor Author

re-run pr-opensuse15-py3

@lukasraska lukasraska changed the title WIP: Job acknowledgement events Job acknowledgement events Jan 12, 2020
@lukasraska lukasraska changed the title Job acknowledgement events SEP-17: Job acknowledgement events Jan 12, 2020
@lukasraska lukasraska marked this pull request as ready for review January 12, 2020 20:05
@lukasraska lukasraska requested a review from a team as a code owner January 12, 2020 20:05
@ghost ghost requested a review from DmitryKuzmenko January 12, 2020 20:05
@dwoz
Copy link
Contributor

dwoz commented Jan 12, 2020

@lukasraska Please add your integration tests. I will take a look at making them less flaky.

@lukasraska
Copy link
Contributor Author

@dwoz Thanks for looking into this, I've pushed them and rebased to latest master. From my tries, the setUpClass method can sometimes raise Empty for no apparent reason (some environments work almost all the time, some raise it almost every time). Also I don't know if the Win test failures are related to this PR, but running just these tests is working fine on my win-server setup.

It also might be worth noting, before the change in logging, these tests using multithreaded Queue were completely failing on Win and OSX (they worked when print was put strategically, as I mentioned on SaltConf).

If you can think of how to make them more successful, tests.integration.modules.test_event can reuse the same mixin and can be re-enabled.

@waynew
Copy link
Contributor

waynew commented Jan 16, 2020

It also might be worth noting, before the change in logging, these tests using multithreaded Queue were completely failing on Win and OSX (they worked when print was put strategically, as I mentioned on SaltConf).

That's interesting, and hints at a race condition. It may be something else, but print involves a pretty hefty wait on that thread along with the system call(s). You should check if a time.sleep(1) accomplishes the same thing. That would be a strong indication that we're running into a race condition. If it's only the print that works, though, that's pretty confusing.

@Ch3LL Ch3LL removed the request for review from a team April 15, 2020 14:28
@sagetherage sagetherage added the merge-conflict PR has a merge conflict label Nov 18, 2020
@sagetherage sagetherage requested review from dwoz and removed request for DmitryKuzmenko November 18, 2020 18:51
@dwoz dwoz added the Abandoned label Dec 10, 2023
@dwoz
Copy link
Contributor

dwoz commented Dec 10, 2023

@lukasraska are you still interested in pushing this over the finish line?

Copy link

codecov bot commented Dec 10, 2023

Codecov Report

Merging #55820 (55f0ae9) into master (cdca5aa) will decrease coverage by 2.28%.
Report is 13571 commits behind head on master.
The diff coverage is 91.67%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #55820      +/-   ##
==========================================
- Coverage   42.89%   40.60%   -2.28%     
==========================================
  Files        1556     1549       -7     
  Lines      281049   272241    -8808     
  Branches    62376    58660    -3716     
==========================================
- Hits       120536   110526   -10010     
- Misses     146825   149369    +2544     
+ Partials    13688    12346    -1342     
Flag Coverage Δ
amazon2 38.92% <50.00%> (-0.19%) ⬇️
archlts 16.40% <33.34%> (-0.02%) ⬇️
centos7 39.55% <83.34%> (-1.24%) ⬇️
centos8 ?
cloud ?
debian10 39.05% <50.00%> (-0.16%) ⬇️
debian9 39.23% <50.00%> (-0.20%) ⬇️
fedora30 39.02% <50.00%> (-0.29%) ⬇️
fedora31 39.02% <50.00%> (-0.22%) ⬇️
m2crypto 39.89% <50.00%> (-0.24%) ⬇️
macosxhighsierra ?
macosxmojave 38.25% <50.00%> (-0.14%) ⬇️
macosxsierra ?
opensuse15 38.93% <50.00%> (-0.40%) ⬇️
proxy 23.81% <50.00%> (-0.02%) ⬇️
py3 40.53% <83.34%> (-2.16%) ⬇️
pycryptodomex 39.79% <50.00%> (-0.45%) ⬇️
runtests 40.53% <83.34%> (-2.16%) ⬇️
tcp 39.81% <50.00%> (-0.47%) ⬇️
ubuntu1604 39.83% <83.34%> (-0.25%) ⬇️
ubuntu1804 39.07% <50.00%> (-0.24%) ⬇️
windows2016 ?
windows2019 ?
zeromq 40.51% <83.34%> (-1.40%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
salt/minion.py 39.17% <100.00%> (+3.66%) ⬆️
salt/metaproxy/proxy.py 30.77% <83.34%> (+8.53%) ⬆️

... and 267 files with indirect coverage changes

@dwoz dwoz added this to the Argon v3008.0 milestone Dec 18, 2023
Copy link
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests need to be migrated to pytest

--------------------

.. versionadded:: Sodium

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chlorine or Argon

@twangboy
Copy link
Contributor

Also needs a changelog

@dwoz
Copy link
Contributor

dwoz commented Jul 5, 2024

This can be re-based and re-opened if it should get into 3008.x

@dwoz dwoz closed this Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Abandoned merge-conflict PR has a merge conflict
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants