[SHACK-179] Support for upgrading chef-client #112

marcparadise · 2018-05-09T17:52:09Z

This change updates the install action to verify that the target host
is running a minimum chef client version, and show an error when they are not.

It contains the logic to automatically upgrade, but that path has been disabled until
we determine how to best safely handle automatic upgrades.

The converge command and i18n has been updated to report
whether an upgrade or install is being performed, and to include
version info in install messages.

This also includes:

beginning to consolidate all things that rae in the domain of the target system (mappings of OS, platform, etc) into TargetHost.
error handling within actions so that command is notified of a raised exception. Implemented a default notification handler that accepts the error notification and marks the current task as failed via the reporter. This corrects an issue where a failed action would correctly report the error, but the visual status was never updated to reflect the failure.

TODO:

add flag to disable install/upgrade
error if install/ugprade is disabled and target does not have chef or does not have it at the minimum version
tests for new functionality of target_host
ensure we properly raise UnsupportedOS - currently this lives in the action, but it should probably now move to TargetHost
- Where it is now works, but a minor refactor would make it make more sense. I'll add a card...
Remove TODO around old versions - tball's suggestion below is the default behavior with the current logic.

tyler-ball

One minor comment but I think this is looking good so far

tyler-ball · 2018-05-09T18:06:38Z

components/chef-workstation/lib/chef-workstation/target_host.rb

+                        # installer action
+                      end
+      manifest_content = backend.file(manifest_path).content
+      # TODO - handle where it is installed, but at such an old version there is no manifest?


This seems something worth fixing as part of this PR. Perhaps if the manifest file does not exist we can return a version of 0.0.0? That would force an upgrade

It occurs to me that we do, indirectly - if the file content is nil, we treat it as not installed.

cheeseplus · 2018-05-09T21:52:07Z

components/chef-workstation/i18n/en.yml

-    We plan to support a wide range of target operating systems,
-    but during this targeted pre-release we are constraining our efforts
+    We plan to support a range of target operating systems,
+    but during this targeted beta we are constraining our efforts
    to Windows and Linux.


Windows and Linux but not OSX?

For the target host, yeah - at least for now: https://chefio.slack.com/archives/C8PKULC6S/p1525963099000551

marcparadise · 2018-05-10T15:33:13Z

Outstanding items have been completed. There is further discussion around whether we want the default desired behavior to be an upgrade, or if this should require explicit action on the part of the operator. Any changes as a result of that discussion will be implemented under a separate PR.

marcparadise · 2018-05-10T15:39:13Z

components/chef-workstation/lib/chef-workstation/action/install_chef.rb

@@ -3,20 +3,11 @@
 require "chef-workstation/action/install_chef/linux"

 module ChefWorkstation::Action::InstallChef
-  def self.instance_for_target(target_host, opts = {})
+  def self.instance_for_target(target_host, opts = { check_only: false })


Moved the 'unsupported os' logic into TargetHost, so that we can start the process of not scattering OS-specific behaviors in places that don't need to worry about it.

jonsmorrow

This is a tentative approval pending our conversation at standup about wether we want to default to upgrade or not.

jonsmorrow · 2018-05-10T15:41:42Z

components/chef-workstation/i18n/en.yml

+    requires a minimum version of %2.
+
+    Re-running this command without the '--no-install' flag will permit
+    me to automatically upgrade the target to the latest Chef client.


We don't need to change this but we should decide what we want. I personally don't like tools the personify themselves such as 'me' in this message. I'd rather see it say something like:

Re-running this command without the '--no-install' flag will automatically perform the installation.

I'm alright with the new wording. In general I like personification for a couple of reasons:

it gives more of a feel with interacting with an agent doing work on your behalf

it avoid awkward phrasing that exists only to avoid naming a sentence subject. (Your suggestion avoids this pitfall)

tyler-ball

One request but 🚀 🍰 otherwise!

tyler-ball · 2018-05-10T18:31:11Z

components/chef-workstation/lib/chef-workstation/command/target/converge.rb

+           long: "--[no-]install",
+           default: true,
+           description:  T.install_description(Action::InstallChef::Base::MIN_CHEF_VERSION)
+


I think this needs boolean: true also

jonsmorrow

LGTM

jonsmorrow · 2018-05-10T18:32:02Z

components/chef-workstation/lib/chef-workstation/action/install_chef/base.rb

+      #       will get re-visited once we determine how we want automatic
+      #       upgrades to behave.
+      # @upgrading = true
+      # perform_local_install


Thanks for the comment

This change updates the install action to perform installation when chef client is present on the target, but below minimum version. The converge command and i18n has been updated to report whether an upgrade or install is being performed, and to include version info in install messages.

The --no-install flag will prevent install or upgrade of the target's chef client installation. If an update or installation is required, an appropriate error message is shown with instructions on permitting the change. This also improves in-task error handling so that the UI gets a chance to be refreshed when a task fails. Previously the error would be reported, but the summary 'spinner' line would never be updated for the failure.

This continues the move of consolidating all target-related lookups and translations into TargetHost, which is the only place that should need to know about them. This change also adds test coverage for the new TargetHost methods get_chef_version_manifest, base_os, and installed_chef_version.

Until we determine how we want to handle upgrading chef client on workstation-managed nodes, we have decided to disable the behavior of automatically upgrading out-of-date chef clients This changes the wording of CHEFINS003 to reflect that, and changes the install action so that it will simply raise if the chef client installation is out of date instead of conditionally continuing.

marcparadise requested a review from a team May 9, 2018 17:52

marcparadise added the do-not-merge label May 9, 2018

marcparadise force-pushed the SHACK-179/upgrade-client branch from 9a0c3d8 to 0d29f50 Compare May 9, 2018 17:54

tyler-ball reviewed May 9, 2018

View reviewed changes

marcparadise force-pushed the SHACK-179/upgrade-client branch from ce756ca to 48a4a1c Compare May 9, 2018 21:31

cheeseplus reviewed May 9, 2018

View reviewed changes

marcparadise removed the do-not-merge label May 10, 2018

marcparadise force-pushed the SHACK-179/upgrade-client branch from 1fd1c92 to cc3f341 Compare May 10, 2018 15:33

marcparadise commented May 10, 2018

View reviewed changes

jonsmorrow previously approved these changes May 10, 2018

View reviewed changes

marcparadise dismissed jonsmorrow’s stale review via aa01f6a May 10, 2018 16:46

marcparadise force-pushed the SHACK-179/upgrade-client branch 5 times, most recently from 873f8eb to 84100d6 Compare May 10, 2018 18:24

tyler-ball previously approved these changes May 10, 2018

View reviewed changes

jonsmorrow previously approved these changes May 10, 2018

View reviewed changes

cheeseplus previously approved these changes May 10, 2018

View reviewed changes

marcparadise dismissed stale reviews from cheeseplus, jonsmorrow, and tyler-ball via 055efa5 May 10, 2018 18:37

marcparadise force-pushed the SHACK-179/upgrade-client branch from 84100d6 to 055efa5 Compare May 10, 2018 18:37

jonsmorrow previously approved these changes May 10, 2018

View reviewed changes

marcparadise dismissed jonsmorrow’s stale review via 79cbc60 May 10, 2018 18:45

marcparadise force-pushed the SHACK-179/upgrade-client branch from 055efa5 to 79cbc60 Compare May 10, 2018 18:45

marcparadise added 3 commits May 10, 2018 14:47

marcparadise force-pushed the SHACK-179/upgrade-client branch from 79cbc60 to e58a142 Compare May 10, 2018 18:47

marcparadise merged commit e8417d7 into master May 10, 2018

chef-ci deleted the SHACK-179/upgrade-client branch May 10, 2018 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SHACK-179] Support for upgrading chef-client #112

[SHACK-179] Support for upgrading chef-client #112

marcparadise commented May 9, 2018 •

edited

Loading

tyler-ball left a comment

tyler-ball May 9, 2018

marcparadise May 9, 2018

cheeseplus May 9, 2018

marcparadise May 10, 2018

marcparadise commented May 10, 2018

marcparadise May 10, 2018

jonsmorrow left a comment

jonsmorrow May 10, 2018

marcparadise May 10, 2018

tyler-ball left a comment

tyler-ball May 10, 2018

jonsmorrow left a comment

jonsmorrow May 10, 2018

[SHACK-179] Support for upgrading chef-client #112

[SHACK-179] Support for upgrading chef-client #112

Conversation

marcparadise commented May 9, 2018 • edited Loading

tyler-ball 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

marcparadise commented May 10, 2018

Choose a reason for hiding this comment

jonsmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyler-ball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonsmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcparadise commented May 9, 2018 •

edited

Loading