[provisioner] Add autoyast self update option by nicolasbock · Pull Request #823 · crowbar/crowbar-core

nicolasbock · 2016-11-23T20:25:56Z

AutoYaST can update itself before installing the OS. This patch enables
this feature and adds a field to the provisioner barclamp so that a
repository URL can be set by the user. The special token <ADMINWEB> resolves
to the IP:port of the admin node. An empty URL defaults to the SUSE
customer center (scc.suse.com).

vuntz · 2016-11-25T16:28:22Z

      "shell_prompt": "USER@HOST:CWD SUFFIX",
      "enable_pxe": true,
+      "self_update": false,
+      "self_update_url": "http://updates.suse.com/sle12/$arch",


Shouldn't we default this to a repo that is mirrored on the admin server, like all the other repos we have there?

Also, I'm not convinced we need a boolean to enable this or not. Either the repo is present locally (we enable), or the user provides an external URL (we enable), or the repo is not present locally and there's no url (we disable)

@vuntz Thanks. I changed the PR.

vuntz · 2016-11-28T10:13:44Z

      <import_gpg_key config:type="boolean">true</import_gpg_key>
    </signature-handling>
    <storage/>
+    <self_update_url><%= @self_update_url %></self_update_url>


Didn't we want to add some unless @self_update_url.nil? || @self_update_url.empty?

done. Now, if the URL is empty AutoYaST will default to scc.suse.com if it's there.

vuntz · 2016-11-28T10:16:29Z

      "access_keys": "",
      "shell_prompt": "USER@HOST:CWD SUFFIX",
      "enable_pxe": true,
+      "self_update_url": "http://crowbar/sle12/$arch",


I would change the default to: http://<ADMINWEB>/suse-12.2/$arch/repos/installer-update (or something similar).

A couple of things:

<ADMINWEB> should be substituted in the rails app (see glance & tempest barclamps)

if we put suse-12.2 in the path, then maybe that attribute should live in a versioned suse attribute path somewhere (we already have that in the provisioner barclamp, I think)

we need a doc bug to document how to mirror that repo in the location

I've add the <ADMINWEB> alias.

Opened doc bug: https://bugzilla.suse.com/show_bug.cgi?id=1012469

I reverted the default to "" which defaults to scc.suse.com. Or would you prefer a versioned local URL?

vuntz · 2016-11-28T10:16:38Z

    %span.help-block
      = t(".access_keys_hint")

+    = boolean_field :self_update


This one is gone :-)

Good catch! I removed it.

houndci-bot · 2016-11-28T20:35:57Z

                      packages: packages,
                      repos: repos,
+                      self_update_url: node[:provisioner][:self_update_url].gsub(
+                        /<ADMINWEB>/, "#{admin_ip}:#{web_port}"),


Style/MultilineMethodCallBraceLayout: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

vuntz · 2016-11-29T09:09:42Z

      edit_attributes:
        access_keys: 'Additional SSH keys'
        access_keys_hint: 'Each SSH key must be on its own line'
+        self_update_url: 'AutoYaST Self-update URL (The alias <ADMINWEB> can be used to specify the admin node)'


s/admin node/admin server/

vuntz · 2016-11-29T09:10:05Z

      "access_keys": "",
      "shell_prompt": "USER@HOST:CWD SUFFIX",
      "enable_pxe": true,
+      "self_update_url": "",


So we don't provide a default value that just works? :-)

:) Well, the default in this case is to use scc.suse.com which works. But we could also use a crowbar local default. Which would you prefer?

I think the default should be the mirror on the admin server.

vuntz · 2016-11-29T09:10:37Z

          append << "ifcfg=dhcp4 netwait=60"
          append << "squash=0" # workaround bsc#962397
          append << "autoupgrade=1" if mnode[:state] == "os-upgrading"
+          append << "self_update"


Shouldn't we pass this only if self_update_url is not empty?

Also, if the URL contains <ADMINWEB>, I think we could do some quick check with File.exists? so that if we use a default with <ADMINWEB> but the repo is not mirrored, we just don't enable the update feature.

I added settings to autoyast.xml so that failures in self update don't mean failure anymore. AutoYaST will log those failure but continue with the installation process.

I'll add a check whether the repository is actually there.

I guess the check you mention here is still not there? :-)

I added it now.

houndci-bot · 2016-11-30T20:48:39Z

+    return if all_nodes.empty?
+
+    nodes = NodeObject.find("roles:provisioner-server")
+    unless nodes.nil? or nodes.length < 1


Style/AndOr: Use || instead of or. (https://github.com/bbatsov/ruby-style-guide#no-and-or-or)
Style/ZeroLengthPredicate: Use empty? instead of length < 1.

houndci-bot · 2016-11-30T20:48:40Z

+    #      "#{proposal["attributes"]["provisioner"]["self_update_url"]}"
+    #    ))
+    #  end
+    #end


Style/LeadingCommentSpace: Missing space after #. (https://github.com/bbatsov/ruby-style-guide#hash-space)

houndci-bot · 2016-11-30T20:48:41Z


+    @logger.info("validating existence of #{proposal["attributes"]["provisioner"]["self_update_url"]}")
+
+    #if /<ADMINWEB>/ =~ proposal["attributes"]["provisioner"]["self_update_url"]


Style/LeadingCommentSpace: Missing space after #. (https://github.com/bbatsov/ruby-style-guide#hash-space)

houndci-bot · 2016-11-30T20:48:41Z

      end
    end

+    @logger.info("validating existence of #{proposal["attributes"]["provisioner"]["self_update_url"]}")


Metrics/LineLength: Line is too long. [103/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

nicolasbock · 2016-11-30T22:08:53Z

@vuntz Here is where I am at with this PR:

The self update feature is always enabled now
I added some settings to the autoyast file so that a missing or unavailable self update URL is not fatal. A warning is shown and after 10 seconds the installation process continues. The warning is logged and will show up in a supportconfig.
The default URL points to a mirrored repository on the admin server
We don't verify that that mirror exists and instead rely on the logged error message from AutoYaST. To verify we need to know the http server root directory and replace the proper parts of the URI with that directory. Since we don't know what arch we will deploy the role on, we have to be happy with any $arch and testing for that seemed more than a line of code. Of course I might very well not seeing the simple solution here, so please correct me if I am making this too complicated 😉.
An empty URL means update from scc.suse.com
I still don't fully understand how the defaults from a role end up in a node's attributes. This PR replaces the <ADMINWEB> shortcut in the chef recipe. I tried to follow the glance barclamp but couldn't get it to work here. i.e. if I replace <ADMINWEB> in apply_role_pre_chef_call in models/provider_service.rb the replace URI does not end up in the node.

rsalevsky · 2016-12-02T10:33:21Z

@nicolasbock Can you please change the prefix from [provisioner] to provisioner:?

nicolasbock · 2016-12-02T12:36:23Z

@rsalevsky oop, sorry, of course.

aspiers

No commit message and no PR description => automatic -1 ;-p

nicolasbock · 2016-12-13T21:11:46Z

@aspiers I have added a description to the commit. Could you have another look?

vuntz · 2017-01-06T13:03:44Z

            "access_keys": { "type": "str", "required": true },
            "shell_prompt": { "type": "str", "required": true },
            "enable_pxe": { "type": "bool", "required": true },
+            "self_update_url": { "type": "str", "required": true },


Sorry, I failed to notice this earlier on, but we have a suse attribute, and this should live in there. See line 56.

vuntz · 2017-01-06T13:05:14Z

          append << "ifcfg=dhcp4 netwait=60"
          append << "squash=0" # workaround bsc#962397
          append << "autoupgrade=1" if mnode[:state] == "os-upgrading"
+          append << "self_update"


I guess the check you mention here is still not there? :-)

houndci-bot · 2017-01-10T14:10:04Z

+      do_self_update = node[:provisioner][:suse][:autoyast][:do_self_update]
+      if do_self_update && ! expanded_self_update_url.empty?
+        do_self_update = system("wget --quiet --spider #{expanded_self_update_url.gsub("$arch", "#{arch}")}")
+        if ! do_self_update


Metrics/BlockNesting: Avoid more than 3 levels of block nesting. (https://github.com/bbatsov/ruby-style-guide#three-is-the-number-thou-shalt-count)
Style/NegatedIf: Favor unless over if for negative conditions. (https://github.com/bbatsov/ruby-style-guide#unless-for-negatives)
Style/SpaceAfterNot: Do not leave space between ! and its argument. (https://github.com/bbatsov/ruby-style-guide#no-space-bang)

houndci-bot · 2017-01-10T14:10:05Z

+      )
+      do_self_update = node[:provisioner][:suse][:autoyast][:do_self_update]
+      if do_self_update && ! expanded_self_update_url.empty?
+        do_self_update = system("wget --quiet --spider #{expanded_self_update_url.gsub("$arch", "#{arch}")}")


Style/UnneededInterpolation: Prefer to_s over string interpolation.
Metrics/LineLength: Line is too long. [109/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

houndci-bot · 2017-01-10T14:10:05Z

+        "<ADMINWEB>", "#{admin_ip}:#{web_port}"
+      )
+      do_self_update = node[:provisioner][:suse][:autoyast][:do_self_update]
+      if do_self_update && ! expanded_self_update_url.empty?


Style/SpaceAfterNot: Do not leave space between ! and its argument. (https://github.com/bbatsov/ruby-style-guide#no-space-bang)

houndci-bot · 2017-01-10T14:14:09Z

+      if do_self_update && !expanded_self_update_url.empty?
+        do_self_update = system("wget --quiet --spider " +
+                                "#{expanded_self_update_url.gsub("$arch", "#{arch}")}")
+        unless do_self_update


Metrics/BlockNesting: Avoid more than 3 levels of block nesting. (https://github.com/bbatsov/ruby-style-guide#three-is-the-number-thou-shalt-count)

houndci-bot · 2017-01-10T14:14:09Z

+      do_self_update = node[:provisioner][:suse][:autoyast][:do_self_update]
+      if do_self_update && !expanded_self_update_url.empty?
+        do_self_update = system("wget --quiet --spider " +
+                                "#{expanded_self_update_url.gsub("$arch", "#{arch}")}")


Style/UnneededInterpolation: Prefer to_s over string interpolation.

houndci-bot · 2017-01-10T14:14:10Z

+      )
+      do_self_update = node[:provisioner][:suse][:autoyast][:do_self_update]
+      if do_self_update && !expanded_self_update_url.empty?
+        do_self_update = system("wget --quiet --spider " +


Style/LineEndConcatenation: Use \ instead of + or << to concatenate those strings.

houndci-bot · 2017-01-10T14:15:52Z

+      do_self_update = node[:provisioner][:suse][:autoyast][:do_self_update]
+      if do_self_update && !expanded_self_update_url.empty?
+        do_self_update = system("wget --quiet --spider " \
+                                expanded_self_update_url.gsub("$arch", "#{arch}"))


unexpected token tIDENTIFIER
unexpected token tRPAREN

nicolasbock · 2017-01-16T16:12:49Z

@aspiers I have added a commit message and a PR comment.

nicolasbock · 2017-01-16T16:14:25Z

@vuntz I think I have addressed the issues you raised. Could you have another look?

AutoYaST can update itself before installing the OS. This patch enables this feature and adds a field to the provisioner barclamp so that a repository URL can be set by the user. The special token `<ADMINWEB>` resolves to the IP:port of the admin node. An empty URL defaults to the SUSE customer center (scc.suse.com).

Complaint addressed

aspiers · 2017-01-25T15:15:36Z

@nicolasbock Removed my -1. I can't give +1 because I don't even understand why autoyast would need to self-update :-) If there's newer autoyast code then why wouldn't it be simply provided via TFTP/HTTP the first time around?

nicolasbock · 2017-01-25T16:38:15Z

@aspiers Thanks! Regarding your question: I haven't checked but am guessing that the autoyast comes from the image pulled from tftp on the admin node. I don't know though how and where from it is updated and I think your question is very valid 😄.

I forgot to add this Trello card.

nicolasbock · 2017-03-10T19:16:42Z

@vuntz Could you have another look, please?

nicolasbock added the wip label Nov 23, 2016

nicolasbock force-pushed the installer_update branch 3 times, most recently from b3caa83 to 910de87 Compare November 24, 2016 13:49

vuntz suggested changes Nov 25, 2016

View reviewed changes

nicolasbock force-pushed the installer_update branch from 910de87 to 05b21f5 Compare November 26, 2016 13:18

vuntz reviewed Nov 28, 2016

View reviewed changes

nicolasbock force-pushed the installer_update branch from 05b21f5 to 8f70db2 Compare November 28, 2016 20:35

houndci-bot reviewed Nov 28, 2016

View reviewed changes

nicolasbock force-pushed the installer_update branch from 8f70db2 to df35e12 Compare November 28, 2016 20:45

nicolasbock removed the wip label Nov 28, 2016

vuntz suggested changes Nov 29, 2016

View reviewed changes

nicolasbock force-pushed the installer_update branch from df35e12 to 011cbf1 Compare November 30, 2016 20:48

houndci-bot reviewed Nov 30, 2016

View reviewed changes

nicolasbock force-pushed the installer_update branch 2 times, most recently from 3e5c773 to d719711 Compare November 30, 2016 21:53

nicolasbock force-pushed the installer_update branch from d719711 to b9582db Compare December 2, 2016 12:37

aspiers previously requested changes Dec 12, 2016

View reviewed changes

nicolasbock force-pushed the installer_update branch from b9582db to f3afca5 Compare December 13, 2016 21:11

vuntz suggested changes Jan 6, 2017

View reviewed changes

nicolasbock force-pushed the installer_update branch from f3afca5 to 08ae4fc Compare January 10, 2017 14:09

houndci-bot reviewed Jan 10, 2017

View reviewed changes

nicolasbock force-pushed the installer_update branch 2 times, most recently from 69174ee to e0eb5d2 Compare January 10, 2017 14:14

houndci-bot reviewed Jan 10, 2017

View reviewed changes

nicolasbock force-pushed the installer_update branch from e0eb5d2 to 19d6255 Compare January 10, 2017 14:15

houndci-bot reviewed Jan 10, 2017

View reviewed changes

nicolasbock force-pushed the installer_update branch from 19d6255 to e045823 Compare January 10, 2017 14:16

nicolasbock force-pushed the installer_update branch from e045823 to ffaef28 Compare January 23, 2017 19:18

dirkmueller modified the milestones: Cloud 7 Update1, Cloud 7 Update2 Jan 27, 2017

jsuchome added the needs backport to SOC7 (stable/4.0) label Jun 15, 2017

dirkmueller removed the needs backport to SOC7 (stable/4.0) label Oct 5, 2017

JanZerebecki added the wip label Jan 13, 2021


		@logger.info("validating existence of #{proposal["attributes"]["provisioner"]["self_update_url"]}")

		#if /<ADMINWEB>/ =~ proposal["attributes"]["provisioner"]["self_update_url"]

Conversation

nicolasbock commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasbock commented Nov 30, 2016

Uh oh!

rsalevsky commented Dec 2, 2016

Uh oh!

nicolasbock commented Dec 2, 2016

Uh oh!

aspiers left a comment

Choose a reason for hiding this comment

Uh oh!

nicolasbock commented Dec 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nicolasbock commented Nov 23, 2016 •

edited

Loading