Skip to content

Fix error on upgrade caused by new vendordata2 attributes#869

Merged
OddBloke merged 3 commits into
canonical:masterfrom
TheRealFalcon:fix-pickle-upgrade
Apr 19, 2021
Merged

Fix error on upgrade caused by new vendordata2 attributes#869
OddBloke merged 3 commits into
canonical:masterfrom
TheRealFalcon:fix-pickle-upgrade

Conversation

@TheRealFalcon
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon commented Apr 16, 2021

Proposed Commit Message

Fix error on upgrade caused by new vendordata2 attributes

In #777, we added 'vendordata2' and 'vendordata2_raw' attributes to
the DataSource class, but didn't use the upgrade framework to deal
with an unpickle after upgrade. This commit adds the necessary
upgrade code.

Additionally, added a smaller-scope upgrade test to our integration
tests that will be run on every CI run so we catch these issues
immediately in the future.

LP: #1922739

Additional Context

We haven't caught this is our current upgrade test because we haven't been checking for error status of cloud-init status. I added a check for that in the larger upgrade test as well.

Test Steps

Since the latest cloud-init version is already in LXD images, it's not as straightforward. You can launch a xenial instance and then upgrade to the latest PPA/deb. For other releases, you can launch an instance, manually downgrade to 20.4.1, run a cloud-init clean, then upgrade to latest. I've manually reproduced the bug both ways and manually verified the bug both ways.

I also locally reproduced/verified the new integration test by launching the test against a xenial instance (CI uses focal).

@TheRealFalcon TheRealFalcon added the wip Work in progress, do not land label Apr 16, 2021
@TheRealFalcon TheRealFalcon force-pushed the fix-pickle-upgrade branch 3 times, most recently from 33d03c5 to 400b44e Compare April 16, 2021 18:43
@TheRealFalcon TheRealFalcon changed the title Fix pickle upgrade Fix error on upgrade caused by new vendordata2 attributes Apr 16, 2021
@TheRealFalcon TheRealFalcon requested a review from OddBloke April 16, 2021 19:14
@realizelol
Copy link
Copy Markdown

Thank you,

this fix is definitvely working.

@TheRealFalcon TheRealFalcon removed the wip Work in progress, do not land label Apr 19, 2021
Copy link
Copy Markdown
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're missing some non-integration testing here, and I have concerns about how we got here, but this otherwise looks pretty good to me; I'm going to run the test locally now.

Comment thread tests/integration_tests/test_upgrade.py
Comment thread tests/integration_tests/test_upgrade.py Outdated
else:
pytest.skip(not_run_message)

launch_kwargs = {'image_id': session_cloud._get_initial_image()}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we promote _get_initial_image to the public API?

(Side note, which I noticed when reading _get_initial_images code: should we be caching that value so that we avoid getting different "initial" images if we call that before and after an image publication?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by before and after an image publication?

Although, now that I look at it again, should I just make released_image_id public and use that instead?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by before and after an image publication?

We'll call _get_initial_image once when session_cloud is first instantiated (in the entire test run), and once again here: if a release image is released between those two calls, we'll run this test and the other tests against different images.

Although, now that I look at it again, should I just make released_image_id public and use that instead?

Aha, yeah, I guess we are caching the value already! 👍

Comment thread tests/integration_tests/test_upgrade.py
Comment thread cloudinit/sources/__init__.py
Comment thread cloudinit/sources/__init__.py
@OddBloke
Copy link
Copy Markdown
Collaborator

I'm going to run the test locally now.

I've confirmed that the test in this branch fails when run against an old focal image with a deb built from main:

$ CLOUD_INIT_OS_IMAGE=ubuntu:e0c3495ffd48::ubuntu::focal CLOUD_INIT_CLOUD_INIT_SOURCE=./cloud-init_21.1-34-g45db197c-1\~bddeb\~20.04.1_all.deb pytest tests/integration_tests/test_upgrade.py
...
FAILED tests/integration_tests/test_upgrade.py::test_upgrade_package - assert False

and passes with a deb built from this branch:

$ CLOUD_INIT_OS_IMAGE=ubuntu:e0c3495ffd48::ubuntu::focal CLOUD_INIT_CLOUD_INIT_SOURCE=./cloud-init_21.1-36-g5933b497-1\~bddeb\~20.04.1_all.deb pytest tests/integration_tests/test_upgrade.py
...
PASSED

Comment thread tests/integration_tests/test_upgrade.py
@TheRealFalcon
Copy link
Copy Markdown
Contributor Author

@OddBloke I believe I addressed your comments. I had a question/comment about the unit test.

I added a new test. It failed without this branch against the already existing pickles. Is there a reason to add a new pickle? I'm not sure I understand the reason for adding new pickles provided we start at a sufficiently old starting point.

Copy link
Copy Markdown
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@OddBloke OddBloke merged commit d132356 into canonical:master Apr 19, 2021
@TheRealFalcon TheRealFalcon deleted the fix-pickle-upgrade branch April 19, 2021 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants