-
Notifications
You must be signed in to change notification settings - Fork 822
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
Add update_etc_hosts as default module on *BSD #479
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last time i looked over this, #385 i only focused on configuration management, and didn't look closer at other modules
could you also provide us with the log of a boot like in that pr?
Here is boot log (
Here is content of
It match to template of |
Here's the current of FreeBSD's note the preference of IPv6 and IPv4 is now reversed. unrelatedly, what's up with those |
Yes, I will update source branch. I would like to point out that my knowledge of the FreeBSD environment is limited and I am asking for limited confidence.
Mount errors is regarding datasource discovery here: https://github.com/canonical/cloud-init/blob/master/cloudinit/sources/DataSourceRbxCloud.py#L81-L85 I works for RbxCloud and will be happy to optimize the process if you have suggestions. |
https://github.com/NetBSD/src/blob/HEAD/etc/hosts ← NetBSD is the same
nah, let's leave this for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost good to go
I have no experience in NetBSD. I fully trust your comments. |
config/cloud.cfg.tmpl
Outdated
- update_etc_hosts | ||
{% if variant.endswith("bsd") %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the logic here needs inverting (to match its previous state); we should emit these lines only if we are not on BSD, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ad-m It was only this line that needed updating, to match the previous meaning. We are generalising variant not in [...]
so this should be:
{% if variant.endswith("bsd") %} | |
{% if not variant.endswith("bsd") %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at this! I have one inline comment.
Co-authored-by: Mina Galić (deprecated: Igor Galić) <me+github@igalic.co>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs work; I've tried rephrasing my suggestion(s), let me know if you need further clarification!
config/cloud.cfg.tmpl
Outdated
- update_etc_hosts | ||
{% if variant.endswith("bsd") %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ad-m It was only this line that needed updating, to match the previous meaning. We are generalising variant not in [...]
so this should be:
{% if variant.endswith("bsd") %} | |
{% if not variant.endswith("bsd") %} |
config/cloud.cfg.tmpl
Outdated
@@ -33,7 +33,7 @@ ssh_pwauth: 0 | |||
# This will cause the set+update hostname module to not operate (if true) | |||
preserve_hostname: false | |||
|
|||
{% if variant.endswith("bsd") %} | |||
{% if not variant.endswith("bsd") %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect; this stanza is only present to workaround issues seen on BSDs, so it should only be emitted under the condition that was already here.
(If the issue you were fixing was that RbxCloud
wasn't being found on *BSD, then you'll want to add it to the datasource_list
that this if
clause guards.)
@OddBloke, Sorry, I corrected the wrong line quickly from the editor. When I saw error , you were able to publish post with new comments before I pushed the rebased changes. |
No worries, thanks for the updates! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM now, thank you! @igalic, are you happy with this now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💜
If I understand correctly, Cloud-init adopts the default configuration to run all supported modules on a given distro. For unknown reasons, the update_etc_hosts module does not run on FreeBSD and NetBSD, although:
update_etc_hosts
has sense and retains functionality,update_etc_hosts
is working on the platform, in accordance with its documentation and manual tests.In our environment, we assume that all images should have
/etc/hosts
file managed by default. FreeBSD is an exception due default Cloud-init config.According Git history @goneri provided recently support for NetBSD and follow that configuration, so I ask him for review.
This distinction has been made to this configuration file in a change 41d46bf but that patch follow earlier configuration file
https://github.com/canonical/cloud-init/blob/e7c95208451422cfd3da7edfbd67dd271e7aa337/config/cloud.cfg-freebsd which turned it off in the patch c36d755. However, assumptions behind disable this module no longer seem valid.