Fixes #29799 - Move --clear-pulp-content to hooks/ #540

wbclark · 2020-07-15T17:39:14Z

I implemented this as separate options for Pulp2 and Pulpcore. This has at least two key advantages -- first, the user may wish to clear one but not the other. Second, when Pulp2 is later removed it makes it very easy to remove --clear-pulp-data without touching --clear-pulpcore-data.

hooks/boot/12-clear_pulp_content.rb

hooks/pre/12-clear_pulp_content.rb

wbclark · 2020-07-31T15:24:51Z

@ehelms I introduced hooks/boot/05-data-helpers.rb. Now we have minimal hooks/pre/{10-reset_data.rb,12-clear_pulp_content.rb,13-clear_pulpcore_content.rb} which call the reusable helpers.

hooks/boot/05-data-helpers.rb

ekohl · 2020-07-31T15:42:47Z

hooks/boot/05-data-helpers.rb

+
+  def pg_command_base(config, command, args)
+    port = "-p #{config[:port]}" if config[:port]
+    "PGPASSWORD='#{config[:password]}' #{command} -U #{config[:username]} -h #{config[:host]} #{port} #{args}"


The downside of using it like this is that you can see the password in ps aux. It would be much better to pass an environment variable via native Ruby methods rather than relying on shell.

Checking https://github.com/theforeman/foreman-installer/blob/develop/hooks/boot/01-kafo-hook-extensions.rb#L119 and https://github.com/theforeman/kafo/blob/master/lib/kafo/puppet_command.rb#L49 , I think it makes sense to solve this in a separate PR.

hooks/boot/05-data-helpers.rb

hooks/boot/13-clear_pulpcore_content.rb

hooks/boot/01-kafo-hook-extensions.rb

ehelms

Untested, but the structure looks good

hooks/pre/13-clear_pulpcore_content.rb

ekohl · 2020-08-18T12:31:34Z

hooks/pre/13-clear_pulpcore_content.rb

+end
+
+if app_value(:clear_pulpcore_content) && !app_value(:noop)
+  response = ask('Are you sure you want to continue? This will clear all Pulpcore content including repositories and synced content. [y/n]')


It doesn't remove synced content, only the database. Not sure it matters to end users, but they may be surprised to find the disk usage is still high.

I took inspiration from (read: shamelessly copied) @ianballou 's work on #504 into a new helper in hooks/boot/05-data-helpers.rb and incorporated it in hooks/pre/13-clear_pulpcore_content.rb

ekohl · 2020-09-02T09:10:10Z

hooks/pre/12-clear_pulp_content.rb

+  content_dir = '/var/lib/pulp/content'
+  if File.directory?(content_dir)
+    logger.debug "Removing Pulp content inside \'#{content_dir}\'."
+    FileUtils.rm_rf('/var/lib/pulp/content/*')


https://ruby-doc.org/stdlib-2.4.1/libdoc/fileutils/rdoc/FileUtils.html#method-c-rm_r suggests you need to call glob explicitly.

Suggested change

FileUtils.rm_rf('/var/lib/pulp/content/*')

FileUtils.rm_rf(Dir.glob(File.join(content_dir, '*')))

I'm also wondering about the docs for it that mention a security risk. I guess this is a timing attack where it would start to process it, read a directory and then replace that with a symlink. I wouldn't be too worried about this.

hooks/pre/12-clear_pulp_content.rb

wbclark · 2020-09-02T19:02:06Z

Thanks for the reviews @ekohl . It is updated with proper escaping

hooks/pre/12-clear_pulp_content.rb

wbclark · 2020-09-03T16:04:58Z

@ekohl I fixed the remaining issues with escaping and now use a single method reset_repo_importers for both local and remote mongodb.

I then added a 2nd commit which moves shared helpers to the hook context extension.

hooks/pre/12-clear_pulp_content.rb

wbclark · 2020-09-03T16:19:36Z

@ekohl Updated the first commit to address the above comments, except the one about the --reset-data warning... please see above discussion about it and let me know your thoughts.

ekohl · 2020-09-04T14:16:52Z

hooks/pre/12-clear_pulp_content.rb

+  if !pulp_enabled?
+    logger.warn 'Skipping Pulp content reset because Pulp feature is disabled in scenario answers.'
+  elsif app_value(:reset_data)
+    logger.debug "Skipping \'--clear-pulp-content\' because Pulp data and content will be reset with '\--reset-data\'."


Still has redundant escaping. The warn below as well.

Sorry, this should be in single quotes I think. Once that is fixed, the escaping becomes necessary.

@ekohl There are similar issues in hooks/pre/10-data_reset.rb so I added a 3rd commit that fixes them. But if you prefer I can move that commit to a new branch and PR.

ekohl

Untested, but the code looks good to me.

theforeman-bot added Needs testing Not yet reviewed labels Jul 15, 2020

ekohl reviewed Jul 15, 2020

View reviewed changes

hooks/boot/12-clear_pulp_content.rb Outdated Show resolved Hide resolved

ekohl added Waiting on contributor and removed Not yet reviewed labels Jul 15, 2020

wbclark commented Jul 15, 2020

View reviewed changes

hooks/pre/12-clear_pulp_content.rb Outdated Show resolved Hide resolved

wbclark force-pushed the 29799_clear_pulp_content branch from aea0c40 to 0f92116 Compare July 16, 2020 16:36

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jul 16, 2020

ehelms added the Waiting on contributor label Jul 31, 2020

wbclark force-pushed the 29799_clear_pulp_content branch from 0f92116 to 1e18e3e Compare July 31, 2020 15:20

theforeman-bot removed the Waiting on contributor label Jul 31, 2020

ekohl requested changes Jul 31, 2020

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Jul 31, 2020

wbclark force-pushed the 29799_clear_pulp_content branch from 1e18e3e to a2f7307 Compare July 31, 2020 20:27

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jul 31, 2020

wbclark force-pushed the 29799_clear_pulp_content branch 3 times, most recently from 02a924f to d81a1c2 Compare July 31, 2020 23:40

ehelms reviewed Aug 10, 2020

View reviewed changes

hooks/boot/13-clear_pulpcore_content.rb Outdated Show resolved Hide resolved

ehelms added the Waiting on contributor label Aug 17, 2020

wbclark force-pushed the 29799_clear_pulp_content branch from d81a1c2 to 461838e Compare August 17, 2020 22:45

theforeman-bot removed the Waiting on contributor label Aug 17, 2020

wbclark commented Aug 17, 2020

View reviewed changes

hooks/boot/01-kafo-hook-extensions.rb Show resolved Hide resolved

ehelms approved these changes Aug 18, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Aug 18, 2020

ekohl reviewed Aug 18, 2020

View reviewed changes

ekohl reviewed Sep 2, 2020

View reviewed changes

wbclark force-pushed the 29799_clear_pulp_content branch from 66cfbb7 to 5006d58 Compare September 2, 2020 16:03

ekohl requested changes Sep 2, 2020

View reviewed changes

hooks/pre/12-clear_pulp_content.rb Outdated Show resolved Hide resolved

hooks/pre/12-clear_pulp_content.rb Outdated Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Needs re-review labels Sep 2, 2020

wbclark force-pushed the 29799_clear_pulp_content branch from 5006d58 to 71ac28a Compare September 2, 2020 18:41

theforeman-bot added Needs re-review and removed Waiting on contributor labels Sep 2, 2020

wbclark force-pushed the 29799_clear_pulp_content branch from 71ac28a to e55c33e Compare September 2, 2020 18:42

ehelms approved these changes Sep 3, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Sep 3, 2020

ekohl requested changes Sep 3, 2020

View reviewed changes

hooks/pre/12-clear_pulp_content.rb Outdated Show resolved Hide resolved

hooks/pre/12-clear_pulp_content.rb Outdated Show resolved Hide resolved

hooks/pre/12-clear_pulp_content.rb Outdated Show resolved Hide resolved

theforeman-bot added the Waiting on contributor label Sep 3, 2020

wbclark force-pushed the 29799_clear_pulp_content branch from e55c33e to bbd53fb Compare September 3, 2020 16:02

theforeman-bot added Needs re-review and removed Waiting on contributor labels Sep 3, 2020

ekohl reviewed Sep 3, 2020

View reviewed changes

hooks/pre/12-clear_pulp_content.rb Show resolved Hide resolved

hooks/pre/12-clear_pulp_content.rb Show resolved Hide resolved

hooks/pre/12-clear_pulp_content.rb Outdated Show resolved Hide resolved

hooks/pre/12-clear_pulp_content.rb Outdated Show resolved Hide resolved

wbclark force-pushed the 29799_clear_pulp_content branch from bbd53fb to 234e0ab Compare September 3, 2020 16:18

wbclark force-pushed the 29799_clear_pulp_content branch from 234e0ab to d7e0490 Compare September 4, 2020 14:07

ekohl reviewed Sep 4, 2020

View reviewed changes

wbclark added 3 commits September 4, 2020 10:28

Fixes #29799 - Move --clear-pulp-content to hooks/

51fd29c

Move shared helpers to hook context extension

ee88f2b

Use proper quotes and escaping in data reset

df30ac0

wbclark force-pushed the 29799_clear_pulp_content branch from d7e0490 to df30ac0 Compare September 4, 2020 14:32

ekohl approved these changes Sep 4, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Sep 4, 2020

wbclark merged commit 4548bcc into theforeman:develop Sep 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #29799 - Move --clear-pulp-content to hooks/ #540

Fixes #29799 - Move --clear-pulp-content to hooks/ #540

wbclark commented Jul 15, 2020

wbclark commented Jul 31, 2020 •

edited

Loading

ekohl Jul 31, 2020

wbclark Jul 31, 2020

ehelms left a comment

ekohl Aug 18, 2020

wbclark Aug 18, 2020

ekohl Sep 2, 2020

wbclark commented Sep 2, 2020

wbclark commented Sep 3, 2020

wbclark commented Sep 3, 2020

ekohl Sep 4, 2020

wbclark Sep 4, 2020

wbclark Sep 4, 2020

ekohl left a comment

	FileUtils.rm_rf('/var/lib/pulp/content/*')
	FileUtils.rm_rf(Dir.glob(File.join(content_dir, '*')))

Fixes #29799 - Move --clear-pulp-content to hooks/ #540

Fixes #29799 - Move --clear-pulp-content to hooks/ #540

Conversation

wbclark commented Jul 15, 2020

wbclark commented Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms left a comment

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

wbclark commented Sep 2, 2020

wbclark commented Sep 3, 2020

wbclark commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

wbclark commented Jul 31, 2020 •

edited

Loading