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

Emit stopping/starting events when "salt-cloud -a stop/start" is used. #44400

Merged

Conversation

@vitaliyf
Copy link
Contributor

@vitaliyf vitaliyf commented Nov 4, 2017

What does this PR do?

salt-cloud emits numerous events when instances are created, configured and destroyed, but not when they are stopped or started.

New events were added using code from "destroy" method, so they follow the same naming pattern and have same attributes.

What issues does this PR fix or reference?

Previous Behavior

No events are fired when stop/start is used.

New Behavior

New events will be seen on the event bus, for example:

salt/cloud/my-server.example.com/stopping {
    "_stamp": "2017-11-01T21:16:39.772554",
    "event": "stopping instance",
    "instance_id": "i-17517664499b5503a",
    "name": "my-server.example.com"
}

Tests written?

No

Commits signed with GPG?

Yes

@vitaliyf
Copy link
Contributor Author

@vitaliyf vitaliyf commented Nov 4, 2017

PR against develop as requested in #44375

Copy link
Contributor

@cachedout cachedout left a comment

Is this a new event? If so, does it need to be added to other providers other than just EC2?

@rallytime
Copy link
Contributor

@rallytime rallytime commented Nov 6, 2017

This will be useful to have! But, yes, these are both new events and should be added to the other cloud providers. As well as documented here.

Would you be able to make those additions @vitaliyf?

@vitaliyf
Copy link
Contributor Author

@vitaliyf vitaliyf commented Nov 7, 2017

Good question - only about half of drivers have stop/start methods in them and I am actually planning to add support to one more (softlayer and maybe softlayer_hw). But yes, it would make sense to add these to those that already support stop/start and of course document them.

I can do that - though I can't test majority of them.

Before I go copy/pasting the same thing into 20 files - any idea why there isn't a helper function to emit all cloud events, since they are all structured very similarly?

@rallytime
Copy link
Contributor

@rallytime rallytime commented Nov 7, 2017

Thanks @vitaliyf! All salt events are fired like this, whether they are cloud, master, or minion events. Obviously, there is some cloud-specific tags in there, but the general structure & idea is the same. Copying and pasting that information into each of the start & stop functions is the way to go in this case.

I would add the new start and stop functions for the softlayer/softlayer_hw drivers in a new PR. That way it can be reviewed separately from adding these new cloud events.

I think in addition to documenting these events as I mentioned above, it would also be prudent to make a note of these new events in the cloud section of the Oxygen release notes.

@rallytime
Copy link
Contributor

@rallytime rallytime commented Dec 5, 2017

Hi @vitaliyf - Just checking in here. Did you have a chance to come around to this PR?

@rallytime
Copy link
Contributor

@rallytime rallytime commented Dec 22, 2017

Hi @vitaliyf - Just a bump on this to see if you had a chance to come back around.

@vitaliyf
Copy link
Contributor Author

@vitaliyf vitaliyf commented Dec 25, 2017

Yes, I'll try to get some time this week and add events to all stop/start functions that exist.

@rallytime
Copy link
Contributor

@rallytime rallytime commented May 8, 2018

Hey @vitaliyf - Just checking in here. Were you ever able to come back to this?

@rallytime
Copy link
Contributor

@rallytime rallytime commented May 31, 2018

After further consideration, we don't need to wait on adding this to all cloud drivers. Let's get this in and future events can be added later. Thanks for this @vitaliyf!

@rallytime rallytime merged commit 2bac7b2 into saltstack:develop May 31, 2018
6 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants