Fixes #33620 - Command for upgrading foreman-maintain to major version #546

upadhyeammit · 2021-10-05T19:50:45Z

No description provided.

theforeman-bot · 2021-10-05T19:50:47Z

Issues: #33620

upadhyeammit · 2021-10-07T13:54:09Z

This covers below use cases:

Does not limit users to upgrade foreman-maintain package from just next major version. This is considering administrator knows what he is doing and in future if skipped version upgrade is possible?
Does not allow user to upgrade package from current version of Satellite as that should be handled by already available upgrade mechanism followed by the upgrade.
Disables the new maintenance repository on successful upgrade and re-enables repositories which were configured on the system.
Failure in package upgrade rescued, it does not rescue the failure of enabling the repository using yum-config-manager. The rescue steps disables the new maintenance repository and re-enables repos which were configured on system.
Takes cares of situation when trying to enable different major version repos can dump enabled_repos.yml file with multiple maintenance repositories. Allows only one maintenance repository on system as per the version.
Introduces a procedure to dump the enabled repo ids on disk.
Does not allow to enable N-1 version maintenance repository.
While doing the upgrade only specific maintenance repository is enabled and no other repositories are enabled.

upadhyeammit · 2021-10-07T17:51:33Z

@evgeni @ekohl this is not urgent, whenever time permits request to review.

definitions/procedures/repositories/backup_enabled_repos.rb

ekohl · 2021-10-12T12:44:36Z

definitions/scenarios/self_upgrade.rb

+        @stored_enabled_repos_ids = []
+        backup_dir = File.expand_path(ForemanMaintain.config.backup_dir)
+        File.open(File.join(backup_dir, 'enabled_repos.yml'), 'r') do |repos_file|
+          @stored_enabled_repos_ids = YAML.load(repos_file.read)
+        end


This file doesn't have to exist. BackupEnabledRepos only writes it if it's non-empty. I think this would fail then. It also won't be in older backups. You probably need to deal with that to prevent exceptions. You could also use File.read instead of File.open + .read, but that's a small nit.

Suggested change

@stored_enabled_repos_ids = []

backup_dir = File.expand_path(ForemanMaintain.config.backup_dir)

File.open(File.join(backup_dir, 'enabled_repos.yml'), 'r') do |repos_file|

@stored_enabled_repos_ids = YAML.load(repos_file.read)

end

path = File.expand_path('enabled_repos.yml', ForemanMaintain.config.backup_dir)

@stored_enabled_repos_ids = File.file?(path) ? YAML.load(File.read(path)) : []

Nice catch, what I am expecting,

If system is not having any repositories enabled then not having the yml dump is fine. However this also let me thought the situation where yml file has old repositories which are not relevant to current state of the system and hence I feel I should clean up this file after successfully executing enable_repos(repos_ids_to_reenable).

definitions/scenarios/self_upgrade.rb

lib/foreman_maintain/cli/self_upgrade_command.rb

ehelms · 2021-10-20T01:06:47Z

Is there a mechanism by which, for testing purposes, the repository handling could be skipped or configured to test this without first having to have the new maintenance repository in existence?

upadhyeammit · 2021-10-21T12:34:43Z

Is there a mechanism by which, for testing purposes, the repository handling could be skipped or configured to test this without first having to have the new maintenance repository in existence?

I am pushing a new change which has additional flag to skip enabling repository

ehelms · 2021-10-21T15:50:41Z

Thinking on testing, and ahead towards Foreman use case, if the repository could be made configurable either via a foreman-maintain configuration file or an ENV variable then the code becomes more usable and able to test by pointing it at whatever repository you want to get updates. Generally, trying to split the mechanism from the configuration (details) will allow flexibility.

upadhyeammit · 2021-10-21T15:53:15Z

@ekohl @ehelms wish to know your thoughts on below use cases,

The BackupEnabledRepos only does the backup when there are some repositories enabled. This exposes to the situation where we are having enabled_repos.yml file which has the stale repo details and we end up enabling those.
I added the rescue scenario as I was under impression that we serialize and de serialize every scenario kind but that's not the case, I can see its implemented for upgrade scenario only. This means the self upgrade wont resume if we terminate the process in between.
So without rescue scenario as we always want to re-enable repositories we have on system I can skip the storing part, however if user terminates the process before reenabling the repositories then we wont be able to restore the state of the repositories. This brings me to the first point of actually saving repos in yml and not writing anything to enabled_repos.yml if repolist is empty.

Technically I can just go and enable whatever the required repositories for the X.Y version of Satellite but then that wont work for disconnected setups and there can still be some repositories user has configured which we want to reenable.

ekohl · 2021-10-21T15:52:47Z

definitions/procedures/repositories/backup_enabled_repos.rb

+      backup_dir = File.expand_path(ForemanMaintain.config.backup_dir)
+      unless enabled_repos_ids.empty?


You can only expand the path if there are any repos:

Suggested change

backup_dir = File.expand_path(ForemanMaintain.config.backup_dir)

unless enabled_repos_ids.empty?

unless enabled_repos_ids.empty?

backup_dir = File.expand_path(ForemanMaintain.config.backup_dir)

ekohl · 2021-10-21T15:54:59Z

definitions/scenarios/self_upgrade.rb

+  class SelfUpgradeRescue < SelfUpgradeBase
+    metadata do
+      label :rescue_self_upgrade
+      description 'Disables all version specific maintenance repo and,'\


Suggested change

description 'Disables all version specific maintenance repo and,'\

description 'Disables all version specific maintenance repos and,'\

ekohl · 2021-10-21T15:56:09Z

lib/foreman_maintain/cli/self_upgrade_command.rb

+      option ['--target-version'], 'TARGET_VERSION', 'Major version of the Satellite or Capsule'\
+      														 ', e.g 7.0', :required => true


I'm not sure if this is allowed coding style wise, but it avoids breaking up the string which I think makes it easier to read.

Suggested change

option ['--target-version'], 'TARGET_VERSION', 'Major version of the Satellite or Capsule'\

', e.g 7.0', :required => true

option ['--target-version'], 'TARGET_VERSION',

'Major version of the Satellite or Capsule, e.g 7.0', :required => true

definitions/scenarios/self_upgrade.rb

upadhyeammit · 2021-10-26T11:32:38Z

Thinking on testing, and ahead towards Foreman use case, if the repository could be made configurable either via a foreman-maintain configuration file or an ENV variable then the code becomes more usable and able to test by pointing it at whatever repository you want to get updates. Generally, trying to split the mechanism from the configuration (details) will allow flexibility.

Now you can export the maintenance_repo env var and it will try to enable that repo to update package. I did not disable this repo considering it can be foreman repo itself.
Removed the option which I had introduced to skip repo enablement.

ekohl

I'm unsure about the inline comment. Overall I think this looks good from reading it, though I didn't test it.

ekohl · 2021-10-26T11:35:55Z

definitions/scenarios/self_upgrade.rb

+    end
+
+    def maintenance_repo_version
+      return '6' if current_version == '6.10'


Just wondering: would it technically be 6 for 6.*? Do you want compatibility with that or is it irrelevant? So:

Suggested change

return '6' if current_version == '6.10'

return '6' if current_version.start_with?('6.')

I thought to keep it 6.10 as new package wont be available for older versions.

Will we already have a separate repo for 6.10 then? Or is this procedure in some other way guarded to be unavailable on 6.9 and older?

evgeni · 2021-10-26T12:30:08Z

definitions/features/system_repos.rb

@@ -29,6 +33,10 @@ def disable_repos(repo_ids)
    execute!("yum-config-manager --disable #{repo_ids.join(',')}")
  end

+  def enable_repos(repo_ids)
+    execute!("yum-config-manager --enable #{repo_ids.join(',')}")


at least on EL8 this is not present by default.

given this is also used in disable_repos which is old code, I guess we should add this to "EL8" todos, as it's not strictly related to this PR.

True, this won't be a factor until upgrades to 7.1 or upstream makes use of this. I'm fine with leaving as is and tackling later if we have a EL8 tracker somewhere to add this to?

There is https://projects.theforeman.org/issues/27511 and Amit already added https://projects.theforeman.org/issues/33788 to it :)

ekohl · 2021-10-26T15:24:20Z

3\. This brings me to the first point of actually saving repos in yml and not writing anything to `enabled_repos.yml` if repolist is empty.

I do think this is a good idea. This allows you to distinguish offline setups from setups. That said, you should still deal with the file missing in a way that it doesn't hard crash foreman_maintain.

ehelms

I see a few comments to address or resolve, so I will leave those to also be tackled. I think this looks good at the moment and is good to merge until we have all the repositories in place to test.

definitions/scenarios/self_upgrade.rb

ehelms · 2021-11-18T13:07:01Z

@upadhyeammit looks good! I think this can be squashed and merged.

upadhyeammit · 2021-11-18T14:28:55Z

@ehelms there were few things I have handled in latest commit. The confine block is not working for the scenario out of the box, need to check what is missing for that. Accordingly as of now this scenario is limited using if condition in compose. And I will raise another pr to change it for PuppetDisable command as well.

definitions/scenarios/self_upgrade.rb

… instead of match?

upadhyeammit · 2021-11-23T12:59:23Z

@ehelms changes have been pushed, if there are no more comments then I will squash and merge.

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Oct 5, 2021

upadhyeammit force-pushed the 33620 branch from 4aa5ea6 to 1987b76 Compare October 5, 2021 19:55

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Oct 5, 2021

upadhyeammit force-pushed the 33620 branch from 1987b76 to beda682 Compare October 6, 2021 13:38

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Oct 6, 2021

upadhyeammit force-pushed the 33620 branch from beda682 to ba1a90f Compare October 7, 2021 14:13

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Oct 7, 2021

ekohl reviewed Oct 12, 2021

View reviewed changes

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Oct 21, 2021

ekohl reviewed Oct 21, 2021

View reviewed changes

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Oct 26, 2021

ekohl approved these changes Oct 26, 2021

View reviewed changes

theforeman-bot removed the Not yet reviewed label Oct 26, 2021

evgeni reviewed Oct 26, 2021

View reviewed changes

ehelms approved these changes Oct 28, 2021

View reviewed changes

ehelms reviewed Nov 10, 2021

View reviewed changes

definitions/scenarios/self_upgrade.rb Outdated Show resolved Hide resolved

upadhyeammit added 4 commits November 18, 2021 17:47

Fixes #33620 - Command for upgrading foreman-maintain to major version

25bf0ed

Refs #33620 - flag to skip repo enablement and refactoring

456f61f

Refs #33620 - use maintenance_repo env var if available

131fb5e

Refs #33620 - update repo names

0c055a0

upadhyeammit force-pushed the 33620 branch from 627920b to 0c055a0 Compare November 18, 2021 12:21

theforeman-bot added Needs re-review Waiting on contributor and removed Waiting on contributor labels Nov 18, 2021

ehelms mentioned this pull request Nov 18, 2021

Fixes #33245 - use correct repo names on EL8 #561

Merged

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Nov 18, 2021

ehelms reviewed Nov 18, 2021

View reviewed changes

definitions/scenarios/self_upgrade.rb Show resolved Hide resolved

Refs #33620 - handle nil for env, limit run on foreman 2.5, use match…

d6d76c4

… instead of match?

upadhyeammit force-pushed the 33620 branch from ba87c62 to d6d76c4 Compare November 23, 2021 10:02

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Nov 23, 2021

ehelms approved these changes Nov 23, 2021

View reviewed changes

theforeman-bot removed the Needs re-review label Nov 23, 2021

upadhyeammit merged commit 894e362 into theforeman:master Nov 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #33620 - Command for upgrading foreman-maintain to major version #546

Fixes #33620 - Command for upgrading foreman-maintain to major version #546

upadhyeammit commented Oct 5, 2021

theforeman-bot commented Oct 5, 2021

upadhyeammit commented Oct 7, 2021

upadhyeammit commented Oct 7, 2021

ekohl Oct 12, 2021

upadhyeammit Oct 21, 2021

ehelms commented Oct 20, 2021

upadhyeammit commented Oct 21, 2021

ehelms commented Oct 21, 2021

upadhyeammit commented Oct 21, 2021

ekohl Oct 21, 2021

ekohl Oct 21, 2021

ekohl Oct 21, 2021

upadhyeammit commented Oct 26, 2021

ekohl left a comment

ekohl Oct 26, 2021

upadhyeammit Oct 26, 2021

ekohl Oct 26, 2021

evgeni Oct 26, 2021

ehelms Oct 28, 2021

evgeni Oct 29, 2021

ekohl commented Oct 26, 2021

ehelms left a comment

ehelms commented Nov 18, 2021

upadhyeammit commented Nov 18, 2021

upadhyeammit commented Nov 23, 2021

		backup_dir = File.expand_path(ForemanMaintain.config.backup_dir)
		unless enabled_repos_ids.empty?

	description 'Disables all version specific maintenance repo and,'\
	description 'Disables all version specific maintenance repos and,'\

		option ['--target-version'], 'TARGET_VERSION', 'Major version of the Satellite or Capsule'\
		', e.g 7.0', :required => true

	return '6' if current_version == '6.10'
	return '6' if current_version.start_with?('6.')

Fixes #33620 - Command for upgrading foreman-maintain to major version #546

Fixes #33620 - Command for upgrading foreman-maintain to major version #546

Conversation

upadhyeammit commented Oct 5, 2021

theforeman-bot commented Oct 5, 2021

upadhyeammit commented Oct 7, 2021

upadhyeammit commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Oct 20, 2021

upadhyeammit commented Oct 21, 2021

ehelms commented Oct 21, 2021

upadhyeammit commented Oct 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upadhyeammit commented Oct 26, 2021

ekohl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Oct 26, 2021

ehelms left a comment

Choose a reason for hiding this comment

ehelms commented Nov 18, 2021

upadhyeammit commented Nov 18, 2021

upadhyeammit commented Nov 23, 2021