Skip to content

actions in plugin __enter__ methods should be called outside of __enter__ #263

@bmoyles

Description

@bmoyles

We do a fair bit of setup in __enter__ in plugins, and in many cases, may end up throwing exceptions in __enter__. When an exception is raised in __enter__, we do NOT jump to__exit__ for that context manager -- the exception lands in the outer context. When that happens, any actions executed in __enter__ prior to the exception will not be unwound by __exit__, and depending on the plugin in which the exception occurred, may leave resources stranded.

A good example:
https://github.com/Netflix/aminator/blob/master/aminator/environment.py#L60
Here we enter the volume context, but looking at __enter__ for the Linux volume manager plugin:
https://github.com/Netflix/aminator/blob/master/aminator/plugins/volume/linux.py#L102
there are a number of places where we may throw an exception (eg, bad return from fsck). When that happens in this specific plugin, it's the finalizer that sees the exception and propagates it upwards. The attached volume is never detached and cleaned up, and we effectively leak a volume.

Ideally, within the environment, once we've established a context, we should explicitly call a method on that context to do setup/initialization rather than implicitly do it when entering the context so that plugin's context manager can clean up resources.

With that change, the provision() method of the environment at https://github.com/Netflix/aminator/blob/master/aminator/environment.py#L55
might look something like:

    def provision(self):
        log.info('Beginning amination! Package: {0}'.format(self._config.context.package.arg))
        with self.metrics:  # pylint: disable=no-member
            with self.cloud as cloud:  # pylint: disable=no-member
                cloud.setup()
                with self.finalizer(cloud) as finalizer:  # pylint: disable=no-member
                    finalizer.setup()
                    with self.volume(self.cloud, self.blockdevice) as volume:  # pylint: disable=no-member
                        volume.setup()
                        with self.distro(volume) as distro:  # pylint: disable=no-member
                            distro.setup()
                            success = self.provisioner(distro).provision()  # pylint: disable=no-member
                            if not success:
                                log.critical('Provisioning failed!')
                                return False
                        success = finalizer.finalize()
                        if not success:
                            log.critical('Finalizing failed!')
                            return False
        return True

(cc @kvick @coryb @scotte )

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions