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

Use hostnamectl instead of hostname to set the hostname under SUSE #11100

Merged
merged 1 commit into from Oct 8, 2019

Conversation

@chkpnt
Copy link
Contributor

commented Oct 3, 2019

hostname isn't part of the boxes openSUSE currently provides. As openSUSE and SLE are using systemd quite a while, setting the hostname using hostnamectl should be fine.

Without this fix, the transient hostname will stay on localhost until the VM is restarted.

I've tested this change against openSUSE Leap 15.0, Leap 15.1 and Tumbleweed using this Vagrantfile:

Vagrant.configure("2") do |config|

  config.vm.define "Leap15-0" do |sut|
    sut.vm.box = "Leap-15.0.x86_64"
    sut.vm.box_url = "https://download.opensuse.org/repositories/Virtualization:/Appliances:/Images:/openSUSE-Leap-15.0/images/boxes/Leap-15.0.x86_64.json"
    sut.vm.hostname = "leap15-0.example.com"
  end

  config.vm.define "Leap15-1" do |sut|
    sut.vm.box = "Leap-15.1.x86_64"
    sut.vm.box_url = "https://download.opensuse.org/repositories/Virtualization:/Appliances:/Images:/openSUSE-Leap-15.1/images/boxes/Leap-15.1.x86_64.json"
    sut.vm.hostname = "leap15-1.example.com"
  end

  config.vm.define "Tumbleweed" do |sut|
    sut.vm.box = "opensuse/openSUSE-Tumbleweed-Vagrant.x86_64"
    sut.vm.hostname = "tumbleweed.example.com"
  end

end
@hashicorp-cla

This comment has been minimized.

Copy link

commented Oct 3, 2019

CLA assistant check
All committers have signed the CLA.

@chkpnt

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

Nevertheless I'm unsure if this is the right way or if hostname should be part of the boxes. Maybe @dcermak, who seems to be the maintainer of Virtualization:Appliances:Images:openSUSE-Leap-15.1, should proclaim his opinion.

Copy link
Contributor

left a comment

Overall I'm very much in favor of this change, as I've completely missed that hostname is now missing from these boxes...

I've noted just two details that I'd like to clarify first.

@@ -5,11 +5,10 @@ class ChangeHostName
def self.change_host_name(machine, name)
comm = machine.communicate

if !comm.test("hostname -f | grep '^#{name}$'", sudo: false)
if !comm.test("getent hosts '#{name}'", sudo: false)

This comment has been minimized.

Copy link
@dcermak

dcermak Oct 4, 2019

Contributor

This does not query the hostname though, this only queries the settings from /etc/hosts. hostnamectl status --static will give you the current full hostname.

This comment has been minimized.

Copy link
@chkpnt

chkpnt Oct 4, 2019

Author Contributor

Thanks for your comments!

hostname status --static will print the hostname, not the FQDN which depends on the resolver. And this is what hostname -f is doing: It uses the hostname, gives it to the resolver and returns the result.
getent hosts '#{name}' is the command I've found which seems to be the closest to hostname -f, as on the base box the resolver will use /etc/hosts.

Of course one could discuss if this test is really necessary or if comparing the hostname (basename) would be sufficient. I guess the original authors wanted vagrant to be able to change the domain part of the box' fqdn after the initial start.

This comment has been minimized.

Copy link
@dcermak

dcermak Oct 7, 2019

Contributor

Of course one could discuss if this test is really necessary or if comparing the hostname (basename) would be sufficient. I guess the original authors wanted vagrant to be able to change the domain part of the box' fqdn after the initial start.

I think this merely tries to find out if vagrant already changed the hostname of the box, so it makes sense to query the boxes' fqdn instead of the hostname.

Thanks for clarifying this!

basename = name.split(".", 2)[0]
comm.sudo <<-EOH.gsub(/^ {14}/, '')
echo '#{basename}' > /etc/HOSTNAME
hostname '#{basename}'
hostnamectl set-hostname '#{basename}'

This comment has been minimized.

Copy link
@dcermak

dcermak Oct 4, 2019

Contributor

Why not hostnamectl set-hostname '#{name}'? In the example Vagrantfile that you provide this results in a hostname leap15-0 instead of leap15-0.example.com. Note that if you run hostnamectl set-hostname leap15-0.example.com then the bash prompt will still display vagrant@leap15-0:~> if that is what you wanted to achieve.

Suggested change
hostnamectl set-hostname '#{basename}'
hostnamectl set-hostname '#{name}'

This comment has been minimized.

Copy link
@chkpnt

chkpnt Oct 4, 2019

Author Contributor

But this would write #{name} into /etc/hostname and /etc/HOSTNAME. As far as I know only the host's name, which is the '#{basename}' belongs to these files.

This comment has been minimized.

Copy link
@dcermak

dcermak Oct 7, 2019

Contributor

That is right, so let's keep it that way.

@dcermak

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

Nevertheless I'm unsure if this is the right way or if hostname should be part of the boxes. Maybe @dcermak, who seems to be the maintainer of Virtualization:Appliances:Images:openSUSE-Leap-15.1, should proclaim his opinion.

I try to keep the images as bare-bones as possible and not include anything that is not in the default patterns. So if hostname got dropped, we should switch to using hostnamectl.

@briancain briancain added the guest/suse label Oct 4, 2019
@dcermak
dcermak approved these changes Oct 7, 2019
Copy link
Contributor

left a comment

Looks good to me and can be imho merged.

@briancain briancain added this to the 2.2 milestone Oct 7, 2019
Copy link
Member

left a comment

Thanks for taking the time to make a pull request for Vagrant! The changes look good to me, and work with a few of the various opensuse boxes I tested it with. 🎉 👍

@briancain briancain modified the milestones: 2.2, 2.2.6 Oct 8, 2019
@briancain briancain merged commit ea55028 into hashicorp:master Oct 8, 2019
6 checks passed
6 checks passed
ci/circleci: test_ruby23 Your tests passed on CircleCI!
Details
ci/circleci: test_ruby24 Your tests passed on CircleCI!
Details
ci/circleci: test_ruby25 Your tests passed on CircleCI!
Details
ci/circleci: test_ruby26 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.