Fix unpickle for source paths missing run_dir#863
Conversation
OddBloke
left a comment
OddBloke
left a comment
There was a problem hiding this comment.
Thanks Lucas! It's good to see this framework seems to be up to the second challenge it's presented with! I have some inline thoughts, but this is a good first pass.
|
|
||
| def _unpickle(self, ci_pkl_version: int) -> None: | ||
| """Perform deserialization fixes for Distro.""" | ||
| if "run_dir" not in self.paths.__dict__: |
There was a problem hiding this comment.
| if "run_dir" not in self.paths.__dict__: | |
| if not hasattr(self.paths, "run_dir"): |
because it handles properties appropriately:
In [1]: class Foo:
...: @property
...: def run_dir(self):
...: pass
...:
In [2]: "run_dir" in Foo().__dict__
Out[2]: False
In [3]: hasattr(Foo(), "run_dir")
Out[3]: True| # when loading the pickle object on newer versions of cloud-init | ||
| # we will rely on this attribute. To fix that, we are now | ||
| # manually adding that attribute here. | ||
| self.paths.run_dir = "/run/cloud-init" |
There was a problem hiding this comment.
This is likely safe, but run_dir is configurable. It's possible that someone could have configured a non-default run_dir even without a cloud-init that supports it (e.g. by applying the same configuration file to all their images). To ensure that we pick such configuration up, I think we should probably reinstantiate Paths with the same configuration as the pickled one, and then pull run_dir out of it. Roughly:
self.paths.run_dir = Paths(self.paths.cfgs, ds=self.paths.datasource).run_dirDoes that make sense? (Do you agree?)
There was a problem hiding this comment.
It does make sense. I missed that this could configured by the user. So doing that is the safest approach.
| def __str__(self): | ||
| return type_utils.obj_name(self) | ||
|
|
||
| def _unpickle(self, ci_pkl_version: int) -> None: |
There was a problem hiding this comment.
Does this need to be on DataSource? Could it instead be on Paths itself?
As background, the networking fix had to be in Distro._unpickle because affected Networking instances don't have any instance state: this means that they get unpickled differently, so the hooks we use don't get called. In this case, however, all Paths instances do have instance state, so the hooks we use should get called.
There was a problem hiding this comment.
You are totally right here. I missed that the Paths object can be instantiated directly. I will move the code there
| return type_utils.obj_name(self) | ||
|
|
||
| def _unpickle(self, ci_pkl_version: int) -> None: | ||
| """Perform deserialization fixes for Distro.""" |
On the source class, we require the use of paths.run_dir to perform some operations. On older cloud-init version, the Paths class does not have the run_dir attribute. To fix that, we are now manually adding that attribute in the Paths object if doesn't exist in the unpickle operation. LP: #1899299
lucasmoura
force-pushed
the
fix-pickle-for-paths-run-dir-attribute
branch
from
91bc12b to
c02a628
Compare
|
@OddBloke Thank you for the review and the context here. I have updated the code |
OddBloke
left a comment
OddBloke
left a comment
There was a problem hiding this comment.
Thanks Lucas! One minor nit which I'll apply now.
3 participants
Proposed Commit Message
Fix unpickle for source paths missing run_dir
On the source class, we require the use of paths.run_dir to perform some operations. On older cloud-init version, the Paths class does not have the run_dir attribute. To fix that, we are now manually adding that attribute in the source paths
object if doesn't exist in the unpickle operation.
LP: #1899299
Test Steps
To reproduce this issue, run the following script:
By looking at the
/var/log/ubuntu-advantage.logwe will see that the error is becausecloud-idfails with the error:We can confirm that by running
cloud-idon the container directly.To verify that the fix works, run the following commands:
Checklist: