cloud.cfg.tmpl: add a key to manage fallback network config

[sshedi]

Currently cloud-init generates fallback network config on various
scenarios.

For example:

1. When no DS found
2. There is no 'network' info given in DS metadata.
3. If a DS gives a network config once and upon reboot if DS doesn't
   give any network info, previously set network data will be
   overridden.

This newly introduced key can be used to control this behaviour.

Also, if OS comes with a set of default network files(configs), like in
PhotonOS, cloud-init should not overwrite them by default.

Signed-off-by: Shreenidhi Shedi sshedi@vmware.com

Proposed Commit Message

cloud.cfg.tmpl: add a key to manage fallback network config

Currently cloud-init generates fallback network config on various
scenarios.

For example:
1. When no DS found
2. There is no 'network' info given in DS metadata and so on.
3. If a DS gives a network config once and upon reboot if DS doesn't
   give any network info, previously set network data will be
   overridden.

This newly introduced key can be used to control this behaviour.

Also, if OS comes with a set of default network files(configs), like in
PhotonOS, cloud-init should not overwrite them by default.

Additional Context

Test Steps

Checklist:

• My code follows the process laid out in the documentation
• [] I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[blackboxsw]

Thank you for the PR @sshedi.

Given that this feels like a corner-case behavior for Photo distribution that is undesirable for most other distros I don't think we want to surface this as a default config option to be set up in /etc/cloud/cloud.cfg for all distributions.

I think this should more tightly coupled to the distro definitions where photon.Distro can override default generate_fallback_config behavior.`

Since your PR looks like it wants Photon to always do nothing when generating fallback config I think your patch could look more like this without any other public-facing config options right?

If this patch makes sense for your needs, please add unittests exercising photon's generate_fallback_config.

diff --git a/cloudinit/distros/photon.py b/cloudinit/distros/photon.py
index 0ced7b5f..20585ec9 100644
--- a/cloudinit/distros/photon.py
+++ b/cloudinit/distros/photon.py
@@ -72,6 +72,13 @@ class Distro(distros.Distro):
         cmd = ['systemctl', 'restart', 'systemd-localed']
         _ret, _out, _err = self.exec_cmd(cmd)
 
+    def generate_fallback_config(self):
+        LOG.debug(
+            'Skipping generate_fallback_config. Rely on PhotonOS default'
+            ' network config'
+        )
+        return None
+
     def install_packages(self, pkglist):
         # self.update_package_sources()
         self.package_command('install', pkgs=pkglist)

[sshedi]

Thank you for the PR @sshedi.

Given that this feels like a corner-case behavior for Photo distribution that is undesirable for most other distros I don't think we want to surface this as a default config option to be set up in /etc/cloud/cloud.cfg for all distributions.

I think this should more tightly coupled to the distro definitions where photon.Distro can override default generate_fallback_config behavior.`

Since your PR looks like it wants Photon to always do nothing when generating fallback config I think your patch could look more like this without any other public-facing config options right?

If this patch makes sense for your needs, please add unittests exercising photon's generate_fallback_config.

diff --git a/cloudinit/distros/photon.py b/cloudinit/distros/photon.py
index 0ced7b5f..20585ec9 100644
--- a/cloudinit/distros/photon.py
+++ b/cloudinit/distros/photon.py
@@ -72,6 +72,13 @@ class Distro(distros.Distro):
         cmd = ['systemctl', 'restart', 'systemd-localed']
         _ret, _out, _err = self.exec_cmd(cmd)
 
+    def generate_fallback_config(self):
+        LOG.debug(
+            'Skipping generate_fallback_config. Rely on PhotonOS default'
+            ' network config'
+        )
+        return None
+
     def install_packages(self, pkglist):
         # self.update_package_sources()
         self.package_command('install', pkgs=pkglist)

I understand. This works, however I want this to be a controllable operation based on user's need.
Shall I keep the config key in cloud.cfg.tmpl just for Photon?

[sshedi]

@blackboxsw can you please review my changes once you get time? I think I have addressed your concerns.

[blackboxsw]

Thanks again @sshedi for continued work on this. Please clarify if my assumptions are correct. I'm trying to walk through this logically to understand how this could work.

I'm also fairly certain that network configuration can't be overridden by userdata because it is processed too late. So I don't expect #cloud-config\ngenerate_fallback_config: true to be able to override what you have on disk. I think the only way to override your default behavior for PhotonOS would be for someone to generate their own custom PhotonOS image with a file in /etc/cloud/cloud.cfg overriding "sys_config: generate_fallback_config: true".

Is there any way within VMware's products for an image launch to customize kernel params provided to the instance being launched? If network-config:disabled is added the the kernel commandline for default PhotonOS launches then PhotonOS deployments would generally not setup networking. If custom configuration provided network-config, then VMware could avoid disabling network config via kernel cmdline params?

That said, if you still want to make rendering fallback configuration optional (even without being able to override via userdata) I can see us adding this feature, but we need to find a home for some documentation around it.
And bikeshedding the name a bit to use the same naming convention for other flags, let's use a "disable_" prefix I'd be looking for a diff like this

diff --git a/cloudinit/distros/photon.py b/cloudinit/distros/photon.py
index 1a55b353..561c77d8 100644
--- a/cloudinit/distros/photon.py
+++ b/cloudinit/distros/photon.py
@@ -56,18 +56,17 @@ class Distro(distros.Distro):
             return False, None, None
 
     def generate_fallback_config(self):
-        key = 'use_fallback_netcfg'
-        use_fallback_netcfg = self._cfg.get(key, False)
-        LOG.debug('%s value is: %s', key, use_fallback_netcfg)
-
-        if use_fallback_netcfg:
-            return net.generate_fallback_config()
-
-        LOG.info(
-            'Skipping generate_fallback_config. Rely on PhotonOS default '
-            'network config'
-        )
-        return None
+        key = 'disable_fallback_config'
+        disable_fallback_netcfg = self._cfg.get(key, True)
+        LOG.debug('%s value is: %s', key, disable_fallback_netcfg)
+
+        if disable_fallback_netcfg:
+            LOG.info(
+                'Skipping generate_fallback_config. Rely on PhotonOS default '
+                'network config'
+            )
+            return None
+        return net.generate_fallback_config()
 
     def apply_locale(self, locale, out_fn=None):
         # This has a dependancy on glibc-i18n, user need to manually install it
diff --git a/config/cloud.cfg.tmpl b/config/cloud.cfg.tmpl
index da6446cc..bdfc1f82 100644
--- a/config/cloud.cfg.tmpl
+++ b/config/cloud.cfg.tmpl
@@ -312,10 +312,10 @@ system_info:
 
    ssh_svcname: sshd
 
-   # If set to true, cloud-init will create fallback netcfg
+   # If set to false, cloud-init will create fallback netcfg
    # In Photon, we have default network settings already, hence if network
    # settings are not explicitly given in metadata, don't use fallback netcfg.
-   use_fallback_netcfg: false
+   disable_fallback_netcfg: true
 {% endif %}
 {% if variant in ["freebsd", "netbsd", "openbsd"] %}
    network:
diff --git a/doc/rtd/topics/availability.rst b/doc/rtd/topics/availability.rst
index a45a49d6..b84b6076 100644
--- a/doc/rtd/topics/availability.rst
+++ b/doc/rtd/topics/availability.rst
@@ -26,6 +26,7 @@ OpenBSD and DragonFlyBSD:
 - Gentoo Linux
 - NetBSD
 - OpenBSD
+- Photon OS
 - RHEL/CentOS
 - SLES/openSUSE
 - Ubuntu
diff --git a/doc/rtd/topics/network-config.rst b/doc/rtd/topics/network-config.rst
index 5f7a74f8..34c42f6f 100644
--- a/doc/rtd/topics/network-config.rst
+++ b/doc/rtd/topics/network-config.rst
@@ -104,6 +104,7 @@ interface given the information it has available.
 Finally after selecting the "right" interface, a configuration is
 generated and applied to the system.
 
+**Note:** PhotonOS disables fallback networking configuration by default leaving network unrendered when no other network config is provided. If fallback config is still desired on PhotonOS, it can be enabled by providing `disable_fallback_netcfg: false` in either `#cloud-config` or `/etc/cloud/cloud.cfg:sys_config` settings.
 
 Network Configuration Sources
 =============================

[sshedi]

I'm also fairly certain that network configuration can't be overridden by userdata because it is processed too late. So I don't expect #cloud-config\ngenerate_fallback_config: true to be able to override what you have on disk. I think the only way to override your default behavior for PhotonOS would be for someone to generate their own custom PhotonOS image with a file in /etc/cloud/cloud.cfg overriding "sys_config: generate_fallback_config: true".

Thanks for your valuable insights on this PR @blackboxsw really learning good things here.
Yes, that's the whole idea. We don't want to use fallback network config.
We are currently using network: {config: disabled} in a cfg file under /etc/cloud/cloud.cfg.d so by default all network changes are disabled otherwise, cloud-init overwrites network config with fallback config on subsequent boots.

This will make things a bit better. If user wants fallback data feature, they can generate a custom OVA image with the changes you mentioned above but majorly we expect network settings to come from metadata of DS, if there is no network config in metadata, we rely on our own network files.

And no problem. I will rename the configuration setting name and thanks for the help with documentation and overall implementation.

Previously I had a really nice experience working with @TheRealFalcon and now with you. So far from my little experience in open-source, cloud-init maintainers are really welcoming and helpful. Feels good to work with you guys.

[sshedi]

@TheRealFalcon @blackboxsw - can you please review and merge this PR?

[TheRealFalcon]

Just one small update to docs.

[TheRealFalcon]

Thanks! LGTM now. The integration test failure is unrelated (I've seen it once before), so I'll restart the job and we should be good to go.

Comments have been addressed

[sshedi]

Thanks! LGTM now. The integration test failure is unrelated (I've seen it once before), so I'll restart the job and we should be good to go.

Thanks a lot :) have a nice time ahead.