Merge ~t0rrant/cloud-init:1819966-sysconfig-options into cloud-init:master

Proposed by Manuel Torrinha
Status: Work in progress
Proposed branch: ~t0rrant/cloud-init:1819966-sysconfig-options
Merge into: cloud-init:master
Diff against target: 145 lines (+139/-0)
1 file modified
cloudinit/config/cc_sysconfig.py (+139/-0)
Reviewer Review Type Date Requested Status
cloud-init Commiters Pending
Review via email: mp+371948@code.launchpad.net

Commit message

Added support for arbitrary options in sysconfig

These options should be added within the `sysconfig` key, as such:

```
network:
  version: 2
  sysconfig:
    NTPSERVERARGS: "minpoll 3 maxpoll 4"
    RES_OPTIONS: "rotate"
```

LP: #1819966
Signed-off-by: Manuel Torrinha <email address hidden>

Description of the change

Added a new handle (handle_sysconfig) to deal with the new key.

Using the options passed in the cloud-init config file a /etc/sysconfig/network file is generated contemplating those options.

To post a comment you must log in.
Revision history for this message
Manuel Torrinha (t0rrant) wrote :

I do not feel comfortable with having a dummy handler for this, I guess
the purpose of the `network` key is exclusive for network interfaces and
network interface interaction. Having a handle_ function is perhaps not
the best choice.

For now this does what is intended, will eventually discuss this with the
cloud-init team and maybe this will be done in some other way.

On a final note, when running tox tests I get several errors/warnings, however:

```
...
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)
```

Revision history for this message
Ryan Harper (raharper) wrote :

Hi Manual,

Thanks for creating a potential fix. The various network-config formats that cloud-init handles don't lend themselves to writing arbitrary configuration values to renderer specific files, in your case, the NTPSERVERARGS is a sysconfig renderer specific value that doesn't have an equivalent in other backends, such as eni (etc/network/interfaces) nor netplan.

To address the bug I think the best option is use cloud-init write_files config module to append the string you're interested in.

#cloud-config
write_files:
  - content: |
        NTPSERVERARGS="minpoll 3 maxpoll 3"
        RES_OPTIONS="rotate"
    path: /etc/sysconfig/network
    append: true

There is also the cc_ntp module, which controls enabling ntp service, and configuring
but this would only cover your ntp configuration using a custom template.

#cloud-config
ntp:
  enabled: true
  config:
     template: |
         ## template:jinja
         # My NTP Client config
         {% if pools -%}# pools{% endif %}
         {% for pool in pools -%}
         pool {{pool}} iburst minpoll 3 maxpoll 4
         {% endfor %}
         {%- if servers %}# servers
         {% endif %}
         {% for server in servers -%}
         server {{server}} iburst
         {% endfor %}

Revision history for this message
Brendan Germain (bgermain1689) wrote :

does the cc_ntp module work alongside dhcp? what if your ntp servers are obtained via dhcp?

Revision history for this message
Manuel Torrinha (t0rrant) wrote :

Hi,

@Ryan: the `NTPSERVERARGS` was just an example from the original issue #1819966, what got me there in the first place was that when I wrote anything to `/etc/sysconfig/network` it was replaced by static content generated by cloud-init's `render_network_state` function on reboot.
From what I can see in the `cc_write_files` module, using that to generate the `/etc/sysconfig/network` file will only work on instance creation (default `frequency = PER_INSTANCE` for that module), and only if it comes before the network handlers. If you look into `cloudinit/net/sysconfig.py` the original code uses `util.write_file` method which overwrites the contents of the parameter file by default.

@Brendan: yes the ntp servers are obtained via DHCP and that is not an issue here, just the case that we may want persistent controllable settings independent of the DHCP data received.

At the time there is not really a clean way to configure the `/etc/sysconfig/network` with arbitrary options, which I guess is desirable.

The rationale for this is to be able to specify persistent network options that are not obtainable from DHCP (i.e: resolver option `rotate`), either because the information received from the DHCP server is incomplete, there are independent networks for each of my NICs thus one DHCP information taking priority on the other, because you may not have in control of what information in send from the DHCP server, or even just because you want to =)

That said, is there a better suited place in the code to add this functionality?

Cheers!

Revision history for this message
Ryan Harper (raharper) wrote :

There is no direct integration in the module for dhcp provided ntp servers at this time. It's possible but there isn't a great interface (AFAIK) to query your OS for the dhcp response for a given interface.

Revision history for this message
Manuel Torrinha (t0rrant) wrote :

Hi Ryan,

> There is no direct integration in the module for dhcp provided ntp servers at
> this time. It's possible but there isn't a great interface (AFAIK) to query
> your OS for the dhcp response for a given interface.

sorry but I don't understand your comment. The proposed change has nothing to do with what the OS stores from a DHCP response, neither with NTP specifically, in fact the point here is being able to have options defined which cannot come from a DHCP response.

The related bug may be misleading, but the OP in the related bug just wants to permanently define NTP options in the `/etc/sysconfig/network` file, not save options from a DHCP response.

Cheers!

Revision history for this message
Ryan Harper (raharper) wrote :

The best way to update the /etc/sysconfig/network contents with custom values is via the write_files in append mode.

A more robust solution would be to implement a config module: cc_sysconfig.py which could read in existing files from /etc/sysconfig, parse them with util.load_shell_content() which returns a dict of key=values; and let users provide new key/value:

sysconfig:
   network:
       mode: append # <update|append|overwrite>
       content:
  NTPSERVERARGS: "minpoll 3 maxpoll 3"
  RES_OPTION: "rotate"

Append mode would merely append the KEY=VALUE pairs from the content dictionary in the file specified (/etc/sysconfig/network).

Update would replace the existing KEY with the new value, if KEY wasn't present, it would append it.

overwrite would allow writing a completely new file with the KEY/VALUE pairs provided.

Some challenges include retaining existing comments, indicating where cloud-init made changes within the file, handling interactions from cloudinit.net whose's renderers currently update/write values.

Revision history for this message
Manuel Torrinha (t0rrant) wrote :

Ryan, thanks for the suggestions.

Using write_files module with append mode seems counter-intuitive, although is a reasonable workaround.

I will look into implementing that cc_sysconfig module. What is your thought on making cloudinit.net renderer to leave the `/etc/sysconfig/network` file as is, i.e: not writing the `NETWORKING=yes` at all, if the user does not specify anything with the sysconfig module? Would there be any downside to this?

Revision history for this message
Ryan Harper (raharper) wrote :

cloud-init really relies on networking in almost all cases; unless networking config is disable, cloud-init is going to generate a network config for the system and as I understand sysconfig, we will need to set NETWORKING=yes to ensure that the OS networking service brings up networking.

However, instead of *rendering* a new file, cloudinit.net.sysconfig would instead utilize whatever primitive class cc_sysconfig would use to read existing content and then toggle the specified value. This would avoid networking from clobbering values it doesn't care about.

4128843... by Manuel Torrinha

Revert "Added support for arbitrary options in sysconfig"

This reverts commit aa36d811

Signed-off-by: Manuel Torrinha <email address hidden>

27838f7... by Manuel Torrinha

Revert "Added support for arbitrary options in sysconfig"

This reverts commit aa36d811

Signed-off-by: Manuel Torrinha <email address hidden>

7518f9c... by Manuel Torrinha

WIP: update logic missing

Signed-off-by: Manuel Torrinha <email address hidden>

83c9823... by Manuel Torrinha

WIP: added update logic

missing tests

Signed-off-by: Manuel Torrinha <email address hidden>

d1eac38... by Manuel Torrinha

WIP: code cleanup

missing tests

Signed-off-by: Manuel Torrinha <email address hidden>

Revision history for this message
Manuel Torrinha (t0rrant) wrote :

Hey Ryan,

I'm having trouble finding the best way to test the code, I can always launch a full stack for this but that feels a bit overpowered for testing just one module.

Is there any way I could test this on my development environment, for the network in my first commit I could test it with the `cloud-init devel` command:

  cloud-init devel net-convert --debug -D centos -O sysconfig --network-data sysconfig_test.yaml -k yaml -d .

Revision history for this message
Ryan Harper (raharper) wrote :

For in-tree testing there are two main options; first writting unittests; under tests/unitests/test_handlers/test_<module>.py are most of the config module tests.

For an integration test, the next best way is to build the package, via ./package/[brpm|bddeb] which can create a .rpm or deb you can install to a target system.

We typically use LXD to create a system container where we install the new package.

You can run a single module like list:

cloud-init --debug single --name cc_name_of_module --frequency always

Unmerged commits

d1eac38... by Manuel Torrinha

WIP: code cleanup

missing tests

Signed-off-by: Manuel Torrinha <email address hidden>

83c9823... by Manuel Torrinha

WIP: added update logic

missing tests

Signed-off-by: Manuel Torrinha <email address hidden>

7518f9c... by Manuel Torrinha

WIP: update logic missing

Signed-off-by: Manuel Torrinha <email address hidden>

27838f7... by Manuel Torrinha

Revert "Added support for arbitrary options in sysconfig"

This reverts commit aa36d811

Signed-off-by: Manuel Torrinha <email address hidden>

4128843... by Manuel Torrinha

Revert "Added support for arbitrary options in sysconfig"

This reverts commit aa36d811

Signed-off-by: Manuel Torrinha <email address hidden>

aa36d81... by Manuel Torrinha

Added support for arbitrary options in sysconfig

I do not feel comfortable with having a dummy handler for this, I guess
the purpose of the `network` key is exclusive for network interfaces and
network interface interaction. Having a handle_ function is perhaps not
the best choice.

For now this does what is intended, will discuss this with the
cloud-init team and maybe this will be done in some other way.

Signed-off-by: Manuel Torrinha <email address hidden>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/cloudinit/config/cc_sysconfig.py b/cloudinit/config/cc_sysconfig.py
0new file mode 1006440new file mode 100644
index 0000000..c00ddb5
--- /dev/null
+++ b/cloudinit/config/cc_sysconfig.py
@@ -0,0 +1,139 @@
1# Copyright (C) 2019 Manuel Torrinha
2#
3# Author: Manuel Torrinha <manuel.torrinha@tecnico.ulisboa.pt>
4#
5# This file is part of cloud-init. See LICENSE file for license information.
6
7"""
8Sysconfig
9-----------
10**Summary:** configure /etc/sysconfig/network
11
12This module is intended to configure sysconfig files, preserving their
13content if so specified.
14
15.. note: at this time only the /etc/sysconfig/network file is supported
16
17**Internal name:** ``cc_sysconfig``
18
19**Module frequency:** per instance
20
21**Supported distros:** centos, rhel
22
23**Config keys**::
24
25 sysconfig:
26 network:
27 mode: <update|append|overwrite>
28 content:
29 NTPSERVERARGS: "minpoll 3 maxpoll 3"
30 RES_OPTION: "rotate"
31"""
32
33from cloudinit.settings import PER_INSTANCE
34from cloudinit import util
35
36frequency = PER_INSTANCE
37
38distros = ['centos', 'rhel']
39
40
41class SysconfigHandlers:
42
43 def __init__(self):
44 pass
45
46 @staticmethod
47 def read_current_config(source="/etc/sysconfig/network"):
48 """
49 :param source: the config file to read from, defaults to
50 /etc/sysconfig/network
51 :return:
52 """
53 fp = open(source, "r")
54 current_config_dict = util.load_shell_content(fp, add_empty=True)
55 fp.close()
56 return current_config_dict
57
58 def network_handler(self, cfg, log):
59 """
60 :param cfg: data within the 'network' configuration key
61 :param log: Pre-initialized Python logger object to use for logging.
62 .. Example::
63
64 sysconfig:
65 network:
66 mode: <update|append|overwrite>
67 file: "/etc/sysconfig/network"
68 content:
69 NTPSERVERARGS: "minpoll 3 maxpoll 3"
70 RES_OPTION: "rotate"
71 """
72 mode = cfg['mode']
73 new_config = cfg['content']
74 omode = "w"
75 current_config = dict()
76 output = str()
77
78 if 'file' in cfg.keys():
79 source_file = cfg['file']
80 else:
81 source_file = "/etc/sysconfig/network"
82
83 if mode == 'overwrite':
84 omode = "w"
85 elif mode == 'append':
86 omode = "a"
87 elif mode == 'update':
88 # read current config
89 current_config = self.read_current_config(source=source_file)
90 omode = "w"
91 else: # invalid mode
92 log.error("Invalid mode: {}".format(mode))
93 return
94
95 # merge new options, whatever values are common, the new ones are kept
96 for key, value in {**current_config, **new_config}:
97 output += "{}={}\n".format(key, value)
98
99 # write file
100 util.write_file(source_file,
101 output, omode=omode)
102 return
103
104
105def handle(name, _cfg, _cloud, log, _args):
106 """
107 Handler for sysconfig
108
109 :param name: The module name "sysconfig" from cloud.cfg
110 :param _cfg: A nested dict containing the entire cloud config contents.
111 :param _cloud: The L{CloudInit} object in use.
112 :param log: Pre-initialized Python logger object to use for logging.
113 :param _args: Any module arguments from cloud.cfg
114 """
115 if 'sysconfig' not in _cfg:
116 log.debug(("Skipping module named %s,"
117 " no 'sysconfig' key in configuration"), name)
118 return
119
120 # a list of supported configuration keys under `sysconfig`.
121 # a handler function for each should be implemented in the
122 # SysconfigHandlers class
123 supported_handlers = ['network']
124
125 # get options
126 cfg = _cfg['sysconfig']
127
128 # call correct handler
129 sysconfig_handlers = SysconfigHandlers()
130 for _handler in supported_handlers:
131 try:
132 _handler_func = getattr(sysconfig_handlers,
133 '%s_handler' % _handler)
134 _handler_func(cfg[_handler], log)
135 except AttributeError:
136 log.error("Handler for {} not implemented!".format(_handler))
137 return
138
139# vi: ts=4 expandtab

Subscribers

People subscribed via source and target branches