Fix workers not restarting when auth/endpoints are changed. #22269

agrare · 2022-12-06T16:22:37Z

When changes are made to the endpoints or authentications workers which run with an open connection like event catchers and streaming refresh workers have to be restarted.

This was done via after_update_authentication called from update_authentication. The issue is that this method is no longer called when providers are updated via the API with DDF parameters.

We also have some before_save :stop_event_monitor_queue_on_change, :stop_refresh_worker_queue_on_change but these aren't invoked because auth/endpoint changes aren't part of the ext_management_system record.

Ref ManageIQ/manageiq-providers-openshift#235 (comment)

TODO

Specs for edit_with_params
Handle child managers' workers

Depends on:

Cross-Repo: ManageIQ/manageiq-cross_repo-tests#720

agrare · 2022-12-06T17:49:28Z

app/models/ext_management_system.rb

+      endpoints_changed       = false
+      authentications_changed = false


NOTE I'm not sure how necessary it is to differentiate between endpoints and authentications changing. Currently as far as I can tell we only use this information to restart workers and we need to do that when either endpoints or authentications change.

agrare · 2022-12-06T18:56:04Z

app/models/ext_management_system.rb

      _log.info("EMS: [#{name}], Hostname or IP address has changed, stopping Event Monitor.  It will be restarted by the WorkerMonitor.")
      stop_event_monitor_queue
    end
  end

  def stop_event_monitor_queue_on_credential_change
-    if event_monitor_class && !self.new_record? && self.credentials_changed?


NOTE credentials_changed? used an instance variable set early in #update_authentications but it seemed easier to just check default_authentication.changed?

I didn't want to rewrite the credentials_changed? method to use this in case something else depended on that exact behavior.

Also that method only checked auth_user_pwd which wouldn't work for k8s/gce/etc... which use an auth_key.

I would like to be able to specify e.g. authentication_for_events rather than default_authentication in case a provider doesn't use the default_auth for events.

Is that something you want to do as a followup or for this PR?

I should be able to get that working in this PR

I've added authtype_for_event/refresh and endpoint_role_for_event/refresh so that the callbacks can check the changes for endpoints they care about

agrare · 2022-12-06T19:12:19Z

Example changing the credentials for an Openshift provider

INFO -- evm: MIQ(ManageIQ::Providers::Openshift::ContainerManager#stop_refresh_worker_queue_on_credential_change) EMS: [CRC], Credentials have changed, stopping Refresh Worker.  It will be restarted by the WorkerMonitor.
INFO -- evm: MIQ(MiqQueue.put) Message id: [7862], Zone: [default], Role: [], Server: [000be0ae-9877-4482-9226-a3b1a07bcd67], MiqTask id: [], Handler id: [], Ident: [miq_server], Target id: [], Instance id: [1], Task id: [], Command: [MiqServer.stop_worker], Timeout: [600], Priority: [100], State: [ready], Deliver On:        [], Data: [], Args: [10, nil]
INFO -- evm: MIQ(MiqQueue#deliver) Message id: [7862], Delivering...
INFO -- evm: MIQ(MiqServer::WorkerManagement::Process#stop_worker) Stopping Worker [ManageIQ::Providers::Openshift::ContainerManager::RefreshWorker] with ID: [10], PID: [107883], GUID: [d4068664-8dc3-4177-8d77-67c807b91ad4], status [started]...

agrare · 2022-12-06T19:25:02Z

@miq-bot cross-repo-test ManageIQ/manageiq-providers-kubernetes#479 ManageIQ/manageiq-providers-vmware#841

From Pull Request: ManageIQ/manageiq#22269

spec/models/ext_management_system_spec.rb

app/models/ext_management_system.rb

From Pull Request: ManageIQ/manageiq#22269

Fryguy · 2022-12-07T20:25:58Z

Note that there's a WIP commit message in here, that should be cleaned up when this is ready.

miq-bot · 2023-03-13T00:00:22Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

Fryguy · 2023-03-20T19:26:24Z

@agrare Is this ready to go? Maybe a cross-repo test?

When changes are made to the endpoints or authentications workers which run with an open connection like event catchers and streaming refresh workers have to be restarted. This was done via `after_update_authentication` called from `update_authentication`. The issue is that this method is no longer called when providers are updated via the API with DDF parameters.

miq-bot · 2023-06-19T13:51:22Z

Checked commits agrare/manageiq@2c4ff02~...6f5a865 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
4 files checked, 0 offenses detected
Everything looks fine. 🏆

kbrock · 2023-06-20T00:37:19Z

ok, ManageIQ/manageiq-cross_repo-tests#720 is green.

Does this mean we are ready to go?

miq-bot · 2023-09-25T00:00:15Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

miq-bot · 2024-01-01T00:00:19Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

miq-bot · 2024-03-07T16:05:29Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2024-06-10T00:00:12Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

miq-bot · 2024-09-16T00:00:17Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

agrare added bug wip providers labels Dec 6, 2022

agrare requested review from Fryguy and kbrock as code owners December 6, 2022 16:22

agrare force-pushed the endpoint_auth_changed_callbacks_edit_with_params branch from 1ee330a to 0733f89 Compare December 6, 2022 17:48

agrare commented Dec 6, 2022

View reviewed changes

agrare force-pushed the endpoint_auth_changed_callbacks_edit_with_params branch from 0733f89 to eb74c6c Compare December 6, 2022 18:40

agrare commented Dec 6, 2022

View reviewed changes

agrare changed the title ~~[WIP] Fix workers not restarting when auth/endpoints are changed.~~ Fix workers not restarting when auth/endpoints are changed. Dec 6, 2022

agrare force-pushed the endpoint_auth_changed_callbacks_edit_with_params branch from 0015fd8 to d42f66f Compare December 6, 2022 19:15

miq-bot removed the wip label Dec 6, 2022

This was referenced Dec 6, 2022

Use the new after_update_endpoints callback for RefreshWorker ManageIQ/manageiq-providers-vmware#841

Open

Add after_update_endpoints to restart RefreshWorker ManageIQ/manageiq-providers-kubernetes#479

Open

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Dec 6, 2022

Running tests for agrare

8fda624

From Pull Request: ManageIQ/manageiq#22269

miq-bot mentioned this pull request Dec 6, 2022

[BOT] Cross repo test for ManageIQ/manageiq#22269 ManageIQ/manageiq-cross_repo-tests#720

Open

Fryguy reviewed Dec 6, 2022

View reviewed changes

spec/models/ext_management_system_spec.rb Outdated Show resolved Hide resolved

Fryguy reviewed Dec 6, 2022

View reviewed changes

spec/models/ext_management_system_spec.rb Show resolved Hide resolved

Fryguy reviewed Dec 6, 2022

View reviewed changes

app/models/ext_management_system.rb Outdated Show resolved Hide resolved

agrare force-pushed the endpoint_auth_changed_callbacks_edit_with_params branch 3 times, most recently from 1ed4d61 to 0394dd3 Compare December 7, 2022 17:00

This was referenced Dec 7, 2022

Update stop_event_monitor_queue_on_change to take changes ManageIQ/manageiq-providers-openstack#832

Open

Update stop_event_monitor_queue_on_change to take changes ManageIQ/manageiq-providers-nuage#283

Open

agrare added a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Dec 7, 2022

Running tests for agrare

4487204

From Pull Request: ManageIQ/manageiq#22269

Fryguy self-assigned this Dec 7, 2022

agrare force-pushed the endpoint_auth_changed_callbacks_edit_with_params branch from 0394dd3 to 1215980 Compare December 9, 2022 15:18

miq-bot added the dependencies label Dec 9, 2022

miq-bot added the stale label Mar 13, 2023

agrare force-pushed the endpoint_auth_changed_callbacks_edit_with_params branch from b9d480e to 2ebfd61 Compare March 14, 2023 15:46

kbrock removed the stale label Mar 15, 2023

agrare added 6 commits June 19, 2023 09:49

Move callbacks before save for #changed? and #changes?

8ad2e53

Remove last caller of credentials_changed?

e250c81

Only call after_update_* when something changed

6a43cb9

Pass endpoint/auth changes to callbacks

2016808

Fix Performance/MethodObjectAsBlock warning

6f5a865

agrare force-pushed the endpoint_auth_changed_callbacks_edit_with_params branch from 2ebfd61 to 6f5a865 Compare June 19, 2023 13:50

miq-bot added the stale label Sep 25, 2023

agrare removed the stale label Sep 25, 2023

miq-bot added the stale label Jan 1, 2024

miq-bot added the unmergeable label Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix workers not restarting when auth/endpoints are changed. #22269

Fix workers not restarting when auth/endpoints are changed. #22269

agrare commented Dec 6, 2022 •

edited

Loading

agrare Dec 6, 2022

agrare Dec 6, 2022

Fryguy Dec 6, 2022

agrare Dec 6, 2022

agrare Dec 7, 2022

agrare commented Dec 6, 2022 •

edited

Loading

agrare commented Dec 6, 2022

Fryguy commented Dec 7, 2022

miq-bot commented Mar 13, 2023

Fryguy commented Mar 20, 2023

miq-bot commented Jun 19, 2023

kbrock commented Jun 20, 2023

miq-bot commented Sep 25, 2023

miq-bot commented Jan 1, 2024

miq-bot commented Mar 7, 2024

miq-bot commented Jun 10, 2024

miq-bot commented Sep 16, 2024

Fix workers not restarting when auth/endpoints are changed. #22269

Are you sure you want to change the base?

Fix workers not restarting when auth/endpoints are changed. #22269

Conversation

agrare commented Dec 6, 2022 • edited Loading

agrare Dec 6, 2022

Choose a reason for hiding this comment

agrare Dec 6, 2022

Choose a reason for hiding this comment

Fryguy Dec 6, 2022

Choose a reason for hiding this comment

agrare Dec 6, 2022

Choose a reason for hiding this comment

agrare Dec 7, 2022

Choose a reason for hiding this comment

agrare commented Dec 6, 2022 • edited Loading

agrare commented Dec 6, 2022

Fryguy commented Dec 7, 2022

miq-bot commented Mar 13, 2023

Fryguy commented Mar 20, 2023

miq-bot commented Jun 19, 2023

kbrock commented Jun 20, 2023

miq-bot commented Sep 25, 2023

miq-bot commented Jan 1, 2024

miq-bot commented Mar 7, 2024

miq-bot commented Jun 10, 2024

miq-bot commented Sep 16, 2024

agrare commented Dec 6, 2022 •

edited

Loading

agrare commented Dec 6, 2022 •

edited

Loading