New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix image expiry, init storage handling when nested and remote storage operations #3906

Merged
merged 13 commits into from Oct 6, 2017

Conversation

4 participants
@stgraber
Member

stgraber commented Oct 5, 2017

Closes #3904

stgraber added some commits Oct 5, 2017

init: Only nest btrfs if container is on btrfs
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
tests: Fix shell return value masking
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
images: Respect disabled cache expiry
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
images: Store UploadedAt as RFC3399
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
patches: Convert UploadedAt to RFC3399
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
tests: Fix image expiry test
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
daemon: Simplify time channels
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
daemon: Fix handling of config triggers
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
tests: Fix bad raw.lxc test
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
daemon: Don't update images while pruning
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
images: Actually get the list of images to remove
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
test: Setup basic channel handler for triggers
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>

@stgraber stgraber changed the title from Fix handling of image expiry to Fix image expiry, init storage handling when nested and remote storage operations Oct 6, 2017

lxc/storage: Fix remote operations
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Oct 6, 2017

Member

That's a potpourri of a branch. :)

Member

brauner commented Oct 6, 2017

That's a potpourri of a branch. :)

@@ -76,7 +76,7 @@ func ImagesGetExpired(db *sql.DB, expiry int64) ([]string, error) {
results = append(results, r[0].(string))
}
return []string{}, nil

This comment has been minimized.

@brauner

brauner Oct 6, 2017

Member

Oh wow...

@brauner

brauner Oct 6, 2017

Member

Oh wow...

This comment has been minimized.

@freeekanayaka

freeekanayaka Oct 6, 2017

Member

@brauner this went unnoticed despite the review (#3817), unit tests! :)

@freeekanayaka

freeekanayaka Oct 6, 2017

Member

@brauner this went unnoticed despite the review (#3817), unit tests! :)

This comment has been minimized.

@sanderbrandenburg

sanderbrandenburg Oct 6, 2017

I was trying to find this bug in issue #3904, but I found a problem with the unit/integration test instead :)

@sanderbrandenburg

sanderbrandenburg Oct 6, 2017

I was trying to find this bug in issue #3904, but I found a problem with the unit/integration test instead :)

This comment has been minimized.

@freeekanayaka

freeekanayaka Oct 6, 2017

Member

IMO you need both (integration AND unit). Since integration tests are more complex, they are more likely to be broken by themselves or miss corner cases.

@freeekanayaka

freeekanayaka Oct 6, 2017

Member

IMO you need both (integration AND unit). Since integration tests are more complex, they are more likely to be broken by themselves or miss corner cases.

@brauner brauner merged commit ce011b6 into lxc:master Oct 6, 2017

5 checks passed

Branch target Branch target is correct
Details
Signed-off-by All commits signed-off
Details
Testsuite Testsuite passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
return err
}
_, err = db.Exec(d.db, "UPDATE images SET upload_date=? WHERE id=?", image.UploadedAt, id)

This comment has been minimized.

@freeekanayaka

freeekanayaka Oct 6, 2017

Member

I'm a concerned about doing database work/fixes/migrations using the patches system, because:

  1. It's not transactional, if a single db.XXX api of the one above fails, but the previous ones have succeeded, you are in an undetermined state, in principle. This might not be a practical concern now, but it will be more with clustering.

  2. More importantly, these patches are run after all database updates have been applied, so for example the statement "UPDATE images SET upload_date=? WHERE id=?" could fail if at some point (say) we change the schema of the images table or something like that, and the user upgrades from a version that had a schema compatible with the query in the patch to a version with a schema that is incompatible with the query. We could change the statement to match the new schema, but it could be easily missed since we don't cover each and every upgrade path with integration tests.

In this case, I see no reason for not writing a regular database update instead of a patch.

@freeekanayaka

freeekanayaka Oct 6, 2017

Member

I'm a concerned about doing database work/fixes/migrations using the patches system, because:

  1. It's not transactional, if a single db.XXX api of the one above fails, but the previous ones have succeeded, you are in an undetermined state, in principle. This might not be a practical concern now, but it will be more with clustering.

  2. More importantly, these patches are run after all database updates have been applied, so for example the statement "UPDATE images SET upload_date=? WHERE id=?" could fail if at some point (say) we change the schema of the images table or something like that, and the user upgrades from a version that had a schema compatible with the query in the patch to a version with a schema that is incompatible with the query. We could change the statement to match the new schema, but it could be easily missed since we don't cover each and every upgrade path with integration tests.

In this case, I see no reason for not writing a regular database update instead of a patch.

This comment has been minimized.

@stgraber

stgraber Oct 6, 2017

Member

A DB schema update cannot be backported to 2.0, patches can, that's why this is a patch.

It's true I should probably have used this within a transaction, that being said this particular patch can be re-executed perfectly safely so it doesn't actually matter in the end.

@stgraber

stgraber Oct 6, 2017

Member

A DB schema update cannot be backported to 2.0, patches can, that's why this is a patch.

It's true I should probably have used this within a transaction, that being said this particular patch can be re-executed perfectly safely so it doesn't actually matter in the end.

This comment has been minimized.

@freeekanayaka

freeekanayaka Oct 7, 2017

Member

I see. It's something we'll need to fix then, because in a cluster we'll want patches to be something that is applied locally once per-node, while database changes like this should be applied once cluster-wide by the leader using dqlite.

@freeekanayaka

freeekanayaka Oct 7, 2017

Member

I see. It's something we'll need to fix then, because in a cluster we'll want patches to be something that is applied locally once per-node, while database changes like this should be applied once cluster-wide by the leader using dqlite.

This comment has been minimized.

@stgraber

stgraber Oct 7, 2017

Member

@freeekanayaka I think keeping the DB updates for schema changes makes sense, but we may want to expand the patches system to allow for different scopes. So a patch could be marked as either needing to apply to all the nodes individually (when moving stuff on the filesystem or doing other node-local actions) or be run once for the whole cluster (data migration, changing default profiles, ...)

@stgraber

stgraber Oct 7, 2017

Member

@freeekanayaka I think keeping the DB updates for schema changes makes sense, but we may want to expand the patches system to allow for different scopes. So a patch could be marked as either needing to apply to all the nodes individually (when moving stuff on the filesystem or doing other node-local actions) or be run once for the whole cluster (data migration, changing default profiles, ...)

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