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

Run beacons on the only one minion instance. Return to all masters. #53344

Closed
wants to merge 7 commits into from

Conversation

@DmitryKuzmenko
Copy link
Contributor

commented Jun 3, 2019

What does this PR do?

In multimaster environments run beacons on the only one minion instance. The instance associated with the first master in the list.
The results are reported through minion event bus to all minion instances thus fired to all connected masters.

What issues does this PR fix or reference?

Fixes #49680
Fixes #49663

Previous Behavior

Beacons were executed on all minion instances. As a result the minion does extra work and sometimes beacons work unexpectedly like in the case of inotify beacon that was reporting results to the only one random master.

New Behavior

Beacons executed once. Results reported to all connected masters.

Tests written?

Manual.

Commits signed with GPG?

Yes

@DmitryKuzmenko DmitryKuzmenko requested a review from saltstack/team-core as a code owner Jun 3, 2019
@waynew

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

I'm pretty new to beacons (and the multimaster setup), so forgive me if this question doesn't make sense.

For inotify, isn't the purpose to watch a file/directory for changes? Wouldn't we want to be performing that check on each of the minions? At least the targeted ones? Or is there something with the multimaster/beacon setup that should change the way I'm thinking?

@DmitryKuzmenko

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@waynew Here I'm speaking about the minion object instances in one minion process. That is the way minion works in multimaster setup. It's logically one minion in terms of SaltStack.

@DmitryKuzmenko DmitryKuzmenko force-pushed the DSRCorporation:bugs/beacons branch from efdc401 to 7857362 Jun 3, 2019
@DmitryKuzmenko DmitryKuzmenko changed the title [WIP] Run beacons on the only one minion instance. Return to all masters. Run beacons on the only one minion instance. Return to all masters. Jun 3, 2019
@waynew

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

Ah, that was what I was missing! So this is just saying that on each minion there are N minion processes, and only one of them should be doing the beacon work, right?

Is there some check to make sure that there is a beacon leader?

@DmitryKuzmenko

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@waynew multiple minion object instances work inside one minion process. Beacon leader is the minion handling the first master in the list. Others are not. It's handled on the creation time here:
https://github.com/saltstack/salt/pull/53344/files#diff-b5732eecf0273ccf6a96e1c720198dc6R996

@bryceml

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

re-run full all

@codecov

This comment has been minimized.

Copy link

commented Jun 6, 2019

Codecov Report

Merging #53344 into 2018.3 will decrease coverage by 4.5%.
The diff coverage is 4.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           2018.3   #53344      +/-   ##
==========================================
- Coverage   16.44%   11.93%   -4.51%     
==========================================
  Files        1468     1511      +43     
  Lines      242401   257076   +14675     
  Branches    51554    55056    +3502     
==========================================
- Hits        39852    30689    -9163     
- Misses     198407   224161   +25754     
+ Partials     4142     2226    -1916
Flag Coverage Δ
#arch ?
#centos7 ?
#fedora28 11.48% <ø> (?)
#proxy ?
#py2 11.81% <4.54%> (-4.5%) ⬇️
#py3 11.13% <ø> (-0.42%) ⬇️
#raet ?
#ubuntu1604 11.35% <ø> (-3.81%) ⬇️
#windows2016 9.12% <4.54%> (?)
Impacted Files Coverage Δ
salt/minion.py 17.16% <4.54%> (ø)
salt/states/stateconf.py 0% <0%> (-100%) ⬇️
salt/auth/auto.py 0% <0%> (-100%) ⬇️
salt/cli/ssh.py 0% <0%> (-85.72%) ⬇️
salt/auth/sharedsecret.py 0% <0%> (-83.34%) ⬇️
salt/pillar/cmd_yaml.py 0% <0%> (-76.93%) ⬇️
salt/runners/error.py 0% <0%> (-75%) ⬇️
salt/wheel/minions.py 0% <0%> (-75%) ⬇️
salt/cli/key.py 0% <0%> (-70.59%) ⬇️
salt/modules/hosts.py 12.34% <0%> (-70.38%) ⬇️
... and 361 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07a3112...4c65832. Read the comment docs.

3 similar comments
@codecov

This comment has been minimized.

Copy link

commented Jun 6, 2019

Codecov Report

Merging #53344 into 2018.3 will decrease coverage by 4.5%.
The diff coverage is 4.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           2018.3   #53344      +/-   ##
==========================================
- Coverage   16.44%   11.93%   -4.51%     
==========================================
  Files        1468     1511      +43     
  Lines      242401   257076   +14675     
  Branches    51554    55056    +3502     
==========================================
- Hits        39852    30689    -9163     
- Misses     198407   224161   +25754     
+ Partials     4142     2226    -1916
Flag Coverage Δ
#arch ?
#centos7 ?
#fedora28 11.48% <ø> (?)
#proxy ?
#py2 11.81% <4.54%> (-4.5%) ⬇️
#py3 11.13% <ø> (-0.42%) ⬇️
#raet ?
#ubuntu1604 11.35% <ø> (-3.81%) ⬇️
#windows2016 9.12% <4.54%> (?)
Impacted Files Coverage Δ
salt/minion.py 17.16% <4.54%> (ø)
salt/states/stateconf.py 0% <0%> (-100%) ⬇️
salt/auth/auto.py 0% <0%> (-100%) ⬇️
salt/cli/ssh.py 0% <0%> (-85.72%) ⬇️
salt/auth/sharedsecret.py 0% <0%> (-83.34%) ⬇️
salt/pillar/cmd_yaml.py 0% <0%> (-76.93%) ⬇️
salt/runners/error.py 0% <0%> (-75%) ⬇️
salt/wheel/minions.py 0% <0%> (-75%) ⬇️
salt/cli/key.py 0% <0%> (-70.59%) ⬇️
salt/modules/hosts.py 12.34% <0%> (-70.38%) ⬇️
... and 361 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07a3112...4c65832. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Jun 6, 2019

Codecov Report

Merging #53344 into 2018.3 will decrease coverage by 4.5%.
The diff coverage is 4.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           2018.3   #53344      +/-   ##
==========================================
- Coverage   16.44%   11.93%   -4.51%     
==========================================
  Files        1468     1511      +43     
  Lines      242401   257076   +14675     
  Branches    51554    55056    +3502     
==========================================
- Hits        39852    30689    -9163     
- Misses     198407   224161   +25754     
+ Partials     4142     2226    -1916
Flag Coverage Δ
#arch ?
#centos7 ?
#fedora28 11.48% <ø> (?)
#proxy ?
#py2 11.81% <4.54%> (-4.5%) ⬇️
#py3 11.13% <ø> (-0.42%) ⬇️
#raet ?
#ubuntu1604 11.35% <ø> (-3.81%) ⬇️
#windows2016 9.12% <4.54%> (?)
Impacted Files Coverage Δ
salt/minion.py 17.16% <4.54%> (ø)
salt/states/stateconf.py 0% <0%> (-100%) ⬇️
salt/auth/auto.py 0% <0%> (-100%) ⬇️
salt/cli/ssh.py 0% <0%> (-85.72%) ⬇️
salt/auth/sharedsecret.py 0% <0%> (-83.34%) ⬇️
salt/pillar/cmd_yaml.py 0% <0%> (-76.93%) ⬇️
salt/runners/error.py 0% <0%> (-75%) ⬇️
salt/wheel/minions.py 0% <0%> (-75%) ⬇️
salt/cli/key.py 0% <0%> (-70.59%) ⬇️
salt/modules/hosts.py 12.34% <0%> (-70.38%) ⬇️
... and 361 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07a3112...4c65832. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Jun 6, 2019

Codecov Report

Merging #53344 into 2018.3 will decrease coverage by 4.5%.
The diff coverage is 4.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           2018.3   #53344      +/-   ##
==========================================
- Coverage   16.44%   11.93%   -4.51%     
==========================================
  Files        1468     1511      +43     
  Lines      242401   257076   +14675     
  Branches    51554    55056    +3502     
==========================================
- Hits        39852    30689    -9163     
- Misses     198407   224161   +25754     
+ Partials     4142     2226    -1916
Flag Coverage Δ
#arch ?
#centos7 ?
#fedora28 11.48% <ø> (?)
#proxy ?
#py2 11.81% <4.54%> (-4.5%) ⬇️
#py3 11.13% <ø> (-0.42%) ⬇️
#raet ?
#ubuntu1604 11.35% <ø> (-3.81%) ⬇️
#windows2016 9.12% <4.54%> (?)
Impacted Files Coverage Δ
salt/minion.py 17.16% <4.54%> (ø)
salt/states/stateconf.py 0% <0%> (-100%) ⬇️
salt/auth/auto.py 0% <0%> (-100%) ⬇️
salt/cli/ssh.py 0% <0%> (-85.72%) ⬇️
salt/auth/sharedsecret.py 0% <0%> (-83.34%) ⬇️
salt/pillar/cmd_yaml.py 0% <0%> (-76.93%) ⬇️
salt/runners/error.py 0% <0%> (-75%) ⬇️
salt/wheel/minions.py 0% <0%> (-75%) ⬇️
salt/cli/key.py 0% <0%> (-70.59%) ⬇️
salt/modules/hosts.py 12.34% <0%> (-70.38%) ⬇️
... and 361 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07a3112...4c65832. Read the comment docs.

@DmitryKuzmenko

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

Replaced with #54247

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.