(PE-19438) Stop passing -c to upgrades from MEEP #2
Conversation
…-flag' into pe-modules-next
lib/beaker-pe/install/pe_utils.rb
Outdated
| if opts[:upgrade] && use_meep?(host['previous_pe_ver']) | ||
| # merge answers into pe.conf | ||
| update_pe_conf(opts[:answers]) | ||
| update_pe_conf(opts[:custom_answers]) |
There was a problem hiding this comment.
@jpartlow Looking at how this method works, do we also need to perform a puppet run on the PE infra nodes in order to propagate the pe.conf before upgrade?
There was a problem hiding this comment.
discussed this morning, and determined that we should sync the conf.d dir with beaker since that is the currently documented process, and work on syncing automatically as a stretch goal.
There was a problem hiding this comment.
Where should we do the sync with Beaker?
There was a problem hiding this comment.
I would think it would make sense to do it right here? I should probably add a condition that only does update_pe_conf on the master, and then SCPs the pe.conf over on any other node.
lib/beaker-pe/install/pe_utils.rb
Outdated
| @@ -450,7 +450,15 @@ def do_install hosts, opts = {} | |||
| else | |||
| prepare_host_installer_options(host) | |||
| register_feature_flags!(opts) | |||
| generate_installer_conf_file_for(host, hosts, opts) | |||
|
|
|||
| if opts[:upgrade] && use_meep?(host['previous_pe_ver']) | |||
There was a problem hiding this comment.
So our upgrade cases are:
- from 2016.2.0+ (post-meep)
- Beaker runs the installer such that we make use of recovery
- from < 2016.2.0 (pre-meep)
- Beaker creates a fresh pe.conf using beaker-answers, as if we were doing an install
There was a problem hiding this comment.
Oh, I guess just checking opts[:upgrade] worked because I was testing upgrades from 2017.1 and that version just ignored the next flag.
As for the upgrade cases, that matches up with my expectations. Perhaps I should make that a comment for future us.
There was a problem hiding this comment.
@highb thinking again about the <2016.2.0 cases. In the real world, we expect recover configuration to still produce a working pe.conf, right? Ideally we should test that. The blocking point for us is that we need to put the feature_flag in pe.conf. What happens if we just create a pe.conf with the feature flag in it and then continue to without a pe.conf specified for puppet-enterprise-installer, and let recover configuration do its thing?
lib/beaker-pe/install/pe_utils.rb
Outdated
| @@ -133,7 +133,7 @@ def installer_cmd(host, opts) | |||
|
|
|||
| # If there are no answer overrides, and we are doing an upgrade from 2016.2.0, | |||
| # we can assume there will be a valid pe.conf in /etc that we can re-use. | |||
| if opts[:answers].nil? && opts[:custom_answers].nil? && opts[:type] == :upgrade && !version_is_less(opts[:HOSTS][host.name][:pe_ver], '2016.2.0') | |||
| if opts[:type] == :upgrade && use_meep?(host[:previous_pe_ver]) | |||
There was a problem hiding this comment.
Comment needs updating too I think.
highb
force-pushed
the
dont_provide_answer_file_for_meep_upgrade
branch
3 times, most recently
from
0c54ff5 to
cdf964b
Compare
Prior to this commit we were essentially always passing in a config to the installer during upgrades because we typically provide some sort of custom answers for many tests. This meant that we were not really testing upgrades without a config file being passed in. This commit updates the beaker to not pass in a config/`-c` option on upgrades from a MEEP install. In order to pass in the custom answers, I have instead made use of the `update_pe_conf` method to inject the answers.
highb
force-pushed
the
dont_provide_answer_file_for_meep_upgrade
branch
from
cdf964b to
d6b725a
Compare
|
@highb some of the do_install specs need to be fixed up to account for the additional commands being run. |
lib/beaker-pe/install/pe_utils.rb
Outdated
| @@ -450,9 +452,36 @@ def do_install hosts, opts = {} | |||
| else | |||
| prepare_host_installer_options(host) | |||
| register_feature_flags!(opts) | |||
| generate_installer_conf_file_for(host, hosts, opts) | |||
|
|
|||
| enterprise_path = '/etc/puppetlabs/enterprise' | |||
There was a problem hiding this comment.
The logic in do_install is already very cluttered. Let's take those whole new if/else block and move it out to a descriptive method taking host, hosts, opt.
Might want to do the same for the scp_from call as well for clarity.
There was a problem hiding this comment.
Odd, the github emails for this PR went to my personal mail. Maybe because it's not under the puppetlabs namespace.
Making more descriptive methods sounds like a good idea here, I'll do that and see what's happening with the specs.
The `do_install` method was getting a bit cluttered with too many levels of logic, so I've moved the pe.conf setup steps out into their own method, `setup_pe_conf`
The new functionality in `do_install` to copy the `conf.d` folder
| @@ -37,6 +37,7 @@ module PEUtils | |||
| MEEP_DATA_DIR = '/etc/puppetlabs/enterprise' | |||
| PE_CONF_FILE = "#{MEEP_DATA_DIR}/conf.d/pe.conf" | |||
| NODE_CONF_PATH = "#{MEEP_DATA_DIR}/conf.d/nodes" | |||
| BEAKER_MEEP_TMP = "pe_conf" | |||
There was a problem hiding this comment.
@jpartlow Oh yeah, is there a beaker tmp dir I could be using for this?
There was a problem hiding this comment.
I don't think there is on the test executor itself.
There was a problem hiding this comment.
OK, hopefully pe_conf is a self-explanatory enough directory name.
jpartlow
force-pushed
the
pe-modules-next
branch
from
c91943e to
98d4a9b
Compare
|
This was pulled directly into puppetlabs#59 and then I recreated the pe-modules-next branch. |
Prior to this commit we were essentially always passing in a config
to the installer during upgrades because we typically provide some
sort of custom answers for many tests. This meant that we were not
really testing upgrades without a config file being passed in.
This commit updates the beaker to not pass in a config/
-coptionon upgrades from a MEEP install. In order to pass in the custom answers,
I have instead made use of the
update_pe_confmethod to inject theanswers.