Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG FIX] Linux installers #203

Merged
merged 21 commits into from Feb 24, 2020
Merged

[BUG FIX] Linux installers #203

merged 21 commits into from Feb 24, 2020

Conversation

sam1el
Copy link
Collaborator

@sam1el sam1el commented Feb 21, 2020

Description

This PR addresses the following issues
#201
#188

These 2 were addressed by the same change.
The current code is as follows.

def hab_command
    cmd = ["bash #{Chef::Config[:file_cache_path]}/hab-install.sh", "-v #{hab_version}"]
    cmd.join(' ')
  end
end

This logic does not check for platform or kernel version. This causes the install resource to default to the non-kernel2 version.

The new code verifies kernel release and assigns the correct values accordingly

def hab_command
    cmd = if node['kernel']['release'].to_f < 3.0
            ["bash #{Chef::Config[:file_cache_path]}/hab-install.sh", "-v #{hab_version} -t x86_64-linux-kernel2"]
          else
            ["bash #{Chef::Config[:file_cache_path]}/hab-install.sh", "-v #{hab_version}"]
          end
    cmd.join(' ')
  end
end

This is also a problem for package verification due to to the vast sprawl in what kernels are used by the supported linux distros. With the difference in version numbers vs kernel release between similar platform types. I feel like platform_version isn't an accurate check. In my testing I've seen the following.

  • Currently the provider_hab_package library does the following. The problem with this is, Red hat 6.10 is using kernel2. Suse was released with kernel 2 up to version 11.2. Centos 6 can be on kernel 3 etc.
def platform_target
          if platform_family?('windows')
            'target=x86_64-windows'
          elsif platform_family?('rhel') && node['platform_version'].to_i < 6
            'target=x86_64-linux-kernel2'
          elsif platform_family?('suse') && node['platform_version'].to_i < 6
            'target=x86_64-linux-kernel2'
          else
            ''
          end
        end
  • Changing this library to use kernel release allows for the correct target version to be passed to the hab_package resource.
def platform_target
          if platform_family?('windows')
            'target=x86_64-windows'
          elsif node['kernel']['release'].to_f < 3.0
            'target=x86_64-linux-kernel2'
          else
            ''
          end
        end
  • These changes together are now installing the correct version of habitat which was partially responsible for package and service load failures.
  • All resources appear to be functioning with the expected idempotence.

#47

Since creating hab-sup.service is done by the hab_sup resource which, also calls the install resource, I took a look at the code being used in the library resource_hab_sup_systemd and it was missing the ExecStop which is all that is required to add the systemctl restart I added the line along with a few other functions.

systemd_unit 'hab-sup.service' do
          content(Unit: {
                    Description: 'The Habitat Supervisor',
                  },
                  Service: {
                    LimitNOFILE: (new_resource.limit_no_files if new_resource.limit_no_files),
                    Environment: service_environment,
                    ExecStart: "/bin/hab sup run #{exec_start_options}",
                    ExecStop: '/bin/hab sup term',
                    Restart: 'on-failure',
                    RestartSec: '10',
                    KillMode: 'process',
                  }.compact,
                  Install: {
                    WantedBy: 'default.target',
                  })
          action :create
        end

Issues Resolved

Check List

Jeff Brimager and others added 14 commits February 7, 2020 12:45
…a step to create c:\habitat directory since powershell was failing on that step. placeholder for upgrade enhancement

Signed-off-by: Jeff Brimager <jbrimager@chef.io>
Signed-off-by: Jeff Brimager <jbrimager@chef.io>
Signed-off-by: Jeff Brimager <jbrimager@chef.io>
Signed-off-by: Jeff Brimager <jbrimager@chef.io>
fixed bad syntax in install resource
test
Signed-off-by: Jeff Brimager <jbrimager@chef.io>
Signed-off-by: Jeff Brimager <jbrimager@chef.io>
* fixing delivery errors

Signed-off-by: Jeff Brimager <jbrimager@chef.io>

* missed toml correction in config.rb fixing for delivery tests

Signed-off-by: Jeff Brimager <jbrimager@chef.io>
Signed-off-by: Jeff Brimager <jbrimager@chef.io>
…md template

Signed-off-by: Jeff Brimager <jbrimager@chef.io>
@@ -110,9 +110,7 @@ def strip_version(name)
def platform_target
if platform_family?('windows')
'target=x86_64-windows'
elsif platform_family?('rhel') && node['platform_version'].to_i < 6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This was some hot nonesense here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was causing some serious crazy to happen

Jeff Brimager added 5 commits February 22, 2020 13:19
Signed-off-by: Jeff Brimager <jbrimager@chef.io>
Signed-off-by: Jeff Brimager <jbrimager@chef.io>
Signed-off-by: Jeff Brimager <jbrimager@chef.io>
Signed-off-by: Jeff Brimager <jbrimager@chef.io>
Signed-off-by: Jeff Brimager <jbrimager@chef.io>
@sam1el sam1el requested a review from jonlives February 23, 2020 02:29
elsif platform_family?('rhel') && node['platform_version'].to_i < 6
'target=x86_64-linux-kernel2'
elsif platform_family?('suse') && node['platform_version'].to_i < 6
elsif node['kernel']['release'].to_f < 3.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could easily just .to_i < 3 here as well. No need to treat it as a float

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. It think at first I was accidentally calling version rather than release and didnt understand why I was failing and forgot to change it when i caught my original mistake. Will change that up and push it again

Signed-off-by: Jeff Brimager <jbrimager@chef.io>
@@ -110,7 +110,7 @@ def strip_version(name)
def platform_target
if platform_family?('windows')
'target=x86_64-windows'
elsif node['kernel']['release'].to_f < 3.0
elsif node['kernel']['release'].to_i < 3.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elsif node['kernel']['release'].to_i < 3.0
elsif node['kernel']['release'].to_i < 3

@@ -175,7 +175,7 @@ def hab_path
end

def hab_command
cmd = if node['kernel']['release'].to_f < 3.0
cmd = if node['kernel']['release'].to_i < 3.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cmd = if node['kernel']['release'].to_i < 3.0
cmd = if node['kernel']['release'].to_i < 3

Copy link
Collaborator Author

@sam1el sam1el Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't believe I did that on both of those. They are fixed. Thanks pointing it out.

… for the kernel version check

Signed-off-by: Jeff Brimager <jbrimager@chef.io>
@tas50 tas50 merged commit 5288772 into chef-boneyard:master Feb 24, 2020
chef-expeditor bot pushed a commit that referenced this pull request Feb 24, 2020
Obvious fix; these changes are the result of automation not creative thinking.
@sam1el sam1el deleted the lininstall branch April 15, 2020 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants