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

[BUG] support list values for event_id in salt.wait_for_event #60430

Closed
jwyoungpm opened this issue Jun 23, 2021 · 3 comments · Fixed by #60443
Closed

[BUG] support list values for event_id in salt.wait_for_event #60430

jwyoungpm opened this issue Jun 23, 2021 · 3 comments · Fixed by #60443
Assignees
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Milestone

Comments

@jwyoungpm
Copy link
Contributor

jwyoungpm commented Jun 23, 2021

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

I am trying to use salt orchestration to wait for "old" nodes of a given role to go away because I cannot perform an operation on new nodes of given role until that occurs.

To do this, I would like to listen to salt/presence/change events and have id_list look for values in lost.

This currently does not work because wait_for_event expects the value for event_id to be a str and does not support list[str]

Offending line here: https://github.com/saltstack/salt/blob/v3003/salt/states/saltmod.py#L668

Given the following orchestration state:

wait-for-node-death:
  salt.wait_for_event:
    - name: salt/presence/change
    - id_list:
       - some_old_node
       - another_old_node
    - timeout: 3000
    - event_id: lost

and an event that looks like this:

{
   "data": {
      "new": [], 
      "lost": [
         "some_old_node"
      ], 
      "_stamp': '2021-06-23T15:02:04.621354"
     }, 
     "tag": "salt/presence/change"
}

A value exception is thrown because the linked line is doing:

["some_old_node","another_old_node"].index(["some_old_node"])

This is caught with:

[TRACE   ] wait_for_event: Event identifier 'lost' not in id_list; skipping.

Describe the solution you'd like
A clear and concise description of what you want to happen.

When the event value retrieved from the location @ event_id is a list, iterate through it and pop any values that
match a value in id_list off of id_list

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Option 1:

Salt mine that gets updated by a reactor + orchestration when a node is lost, that way "new" nodes can tell approximately when an "old" node is lost. It seems like overkill to have a reactor + orchestration just to call salt mine.update.

Option 2:

Use something like test.ping to try and find nodes that are no longer responding but I'd still have to do something like loop.until or something similar.

Option 3:

Have a reactor kick off orchestration to re-emit a custom event with just the id of one node when salt/presence/change is emitted, then have my orchestration wait on that custom event.

If there are any other ways that this can be done, open to hearing those but I'd really like to just be able to react when a minion goes goes offline.

I am also happy to contribute to this, it doesn't seem that difficult to accomplish, opening an FR to see if I'm missing something or if there are better/existing ways to do this.

Option 4 (what I ended up doing today):

Have a reactor meant for my specific subset of events listen to ALL presence change events, then do a bunch of jinja2 to determine if my old nodes are still working. I only don't like this because its kicking off orchestration for all presence events for all nodes, not just the nodes I'm interested in, so most executions of the orchestration won't end up doing anything.

Additional context
Add any other context or screenshots about the feature request here.

Please Note
If this feature request would be considered a substantial change or addition, this should go through a SEP process here https://github.com/saltstack/salt-enhancement-proposals, instead of a feature request.

@jwyoungpm jwyoungpm added Feature new functionality including changes to functionality and code refactors, etc. needs-triage labels Jun 23, 2021
@jwyoungpm
Copy link
Contributor Author

Went ahead and opened an MR for how I'd fix this issue - please let me know if there is any feedback. I don't mean to subvert feature review, just seemed like something I could get in pretty easily.

@jwyoungpm
Copy link
Contributor Author

@sagetherage @xeacott just pinging on this one, I have not gotten any feedback on it or the accompanying MR.

@xeacott
Copy link
Contributor

xeacott commented Jul 13, 2021

Hi @jwyoungpm I personally do not have much experience/knowledge in this specific portion of salt, however thank you for opening up a pull request. I'll get the pull request over to the team and see what they think.

@xeacott xeacott added Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around and removed needs-triage labels Jul 13, 2021
@xeacott xeacott added this to the Approved milestone Jul 13, 2021
@sagetherage sagetherage removed the Feature new functionality including changes to functionality and code refactors, etc. label Jul 22, 2021
@sagetherage sagetherage changed the title [FEATURE REQUEST] support list values for event_id in salt.wait_for_event [BUG] support list values for event_id in salt.wait_for_event Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants