Atomic/install.py: Record installs for later use #950
Conversation
Atomic/util.py
Outdated
| @staticmethod | ||
| def write_install_data(install_data): | ||
| with open(ATOMIC_INSTALL_JSON, 'w') as write_data: | ||
| json.dump(install_data, write_data) |
There was a problem hiding this comment.
this can be risky. If something goes wrong in json.dump() we lose the entire db of the images. We should probably write to a temporary file in the same directory first, and then move it to ATOMIC_INSTALL_JSON
There was a problem hiding this comment.
Yes I agree create a tmpfile and rename.
There was a problem hiding this comment.
Should we use file locks? To make sure that two processes accessing the file at the same time don't overight? Or should we create a different file for each installed image?
There was a problem hiding this comment.
I'll implement a file locking mechanism on write.
Atomic/util.py
Outdated
| @staticmethod | ||
| def write_install_data(install_data): | ||
| with open(ATOMIC_INSTALL_JSON, 'w') as write_data: | ||
| json.dump(install_data, write_data) |
There was a problem hiding this comment.
Should we use file locks? To make sure that two processes accessing the file at the same time don't overight? Or should we create a different file for each installed image?
atomic
Outdated
| @@ -124,6 +124,8 @@ def create_parser(help_text): | |||
| help=_("show atomic version and exit")) | |||
| parser.add_argument('--debug', default=False, action='store_true', | |||
| help=_("show debug messages")) | |||
| parser.add_argument('-i', '--ignore', default=False, action='store_true', | |||
| help=_("allows you to ignore certain errors")) | |||
There was a problem hiding this comment.
This is pretty vague. Either specify what you are ignoring, or don't give the user the option.
|
updated with a file locking mechanism |
Atomic/util.py
Outdated
| def write_install_data(install_data): | ||
| with open(ATOMIC_INSTALL_JSON, 'w') as write_data: | ||
| time_out = 0 | ||
| f_lock = False |
There was a problem hiding this comment.
should the lock protect the entire (read + modify + write) operation? Otherwise with two processes trying to write at the same time, we can lose what was written by the process that terminated first if they read the same initial file content.
An alternative can probably be to keep one separate file for each container, like:
ATOMIC_VAR_LIB/install-data/${NAME}.json
so we don't require any sort of synchronization. Would it work?
Prune can cleanup the files that are left there without a related container still active (if the user manually uninstalled it, or the previous rm failed for any reason).
There was a problem hiding this comment.
I kind of like the separate file per container.
There was a problem hiding this comment.
The problem with separate files is it makes it significantly more complicated to look up images by hash, etc. And eventually we come down to the same problem of reading or writing the same file. I'll extend the locking.
baude
force-pushed
the
install
branch
3 times, most recently
from
a99ad0f to
66878b4
Compare
When installing an image, we now write a small bit of json
to /var/lib/atomic/install.json. The json format is:
{
<image_name>: {
id: <image_id>,
install_date: <install_date_in_utc
}
}
This will be used in update, run, etc to ensure that any image
with an INSTALL label is first installed.
| def wrapper(*args, **kwargs): | ||
| get_lock() | ||
| ret = func(*args, **kwargs) | ||
| release_lock() |
There was a problem hiding this comment.
Should we do a try: finally block incase func raises an exception?
|
LGTM Make sure @giuseppe Likes it. |
|
the decorator looks very neat! |
|
☀️ Test successful - status-redhatci |
This reverts: projectatomic#950 See: projectatomic#966 The idea here originally was good, but we have to be a *lot* more careful in introducing data stores than this. They need "schema design", documentation, and most importantly IMO: lifecycle coupling. If I e.g. `rm -rf /var/lib/docker` (as a lot of documentation tells people to do, this install data would stick around. Similarly for deleting content directly in `/ostree/repo`. You could say that's a bug, but it'll happen. The best way to do metadata is to use the underlying store. OSTree in particular already has an API for storing metadata. Any containers we write there should use that. I'm not sure offhand if we can do something similar with Docker. But basically I don't think it's worth carrying another data store *just* to show a warning.
This reverts: projectatomic#950 See: projectatomic#966 The idea here originally was good, but we have to be a *lot* more careful in introducing data stores than this. They need "schema design", documentation, and most importantly IMO: lifecycle coupling. If I e.g. `rm -rf /var/lib/docker` (as a lot of documentation tells people to do, this install data would stick around. Similarly for deleting content directly in `/ostree/repo`. You could say that's a bug, but it'll happen. The best way to do metadata is to use the underlying store. OSTree in particular already has an API for storing metadata. Any containers we write there should use that. I'm not sure offhand if we can do something similar with Docker. But basically I don't think it's worth carrying another data store *just* to show a warning.
The atomic CLI changed behaviour recently (in projectatomic/atomic#950), to require containers with INSTALL labels to be explicitly installed before being run. This is probably something we should have done to begin with. Downstream RHBZ: #1454439
The atomic CLI changed behaviour recently (in projectatomic/atomic#950), to require containers with INSTALL labels to be explicitly installed before being run. This is probably something we should have done to begin with. Downstream RHBZ: #1454439
When installing an image, we now write a small bit of json
to /var/lib/atomic/install.json. The json format is:
{
<image_name>: {
id: <image_id>,
install_date: <install_date_in_utc
}
}
This will be used in update, run, etc to ensure that any image
with an INSTALL label is first installed.