/snapcraft

lifecycle: clean after deleting container #1587

Merged
merged 11 commits into from Oct 30, 2017

Conversation

Reviewers

@kyrofa kyrofa

@elopio elopio

@sergiusens sergiusens

Assignees

@kalikiana kalikiana

Labels
bug
Projects
17.10 (awaiting triage)
Milestone
  2.35
4 participants
@kalikiana @sergiusens @kyrofa @elopio
@kalikiana
Collaborator

kalikiana commented Oct 4, 2017 •
Edited 1 time

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
  • If this is a new feature. Have you discussed the design on the forum?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh unit?

See the forum discussion.

@kalikiana kalikiana self-assigned this Oct 6, 2017

snapcraft/cli/lifecycle.py
project_options = get_project_options(**kwargs)
container_config = env.get_container_config()
- if container_config.use_container:
- step = step or 'pull'
+ if container_config.use_container and step == 'pull' and not parts:
@elopio

elopio Oct 7, 2017

Member

I don't get this @kalikiana. I might be doing something wrong while testing it, but it seems to me that this still just deletes the container.
The execute method in lxd checks if we are cleaning, and if so, it just deletes the container.
In this pr I see nothing to modify that behaviour.

@kalikiana

kalikiana Oct 8, 2017

Collaborator

Sorry about that. It looks like the second commit was lost... I re-did the missing piece. So: clean should be called with the container to delete it, in all other cases "normal" lifecycle clean is used so that it works even if the container is broken or already deleted.

@kyrofa

kyrofa Oct 10, 2017

Member

I'd like to rectify the poor separation of concerns, here. We're going to great lengths to ensure we only ever call lifecycle.containerbuild('clean') if step == 'pull' and not parts, with no apparent reason. We then proceed to pass the step as well as the parts (that we just ensured was empty) to the very generic-sounding lifecycle.containerbuild() method. That method then calls the very generic-sounding lxd.Project().execute() method. Finally, that method checks to see if we're cleaning the pull step with no parts, in which case it blows the container away.

I'd like to see lxd.Project().execute() be written more generically. If it was asked to clean with no parts, fine, blow the container away. If it was asked to clean with parts, do nothing. If something else, call super(). Then we don't have to let the conditions where the container needs to be removed leak to this line, where we're checking those conditions again. We can simply call lifecycle.containerbuild('clean') just like we call lifecycle.clean() and expect it to do the right thing.

@kalikiana
Collaborator

kalikiana commented Oct 10, 2017

Note: All but the snap test have passed
@kyrofa

kyrofa requested changes Oct 10, 2017
View changes

Good progress here, thank you! However, there's a concerns leakage that looks like a bug waiting to happen. I suggest we fix that.

snapcraft/cli/lifecycle.py
project_options = get_project_options(**kwargs)
container_config = env.get_container_config()
- if container_config.use_container:
- step = step or 'pull'
+ if container_config.use_container and step == 'pull' and not parts:
@elopio

elopio Oct 7, 2017

Member

I don't get this @kalikiana. I might be doing something wrong while testing it, but it seems to me that this still just deletes the container.
The execute method in lxd checks if we are cleaning, and if so, it just deletes the container.
In this pr I see nothing to modify that behaviour.

@kalikiana

kalikiana Oct 8, 2017

Collaborator

Sorry about that. It looks like the second commit was lost... I re-did the missing piece. So: clean should be called with the container to delete it, in all other cases "normal" lifecycle clean is used so that it works even if the container is broken or already deleted.

@kyrofa

kyrofa Oct 10, 2017

Member

I'd like to rectify the poor separation of concerns, here. We're going to great lengths to ensure we only ever call lifecycle.containerbuild('clean') if step == 'pull' and not parts, with no apparent reason. We then proceed to pass the step as well as the parts (that we just ensured was empty) to the very generic-sounding lifecycle.containerbuild() method. That method then calls the very generic-sounding lxd.Project().execute() method. Finally, that method checks to see if we're cleaning the pull step with no parts, in which case it blows the container away.

I'd like to see lxd.Project().execute() be written more generically. If it was asked to clean with no parts, fine, blow the container away. If it was asked to clean with parts, do nothing. If something else, call super(). Then we don't have to let the conditions where the container needs to be removed leak to this line, where we're checking those conditions again. We can simply call lifecycle.containerbuild('clean') just like we call lifecycle.clean() and expect it to do the right thing.

@kalikiana
Collaborator

kalikiana commented Oct 11, 2017

@kyrofa Somehow I can reply... something's off with GitHub since today I think. In any case, I changed it as per your suggestion and the logic is only in lxd.py now.
snapcraft/internal/lxd.py
+ if step == 'clean':
+ # we don't execute clean here but rely on CLI to do it
+ # clean with no parts deletes the container
+ if self._get_container_status() and args == ['--step', 'pull']:
@kyrofa

kyrofa Oct 11, 2017 •
Edited 1 time

Member

I think re-purposing this function to clean is both confusing (clean is not a lifecycle step, and then one sees --step pull and gets more confused) and is causing the diff of this PR to be larger than it needs to be (changing the step from None to 'pull' everywhere, which seems unnecessary). How difficult would it be to simply add a clean function here that accepts a step (the step being cleaned) and args?

@kyrofa

kyrofa Oct 11, 2017

Member

Eh, I changed my mind on defaulting step to pull at a higher level, otherwise you'll need to treat None and 'pull' as equivalent here. However, I would still like to see a clean function here instead of execute.

@kalikiana

kalikiana Oct 12, 2017

Collaborator

I reverted the default change now. It's not essential to the PR, it just seemed like a simplification.

And I replaced the subclass' execute logic with a delete method - now lifecycle remains in charge and reading lxd.py doesn't require any knowledge of what lifecycle does with it.

snapcraft/internal/lifecycle.py
- if not step:
- step = 'pull'
-
+def clean(project_options, parts, step):
@kyrofa

kyrofa Oct 11, 2017

Member

Why not just default step='pull' so it continues working the way it was?

snapcraft/tests/test_lifecycle.py
@@ -653,7 +653,7 @@ def test_clean_removes_global_state(self):
plugin: nil
""")
lifecycle.execute('pull', self.project_options)
- lifecycle.clean(self.project_options, parts=None)
+ lifecycle.clean(self.project_options, parts=None, step='pull')
@kyrofa

kyrofa Oct 11, 2017

Member

If you use pull as the default, this is unnecessary.

@kalikiana
Collaborator

kalikiana commented Oct 13, 2017

Tests are green again!
@kyrofa

kyrofa reviewed Oct 13, 2017
View changes

This seems fine to me, now. We'll need to refactor it if the container ever needs to actually do something when individual parts or steps are cleaned, but this solves the problem for now.

@elopio

elopio approved these changes Oct 13, 2017
View changes

If I understand correctly, this calls the clean commands outside of the container. I can see no issue right now, but I think this has the potential to break. I think that if the build is done in the container, the clean should execute in there too. But for now, I think this is a good improvement. Maybe we care about this when it becomes an issue, not now.

@sergiusens
Collaborator

sergiusens commented Oct 16, 2017

the business logic seems to have spread a bit too much into the cli components, hasn't it?
@kyrofa
Member

kyrofa commented Oct 16, 2017

Yes indeed.
@sergiusens

sergiusens reviewed Oct 16, 2017
View changes

Sorry to say, but it would be good to prevent leaking too much into this.

It is acceptable if you want to surface UI (click.echo, echo.warn or prompts) but not for usage of moving common logic.

I would personally rather have it repeated.

@kalikiana
Collaborator

kalikiana commented Oct 20, 2017

@sergiusens I moved all logic back into the container, in the clean() method. Lifecycle knows nothing again like it did before.
@elopio

elopio approved these changes Oct 24, 2017
View changes

To me, this looks more readable than before. Reapproving.

@kyrofa kyrofa dismissed their stale review Oct 24, 2017

Significant change occurred.

@kalikiana
Collaborator

kalikiana commented Oct 25, 2017

I updated the branch against master (no change in code, but it now lives in _project.py).

@sergiusens sergiusens added this to the 2.35 milestone Oct 26, 2017

@kalikiana kalikiana dismissed sergiusens’s stale review Oct 26, 2017

Moved the logic back into the container, out of lifecycle. Needs re-checking.

snapcraft/cli/lifecycle.py
@@ -175,11 +177,12 @@ def clean(parts, step, **kwargs):
"""
project_options = get_project_options(**kwargs)
container_config = env.get_container_config()
+ step = step or 'pull'
@kyrofa

kyrofa Oct 30, 2017 •
Edited 1 time

Member

You can fix LP: #1727445 right here by not doing this and handling both cases within Project.clean().

@sergiusens sergiusens added the bug label Oct 30, 2017

@sergiusens sergiusens merged commit daca461 into snapcore:master Oct 30, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sergiusens sergiusens referenced this pull request Nov 1, 2017

Merged

Release changelog for 2.35 #1650

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment