Merge lp:~allenap/maas/open-configuration-read-only into lp:~maas-committers/maas/trunk
- open-configuration-read-only
- Merge into trunk
Proposed by
Gavin Panella
| Status: | Merged |
|---|---|
| Approved by: | Gavin Panella |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 4551 |
| Proposed branch: | lp:~allenap/maas/open-configuration-read-only |
| Merge into: | lp:~maas-committers/maas/trunk |
| Diff against target: |
747 lines (+260/-70) 12 files modified
src/maasserver/dns/tests/test_config.py (+1/-1) src/maasserver/management/commands/_config.py (+2/-2) src/maasserver/management/commands/tests/test_config.py (+1/-1) src/provisioningserver/cluster_config_command.py (+1/-1) src/provisioningserver/config.py (+112/-26) src/provisioningserver/drivers/osystem/tests/test_custom.py (+1/-1) src/provisioningserver/import_images/tests/test_boot_resources.py (+1/-1) src/provisioningserver/pserv_services/tests/test_tftp.py (+4/-4) src/provisioningserver/testing/config.py (+1/-1) src/provisioningserver/tests/test_cluster_config_command.py (+2/-2) src/provisioningserver/tests/test_config.py (+131/-27) src/provisioningserver/tests/test_diskless.py (+3/-3) |
| To merge this branch: | bzr merge lp:~allenap/maas/open-configuration-read-only |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Mike Pontillo (community) | Approve | ||
|
Review via email:
|
|||
Commit message
By default, open configuration files read-only.
This eliminates the need for locking when using the YAML-based back-end.
Description of the change
To post a comment you must log in.
Revision history for this message
| Gavin Panella (allenap) wrote : | # |
Thanks!
Revision history for this message
| Gavin Panella (allenap) wrote : | # |
I'll land it here, back-port to 1.9, then I'll ping Ryan.
Revision history for this message
| MAAS Lander (maas-lander) wrote : | # |
Download full text (1.0 MiB)
The attempt to merge lp:~allenap/maas/open-configuration-read-only into lp:maas failed. Below is the output from the failed tests.
Hit http://
Get:1 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Get:2 http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Get:11 http://
Get:12 http://
Get:13 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Revision history for this message
| Gavin Panella (allenap) wrote : | # |
I cannot reproduce the landing failure locally:
FAIL: maasserver.
.TestStaticIP
-------
testtools.
File ".../src/
result = function(*args, **kwargs)
File ".../testtools-
in _run_test_method
return self._get_
File ".../src/
211, in test_update_
self.
File ".../testtools-
in assertEqual
self.
File ".../testtools-
in assertThat
raise mismatch_error
testtools.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
| 1 | === modified file 'src/maasserver/dns/tests/test_config.py' |
| 2 | --- src/maasserver/dns/tests/test_config.py 2015-12-01 18:12:59 +0000 |
| 3 | +++ src/maasserver/dns/tests/test_config.py 2015-12-10 20:46:09 +0000 |
| 4 | @@ -520,7 +520,7 @@ |
| 5 | |
| 6 | def test_dns_config_has_NS_record(self): |
| 7 | ip = factory.make_ipv4_address() |
| 8 | - with RegionConfiguration.open() as config: |
| 9 | + with RegionConfiguration.open_for_update() as config: |
| 10 | config.maas_url = 'http://%s/' % ip |
| 11 | nodegroup, node, static = self.create_nodegroup_with_static_ip() |
| 12 | self.patch(settings, 'DNS_CONNECT', True) |
| 13 | |
| 14 | === modified file 'src/maasserver/management/commands/_config.py' |
| 15 | --- src/maasserver/management/commands/_config.py 2015-12-01 18:12:59 +0000 |
| 16 | +++ src/maasserver/management/commands/_config.py 2015-12-10 20:46:09 +0000 |
| 17 | @@ -188,7 +188,7 @@ |
| 18 | help = "Reset local configuration for the MAAS region controller." |
| 19 | |
| 20 | def handle(self, *args, **options): |
| 21 | - with RegionConfiguration.open() as config: |
| 22 | + with RegionConfiguration.open_for_update() as config: |
| 23 | for name, option in gen_configuration_options(): |
| 24 | if options.get(name): |
| 25 | delattr(config, name) |
| 26 | @@ -204,7 +204,7 @@ |
| 27 | help = "Set local configuration for the MAAS region controller." |
| 28 | |
| 29 | def handle(self, *args, **options): |
| 30 | - with RegionConfiguration.open() as config: |
| 31 | + with RegionConfiguration.open_for_update() as config: |
| 32 | for name, option in gen_configuration_options(): |
| 33 | value = options.get(name) |
| 34 | if value is not None: |
| 35 | |
| 36 | === modified file 'src/maasserver/management/commands/tests/test_config.py' |
| 37 | --- src/maasserver/management/commands/tests/test_config.py 2015-12-01 18:12:59 +0000 |
| 38 | +++ src/maasserver/management/commands/tests/test_config.py 2015-12-10 20:46:09 +0000 |
| 39 | @@ -90,7 +90,7 @@ |
| 40 | self.useFixture(RegionConfigurationFixture()) |
| 41 | # Give the option a random value. |
| 42 | value = factory.make_name("foobar") |
| 43 | - with RegionConfiguration.open() as configuration: |
| 44 | + with RegionConfiguration.open_for_update() as configuration: |
| 45 | setattr(configuration, self.option.dest, value) |
| 46 | stdio = call_reset(**{self.option.dest: True}) |
| 47 | # Nothing is echoed back to the user. |
| 48 | |
| 49 | === modified file 'src/provisioningserver/cluster_config_command.py' |
| 50 | --- src/provisioningserver/cluster_config_command.py 2015-12-01 18:12:59 +0000 |
| 51 | +++ src/provisioningserver/cluster_config_command.py 2015-12-10 20:46:09 +0000 |
| 52 | @@ -33,7 +33,7 @@ |
| 53 | and init be passed at the same time, as these are mutually exclusive |
| 54 | parameters. |
| 55 | """ |
| 56 | - with ClusterConfiguration.open() as config: |
| 57 | + with ClusterConfiguration.open_for_update() as config: |
| 58 | if url is not None: |
| 59 | config.maas_url = url |
| 60 | if uuid is not None: |
| 61 | |
| 62 | === modified file 'src/provisioningserver/config.py' |
| 63 | --- src/provisioningserver/config.py 2015-12-02 10:44:25 +0000 |
| 64 | +++ src/provisioningserver/config.py 2015-12-10 20:46:09 +0000 |
| 65 | @@ -142,6 +142,7 @@ |
| 66 | Set, |
| 67 | ) |
| 68 | from provisioningserver.path import get_tentative_path |
| 69 | +from provisioningserver.utils import typed |
| 70 | from provisioningserver.utils.config import ( |
| 71 | DirectoryString, |
| 72 | ExtendedURL, |
| 73 | @@ -335,11 +336,16 @@ |
| 74 | os.close(os.open(path, os.O_CREAT | os.O_APPEND, mode)) |
| 75 | |
| 76 | |
| 77 | +class ConfigurationImmutable(Exception): |
| 78 | + """The configuration is read-only; it cannot be mutated.""" |
| 79 | + |
| 80 | + |
| 81 | class ConfigurationDatabase: |
| 82 | """Store configuration in an sqlite3 database.""" |
| 83 | |
| 84 | - def __init__(self, database): |
| 85 | + def __init__(self, database, *, mutable=False): |
| 86 | self.database = database |
| 87 | + self.mutable = mutable |
| 88 | with self.cursor() as cursor: |
| 89 | cursor.execute( |
| 90 | "CREATE TABLE IF NOT EXISTS configuration " |
| 91 | @@ -367,31 +373,70 @@ |
| 92 | return json.loads(data[0]) |
| 93 | |
| 94 | def __setitem__(self, name, data): |
| 95 | - with self.cursor() as cursor: |
| 96 | - cursor.execute( |
| 97 | - "INSERT OR REPLACE INTO configuration (name, data) " |
| 98 | - "VALUES (?, ?)", (name, json.dumps(data))) |
| 99 | + if self.mutable: |
| 100 | + with self.cursor() as cursor: |
| 101 | + cursor.execute( |
| 102 | + "INSERT OR REPLACE INTO configuration (name, data) " |
| 103 | + "VALUES (?, ?)", (name, json.dumps(data))) |
| 104 | + else: |
| 105 | + raise ConfigurationImmutable( |
| 106 | + "%s: Cannot set `%s'." % (self, name)) |
| 107 | |
| 108 | def __delitem__(self, name): |
| 109 | + if self.mutable: |
| 110 | + with self.cursor() as cursor: |
| 111 | + cursor.execute( |
| 112 | + "DELETE FROM configuration" |
| 113 | + " WHERE name = ?", (name,)) |
| 114 | + else: |
| 115 | + raise ConfigurationImmutable( |
| 116 | + "%s: Cannot set `%s'." % (self, name)) |
| 117 | + |
| 118 | + def __str__(self): |
| 119 | with self.cursor() as cursor: |
| 120 | - cursor.execute( |
| 121 | - "DELETE FROM configuration" |
| 122 | - " WHERE name = ?", (name,)) |
| 123 | - |
| 124 | - @classmethod |
| 125 | - @contextmanager |
| 126 | - def open(cls, dbpath): |
| 127 | + # https://www.sqlite.org/pragma.html#pragma_database_list |
| 128 | + databases = "; ".join( |
| 129 | + "%s=%s" % (name, ":memory:" if path == "" else path) |
| 130 | + for (_, name, path) in cursor.execute("PRAGMA database_list")) |
| 131 | + return "%s(%s)" % (self.__class__.__qualname__, databases) |
| 132 | + |
| 133 | + @classmethod |
| 134 | + @contextmanager |
| 135 | + @typed |
| 136 | + def open(cls, dbpath: str): |
| 137 | + """Open a configuration database. |
| 138 | + |
| 139 | + **Note** that this returns a context manager which will open the |
| 140 | + database READ-ONLY. |
| 141 | + """ |
| 142 | + # Ensure `dbpath` exists... |
| 143 | + touch(dbpath) |
| 144 | + # before opening it with sqlite. |
| 145 | + database = sqlite3.connect(dbpath) |
| 146 | + try: |
| 147 | + yield cls(database, mutable=False) |
| 148 | + except: |
| 149 | + raise |
| 150 | + else: |
| 151 | + database.rollback() |
| 152 | + finally: |
| 153 | + database.close() |
| 154 | + |
| 155 | + @classmethod |
| 156 | + @contextmanager |
| 157 | + @typed |
| 158 | + def open_for_update(cls, dbpath: str): |
| 159 | """Open a configuration database. |
| 160 | |
| 161 | **Note** that this returns a context manager which will close the |
| 162 | - database on exit, saving if the exit is clean. |
| 163 | + database on exit, COMMITTING changes if the exit is clean. |
| 164 | """ |
| 165 | # Ensure `dbpath` exists... |
| 166 | touch(dbpath) |
| 167 | # before opening it with sqlite. |
| 168 | database = sqlite3.connect(dbpath) |
| 169 | try: |
| 170 | - yield cls(database) |
| 171 | + yield cls(database, mutable=True) |
| 172 | except: |
| 173 | raise |
| 174 | else: |
| 175 | @@ -415,11 +460,12 @@ |
| 176 | got to use this. |
| 177 | """ |
| 178 | |
| 179 | - def __init__(self, path): |
| 180 | + def __init__(self, path, *, mutable=False): |
| 181 | super(ConfigurationFile, self).__init__() |
| 182 | self.config = {} |
| 183 | self.dirty = False |
| 184 | self.path = path |
| 185 | + self.mutable = mutable |
| 186 | |
| 187 | def __iter__(self): |
| 188 | return iter(self.config) |
| 189 | @@ -428,13 +474,21 @@ |
| 190 | return self.config[name] |
| 191 | |
| 192 | def __setitem__(self, name, data): |
| 193 | - self.config[name] = data |
| 194 | - self.dirty = True |
| 195 | - |
| 196 | - def __delitem__(self, name): |
| 197 | - if name in self.config: |
| 198 | - del self.config[name] |
| 199 | + if self.mutable: |
| 200 | + self.config[name] = data |
| 201 | self.dirty = True |
| 202 | + else: |
| 203 | + raise ConfigurationImmutable( |
| 204 | + "%s: Cannot set `%s'." % (self, name)) |
| 205 | + |
| 206 | + def __delitem__(self, name): |
| 207 | + if self.mutable: |
| 208 | + if name in self.config: |
| 209 | + del self.config[name] |
| 210 | + self.dirty = True |
| 211 | + else: |
| 212 | + raise ConfigurationImmutable( |
| 213 | + "%s: Cannot set `%s'." % (self, name)) |
| 214 | |
| 215 | def load(self): |
| 216 | """Load the configuration.""" |
| 217 | @@ -467,9 +521,32 @@ |
| 218 | self.path, mode=mode) |
| 219 | self.dirty = False |
| 220 | |
| 221 | - @classmethod |
| 222 | - @contextmanager |
| 223 | - def open(cls, path): |
| 224 | + def __str__(self): |
| 225 | + return "%s(%r)" % (self.__class__.__qualname__, self.path) |
| 226 | + |
| 227 | + @classmethod |
| 228 | + @contextmanager |
| 229 | + @typed |
| 230 | + def open(cls, path: str): |
| 231 | + """Open a configuration file read-only. |
| 232 | + |
| 233 | + This avoids all the locking that happens in `open_for_update`. However, |
| 234 | + it will create the configuration file if it does not yet exist. |
| 235 | + |
| 236 | + **Note** that this returns a context manager which will DISCARD |
| 237 | + changes to the configuration on exit. |
| 238 | + """ |
| 239 | + # Ensure `path` exists... |
| 240 | + touch(path) |
| 241 | + # before loading it in. |
| 242 | + configfile = cls(path, mutable=False) |
| 243 | + configfile.load() |
| 244 | + yield configfile |
| 245 | + |
| 246 | + @classmethod |
| 247 | + @contextmanager |
| 248 | + @typed |
| 249 | + def open_for_update(cls, path: str): |
| 250 | """Open a configuration file. |
| 251 | |
| 252 | Locks are taken so that there can only be *one* reader or writer for a |
| 253 | @@ -477,7 +554,7 @@ |
| 254 | multiple concurrent processes it follows that each process should hold |
| 255 | the file open for the shortest time possible. |
| 256 | |
| 257 | - **Note** that this returns a context manager which will save changes |
| 258 | + **Note** that this returns a context manager which will SAVE changes |
| 259 | to the configuration on a clean exit. |
| 260 | """ |
| 261 | time_opened = None |
| 262 | @@ -488,7 +565,7 @@ |
| 263 | # Ensure `path` exists... |
| 264 | touch(path) |
| 265 | # before loading it in. |
| 266 | - configfile = cls(path) |
| 267 | + configfile = cls(path, mutable=True) |
| 268 | configfile.load() |
| 269 | try: |
| 270 | yield configfile |
| 271 | @@ -611,6 +688,15 @@ |
| 272 | with cls.backend.open(filepath) as store: |
| 273 | yield cls(store) |
| 274 | |
| 275 | + @classmethod |
| 276 | + @contextmanager |
| 277 | + def open_for_update(cls, filepath=None): |
| 278 | + if filepath is None: |
| 279 | + filepath = cls.DEFAULT_FILENAME |
| 280 | + ensure_dir(os.path.dirname(filepath)) |
| 281 | + with cls.backend.open_for_update(filepath) as store: |
| 282 | + yield cls(store) |
| 283 | + |
| 284 | |
| 285 | class ConfigurationOption: |
| 286 | """Define a configuration option. |
| 287 | |
| 288 | === modified file 'src/provisioningserver/drivers/osystem/tests/test_custom.py' |
| 289 | --- src/provisioningserver/drivers/osystem/tests/test_custom.py 2015-12-01 18:12:59 +0000 |
| 290 | +++ src/provisioningserver/drivers/osystem/tests/test_custom.py 2015-12-10 20:46:09 +0000 |
| 291 | @@ -32,7 +32,7 @@ |
| 292 | current_dir, 'custom', arch, subarch, release, label) |
| 293 | os.makedirs(dirpath) |
| 294 | factory.make_file(dirpath, filename) |
| 295 | - with ClusterConfiguration.open() as config: |
| 296 | + with ClusterConfiguration.open_for_update() as config: |
| 297 | config.tftp_root = current_dir |
| 298 | return arch, subarch, release, label |
| 299 | |
| 300 | |
| 301 | === modified file 'src/provisioningserver/import_images/tests/test_boot_resources.py' |
| 302 | --- src/provisioningserver/import_images/tests/test_boot_resources.py 2015-12-04 08:16:30 +0000 |
| 303 | +++ src/provisioningserver/import_images/tests/test_boot_resources.py 2015-12-10 20:46:09 +0000 |
| 304 | @@ -196,7 +196,7 @@ |
| 305 | self.storage = self.make_dir() |
| 306 | current_dir = os.path.join(self.storage, 'current') + os.sep |
| 307 | os.makedirs(current_dir) |
| 308 | - with ClusterConfiguration.open() as config: |
| 309 | + with ClusterConfiguration.open_for_update() as config: |
| 310 | config.tftp_root = current_dir |
| 311 | os.rmdir(current_dir) |
| 312 | # Forcing arch to amd64 causes pxelinux.0 to be installed, giving more |
| 313 | |
| 314 | === modified file 'src/provisioningserver/pserv_services/tests/test_tftp.py' |
| 315 | --- src/provisioningserver/pserv_services/tests/test_tftp.py 2015-12-10 05:00:21 +0000 |
| 316 | +++ src/provisioningserver/pserv_services/tests/test_tftp.py 2015-12-10 20:46:09 +0000 |
| 317 | @@ -216,7 +216,7 @@ |
| 318 | |
| 319 | @inlineCallbacks |
| 320 | def test_get_reader_converts_404s_to_tftp_error(self): |
| 321 | - with ClusterConfiguration.open() as config: |
| 322 | + with ClusterConfiguration.open_for_update() as config: |
| 323 | config.cluster_uuid = factory.make_UUID() |
| 324 | |
| 325 | backend = TFTPBackend( |
| 326 | @@ -229,7 +229,7 @@ |
| 327 | |
| 328 | @inlineCallbacks |
| 329 | def test_get_reader_converts_other_exceptions_to_tftp_error(self): |
| 330 | - with ClusterConfiguration.open() as config: |
| 331 | + with ClusterConfiguration.open_for_update() as config: |
| 332 | config.cluster_uuid = factory.make_UUID() |
| 333 | |
| 334 | exception_type = factory.make_exception_type() |
| 335 | @@ -258,7 +258,7 @@ |
| 336 | # For paths matching PXEBootMethod.match_path, TFTPBackend.get_reader() |
| 337 | # returns a Deferred that will yield a BytesReader. |
| 338 | cluster_uuid = factory.make_UUID() |
| 339 | - with ClusterConfiguration.open() as config: |
| 340 | + with ClusterConfiguration.open_for_update() as config: |
| 341 | config.cluster_uuid = cluster_uuid |
| 342 | mac = factory.make_mac_address("-") |
| 343 | config_path = compose_config_path(mac) |
| 344 | @@ -361,7 +361,7 @@ |
| 345 | # arch field of the parameters (mapping from pxe to maas |
| 346 | # namespace). |
| 347 | cluster_uuid = factory.make_UUID() |
| 348 | - with ClusterConfiguration.open() as config: |
| 349 | + with ClusterConfiguration.open_for_update() as config: |
| 350 | config.cluster_uuid = cluster_uuid |
| 351 | config_path = b"pxelinux.cfg/default-arm" |
| 352 | backend = TFTPBackend( |
| 353 | |
| 354 | === modified file 'src/provisioningserver/testing/config.py' |
| 355 | --- src/provisioningserver/testing/config.py 2015-12-01 18:12:59 +0000 |
| 356 | +++ src/provisioningserver/testing/config.py 2015-12-10 20:46:09 +0000 |
| 357 | @@ -94,7 +94,7 @@ |
| 358 | self.path = path.join( |
| 359 | self.useFixture(TempDirectory()).path, |
| 360 | path.basename(self.configuration.DEFAULT_FILENAME)) |
| 361 | - with self.configuration.open(self.path) as config: |
| 362 | + with self.configuration.open_for_update(self.path) as config: |
| 363 | for key, value in self.options.items(): |
| 364 | setattr(config, key, value) |
| 365 | # Export this filename to the environment, so that subprocesses will |
| 366 | |
| 367 | === modified file 'src/provisioningserver/tests/test_cluster_config_command.py' |
| 368 | --- src/provisioningserver/tests/test_cluster_config_command.py 2015-12-01 18:12:59 +0000 |
| 369 | +++ src/provisioningserver/tests/test_cluster_config_command.py 2015-12-10 20:46:09 +0000 |
| 370 | @@ -153,7 +153,7 @@ |
| 371 | |
| 372 | def test_config_set_cluster_uuid_without_setting_does_nothing(self): |
| 373 | expected_previous_value = str(uuid.uuid4()) |
| 374 | - with ClusterConfiguration.open() as config: |
| 375 | + with ClusterConfiguration.open_for_update() as config: |
| 376 | config.cluster_uuid = expected_previous_value |
| 377 | with ClusterConfiguration.open() as config: |
| 378 | observed_previous_value = config.cluster_uuid |
| 379 | @@ -177,7 +177,7 @@ |
| 380 | |
| 381 | def test_config_init_when_already_configured_does_nothing(self): |
| 382 | expected_previous_value = str(uuid.uuid4()) |
| 383 | - with ClusterConfiguration.open() as config: |
| 384 | + with ClusterConfiguration.open_for_update() as config: |
| 385 | config.cluster_uuid = expected_previous_value |
| 386 | with ClusterConfiguration.open() as config: |
| 387 | observed_previous_value = config.cluster_uuid |
| 388 | |
| 389 | === modified file 'src/provisioningserver/tests/test_config.py' |
| 390 | --- src/provisioningserver/tests/test_config.py 2015-12-01 18:12:59 +0000 |
| 391 | +++ src/provisioningserver/tests/test_config.py 2015-12-10 20:46:09 +0000 |
| 392 | @@ -6,7 +6,11 @@ |
| 393 | __all__ = [] |
| 394 | |
| 395 | import contextlib |
| 396 | -from operator import methodcaller |
| 397 | +from operator import ( |
| 398 | + delitem, |
| 399 | + methodcaller, |
| 400 | + setitem, |
| 401 | +) |
| 402 | import os.path |
| 403 | import sqlite3 |
| 404 | from uuid import uuid4 |
| 405 | @@ -27,6 +31,7 @@ |
| 406 | Configuration, |
| 407 | ConfigurationDatabase, |
| 408 | ConfigurationFile, |
| 409 | + ConfigurationImmutable, |
| 410 | ConfigurationMeta, |
| 411 | ConfigurationOption, |
| 412 | is_dev_environment, |
| 413 | @@ -36,6 +41,7 @@ |
| 414 | from provisioningserver.utils.fs import RunLock |
| 415 | from testtools import ExpectedException |
| 416 | from testtools.matchers import ( |
| 417 | + Equals, |
| 418 | FileContains, |
| 419 | FileExists, |
| 420 | Is, |
| 421 | @@ -144,17 +150,39 @@ |
| 422 | with expected_exception: |
| 423 | config.foo = "bar" |
| 424 | |
| 425 | - def test_opens_using_backend(self): |
| 426 | + def test_open_uses_backend_as_context_manager(self): |
| 427 | config_file = self.make_file() |
| 428 | backend = self.patch(ExampleConfiguration, "backend") |
| 429 | - with backend.open(config_file) as config: |
| 430 | - backend_ctx = backend.open.return_value |
| 431 | + with ExampleConfiguration.open(config_file) as config: |
| 432 | + # The backend was opened using open() too. |
| 433 | + self.assertThat(backend.open, MockCalledOnceWith(config_file)) |
| 434 | # The object returned from backend.open() has been used as the |
| 435 | # context manager, providing `config`. |
| 436 | - self.assertThat(config, Is(backend_ctx.__enter__.return_value)) |
| 437 | - # We're within the context, as expected. |
| 438 | - self.assertThat(backend_ctx.__exit__, MockNotCalled()) |
| 439 | - # The context has been exited. |
| 440 | + backend_ctx = backend.open.return_value |
| 441 | + self.assertThat(config.store, Is( |
| 442 | + backend_ctx.__enter__.return_value)) |
| 443 | + # We're within the context, as expected. |
| 444 | + self.assertThat(backend_ctx.__exit__, MockNotCalled()) |
| 445 | + # The backend context has also been exited. |
| 446 | + self.assertThat( |
| 447 | + backend_ctx.__exit__, |
| 448 | + MockCalledOnceWith(None, None, None)) |
| 449 | + |
| 450 | + def test_open_for_update_uses_backend_as_context_manager(self): |
| 451 | + config_file = self.make_file() |
| 452 | + backend = self.patch(ExampleConfiguration, "backend") |
| 453 | + with ExampleConfiguration.open_for_update(config_file) as config: |
| 454 | + # The backend was opened using open_for_update() too. |
| 455 | + self.assertThat( |
| 456 | + backend.open_for_update, MockCalledOnceWith(config_file)) |
| 457 | + # The object returned from backend.open_for_update() has been used |
| 458 | + # as the context manager, providing `config`. |
| 459 | + backend_ctx = backend.open_for_update.return_value |
| 460 | + self.assertThat(config.store, Is( |
| 461 | + backend_ctx.__enter__.return_value)) |
| 462 | + # We're within the context, as expected. |
| 463 | + self.assertThat(backend_ctx.__exit__, MockNotCalled()) |
| 464 | + # The backend context has also been exited. |
| 465 | self.assertThat( |
| 466 | backend_ctx.__exit__, |
| 467 | MockCalledOnceWith(None, None, None)) |
| 468 | @@ -171,10 +199,10 @@ |
| 469 | def make_database_store(self): |
| 470 | database = sqlite3.connect(":memory:") |
| 471 | self.addCleanup(database.close) |
| 472 | - return ConfigurationDatabase(database) |
| 473 | + return ConfigurationDatabase(database, mutable=True) |
| 474 | |
| 475 | def make_file_store(self): |
| 476 | - return ConfigurationFile(self.make_file()) |
| 477 | + return ConfigurationFile(self.make_file(), mutable=True) |
| 478 | |
| 479 | def make_config(self): |
| 480 | store = self.make_store(self) |
| 481 | @@ -232,14 +260,14 @@ |
| 482 | |
| 483 | def test_adding_configuration_option(self): |
| 484 | database = sqlite3.connect(":memory:") |
| 485 | - config = ConfigurationDatabase(database) |
| 486 | + config = ConfigurationDatabase(database, mutable=True) |
| 487 | config["alice"] = {"abc": 123} |
| 488 | self.assertEqual({"alice"}, set(config)) |
| 489 | self.assertEqual({"abc": 123}, config["alice"]) |
| 490 | |
| 491 | def test_replacing_configuration_option(self): |
| 492 | database = sqlite3.connect(":memory:") |
| 493 | - config = ConfigurationDatabase(database) |
| 494 | + config = ConfigurationDatabase(database, mutable=True) |
| 495 | config["alice"] = {"abc": 123} |
| 496 | config["alice"] = {"def": 456} |
| 497 | self.assertEqual({"alice"}, set(config)) |
| 498 | @@ -247,7 +275,7 @@ |
| 499 | |
| 500 | def test_getting_configuration_option(self): |
| 501 | database = sqlite3.connect(":memory:") |
| 502 | - config = ConfigurationDatabase(database) |
| 503 | + config = ConfigurationDatabase(database, mutable=True) |
| 504 | config["alice"] = {"abc": 123} |
| 505 | self.assertEqual({"abc": 123}, config["alice"]) |
| 506 | |
| 507 | @@ -258,7 +286,7 @@ |
| 508 | |
| 509 | def test_removing_configuration_option(self): |
| 510 | database = sqlite3.connect(":memory:") |
| 511 | - config = ConfigurationDatabase(database) |
| 512 | + config = ConfigurationDatabase(database, mutable=True) |
| 513 | config["alice"] = {"abc": 123} |
| 514 | del config["alice"] |
| 515 | self.assertEqual(set(), set(config)) |
| 516 | @@ -267,7 +295,7 @@ |
| 517 | # ConfigurationDatabase.open() returns a context manager that closes |
| 518 | # the database on exit. |
| 519 | config_file = os.path.join(self.make_dir(), "config") |
| 520 | - config = ConfigurationDatabase.open(config_file) |
| 521 | + config = ConfigurationDatabase.open_for_update(config_file) |
| 522 | self.assertIsInstance(config, contextlib._GeneratorContextManager) |
| 523 | with config as config: |
| 524 | self.assertIsInstance(config, ConfigurationDatabase) |
| 525 | @@ -298,7 +326,7 @@ |
| 526 | config_file = os.path.join(self.make_dir(), "config") |
| 527 | config_key = factory.make_name("key") |
| 528 | config_value = factory.make_name("value") |
| 529 | - with ConfigurationDatabase.open(config_file) as config: |
| 530 | + with ConfigurationDatabase.open_for_update(config_file) as config: |
| 531 | config[config_key] = config_value |
| 532 | with ConfigurationDatabase.open(config_file) as config: |
| 533 | self.assertEqual(config_value, config[config_key]) |
| 534 | @@ -310,13 +338,51 @@ |
| 535 | exception_type = factory.make_exception_type() |
| 536 | # Set a configuration option, then crash. |
| 537 | with ExpectedException(exception_type): |
| 538 | - with ConfigurationDatabase.open(config_file) as config: |
| 539 | + with ConfigurationDatabase.open_for_update(config_file) as config: |
| 540 | config[config_key] = config_value |
| 541 | raise exception_type() |
| 542 | # No value has been saved for `config_key`. |
| 543 | with ConfigurationDatabase.open(config_file) as config: |
| 544 | self.assertRaises(KeyError, lambda: config[config_key]) |
| 545 | |
| 546 | + def test_as_string(self): |
| 547 | + database = sqlite3.connect(":memory:") |
| 548 | + config = ConfigurationDatabase(database) |
| 549 | + self.assertThat(str(config), Equals( |
| 550 | + "ConfigurationDatabase(main=:memory:)")) |
| 551 | + |
| 552 | + |
| 553 | +class TestConfigurationDatabaseMutability(MAASTestCase): |
| 554 | + """Tests for `ConfigurationDatabase` mutability.""" |
| 555 | + |
| 556 | + def test_immutable(self): |
| 557 | + database = sqlite3.connect(":memory:") |
| 558 | + config = ConfigurationDatabase(database, mutable=False) |
| 559 | + self.assertRaises(ConfigurationImmutable, setitem, config, "alice", 1) |
| 560 | + self.assertRaises(ConfigurationImmutable, delitem, config, "alice") |
| 561 | + |
| 562 | + def test_mutable(self): |
| 563 | + database = sqlite3.connect(":memory:") |
| 564 | + config = ConfigurationDatabase(database, mutable=True) |
| 565 | + config["alice"] = 1234 |
| 566 | + del config["alice"] |
| 567 | + |
| 568 | + def test_open_yields_immutable_backend(self): |
| 569 | + config_file = os.path.join(self.make_dir(), "config") |
| 570 | + config_key = factory.make_name("key") |
| 571 | + with ConfigurationDatabase.open(config_file) as config: |
| 572 | + with ExpectedException(ConfigurationImmutable): |
| 573 | + config[config_key] = factory.make_name("value") |
| 574 | + with ExpectedException(ConfigurationImmutable): |
| 575 | + del config[config_key] |
| 576 | + |
| 577 | + def test_open_for_update_yields_mutable_backend(self): |
| 578 | + config_file = os.path.join(self.make_dir(), "config") |
| 579 | + config_key = factory.make_name("key") |
| 580 | + with ConfigurationDatabase.open_for_update(config_file) as config: |
| 581 | + config[config_key] = factory.make_name("value") |
| 582 | + del config[config_key] |
| 583 | + |
| 584 | |
| 585 | class TestConfigurationFile(MAASTestCase): |
| 586 | """Tests for `ConfigurationFile`.""" |
| 587 | @@ -329,14 +395,14 @@ |
| 588 | config={}, dirty=False, path=sentinel.filename)) |
| 589 | |
| 590 | def test_adding_configuration_option(self): |
| 591 | - config = ConfigurationFile(sentinel.filename) |
| 592 | + config = ConfigurationFile(sentinel.filename, mutable=True) |
| 593 | config["alice"] = {"abc": 123} |
| 594 | self.assertEqual({"alice"}, set(config)) |
| 595 | self.assertEqual({"abc": 123}, config["alice"]) |
| 596 | self.assertTrue(config.dirty) |
| 597 | |
| 598 | def test_replacing_configuration_option(self): |
| 599 | - config = ConfigurationFile(sentinel.filename) |
| 600 | + config = ConfigurationFile(sentinel.filename, mutable=True) |
| 601 | config["alice"] = {"abc": 123} |
| 602 | config["alice"] = {"def": 456} |
| 603 | self.assertEqual({"alice"}, set(config)) |
| 604 | @@ -344,7 +410,7 @@ |
| 605 | self.assertTrue(config.dirty) |
| 606 | |
| 607 | def test_getting_configuration_option(self): |
| 608 | - config = ConfigurationFile(sentinel.filename) |
| 609 | + config = ConfigurationFile(sentinel.filename, mutable=True) |
| 610 | config["alice"] = {"abc": 123} |
| 611 | self.assertEqual({"abc": 123}, config["alice"]) |
| 612 | |
| 613 | @@ -353,7 +419,7 @@ |
| 614 | self.assertRaises(KeyError, lambda: config["alice"]) |
| 615 | |
| 616 | def test_removing_configuration_option(self): |
| 617 | - config = ConfigurationFile(sentinel.filename) |
| 618 | + config = ConfigurationFile(sentinel.filename, mutable=True) |
| 619 | config["alice"] = {"abc": 123} |
| 620 | del config["alice"] |
| 621 | self.assertEqual(set(), set(config)) |
| 622 | @@ -408,7 +474,7 @@ |
| 623 | config_file = os.path.join(self.make_dir(), "config") |
| 624 | open(config_file, "wb").close() # touch. |
| 625 | os.chmod(config_file, 0o644) # u=rw,go=r |
| 626 | - with ConfigurationFile.open(config_file): |
| 627 | + with ConfigurationFile.open_for_update(config_file): |
| 628 | perms = FilePath(config_file).getPermissions() |
| 629 | self.assertEqual("rw-r--r--", perms.shorthand()) |
| 630 | perms = FilePath(config_file).getPermissions() |
| 631 | @@ -420,7 +486,7 @@ |
| 632 | config_file = os.path.join(self.make_dir(), "config") |
| 633 | open(config_file, "wb").close() # touch. |
| 634 | os.chmod(config_file, 0o644) # u=rw,go=r |
| 635 | - with ConfigurationFile.open(config_file) as config: |
| 636 | + with ConfigurationFile.open_for_update(config_file) as config: |
| 637 | perms = FilePath(config_file).getPermissions() |
| 638 | self.assertEqual("rw-r--r--", perms.shorthand()) |
| 639 | config["foobar"] = "I am a modification" |
| 640 | @@ -434,7 +500,7 @@ |
| 641 | config_file = os.path.join(self.make_dir(), "config") |
| 642 | open(config_file, "wb").close() # touch. |
| 643 | os.chmod(config_file, 0o644) # u=rw,go=r |
| 644 | - with ConfigurationFile.open(config_file) as config: |
| 645 | + with ConfigurationFile.open_for_update(config_file) as config: |
| 646 | config["foobar"] = "I am a modification" |
| 647 | os.unlink(config_file) |
| 648 | perms = FilePath(config_file).getPermissions() |
| 649 | @@ -446,7 +512,7 @@ |
| 650 | config_file = os.path.join(self.make_dir(), "config") |
| 651 | config_key = factory.make_name("key") |
| 652 | config_value = factory.make_name("value") |
| 653 | - with ConfigurationFile.open(config_file) as config: |
| 654 | + with ConfigurationFile.open_for_update(config_file) as config: |
| 655 | config[config_key] = config_value |
| 656 | self.assertEqual({config_key: config_value}, config.config) |
| 657 | self.assertTrue(config.dirty) |
| 658 | @@ -460,7 +526,7 @@ |
| 659 | exception_type = factory.make_exception_type() |
| 660 | # Set a configuration option, then crash. |
| 661 | with ExpectedException(exception_type): |
| 662 | - with ConfigurationFile.open(config_file) as config: |
| 663 | + with ConfigurationFile.open_for_update(config_file) as config: |
| 664 | config[config_key] = config_value |
| 665 | raise exception_type() |
| 666 | # No value has been saved for `config_key`. |
| 667 | @@ -471,10 +537,48 @@ |
| 668 | config_file = os.path.join(self.make_dir(), "config") |
| 669 | config_lock = RunLock(config_file) |
| 670 | self.assertFalse(config_lock.is_locked()) |
| 671 | - with ConfigurationFile.open(config_file): |
| 672 | + with ConfigurationFile.open_for_update(config_file): |
| 673 | self.assertTrue(config_lock.is_locked()) |
| 674 | self.assertFalse(config_lock.is_locked()) |
| 675 | |
| 676 | + def test_as_string(self): |
| 677 | + config_file = os.path.join(self.make_dir(), "config") |
| 678 | + config = ConfigurationFile(config_file) |
| 679 | + self.assertThat(str(config), Equals( |
| 680 | + "ConfigurationFile(%r)" % config_file)) |
| 681 | + |
| 682 | + |
| 683 | +class TestConfigurationFileMutability(MAASTestCase): |
| 684 | + """Tests for `ConfigurationFile` mutability.""" |
| 685 | + |
| 686 | + def test_immutable(self): |
| 687 | + config_file = os.path.join(self.make_dir(), "config") |
| 688 | + config = ConfigurationFile(config_file, mutable=False) |
| 689 | + self.assertRaises(ConfigurationImmutable, setitem, config, "alice", 1) |
| 690 | + self.assertRaises(ConfigurationImmutable, delitem, config, "alice") |
| 691 | + |
| 692 | + def test_mutable(self): |
| 693 | + config_file = os.path.join(self.make_dir(), "config") |
| 694 | + config = ConfigurationFile(config_file, mutable=True) |
| 695 | + config["alice"] = 1234 |
| 696 | + del config["alice"] |
| 697 | + |
| 698 | + def test_open_yields_immutable_backend(self): |
| 699 | + config_file = os.path.join(self.make_dir(), "config") |
| 700 | + config_key = factory.make_name("key") |
| 701 | + with ConfigurationFile.open(config_file) as config: |
| 702 | + with ExpectedException(ConfigurationImmutable): |
| 703 | + config[config_key] = factory.make_name("value") |
| 704 | + with ExpectedException(ConfigurationImmutable): |
| 705 | + del config[config_key] |
| 706 | + |
| 707 | + def test_open_for_update_yields_mutable_backend(self): |
| 708 | + config_file = os.path.join(self.make_dir(), "config") |
| 709 | + config_key = factory.make_name("key") |
| 710 | + with ConfigurationFile.open_for_update(config_file) as config: |
| 711 | + config[config_key] = factory.make_name("value") |
| 712 | + del config[config_key] |
| 713 | + |
| 714 | |
| 715 | class TestClusterConfiguration(MAASTestCase): |
| 716 | """Tests for `ClusterConfiguration`.""" |
| 717 | |
| 718 | === modified file 'src/provisioningserver/tests/test_diskless.py' |
| 719 | --- src/provisioningserver/tests/test_diskless.py 2015-12-01 18:12:59 +0000 |
| 720 | +++ src/provisioningserver/tests/test_diskless.py 2015-12-10 20:46:09 +0000 |
| 721 | @@ -70,7 +70,7 @@ |
| 722 | os.mkdir(os.path.join(resource_dir, 'diskless')) |
| 723 | current_dir = os.path.join(resource_dir, 'current') + '/' |
| 724 | os.mkdir(current_dir) |
| 725 | - with ClusterConfiguration.open() as config: |
| 726 | + with ClusterConfiguration.open_for_update() as config: |
| 727 | config.tftp_root = current_dir |
| 728 | return resource_dir |
| 729 | |
| 730 | @@ -113,7 +113,7 @@ |
| 731 | storage_dir = self.make_dir() |
| 732 | current_dir = os.path.join(storage_dir, 'current') + '/' |
| 733 | os.mkdir(current_dir) |
| 734 | - with ClusterConfiguration.open() as config: |
| 735 | + with ClusterConfiguration.open_for_update() as config: |
| 736 | config.tftp_root = current_dir |
| 737 | self.assertEqual( |
| 738 | os.path.join(storage_dir, 'diskless', 'store'), |
| 739 | @@ -294,7 +294,7 @@ |
| 740 | osystem = OperatingSystemRegistry[os_name] |
| 741 | mock_xi_params = self.patch(osystem, 'get_xinstall_parameters') |
| 742 | mock_xi_params.return_value = (root_path, 'tgz') |
| 743 | - with ClusterConfiguration.open() as config: |
| 744 | + with ClusterConfiguration.open_for_update() as config: |
| 745 | tftp_root = config.tftp_root |
| 746 | self.assertEqual( |
| 747 | os.path.join( |
Looks good to me!
If you want to land this in 1.9, I think we should test it with custom images. Maybe beisner can help, since he already has a test setup.