Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add the first layout of the new osys architecture #2
Conversation
smoser
and others
added some commits
Jul 24, 2014
and others
added some commits
Nov 22, 2014
harlowja
reviewed
Mar 3, 2015
| + self.expire = expire | ||
| + | ||
| + @abc.abstractproperty | ||
| + def static(self): |
harlowja
reviewed
Mar 3, 2015
| + """ | ||
| + | ||
| + def __init__(self): | ||
| + self._route_items = self._routes() |
harlowja
Mar 3, 2015
Contributor
So this feels sorta odd; but maybe its just me. Why not just lazily populate this information instead of populating it before the object is fully constructed? Doing things before the object actually is ready always seems odd to me.
PCManticore
Mar 3, 2015
Contributor
So if I'm understanding correctly, do you want to lazily populate the routes only when they are needed?
harlowja
reviewed
Mar 3, 2015
| + return self._route_items[index] | ||
| + | ||
| + def __delitem__(self, index): | ||
| + self.__class__.delete(self._route_items[index]) |
PCManticore
Mar 3, 2015
Contributor
The delete methods will work on the class, since it will be pretty strange to delete a route from a route instance. But the delitem special method was added in the case you have a collection already and you want to change it. It could definitely be a YAGNI at this point. Should I drop it?
harlowja
Mar 3, 2015
Contributor
Except classmethods can also be called from the instance, self.delete will look for a instance method, and then just find the class one, so IMHO its prefereable and less confusing just to do that (it also allows people to override delete in other instances).
harlowja
reviewed
Mar 3, 2015
| + | ||
| + | ||
| +@six.add_metaclass(abc.ABCMeta) | ||
| +class BaseRouteCollection(collections.Sequence): |
harlowja
Mar 3, 2015
Contributor
So I'm still wondering, but do we need to prefix all these with 'Base'; is that adding any real value?
harlowja
reviewed
Mar 3, 2015
| + | ||
| + | ||
| +@six.add_metaclass(abc.ABCMeta) | ||
| +class BaseInterface(object): |
harlowja
Mar 3, 2015
Contributor
Same question about prefix things with 'Base'; I think just 'Interface' would have the same meaning, its pretty obvious its a base class due to the six.add_metaclass(abc.ABCMeta) usage.
PCManticore
Mar 3, 2015
Contributor
There needs to be some marker other that six.add_metaclass to determine that a class is a base class when working with it, if it will be used outside of the file where it was defined (you won't see there that it has ABCMeta as a metaclass and it will require an extra step to go and check, while a Base in the name suggests this from the start).
harlowja
Mar 3, 2015
Contributor
Meh, this is python, people will have to look at the base class code to know this anyway. And docs should tell this also.... Prefixing all the things with 'Base' just adds noise imho.
harlowja
reviewed
Mar 3, 2015
| +from cloudinit.osys.windows.util import kernel32 | ||
| +from cloudinit.osys.windows.util import ws2_32 | ||
| + | ||
| + |
harlowja
reviewed
Mar 3, 2015
| + return dhcp_server | ||
| + except Exception as ex: | ||
| + # Not found | ||
| + if ex.errno != 2: |
harlowja
reviewed
Mar 3, 2015
| + | ||
| +from cloudinit.osys.windows.util import kernel32 | ||
| +from cloudinit.osys.windows.util import ws2_32 | ||
| + |
harlowja
Mar 3, 2015
Contributor
Same question about comments, these are better, but still unsure about some of them (what is GAA_??)
|
So looks pretty good to me. Added some comments. The windows stuff seems like it should have more comments and I don't really know if it works (tests here would be great to verify this); I also don't have a windows machine so that's another problem. How did we plan on testing the variations here? Is there any site that can do this (does travis-ci have windows support?)... |
harlowja
reviewed
Mar 3, 2015
| +__all__ = ('abstractclassmethod', ) | ||
| + | ||
| + | ||
| +class abstractclassmethod(classmethod): |
harlowja
Mar 3, 2015
Contributor
This almost seems like it should be in a file @ cloudinit/util.py since this seems more generic and useful outside of 'osys' right?
harlowja
reviewed
Mar 3, 2015
| + general = general_module.General() | ||
| + user_class = users_module.User | ||
| + route_class = network_module.Route | ||
| + interface_class = network_module.Interface |
harlowja
Mar 3, 2015
Contributor
Something feels off, why isn't there a corresponding general_class?
PCManticore
Mar 3, 2015
Contributor
At this point, I didn't feel that it was needed. On the other hand, route_class / user_class / interface_class are actually used for controlling various aspects of what they represent (they have some class methods for creating / deleting objects basically).
harlowja
reviewed
Mar 3, 2015
| + | ||
| + @staticmethod | ||
| + def check_os_version(major, minor, build=0): | ||
| + vi = kernel32.Win32_OSVERSIONINFOEX_W() |
harlowja
reviewed
Mar 3, 2015
| + winreg.KEY_READ) as key: | ||
| + try: | ||
| + dhcp_server = winreg.QueryValueEx(key, "DhcpServer")[0] | ||
| + if dhcp_server == "255.255.255.255": |
PCManticore
Mar 3, 2015
Contributor
Most likely. We don't have support for this in cloudbase-init, that's why it's not present at the moment.
harlowja
reviewed
Mar 3, 2015
| + return interfaces | ||
| + | ||
| + if ret_val: | ||
| + raise Exception("GetAdaptersAddresses failed") |
harlowja
Mar 3, 2015
Contributor
Some other kind of exception would be useful. Perhaps should be a cloudinit/exceptions.py that has these exceptions. Just raising Exception doesn't really allow anyone to catch specific errors/exceptions...
harlowja
reviewed
Mar 3, 2015
| + q = conn.query("SELECT * FROM Win32_NetworkAdapter WHERE " | ||
| + "MACAddress = '{}'".format(mac_address)) | ||
| + if not len(q): | ||
| + raise Exception("Network adapter not found") |
harlowja
reviewed
Mar 3, 2015
| + LOG.debug("Setting static IP address") | ||
| + (ret_val, ) = adapter_config.EnableStatic([address], [netmask]) | ||
| + if ret_val > 1: | ||
| + raise Exception( |
harlowja
reviewed
Mar 3, 2015
| + LOG.debug("Setting static gateways") | ||
| + (ret_val, ) = adapter_config.SetGateways([gateway], [1]) | ||
| + if ret_val > 1: | ||
| + raise Exception("Cannot set gateway on network adapter") |
harlowja
reviewed
Mar 3, 2015
| + LOG.debug("Setting static DNS servers") | ||
| + (ret_val,) = adapter_config.SetDNSServerSearchOrder(dnsnameservers) | ||
| + if ret_val > 1: | ||
| + raise Exception("Cannot set DNS on network adapter") |
harlowja
reviewed
Mar 3, 2015
| + _ComputerNamePhysicalDnsHostname, | ||
| + six.text_type(hostname)) | ||
| + if not ret_val: | ||
| + raise Exception("Cannot set host name") |
harlowja
reviewed
Mar 3, 2015
| + fw_profile = fw_profile.GloballyOpenPorts.Add(fw_port) | ||
| + | ||
| + def firewall_remove_rule(self, _, port, protocol, allow=True): | ||
| + if not allow: |
harlowja
Mar 3, 2015
Contributor
Hmmm, seems like something other than NotImplementedError should be raised here since it actually is implemented but not allowed...
harlowja
reviewed
Mar 3, 2015
| + "store=persistent"] | ||
| + ret_val = subprocess.call(args, shell=False) | ||
| + if ret_val: | ||
| + raise Exception( |
harlowja
reviewed
Mar 3, 2015
| + _general = general.General() | ||
| + | ||
| + if not _general.check_os_version(6, 0): | ||
| + raise Exception( |
harlowja
reviewed
Mar 3, 2015
| + _, stderr = popen.communicate() | ||
| + if popen.returncode or stderr: | ||
| + # Cannot use the return value to determine the outcome | ||
| + raise Exception('Unable to add route: %s' % stderr) |
harlowja
reviewed
Mar 3, 2015
| + args = ['ROUTE', 'ADD', | ||
| + route.destination, | ||
| + 'MASK', route.netmask, route.gateway] | ||
| + popen = subprocess.Popen(args, shell=False, |
harlowja
Mar 3, 2015
Contributor
We should likely take over the code from http://bazaar.launchpad.net/~cloud-init-dev/cloud-init/trunk/view/head:/cloudinit/util.py#L1627 (the subp function there) and use that instead, since its better and more generic for the use-cases here. Put that in cloudinit/util.py and use it?
harlowja
reviewed
Mar 3, 2015
| + ctypes.byref(size), 0) | ||
| + if err != kernel32.ERROR_NO_DATA: | ||
| + if err: | ||
| + raise Exception( |
harlowja
reviewed
Mar 3, 2015
| + kernel32.HeapFree(heap, 0, p_forward_table) | ||
| + p = kernel32.HeapAlloc(heap, 0, ctypes.c_size_t(size.value)) | ||
| + if not p: | ||
| + raise Exception( |
harlowja
reviewed
Mar 3, 2015
| + size = wintypes.ULONG(forward_table_size) | ||
| + p = kernel32.HeapAlloc(heap, 0, ctypes.c_size_t(size.value)) | ||
| + if not p: | ||
| + raise Exception( |
|
Thanks! I'm currently writing tests, but they are not ideal, because they are using mocks. On the other hand, we have a generic CI system written for cloudbase-init and I'm planning to use that for the cloud-init v2 as well, after we will have more than "something". It's pretty generic and can be reused for any CI related business (The code for this system is here:https://github.com/PCManticore/argus-ci/tree/develop) |
|
Ya, it'd be really nice to be able to run this without mocks, like on a real windows test environment. Maybe get microsoft to finish travis-ci/travis-ci#216 ;) |
|
Or travis-ci/travis-ci#2104 ; one of those, ha. |
|
closing this as it is against 2.0 branch |
PCManticore commentedFeb 25, 2015
Hello.
This pull request provides a first layout of how the new OS utils should look, according to the last design spec.
It contains only some of the base classes which were brought to the discussion in the aforementioned spec, mainly the Network namespace, with implementations for the Windows platform. It's not yet complete, since I still need to write the tests, but some feedback to see if I'm on the right way should be enough for now. After this, the rest of the base classes should come, which means we can start already working on bringing the code for other distros inside this new location.