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

Asynchronously run software updates discovery when applying settings #2533

Merged
merged 4 commits into from
Apr 22, 2024

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Apr 18, 2024

Description

This PR makes sure the software updates discovery for all the hosts in the system is triggered asynchronously on save/change settings.
Also, in order to avoid unwanted side effects the Discovery state gets cleared right before triggering async discovery, so that the state gest reconstituted with new settings.

Keeps the save/change settings operation quite immediate not impacting performance when discovery needs to be executed for many hosts.

How was this tested?

Automated tests.

@nelsonkopliku nelsonkopliku self-assigned this Apr 18, 2024
@nelsonkopliku nelsonkopliku added enhancement New feature or request elixir Pull requests that update Elixir code labels Apr 18, 2024
@nelsonkopliku nelsonkopliku force-pushed the async-software-updates-when-saving-settings branch from b5095d7 to ca4992d Compare April 19, 2024 08:52
@nelsonkopliku nelsonkopliku marked this pull request as ready for review April 19, 2024 08:54
@nelsonkopliku nelsonkopliku marked this pull request as draft April 19, 2024 16:27
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.

Nice!
Looks good

test/support/task_case.ex Outdated Show resolved Hide resolved
pids
|> List.delete(pid)
|> wait_for_pids()
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we want to add some after to receive so it doesn't hang forever if the process doesn't end,
Maybe some optional timeout value

Copy link
Member Author

Choose a reason for hiding this comment

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

Here we go e3b075b.

@nelsonkopliku
Copy link
Member Author

thanks @arbulu89! I am still testing a few things out 'cause I noticed a weird behavior, like the thing seems to still be blocking in a start_child and so I was gonna try with just async.

Besides this I was also thinking that maybe we can make async also the discovery happening in SoftwareUpdatesDiscoveryEventHandler.
Since there is only 1 instance of that listener running, every discovery being triggered by a SoftwareUpdatesDiscoveryRequested event would wait for the previous to finish.

What do you think?

@arbulu89
Copy link
Contributor

Besides this I was also thinking that maybe we can make async also the discovery happening in SoftwareUpdatesDiscoveryEventHandler.

I don't find it really needed there. In that scenario, we are not in a rush to make things happen, and besides, once the authentication and requests are separated, it will only "block" the 1st call, and from that point on, all the queries would be done in parallel.
I wouldn't add more async tasks there, as we don't really need that feature (and more, if it is solved in suma code)

@nelsonkopliku
Copy link
Member Author

nelsonkopliku commented Apr 22, 2024

I am not sure the following is completely correct 🤔

from that point on, all the queries would be done in parallel.

What happens in Discovery.discover_host_software_updates (a discovery for a specific host) would still be awaited on for the listener to continue handling the following SoftwareUpdatesDiscoveryRequested event, also when we have only the auth blocked in a genserver linearization.

But maybe I am missing something.

Anyway, I won't change what happens in the listener for the time being.

@nelsonkopliku nelsonkopliku force-pushed the async-software-updates-when-saving-settings branch from ca4992d to e3b075b Compare April 22, 2024 07:43
@nelsonkopliku nelsonkopliku marked this pull request as ready for review April 22, 2024 07:43
@nelsonkopliku nelsonkopliku merged commit 660e23b into main Apr 22, 2024
26 checks passed
@nelsonkopliku nelsonkopliku deleted the async-software-updates-when-saving-settings branch April 22, 2024 07:59
@arbulu89
Copy link
Contributor

@nelsonkopliku Let's start from the beginning.
That event handler by now is just sequential, so it will handle all the events one by one.
What I mean with the "parallelization" is that if you change the event handler to work concurrently, if 2 events arrive at the same time, if the authentication is not done, one of the events will start the authentication, and the other will wait there (as the genserver makes this happen).
Once the authentication is done, the first event will just do the queries for the system_id, etc, and the 2nd as well, because the queries are not part of the gen server anymore, so there is not blocking.

If the authentication fails in both, well, both will fail at the same time

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.

2 participants