Integrate the vagrant-alpine guest plugin

[timschumi]

This pull-request adds the vagrant-alpine plugin originally made by @maier into the main vagrant source tree. The PR might not be ready yet (a maybe-TODO list can be found below), but I'd like to have some opinions from the Vagrant developers on this.

The first commit in the chain contains the unmodified source (only the relevant parts) of the original plugin and is almost exclusively authored by @maier (if we ever get ready to get this merged, we'll probably need some advice on how to handle the CLA with that), the following commits contain various changes to clean up the code and adapt it to the main tree.

Maintainers should be able to push changes to the PR-branch themselves, if anyone else wants to contribute, please send a patch.

[hashicorp-cla]

All committers have signed the CLA.

[briancain]

Hi @timschumi - just a few things:

• In terms of any rubocop warnings, Vagrant doesn't use rubocop at the moment, so disabling or enabling them won't do anything. They will just be treated as comments.
• It looks like the workaround for installing nfs is specific to a Vagrant box, rather than alpine itself? At least from what I'm reading in the comments. Is that boxes used very much these days?
• I think anyone can click the CLA sign link above to agree, even if their original commits aren't in this PR? I will have to check on that though internally to see.
• Please also bring in the unit tests that are in the original plugin

Thanks!

[timschumi]

* In terms of any rubocop warnings, Vagrant doesn't use rubocop at the moment, so disabling or enabling them won't do anything. They will just be treated as comments.

The question that I asked myself was just whether those warnings should be fixed or if they can "stay" (should I remove the comments that suppress those warnings, even if we decide to do nothing about the warnings themselves?).

* It looks like the workaround for installing nfs is specific to a Vagrant box, rather than alpine itself? At least from what I'm reading in the comments. Is that boxes used very much these days?

From what I can tell, the workaround handles removing a (by now) non-existent mirror from the download list. I don't know whether this is an Alpine specific issue or an issue of that specific box, since I can't find any other provider of an version that old (nether the commonly used roboxes nor the first match when searching for "alpine vagrant" supply that version). The comment implies that only Alpine 3.3 (and maybe older) is affected. I'm going to assume that the mirror in question was the default back then, but later taken offline, which would make it an Alpine-specific issue, but only on that version.

The question essentially boils down to "Do we want to keep workarounds for a box that is based on a January 2016 OS release and that isn't commonly found anymore?".

* I _think_ anyone can click the CLA sign link above to agree, even if their original commits aren't in this PR? I will have to check on that though internally to see.

The link appears to contain no user-specific information, just the PR number (and it seems to work without it).

The question is on how to proceed with the CLA, since that is some kind of legal CYA for Hashicorp. Almost everything of the code has been authored by @maier (which is why the first commit contains exclusively unmodified code, so that his contributions can be differentiated from mine), and I can't accept the CLA on his behalf (or rather, the CLA that I sign doesn't cover the code that he contributed?).

Would it be enough if he just signs the CLA without any further connection to the code itself (except for the mention in the first commit description)?

Or should I set him as the author in the first commit, since it's exclusively his code that is added, even if he didn't actually author the commit itself? That would at least set up a connection between him and the contributed code that is actually traceable. (@maier: Would you be OK with this?)

* Please also bring in the unit tests that are in the original plugin

Will do. I thought that those were something that was external-plugin-only, since it didn't say 'test', but 'spec'. I'll add them to the first commit, where the other original code is contained, and force-push it over.

Thanks for taking the time to look at this!

[timschumi]

The latest batch contains the unit tests as well. To continue the split between "unmodified code" and "modifications on top", the first commit contains the original files, which are disabled due to the _spec ending. They are later fixed-up to be runnable and enabled in the alpine: Wire up tests commit.

[maier]

Just for some clarity on the NFS elements. Older Alpine releases didn't have any VBGA packages available, so the only way to share the fs was via NFS. As mentioned, it was for older Alpine releases and VBGA packages are now available in the Alpine repositories. Since the NFS support was 3.3 and earlier which went EOL in 2017, it is probably safe discard.

[briancain]

Thanks @timschumi - for the fix ups and tests!

I would say there's probably no harm in leaving the rubocop comments there for now. We have some future plans to integrate rubocop into Vagrant, so we can deal with it then.

I am currently asking about how HasiCorp wants to deal with the CLA in this situation, and should hopefully have an answer by end of day today or some time this week! 🙏

So based on what @maier mentioned, I think we might be ok to remove the workaround? Those boxes also haven't been updated in about 3 years on Vagrant Cloud, from what I can see. Maybe if we keep the code around in the original repo, we can always bring it back in if users come back with issues, but hopefully not if it's EOL.

Otherwise, thanks for taking the time to make a pull request! I've assigned this to myself to give any code review and also do some basic testing prior to merging.

[briancain]

Ok @maier - So please go ahead and sign the CLA for this PR too if you haven't already: https://cla.hashicorp.com/hashicorp/vagrant?pullRequest=10975

[timschumi]

So based on what @maier mentioned, I think we might be ok to remove the workaround? Those boxes also haven't been updated in about 3 years on Vagrant Cloud, from what I can see. Maybe if we keep the code around in the original repo, we can always bring it back in if users come back with issues, but hopefully not if it's EOL.

I'm all for removing the workaround. The by-now broken repository has been added some time between Alpine 2.7 and 3.0, stayed in until 3.3 (which EOL'd in 2017, as mentioned earlier) and was gone from the list of mirrors by version 3.4. So it only really affects those versions.

[timschumi]

Code wise, I feel like the change_hostname capability is quite bloated, especially for a rather "simple" action, as far as I can tell.

A lot of the aliases (for example sudo(), test() and fqdn) can probably be replaced with their substitutions without making the code unreadable, which would save quite a lot of lines. Same probably goes for the aliases for one-line shell commands, since those are only used once. I prepared a batch of changes here, maybe some other aspects (e.g. replacing/removing should_change? and the ´initialize()/new() wrapping) could be changed as well.

Also, the update code for /etc/hosts doesn't seem to handle IPv6 yet. The optimal solution would probably be to extend the regexp, but I'm unsure on how to implement that, since IPv6-Adresses can be far more complex than IPv4.

[briancain]

Code wise, I feel like the change_hostname capability is quite bloated, especially for a rather "simple" action, as far as I can tell.

A lot of the aliases (for example sudo(), test() and fqdn) can probably be replaced with their substitutions without making the code unreadable, which would save quite a lot of lines. Same probably goes for the aliases for one-line shell commands, since those are only used once. I prepared a batch of changes here, maybe some other aspects (e.g. replacing/removing should_change? and the ´initialize()/new() wrapping) could be changed as well.

Actually for this I don't really mind the extra methods here and am fine leaving it how it is now. It might be a bit overly verbose but I think that's ok. This should be easier to debug failures, compared to other change hostname guest capabilities that essentially try to do it in a big bash script.

Also, the update code for /etc/hosts doesn't seem to handle IPv6 yet. The optimal solution would probably be to extend the regexp, but I'm unsure on how to implement that, since IPv6-Adresses can be far more complex than IPv4.

Hmm, so it looks like all of the other guest capabilities in this case simply update the localhost entry in /etc/hosts, so I'm not sure if this patch is required since the default behavior for this when an IPv6 address has a guests fqdn is to do what other guest caps do (which is adding it to the localhost entry). We could simplify it to match the other guests where it just looks for the guest name only, and if there are no matches it appends it to the localhost entry.

[timschumi]

Actually for this I don't really mind the extra methods here and am fine leaving it how it is now. It might be a bit overly verbose but I think that's ok. This should be easier to debug failures, compared to other change hostname guest capabilities that essentially try to do it in a big bash script.

I agree with you that doing everything in a huge script would not be as easy to debug, but that's not what I'm planning on doing here. I just wanted to remove the one-line functions, since they only provide a different name for those functions. As an example, every occurrence of sudo() can be replaced by machine.communicate.sudo() and still run fine without any other changes. Same goes for the test() function.

The other functions I removed (e.g. update_etc_hostname, refresh_hostname_service, ...) just provided aliases for one-line shell commands, which could be inlined as well (while still keeping them as seperate calls). If they were used anywhere else, defining those functions would make sense, but they aren't.

If having everything in a big script is a problem, we should probably split up the huge chunk of a shell script in nfs_client. The important commands could just be split out into different calls, which (as far as I can tell) should provide exit code checking as well (EDIT: It does/would). The compatibility changes at the top can be (as far as I can tell from the responses) removed alltogether. (EDIT: I was imagining something like this).

Hmm, so it looks like all of the other guest capabilities in this case simply update the localhost entry in /etc/hosts, so I'm not sure if this patch is required since the default behavior for this when an IPv6 address has a guests fqdn is to do what other guest caps do (which is adding it to the localhost entry). We could simplify it to match the other guests where it just looks for the guest name only, and if there are no matches it appends it to the localhost entry.

I'll just take a look at the other guests then and try and adjust the capability accordingly.

[briancain]

Actually for this I don't really mind the extra methods here and am fine leaving it how it is now. It might be a bit overly verbose but I think that's ok. This should be easier to debug failures, compared to other change hostname guest capabilities that essentially try to do it in a big bash script.

I agree with you that doing everything in a huge script would not be as easy to debug, but that's not what I'm planning on doing here. I just wanted to remove the one-line functions, since they only provide a different name for those functions. As an example, every occurrence of sudo() can be replaced by machine.communicate.sudo() and still run fine without any other changes. Same goes for the test() function.

The other functions I removed (e.g. update_etc_hostname, refresh_hostname_service, ...) just provided aliases for one-line shell commands, which could be inlined as well (while still keeping them as seperate calls). If they were used anywhere else, defining those functions would make sense, but they aren't.

Hmm, ok! I do think it would be ok to replace the test and sudo functions, those are already Vagrant helpers, so that's fine. But I do like the other ones mostly because it's easy to read what they are doing, where as the commands themselves might be a little difficult to figure out without it.

If having everything in a big script is a problem, we should probably split up the huge chunk of a shell script in nfs_client. The important commands could just be split out into different calls, which (as far as I can tell) should provide exit code checking as well (EDIT: It does/would). The compatibility changes at the top can be (as far as I can tell from the responses) removed alltogether. (EDIT: I was imagining something like this).

Yes, that would be a good change to make too! I can't remember if Vagrant aborts or raises an exception when a command passed to machine.communicate.sudo fails, but that might be nice to check up on just to make sure the alpine capability works the same.

Hmm, so it looks like all of the other guest capabilities in this case simply update the localhost entry in /etc/hosts, so I'm not sure if this patch is required since the default behavior for this when an IPv6 address has a guests fqdn is to do what other guest caps do (which is adding it to the localhost entry). We could simplify it to match the other guests where it just looks for the guest name only, and if there are no matches it appends it to the localhost entry.

I'll just take a look at the other guests then and try and adjust the capability accordingly.

Sounds good! 👍 Let me know when that's all sorted out and I'll be happy to review the PR! Thanks 🎉

[timschumi]

I now added the changes for converting nfs_client to seperate commands (when a command using sudo() fails, it indeed aborts the provisioning) and the changes to change_host_name that have been previously discussed.

However, I'm still conflicted on what to do with the /etc/hosts updating. Most of the other guest plugins seem to not care and just prepend the new entry to the beginning of the file unless grepping for the new hostname comes up positive (in which case it simply does nothing). Should we do the same here, or rather keep the current logic? If we keep the current logic, I think the PR should be ready to be reviewed, since I can't really find anything else that is in need of a fixup.

[briancain]

Thanks again for taking the time to make a pull request! I've done some basic testing and everything seems to be working. Also thanks @maier for writing the original plugin! 🎉

[ladar]

Glad to hear this is finally making it into the official release. Two questions, when should I migrate the roboxes from the :alt workaround to using :alpine? I have no idea how quickly a new version of vagrant proliferates, and thus no idea when it will be "safe" to require this plugin by default.

Thus my second question: in the interim is it possible to put conditional logic in a Vagrantfile based on whether the version is >= 2.2.6? If the problem can be detected, and the Vagrantfile can fall back to using :alt it will be a smoother transition. A less user friendly option would be printing an error message which tells the user vagrant version 2.2.6 or above is required to use the image. If the former is possible, I'll make the change now. If only the latter is possible, it would make sense to wait for the new version to proliferate, but with a lower tipping point.

If it makes sense to keep using :alt for awhile, then perhaps pull request #11000 can still be helpful.

Thoughts?

[timschumi]

There seems to be a way to require a specific Vagrant version in the Vagrantfile (example from Valve's Proton), as well as retrieving the Vagrant version and doing stuff with that (i.e. if/else, etc.).

However, determinig if this extends to the Vagrantfiles used during box creation (i.e. if this affects the builder or the user) still needs to be subject to testing though (which I might be able to do later).

[ladar]

Also, I haven't looked at the code, but if the plugin requires APKs for shared folders to work, that aren't currently part of the default package set, then it seems logical to include them, whenever I build the images for use with vagrant. That way everything still works, even if the box can't download/install packages. I prefer to remove potential pitfalls, if I can.

[ladar]

There seems to be a way to require a specific Vagrant version in the Vagrantfile (example from Valve's Proton), as well as retrieving the Vagrant version and doing stuff with that (i.e. if/else, etc.).

I prefer to keep my Vagrantfiles rather simple, which is why I started building custom images with packer in the first place. I don't really know what's possible with a Vagrantfile, versus what requires a plugin, nor have I been able to find any documentation on what safeguards (if any) vagrant places on the bundled Vagrantfile. If you know where that is, please point me at it, as its actually been a concern of mine. Are there nefarious images floating around with bundled Vagrantfiles which do nasty things to the host?

I was hoping @briancain might provide some insight. Generally speaking, this seems like a good approach:

if Vagrant::VERSION < '2.2.6'
  $stderr.puts "ERROR: This box requires Vagrant version 2.2.6 or higher. Please upgrade and retry.\n"
  exit 1
end

Then it just becomes a question of what proliferation threshold to wait for, and how long it usually takes to reach it? With vagrant getting added to official repos for various distros, I suspect the tail could be getting longer.

[timschumi]

Also, I haven't looked at the code, but if the plugin requires APKs for shared folders to work, that aren't currently part of the default package set, then it seems logical to include them, whenever I build the images for use with vagrant. That way everything still works, even if the box can't download/install packages. I prefer to remove potential pitfalls, if I can.

In my opinion, one big advantage of the capabilities system is to only install/enable stuff when needed. This is a plus especially when talking about something like Alpine Linux, which is focused on being lean/lightweight and having only the bare stuff running. I'd recommend to not install NFS (or other addons) by default and let the guest plugins handle that. In the particular case of NFS it would be broken anyways, since the guest plugin isn't able to recognize that NFS is installed and it would try to install anyways.

I was hoping @briancain might provide some insight. Generally speaking, this seems like a good approach:

if Vagrant::VERSION < '2.2.6'
  $stderr.puts "ERROR: This box requires Vagrant version 2.2.6 or higher. Please upgrade and retry.\n"
  exit 1
end

Then it just becomes a question of what proliferation threshold to wait for, and how long it usually takes to reach it? With vagrant getting added to official repos for various distros, I suspect the tail could be getting longer.

I have done some testing and it appears that the Vagrantfile is indeed executed on the user's machine. This makes something like the following possible:

diff --git a/tpl/generic-alpine39.rb b/tpl/generic-alpine39.rb
index 56d7098..a46f1f7 100644
--- a/tpl/generic-alpine39.rb
+++ b/tpl/generic-alpine39.rb
@@ -32,7 +32,11 @@ Vagrant.configure(2) do |config|
     v.memory = 2048
     v.driver = "kvm"
     v.video_vram = 256
-    override.vm.guest = :alt
+    if Vagrant::VERSION < '2.2.6' && !Vagrant.has_plugin?("vagrant-alpine")
+      $stdout.puts "WARNING: Setting OS type to 'ALT Linux' as a workaround on libvirt hosts. This might break guest OS specific features.\n"
+      $stdout.puts "Please upgrade to Vagrant 2.2.6 or install the 'vagrant-alpine' plugin to use the proper Alpine Linux guest capabilities.\n"
+      override.vm.guest = :alt
+    end
     v.channel :type => 'unix', :target_name => 'org.qemu.guest_agent.0', :target_type => 'virtio'
   end

This only applies the :alt workaround when the user doesn't have Vagrant 2.2.6 or higher (i.e. Alpine Linux guest support isn't built-in) and the 'vagrant-alpine' plugin isn't installed. That way it doesn't hurt either if we keep the workaround forever, since it's only active when needed.

Another way would obviously be to finally make the jump and require the vagrant-alpine plugin for everyone who doesn't have built-in Alpine support (which would result in less configurations that need support), but that's ultimately your decision.

The warning message should obviously still be subject to discussion (do we even need one? It at least makes sense to point the user to something if issues arise). Also, the message is printed 4 or 5 times consecutively, I assume because the file is loaded multiple times. I'll search for a "proper" logging command that may circumvent that.

If you are fine with an approach like this, I could open a PR that adds those checks to robox-alpine boxes, so that we can track progress there.

[briancain]

Glad to hear this is finally making it into the official release. Two questions, when should I migrate the roboxes from the :alt workaround to using :alpine? I have no idea how quickly a new version of vagrant proliferates, and thus no idea when it will be "safe" to require this plugin by default.

Generally our turn around is a few weeks. We just put out a release not too long ago, so I think probably within the next week or two we should cut another? I'm happy to make sure the alpine guest works prior to release of course, since this was just recently added. That being said I did a bunch of basic smoke testing with the recently merged guest plugin and it seemed to work fine for me (i.e. an nfs folder, rsync folder, some private networks, a provisioner, etc).

Thus my second question: in the interim is it possible to put conditional logic in a Vagrantfile based on whether the version is >= 2.2.6? If the problem can be detected, and the Vagrantfile can fall back to using :alt it will be a smoother transition. A less user friendly option would be printing an error message which tells the user vagrant version 2.2.6 or above is required to use the image. If the former is possible, I'll make the change now. If only the latter is possible, it would make sense to wait for the new version to proliferate, but with a lower tipping point.

Yep...as mentioned already you could place this inside your boxes packaged Vagrantfile at the top and it will be executed when a user downloads and brings up the box:

Vagrant.require_version("<= 2.2.6")

If it makes sense to keep using :alt for awhile, then perhaps pull request #11000 can still be helpful.

Thoughts?

Saw the PR! I will give a more formal review here soon, but overall I'm 👍 since it's mostly just adding some extra safe guards and print statements. I plan on giving a better review though on the PR itself.

Then it just becomes a question of what proliferation threshold to wait for, and how long it usually takes to reach it? With vagrant getting added to official repos for various distros, I suspect the tail could be getting longer.

Unfortunately we don't have much control over when OS distros update their vagrant package :( We encourage people to download the official package instead from vagrantup.com, but yeah, the packages inside OS repos will often be far behind the latest version.

Also, I haven't looked at the code, but if the plugin requires APKs for shared folders to work, that aren't currently part of the default package set, then it seems logical to include them, whenever I build the images for use with vagrant. That way everything still works, even if the box can't download/install packages. I prefer to remove potential pitfalls, if I can.

Interesting...so I actually used the generic boxes to test this PR. It was able to set up and install nfs and rsync with what I assume was the apk tool, since that's what is used for the recently merged alpine guest plugin:

brian@localghost:vagrant-sandbox % be vagrant ssh alpine                                                 ±[●][master]
alpine:~$ apk
apk-tools 2.10.1, compiled for x86_64.

...
... # removed help text for github
...

This apk has coffee making abilities.
alpine:~$ which apk
/sbin/apk

So it seems like we can use apk out of the box with the generic boxes?

This only applies the :alt workaround when the user doesn't have Vagrant 2.2.6 or higher (i.e. Alpine Linux guest support isn't built-in) and the 'vagrant-alpine' plugin isn't installed. That way it doesn't hurt either if we keep the workaround forever, since it's only active when needed.

@timarenz - Yeah...ideally we shouldn't be printing like that since as you mentioned, it will print multiple times as Vagrant parses the Vagrantfile. You might be able to use the built in logger that Vagrant uses, but I think you'll run into the same duplicate message/parsing issue. Maybe instead it could use Vagrants internal logger and print to the DEBUG level rather than INFO?

I do think what @timschumi seems like a good approach for a generic/alpine boxes Vagrantfile. If Vagrant is older than the next release and they aren't using the alpine plugin, switch back to using :alt.

[timschumi]

So it seems like we can use apk out of the box with the generic boxes?

If I understood correctly, the question is whether the necessary packages for NFS (or other things) should be installed on the box by default (not if the APK tool itself is usable), so that the user doesn't have to rely on the plugin being available for NFS to work. I personally am against it, I listed my reasons for that above.

timarenz

Wrong mention (I guess?) ;-)

Yeah...ideally we shouldn't be printing like that since as you mentioned, it will print multiple times as Vagrant parses the Vagrantfile. You might be able to use the built in logger that Vagrant uses, but I think you'll run into the same duplicate message/parsing issue. Maybe instead it could use Vagrants internal logger and print to the DEBUG level rather than INFO?

In my opinion, from an UX point of view the message should probably be visible by default, since manually overriding the guest type to something that doesn't immediately make sense (and especially since it might cause some weird errors down the line) might confuse users. Pointing out that we are using a workaround that might cause weird errors is probably quite beneficial to the user.

[briancain]

So it seems like we can use apk out of the box with the generic boxes?

If I understood correctly, the question is whether the necessary packages for NFS (or other things) should be installed on the box by default (not if the APK tool itself is usable), so that the user doesn't have to rely on the plugin being available for NFS to work. I personally am against it, I listed my reasons for that above.

Oh, I see. I understand that. I believe some guests already have this "install" behavior. I thought the guest capability would check to see if NFS is installed, and if not, go ahead and run this capability. So then other guests that don't have it packaged in are still functional. That to me seems pretty reasonable assuming that's how the alpine guest works. Then if the generic boxes do have nfs, this capability won't be called ever. Same with rsync and smb.

timarenz

Wrong mention (I guess?) ;-)

Hah, nope! I meant you :) In a response to the diff you posted for the warning.

Yeah...ideally we shouldn't be printing like that since as you mentioned, it will print multiple times as Vagrant parses the Vagrantfile. You might be able to use the built in logger that Vagrant uses, but I think you'll run into the same duplicate message/parsing issue. Maybe instead it could use Vagrants internal logger and print to the DEBUG level rather than INFO?

In my opinion, from an UX point of view the message should probably be visible by default, since manually overriding the guest type to something that doesn't immediately make sense (and especially since it might cause some weird errors down the line) might confuse users. Pointing out that we are using a workaround that might cause weird errors is probably quite beneficial to the user.

Yep, agree. So thinking out loud here, one possible solution is for the generic boxes to have something like this...it's a little bit of a hacky workaround since there's no proper way for Vagrantfiles to actually "log" like how logging works in Vagrant core, but I think we'd get the same result for a warning to users when the :alt guest cap is used:

# warning: code not tested, just an example for github!
if Vagrant.version?("< 2.2.6") && !Vagrant.has_plugin?("vagrant-alpine")
   override.vm.guest = :alt
   config.vm.trigger :before :all do |t|
     t.warn = "The right warning string that you choose to put for this situation... :D :D"
   end
end

[timschumi]

I thought the guest capability would check to see if NFS is installed, and if not, go ahead and run this capability.

For that, we'd need the nfs_installed (not nfs_install) capability (same probably goes for the other protocols), which isn't implemented as of right now. Without that, it will always try to install the necessary programs (which doesn't break anything though, since it doesn't error if we try to install packages that are already installed). I'll take a look later anyways and try to implement that.

So then other guests that don't have it packaged in are still functional. That to me seems pretty reasonable assuming that's how the alpine guest works. Then if the generic boxes do have nfs, this capability won't be called ever. Same with rsync and smb.

I just wanted to avoid bloating the Alpine image out of the box, since NFS support can be handled automatically by the plugin as well (obviously only when the plugin/built-in alpine support is available). And without the alpine guest type it doesn't know how to check if NFS is supported (resulting in it trying to install NFS no matter if it's already installed), so it will either fail due to lack of installation commands, or because it picks the wrong set of commands.

timarenz

Wrong mention (I guess?) ;-)

Hah, nope! I meant you :) In a response to the diff you posted for the warning.

I just wanted to point out that you typed (or I assume auto-completed) the wrong username accidentally.

Yep, agree. So thinking out loud here, one possible solution is for the generic boxes to have something like this...it's a little bit of a hacky workaround since there's no proper way for Vagrantfiles to actually "log" like how logging works in Vagrant core, but I think we'd get the same result for a warning to users when the :alt guest cap is used:

Thanks for the code snippet, I'll try that later. I was pretty sure though that I have seen a one-line log command that worked properly, but I can't find it right now.

[ladar]

In my opinion, one big advantage of the capabilities system is to only install/enable stuff when needed. This is a plus especially when talking about something like Alpine Linux, which is focused on being lean/lightweight and having only the bare stuff running. I'd recommend to not install NFS (or other addons) by default and let the guest plugins handle that. In the particular case of NFS it would be broken anyways, since the guest plugin isn't able to recognize that NFS is installed and it would try to install anyways.

It's a fair point. But one of the reasons I started releasing "generic" versions of my box images was because I found the "official" boxes to be so focused on staying small and lean, that they sacrificed reliability and usability. In other words scripts, commands, packages that were normally available by default, when the distro was installed installed via an ISO, would be missing from the official distro images on Vagrant Cloud. With the Roboxes I try to focus on creating "sane" base box images that just work. And I'm willing to sacrifice a little when it comes to size if it means the image will just work by default.Put another way, I'd rather add a few (hopefully small) packages, than see GitHub issues down the road. That said, I don't like opening up the default attack surface with running services that might not be needed. If the packages are installed, but the services disabled by default, I presume the plugin will be able to see that and enable them, even if the box doesn't have internet access?

I have done some testing and it appears that the Vagrantfile is indeed executed on the user's machine. This makes something like the following possible: |diff --git a/tpl/generic-alpine39.rb b/tpl/generic-alpine39.rb index 56d7098..a46f1f7 100644 --- a/tpl/generic-alpine39.rb +++ b/tpl/generic-alpine39.rb @@ -32,7 +32,11 @@ Vagrant.configure(2) do |config| v.memory = 2048 v.driver = "kvm" v.video_vram = 256 - override.vm.guest = :alt + if Vagrant::VERSION < '2.2.6' && !Vagrant.has_plugin?("vagrant-alpine") + $stdout.puts "WARNING: Setting OS type to 'ALT Linux' as a workaround on libvirt hosts. This might break guest OS specific features.\n" + $stdout.puts "Please upgrade to Vagrant 2.2.6 or install the 'vagrant-alpine' plugin to use the proper Alpine Linux guest capabilities.\n" + override.vm.guest = :alt + end v.channel :type => 'unix', :target_name => 'org.qemu.guest_agent.0', :target_type => 'virtio' end | This only applies the |:alt| workaround when the user doesn't have Vagrant 2.2.6 or higher (i.e. Alpine Linux guest support isn't built-in) /and/ the 'vagrant-alpine' plugin isn't installed. That way it doesn't hurt either if we keep the workaround forever, since it's only active when needed.

I like this option. The only question is whether we should suggest the plugin route over simply suggesting the user upgrade. Or at the very least, indicate the plugin should only be used if upgrading isn't an option? I'm wondering what will happen to users with the `vagrant-alpine` plugin installed when they upgrade to 2.2.6, will it be disabled automatically? Or will the newly packaged Alpine handler override the plugin?

Another way would obviously be to finally make the jump and require the vagrant-alpine plugin for everyone who doesn't have built-in Alpine support (which would result in less configurations that need support), but that's ultimately your decision.

I'm willing to consider that for the next Alpine version, aka `alpine311` or `alpine40`, but would rather not release an updated box image, which triggers an auto-update, and causes someone's configs (like my own), to suddenly stop working as a result of the new dependency. Before we make them jump though, we should take a look at which distros offer binary vagrant packages, and what version is available. For example, I believe Fedora and Ubuntu have started offering official packages, but I have no idea how closely they track the most current upstream version. Those users might get frustrated by this requirement, particularly if they're using one of those fancy new immutable host images which make updating/installing packages rather difficult.

The warning message should obviously still be subject to discussion (do we even need one? It at least makes sense to point the user to something if issues arise). Also, the message is printed 4 or 5 times consecutively, I assume because the file is loaded multiple times. I'll search for a "proper" logging command that may circumvent that.

You might be right. If the fall through to using `:alt` works, no warning is needed. At least for awhile. Especially if it prints out multiple times.

If you are fine with an approach like this, I could open a PR that adds those checks to robox-alpine boxes, so that we can track progress there.

Yes, I think this is the right strategy. Go ahead and submit a PR, but no rush, it doesn't make sense to merge until version 2.2.6 is actually released. Also, aren't we going to start using `:alpine` for all of the provider types once that happens? I believe the other providers currently use `:linux` (aka the default) and it's only the libvirt plugin which uses `:alt`?

[timschumi]

@ladar It appears that the generic linux host capability is able to detect if nfs-utils are already installed. Therefore, if the package is preinstalled on the box, users could use NFS without the alpine plugin (either built-in or external). But again, I'm not sure if that's worth it, looking at the number of background processes and open ports.

[timschumi]

Sorry, I only saw your comment now. The comment above was meant as an update to my comment from a few days ago.

That said, I don't like opening up the default attack surface with running services that might not be needed. If the packages are installed, but the services disabled by default, I presume the plugin will be able to see that and enable them, even if the box doesn't have internet access?

At this point in time, the capability (talking about the generic :linux one here) only checks if the mount.nfs binary is present. It doesn't check if the service is enabled. Therefore, NFS wouldn't be installed/enabled automatically if the package is installed but the service is not enabled.

We could solve this by implementing our own nfs_client_installed capability in the alpine plugin (which would check for the service as well), but that doesn't help the question on whether preinstalling NFS will help users that don't have the plugin in either form.

I like this option. The only question is whether we should suggest the plugin route over simply suggesting the user upgrade. Or at the very least, indicate the plugin should only be used if upgrading isn't an option?

Installing the plugin is probably easier for most users, but the built-in plugin will be supported officially and is potentially updated more often in the future. Combined with the fact that the plugin will be used even after a user upgrades to Vagrant 2.2.6, I'd recommend to tell the user that they should use the upgraded Vagrant version if possible and only fall back to the plugin if upgrading Vagrant isn't possible.

I'm wondering what will happen to users with the vagrant-alpine plugin installed when they upgrade to 2.2.6, will it be disabled automatically? Or will the newly packaged Alpine handler override the plugin?

I did some light testing, and it seems like the plugin variant is preferred over the built-in one (which makes sense, since plugins could then be used to update built-in modules). I doubt that this is going to be a problem, but if it proves that it is, we could ask maier to add a warning or error message that displays if the plugin is used on Vagrant 2.2.6 or higher.

You might be right. If the fall through to using :alt works, no warning is needed. At least for awhile. Especially if it prints out multiple times.

The suggestion with triggers in the Vagrantfile works, it now only prints the message once.

Yes, I think this is the right strategy. Go ahead and submit a PR, but no rush, it doesn't make sense to merge until version 2.2.6 is actually released.

Will do.

Also, aren't we going to start using :alpine for all of the provider types once that happens? I believe the other providers currently use :linux (aka the default) and it's only the libvirt plugin which uses :alt?

The other provider types should be covered just fine by the guest OS autodetect (which should pick alpine automatically). It was only a problem on the libvirt boxes, because overriding the guest type manually disables the autodetect.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.