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

Clear software updates discovered health on host deregistration #2436

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Mar 19, 2024

Description

This PR makes sure a SoftwareUpdatesDiscoveryCleared event is emitted when deregistering a host.

I also considered the alternative to hook in the deregistration process manager by dispatching a ClearSoftwareUpdatesDiscovery command when appropriate, but that would have meant possibily emitting also a HostHealthChanged event which is irrelevant for the usecase.

Gotta double check if rollup needs also any intervention.

How was this tested?

Automated tests.

@arbulu89 you might have good points here, that's why I am asking your review as well.

@nelsonkopliku nelsonkopliku marked this pull request as ready for review March 19, 2024 16:14
@nelsonkopliku nelsonkopliku self-assigned this Mar 19, 2024
@nelsonkopliku nelsonkopliku added enhancement New feature or request elixir Pull requests that update Elixir code labels Mar 19, 2024
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

I'm out of context.
But, why do you need this?
I mean, if the host is deregistered, do we care if the setting are cleared or not?
I guess that when it is restored back again, events related to sofware events will come again

@nelsonkopliku
Copy link
Member Author

nelsonkopliku commented Mar 20, 2024

I'm out of context. But, why do you need this? I mean, if the host is deregistered, do we care if the setting are cleared or not? I guess that when it is restored back again, events related to sofware events will come again

the point is that when it comes back, we want to start form a clean sheet, with regards to possibly previously discovered software updates, and let the next discovery tick update with fresh data.

So the options are: we either emit SoftwareUpdatesDiscoveryCleared on deregistration so that the relevant piece of state gets reset or we reset the piece of state when HostRestored is emitted, without emitting SoftwareUpdatesDiscoveryCleared.

I prefer the semantic of emitting the clear up event and let its related apply change the relevant state instead of scattering change state with the HostRestored event as well.

@nelsonkopliku
Copy link
Member Author

btw @arbulu89 we can't give for granted that after a host deregistration/restoration SUMA integration will still be active, meaning that fresh discoveries might not come in later, so if we don't clean up it might happen we have stale state that affects the overall host state.

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

@nelsonkopliku It looks good to me.

@nelsonkopliku nelsonkopliku merged commit f23ce55 into main Mar 26, 2024
32 checks passed
@nelsonkopliku nelsonkopliku deleted the clear-suma-discovery-results-on-deregistration branch March 26, 2024 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elixir Pull requests that update Elixir code enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

3 participants