Fixes #50 - Clean all Qpid queues to zero #52

kgaikwad · 2017-04-16T14:36:02Z

No description provided.

iNecas · 2017-04-18T10:51:35Z

definitions/features/qpid_queues.rb

+      queues_cleared << qname
+    end
+  ensure
+    queues_cleared


why the ensure block here?

Oh..my bad.
I was thought of adding rescue..ensure block here as in clear(qname) method I am triggering a command.
I missed to add rescue block.
I will modify the code and update the PR.

The ensure block doesn't probably work as you expect, try:

def foo raise "Error" rescue => e "rescue" ensure "ensure" end

foo returns "rescue" not "ensure" - the ensure block doesn't have effect on return value

Yes, your right.
Removed ensure block and added return value in rescue itself.
For return statement in ensure block, it was giving rubocop warning - 'Do not return from an ensure block'

iNecas · 2017-04-20T10:21:35Z

After real work testing, I would suggest to:

limit the list of queues we delete only on the persisted ones:

 Failed: Exception: Exception from Agent: {u'error_code': 7, u'error_text': 'not-found: Delete failed. No such queue: 3dfb669f-ba51-4093-9d47-19efc686166d:0.0 (/builddir/build/BUILD/qpid-cpp-0.30/src/qpid/broker/Broker.cpp:1485)'}

It seems we should delete only the persisted ones:

qpid-config --ssl-certificate=/etc/pki/katello/qpid_client_striped.crt -b amqps://localhost:5671  list queue
Objects of type 'queue'
  name                                           durable  autoDelete
  ====================================================================
  85cff499-be17-49b5-a8dd-c41ee06a8e9f:0.0       False    True
  907a50d7-5e79-4eba-a507-e0a810bef063:1.0       False    True
  cd3941be-d52e-4980-8243-62b2bf0318e8:1.0       False    True
  celeryev.5c2d60ec-8586-412f-9e86-3d4d92902283  False    True
  katello_event_queue                            True     False
  pulp.task                                      True     False
  sat61-rhel6.example.com:event                  True     False

the queue can't be deleted when the services are using it: we should stop the services first, katello-service stop --except qpidd

Since this is more a migration step, rather than pre-upgrade check, I suggest doing the updates above, but still not merging, until we distinguish between pre_upgrade checks and upgrade steps.

iNecas · 2017-04-20T10:47:14Z

definitions/procedures/qpid_queues_clear.rb

+  def run
+    with_spinner('clear qpid queues') do |spinner|
+      total_queues_cleared = feature(:qpid_queues).clear_all
+      spinner.update "[#{total_queues_cleared.length}] queues cleared.\


the [14] queues cleared would look better without [], so it would be 14 queues cleared

kgaikwad · 2017-05-22T07:44:03Z

@iNecas ,
Done with changes that suggested by you in previous comments.
Added katello-service feature to start-stop the service.

Due to this, on service start it is creating queues again and executing rerun check.
I guess this will get resolved after adding pre-post migration steps for it.

swapab · 2017-06-01T13:40:30Z

definitions/features/qpid_queues.rb

+  def available_qpid_queues
+    output = qpid_config("list queue --show-property=name \
+                          --show-property=autoDelete | awk '$2 ~ /False/{ print $1 }'")
+    output ? output.split(' ') : []


qpid_config(...).to_s.split(' ')

iNecas · 2017-06-07T19:34:54Z

definitions/procedures/qpid_queues_clear.rb

+
+  def run
+    with_spinner('clear qpid queues') do |spinner|
+      feature(:katello_service).make_stop(spinner, '--exclude qpidd')


We should remember the original state of the service, and start only if it was running before at the end. What about having something like this:

def make_stop(spinner, args) do originally_runner_services = running_services execute!("katello-service stop #{args}") yield ensure execute!("katello-service start --only #{originally_running_services}") end

iNecas · 2017-06-07T19:36:50Z

definitions/features/qpid_queues.rb

@@ -0,0 +1,47 @@
+class Features::QpidQueues < ForemanMaintain::Feature


I think we could generalize this to qpid feature, and collect mutliple functionalities around this feature in the future

iNecas · 2017-06-07T19:38:10Z

lib/foreman_maintain/concerns/system_helpers.rb

@@ -92,6 +92,10 @@ def execute(command, options = {})
      def shellescape(string)
        Shellwords.escape(string)
      end
+
+      def qpid_tools_installed?


Any reason not to put it into the qpid feature? The SystemHelpers should contain only the more generic commands, usable across the definitions.

No specific reason.
I have given QpidQueues as a name of feature that's why I thought this might be needed for other features specific to qpid.
As per your above comment, I will change feature name as qpid and will move this method to feature itself.

iNecas · 2017-06-07T19:40:00Z

Yes, after https://github.com/iNecas/foreman_maintain/pull/67, this should go into migrations steps, as we rely on the installer to fill in the deleted queues, and therefore it should happen in the same phase.

iNecas · 2017-06-23T12:51:31Z

definitions/features/katello_service.rb

+  def make_stop(spinner, args = '')
+    originally_running_services = running_services
+    spinner.update 'Stopping katello running services..'
+    execute!("katello-service stop #{args}")


the args could be called services, so that we can work with it a bit better. After doing so, we can compare the running_services with services and:

stop only running_serivces & services

skip this completely, if the services to be stopped are actually running.

@iNecas ,
Could you please elaborate 2nd point as I am bit confuse here.
As you mentioned firstly I need to rename args as services then

Instead of passing exclude list of services, I have to pass services which needs to be stop.

After that I need to compare running_services with services,
stop only services which are common in both list.

Please correct me.

Oh, I'm missed there is the except word used. In that case, what about having options with possible keys :only and :exclude, but since we would have the list of currently running services, we would use only --only option for the actual katello-service and only in case we don't have an empty list.

@iNecas ,
Yes, it will be better. As we have running_services list value, we can include :only and :exclude option keys.
I will modify and update the PR.

kgaikwad · 2017-06-27T11:54:08Z

@iNecas ,
Done with changes. Ready for review.
One small query:
On service restart few persistent queues will get created. It results into check rerun.
and at the end of check run showing message - {queue_count} queues cleared. These queues will be recreated using installer
If queues are creating on service restart, should I need to change success message? and how we can avoid check rerun.

kgaikwad · 2017-06-27T12:34:11Z

Modified success-info message to - {queue_count} queues cleared. These queues are recreated by restarting katello-services.

iNecas · 2017-08-14T19:24:16Z

I've raised some questions on mailing list if we want to make this step part of the upgrade scenario, as it can affect negatively the clients connected to the system. Perhaps we will need to do some additional queue restore, as described in https://access.redhat.com/solutions/3148641in "ONLY if you end up with broken content under /var/lib/qpidd and dont have a working backup" scenario.

iNecas · 2017-08-14T19:25:37Z

definitions/features/katello_service.rb

+  def make_stop(spinner, options = {})
+    services = find_services_for_only_filter(running_services, options)
+    if services.empty?
+      spinner.update 'No any running katello service..'


nitpick: no katello service running

iNecas · 2017-08-14T19:25:56Z

definitions/features/katello_service.rb

+  def make_start(spinner, options = {})
+    services = find_services_for_only_filter(stopped_services, options)
+    if services.empty?
+      spinner.update 'No any katello service to start.'


iNecas · 2017-08-14T19:27:25Z

@kgaikwad until we get the answers, could you extract the katello_service into separate PR, as there are other use-cases that might use it before we merge this one. Such as #68

kgaikwad · 2017-08-15T09:17:32Z

@iNecas ,
Yes, sure.
Created new PR #83 for Katello-Service feature by creating new issue in Redmine.

Please let me know if any additional changes required in it.
I will update this Qpid PR later once katello-service feature PR get merged.

iNecas · 2017-08-21T06:49:54Z

#83 got merged. Please rebased. Basd on scrum call discussion, it would be good looking into queue re-creation part of the https://access.redhat.com/solutions/3148641

iNecas · 2017-08-21T06:57:09Z

On top of https://access.redhat.com/solutions/3148641, we could perhaps just dump the list of queues before the cleanup and re-create the queues based on the list after deleting the queues. Thoughts @pmoravec ?

pmoravec · 2017-08-21T12:33:30Z

Yes it makes sense - though IMHO only for upgrades where qpid-cpp-server* package was bounced to 0.34 (and when the directory structure has changed). Since doing so during every upgrade would redundantly slow down the upgrade - creating one such queue for one Content Host takes some time (up to 1s) and having thousands of Content Hosts..

2 possible ways how to identify the queues (just the pulp.agent.* are required to identify):

chose from candlepin consumers or katello_systems just the systems with katello-agent package installed - to skip generating the queue for goferd-less systems.
identify what queues were there before (and after) the upgrade):

ls /var/lib/qpidd/.qpidd/qls/jrnl /var/lib/qpidd/.qpidd/qls/jrnl2 /var/lib/qpidd/qls/jrnl /var/lib/qpidd/qls/jrnl2 2> /dev/null | sort -u | grep pulp.agent

jrnl for old dir before upgrade, jrnl2 after upgrade (if e.g. a system registered after yum update but before satellite-installer)
the other pair of dirs is due to RHEL6/RHEL7 different paths (if applicable to upstream)

kgaikwad · 2020-04-17T15:16:57Z

@upadhyeammit,
As you are already working on backup & restore stuff, request to review these changes.
If these changes are not required then we can close this PR.

upadhyeammit · 2020-09-21T12:15:34Z

Closing this PR as the changes are not required now

iNecas reviewed Apr 18, 2017

View reviewed changes

kgaikwad force-pushed the 50_clear_qpid_queues branch 2 times, most recently from 6496532 to 6b01b9a Compare April 19, 2017 16:25

iNecas reviewed Apr 20, 2017

View reviewed changes

kgaikwad force-pushed the 50_clear_qpid_queues branch 2 times, most recently from 71db180 to 9731a97 Compare May 22, 2017 07:38

swapab suggested changes Jun 1, 2017

View reviewed changes

iNecas reviewed Jun 7, 2017

View reviewed changes

kgaikwad force-pushed the 50_clear_qpid_queues branch from 9731a97 to a64f022 Compare June 15, 2017 10:24

iNecas reviewed Jun 23, 2017

View reviewed changes

iNecas mentioned this pull request Jun 23, 2017

Fixes #41 - Add capsule clear #68

Closed

kgaikwad force-pushed the 50_clear_qpid_queues branch from a64f022 to 8dfcf1c Compare June 27, 2017 11:34

kgaikwad force-pushed the 50_clear_qpid_queues branch 2 times, most recently from 3827558 to ae6d1bf Compare June 27, 2017 12:33

swapab approved these changes Jul 13, 2017

View reviewed changes

iNecas reviewed Aug 14, 2017

View reviewed changes

kgaikwad added Needs re-review On Hold labels Aug 15, 2017

kgaikwad mentioned this pull request Aug 16, 2017

Fixes #20604 - katello-service feature to make services start/stop #83

Merged

kgaikwad added Waiting on contributor and removed Needs re-review On Hold labels Sep 5, 2017

Fixes theforeman#50 - Clean all Qpid queues to zero

9e268de

kgaikwad added the On Hold label Sep 5, 2017

kgaikwad force-pushed the 50_clear_qpid_queues branch from ae6d1bf to 9e268de Compare September 5, 2017 12:05

upadhyeammit closed this Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #50 - Clean all Qpid queues to zero #52

Fixes #50 - Clean all Qpid queues to zero #52

kgaikwad commented Apr 16, 2017

iNecas Apr 18, 2017

kgaikwad Apr 18, 2017

iNecas Apr 19, 2017

kgaikwad Apr 19, 2017

iNecas commented Apr 20, 2017

iNecas Apr 20, 2017

kgaikwad commented May 22, 2017

swapab Jun 1, 2017

iNecas Jun 7, 2017 •

edited

iNecas Jun 7, 2017

iNecas Jun 7, 2017

kgaikwad Jun 8, 2017

iNecas commented Jun 7, 2017

iNecas Jun 23, 2017

kgaikwad Jun 26, 2017

iNecas Jun 26, 2017

kgaikwad Jun 26, 2017

kgaikwad commented Jun 27, 2017

kgaikwad commented Jun 27, 2017

iNecas commented Aug 14, 2017

iNecas Aug 14, 2017

iNecas Aug 14, 2017

iNecas commented Aug 14, 2017

kgaikwad commented Aug 15, 2017

iNecas commented Aug 21, 2017

iNecas commented Aug 21, 2017

pmoravec commented Aug 21, 2017

kgaikwad commented Apr 17, 2020

upadhyeammit commented Sep 21, 2020

		@@ -0,0 +1,47 @@
		class Features::QpidQueues < ForemanMaintain::Feature

Fixes #50 - Clean all Qpid queues to zero #52

Fixes #50 - Clean all Qpid queues to zero #52

Conversation

kgaikwad commented Apr 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNecas commented Apr 20, 2017

Choose a reason for hiding this comment

kgaikwad commented May 22, 2017

Choose a reason for hiding this comment

iNecas Jun 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNecas commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaikwad commented Jun 27, 2017

kgaikwad commented Jun 27, 2017

iNecas commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNecas commented Aug 14, 2017

kgaikwad commented Aug 15, 2017

iNecas commented Aug 21, 2017

iNecas commented Aug 21, 2017

pmoravec commented Aug 21, 2017

kgaikwad commented Apr 17, 2020

upadhyeammit commented Sep 21, 2020

iNecas Jun 7, 2017 •

edited