Pass grains in minion start event

[admd]

What does this PR do?

Adds a configuration option to have a selection of grains in the minion start event.

Reason behind this is better integration with Uyuni, especially when a lot of minions are starting in a short time frame - we need to get some minimal data (at least the machine_id) and getting it separately as a reaction to the minion start event (via a separate call or a startup state) generates considerable load on the master.

By default, there will be no grains passed to the event.

Previous Behavior

salt/minion/abid-minion.tf.local/start	{
    "_stamp": "2019-08-01T10:46:22.840597", 
    "cmd": "_minion_event", 
    "data": "Minion abid-minion.tf.local started at Thu Aug  1 12:46:22 2019", 
    "id": "abid-minion.tf.local", 
    "pretag": null, 
    "tag": "salt/minion/abid-minion.tf.local/start"
}

New Behavior

salt/minion/abid-minion.tf.local/start	{
    "_stamp": "2019-08-01T10:46:58.696390", 
    "cmd": "_minion_event", 
    "data": "Minion abid-minion.tf.local started at Thu Aug  1 12:46:58 2019", 
    "grains": {
        "id": "abid-minion.tf.local", 
        "machine_id": "21ee22c6906887c2dd6adeed5d404145"
    }, 
    "id": "abid-minion.tf.local", 
    "pretag": null, 
    "tag": "salt/minion/abid-minion.tf.local/start"
}

Tests written?

Tests will be provided once upstream agreed to approach, this draft PR was opened to make sure the overall approach is OK

Yes/No

Commits signed with GPG?

Yes

[admd]

@waynew @dincamihai

[dincamihai]

Makes sense to me. I don't know if there are side effects to adding a new key to the start events.
Maybe there is already some test that we could update to incorporate this change? @admd

[waynew]

Same here. I would also be interested to know if there's any other performance impact. Obviously it's healthier for the master since it doesn't have to process some other events, but are there any other performance implications?

[admd]

Thank you @waynew & @dincamihai for the input.

@waynew can you please ping someone who might have more experience in this particular part of the code and might point us to any possible side effects.

[max-arnold]

My two cents:

• I would remove the id grain from the examples and replace it with any other grain. All minion events already have the id attribute that could be trusted (unlike the custom added one).
• Why just the start event? Maybe it would be useful to be able to do this:

event_grains:
  - 'salt/*/start':
    - machine_id
    - saltversion
  - 'salt/cloud/*/created':
    - virtual

P.S. Also it would be nice to maybe unify this with autosign_grains, but I wasn't able to figure out how these grains are passed in the payload (salt-run state.event doesn't show them).

[moio]

I would remove the id grain from the examples and replace it with any other grain. All minion events already have the id attribute that could be trusted (unlike the custom added one).

+1

I don't know if there are side effects to adding a new key to the start events.

At least none we are aware of at this point, and we are going to test this properly before asking for the merge.

Why just the start event?

Well, this is the use case we need in Uyuni, so we didn't want to propose an overgeneral solution that's potentially more disruptive/invasive. Of course, if that is deemed a necessary condition for merging we can probably do that.

@waynew: is there any veto from you or any other team member, or any necessary condition you want to see here? Basically we would like to know if the approach is fundamentally acceptable, so that we could proceed to finalize this PR for merging (and base Uyuni code on the assumption this change in Salt will be present at some point).

Thanks!

[max-arnold]

Why just the start event?
Of course, if that is deemed a necessary condition for merging we can probably do that.

@moio I'm just a community member and this was simply a suggestion. Recently I wrote a couple of states that send grains via custom events, and I thought it would be nice to automatically attach grains to any event.

[KChandrashekhar]

@admd any reason this PR is still in draft ? We can have assign someone from Salt team to review this once you publish

[admd]

Hi @KChandrashekhar , only reason it's still in the draft because I wanted to get Ok on the approach, from the salt team, before proceeding. Of course, I can publish it if that's what it takes to get more attention.

[waynew]

@moio I don't have any fundamental concerns - though I'm not yet super familiar with this area of the code so that may not be a glowing endorsement 😅 .

It may be a week or two before there's much spare time to get a solid review of this code - we're working to get the 2019.2.1 release out. I'll bring it up at our next design meeting, though!

[waynew]

@admd discussed this with the team - the biggest concern was that adding to grains can add a significant performance overhead. But if we can specify the grains that should be added to the startup event, and probably default to none - the current behavior - this could be worthwhile.

So, as long as we can "first do no harm", then it should be OK to pass grains on start event (i.e. only when it's explicitly requested).

[admd]

Thank you @waynew for the update. This is exactly how we approached this problem in this PR. By default, there would be no grains passed in the start event as it is today. Only after the user will explicitly add start_event_grains in conf file with the needed grains, we will pass those grains in start event.

[waynew]

@admd in that case, I'd say it's safe to go ahead and flesh this PR out so it's ready for a review 👍

[admd]

Hi @waynew, I have created a new PR #54948 against master and it is ready for the review. I will be closing this PR.