Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce storage volume snapshots #584

Merged
merged 15 commits into from
Jul 2, 2024
Merged

Introduce storage volume snapshots #584

merged 15 commits into from
Jul 2, 2024

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Jun 7, 2024 •
edited
Loading

This add a few features related to custom storage volumes snapshots to pyLXD:

  1. Custom volume snapshot creation
  2. Custom volume edition (including renaming)
  3. Restoring a custom volume from a snapshot
  4. Custom volume snapshot deletion

The unit tests will be added in a later PR if necessary.
Scheduling was already supported since it is done through config key settings on the StorageVolume object.
This also does some refactoring on previously existing code and some other changes made necessary by the new features and tests.

simondeziel
simondeziel reviewed Jun 7, 2024
View reviewed changes
Copy link
Member

@simondeziel simondeziel left a comment

Choose a reason for hiding this comment

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

Thanks for providing access to this early PR. I provided a few comments in this quick first glance.

integration/test_storage.py Outdated Show resolved Hide resolved
integration/test_storage.py Outdated Show resolved Hide resolved
integration/test_storage.py Outdated Show resolved Hide resolved
pylxd/models/storage_pool.py Outdated Show resolved Hide resolved
pylxd/models/storage_pool.py Show resolved Hide resolved
@simondeziel simondeziel marked this pull request as draft June 7, 2024 18:15
@hamistao
Copy link
Contributor Author

hamistao commented Jun 7, 2024

@simondeziel For now I ask to focus the comments on the functions' behaviors. I still have some work to do on comments, docstrings and user messages.

@hamistao hamistao changed the title Add basic usage for storage volume snapshots Introduce storage volume snapshots Jun 11, 2024
@hamistao hamistao force-pushed the storage_volume_snapshot_basic_usage branch 8 times, most recently from 02b1177 to d6feb45 Compare June 13, 2024 20:53
@hamistao hamistao force-pushed the storage_volume_snapshot_basic_usage branch 2 times, most recently from 82cb2df to d8cf378 Compare June 13, 2024 21:26
@codecov Codecov
Copy link

codecov bot commented Jun 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 51.16279% with 42 lines in your changes missing coverage. Please review.

Project coverage is 96.51%. Comparing base (afb1b84) to head (04b6680).
Report is 12 commits behind head on main.

❗ Current head 04b6680 differs from pull request most recent head f9da9c1

Please upload reports for the commit f9da9c1 to get more accurate results.

Files Patch % Lines
pylxd/models/storage_pool.py 44.77% 37 Missing ⚠️
pylxd/models/_model.py 58.33% 5 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #584      +/-   ##
==========================================
- Coverage   97.81%   96.51%   -1.31%     
==========================================
  Files          32       32              
  Lines        3073     3153      +80     
==========================================
+ Hits         3006     3043      +37     
- Misses         67      110      +43

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hamistao hamistao force-pushed the storage_volume_snapshot_basic_usage branch 14 times, most recently from 316e94d to f23163a Compare June 14, 2024 23:48
@hamistao hamistao force-pushed the storage_volume_snapshot_basic_usage branch 2 times, most recently from f60412f to d5b7f52 Compare July 1, 2024 05:36
@hamistao hamistao marked this pull request as ready for review July 1, 2024 05:36
@hamistao
Copy link
Contributor Author

hamistao commented Jul 1, 2024

@simondeziel @markylaing This is ready for review again.

simondeziel
simondeziel requested changes Jul 2, 2024
View reviewed changes
Copy link
Member

@simondeziel simondeziel left a comment

Choose a reason for hiding this comment

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

"integration/test_storage: Remove unused imports" should be folded with integration: Move storage helper functions to testing

doc/source/storage-pools.rst Outdated Show resolved Hide resolved
doc/source/storage-pools.rst Show resolved Hide resolved
@hamistao hamistao force-pushed the storage_volume_snapshot_basic_usage branch from d5b7f52 to 444abde Compare July 2, 2024 08:53
simondeziel
simondeziel requested changes Jul 2, 2024
View reviewed changes
integration/test_storage.py Outdated
new_description = "first snapshot"
first_snapshot.description = new_description
first_snapshot.save(wait=True)
self.assertEquals(volume.snapshots.get("first").description, new_description)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.assertEquals(volume.snapshots.get("first").description, new_description)
self.assertEqual(volume.snapshots.get("first").description, new_description)

This should address the following warning:

integration/test_storage.py::TestStorageVolumeSnapshot::test_create_get_restore_delete_volume_snapshot
  /home/runner/work/pylxd/pylxd/integration/test_storage.py:207: DeprecationWarning: Please use assertEqual instead.
    self.assertEquals(volume.snapshots.get("first").description, new_description)

@hamistao
pylxd: Move managers from storage_pool to managers
c4761b8 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
pylxd/models/storage_pool: Add documentation link to docstring
96e5de0 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd/models/storage_pool: Add content_type to StorageVolume
b84f2aa 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
pylxd/managers: Create StorageVolumeSnapshotManager
19b9cb3 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
pylxd/models/storage_pool: Add restore_from function to `StorageVol…
135af6b 
…ume`

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
pylxd/models: Add post method to Model
1199807 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
pylxd/models/storage-pool: Create StorageVolumeSnapshot
67b8de8 
This class adds support for creating snapshots, restoring a volume from a snapshot, retrieving snapshot information and deleting a snapshot.

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
pylxd/models: Add StorageVolumeSnapshot to pylxd.models
6cd451f 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
pylxd/models/_model: Add __eq__ to Model
554f32e 
This method returns True when two models are of the same class and have the same attributes with the same values.
This is needed for the StorageVolumeSnapshot tests.

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
integration: Move storage helper functions to testing
e6c8f27 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
integration/testing: Add helper functions for snapshot tests
fd3add6 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
integration/test-storage: Use new functions from IntegrationTestCase
a7c6330 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
integration/test_storage: Change API missing error message
e0d5ee8 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
integration/test_storage: Add tests for StorageVolumeSnapshot
ed1a54e 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
doc/source: Add documentation for added features
f9da9c1 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao hamistao force-pushed the storage_volume_snapshot_basic_usage branch from 444abde to f9da9c1 Compare July 2, 2024 13:03
@simondeziel simondeziel merged commit 46b58e6 into canonical:main Jul 2, 2024
8 checks passed
@simondeziel simondeziel mentioned this pull request Jul 3, 2024
Closed
@hamistao hamistao deleted the storage_volume_snapshot_basic_usage branch July 18, 2024 02:55
@hamistao hamistao mentioned this pull request Oct 1, 2024
Closed
simondeziel added a commit that referenced this pull request Oct 2, 2024
@simondeziel
build(deps-dev): bump pytest-cov from 2.10.1 to 5.0.0 (#610)
9d485a4 
Bumps [pytest-cov](https://github.com/pytest-dev/pytest-cov) from 2.10.1
to 5.0.0.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/pytest-dev/pytest-cov/blob/master/CHANGELOG.rst">pytest-cov's
changelog</a>.</em></p>
<blockquote>
<h2>5.0.0 (2024-03-24)</h2>
<ul>
<li>Removed support for xdist rsync (now deprecated).
Contributed by Matthias Reichenbach in
<code>[#623](pytest-dev/pytest-cov#623)
&lt;https://github.com/pytest-dev/pytest-cov/pull/623&gt;</code>_.</li>
<li>Switched docs theme to Furo.</li>
<li>Various legacy Python cleanup and CI improvements.
Contributed by Christian Clauss and Hugo van Kemenade in
<code>[#630](pytest-dev/pytest-cov#630)
&lt;https://github.com/pytest-dev/pytest-cov/pull/630&gt;</code><em>,
<code>[#631](pytest-dev/pytest-cov#631)
&lt;https://github.com/pytest-dev/pytest-cov/pull/631&gt;</code></em>,
<code>[#632](pytest-dev/pytest-cov#632)
&lt;https://github.com/pytest-dev/pytest-cov/pull/632&gt;</code>_ and
<code>[#633](pytest-dev/pytest-cov#633)
&lt;https://github.com/pytest-dev/pytest-cov/pull/633&gt;</code>_.</li>
<li>Added a <code>pyproject.toml</code> example in the docs.
Contributed by Dawn James in
<code>[#626](pytest-dev/pytest-cov#626)
&lt;https://github.com/pytest-dev/pytest-cov/pull/626&gt;</code>_.</li>
<li>Modernized project's pre-commit hooks to use ruff. Initial POC
contributed by
Christian Clauss in
<code>[#584](pytest-dev/pytest-cov#584)
&lt;https://github.com/pytest-dev/pytest-cov/pull/584&gt;</code>_.</li>
</ul>
<h2>4.1.0 (2023-05-24)</h2>
<ul>
<li>Updated CI with new Pythons and dependencies.</li>
<li>Removed rsyncdir support. This makes pytest-cov compatible with
xdist 3.0.
Contributed by Sorin Sbarnea in
<code>[#558](pytest-dev/pytest-cov#558)
&lt;https://github.com/pytest-dev/pytest-cov/pull/558&gt;</code>_.</li>
<li>Optimized summary generation to not be performed if no reporting is
active (for example,
when <code>--cov-report=''</code> is used without
<code>--cov-fail-under</code>).
Contributed by Jonathan Stewmon in
<code>[#589](pytest-dev/pytest-cov#589)
&lt;https://github.com/pytest-dev/pytest-cov/pull/589&gt;</code>_.</li>
<li>Added support for JSON reporting.
Contributed by Matthew Gamble in
<code>[#582](pytest-dev/pytest-cov#582)
&lt;https://github.com/pytest-dev/pytest-cov/pull/582&gt;</code>_.</li>
<li>Refactored code to use f-strings.
Contributed by Mark Mayo in
<code>[#572](pytest-dev/pytest-cov#572)
&lt;https://github.com/pytest-dev/pytest-cov/pull/572&gt;</code>_.</li>
<li>Fixed a skip in the test suite for some old xdist.
Contributed by a bunch of people in
<code>[#565](pytest-dev/pytest-cov#565)
&lt;https://github.com/pytest-dev/pytest-cov/pull/565&gt;</code>_.</li>
</ul>
<h2>4.0.0 (2022-09-28)</h2>
<p><strong>Note that this release drops support for
multiprocessing.</strong></p>
<ul>
<li>
<p><code>--cov-fail-under</code> no longer causes <code>pytest
--collect-only</code> to fail
Contributed by Zac Hatfield-Dodds in
<code>[#511](pytest-dev/pytest-cov#511)
&lt;https://github.com/pytest-dev/pytest-cov/pull/511&gt;</code>_.</p>
</li>
<li>
<p>Dropped support for multiprocessing (mostly because <code>issue 82408
&lt;https://github.com/python/cpython/issues/82408&gt;</code>_). This
feature was
mostly working but very broken in certain scenarios and made the test
suite very flaky and slow.</p>
<p>There is builtin multiprocessing support in coverage and you can
migrate to that. All you need is this in your
<code>.coveragerc</code>::</p>
<p>[run]
concurrency = multiprocessing</p>
</li>
</ul>
<!-- raw HTML omitted -->
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/5295ce01c84262cec88f31255e9ac538718f3047"><code>5295ce0</code></a>
Bump version: 4.1.0 → 5.0.0</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/1181b067972bf94569f8011f3b18f271690f9ab1"><code>1181b06</code></a>
Update changelog.</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/9757222e2e044361e70125ebdd96e5eb87395983"><code>9757222</code></a>
Fix a minor grammar error (<a
href="https://github.com/pytest-dev/pytest-cov/issues/636">#636</a>)</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/9f5cd81a0dbe3fe41681efdbef516c08988fe8ff"><code>9f5cd81</code></a>
Cleanup releasing instructions. Closes <a
href="https://github.com/pytest-dev/pytest-cov/issues/616">#616</a>.</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/93b5047ec5050d63c10a6fe16a09b671a7a03df8"><code>93b5047</code></a>
Add test for pyproject.toml loading without explicit --cov-config. Ref
<a
href="https://github.com/pytest-dev/pytest-cov/issues/508">#508</a>.</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/ff50860d7c67b920503745d92a3f0944cf41f982"><code>ff50860</code></a>
docs: add config instructions for pyproject.toml.</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/4a5a4b5fa4b1c63ddcab5cbc1813798c9b6f1d36"><code>4a5a4b5</code></a>
Keep GitHub Actions up to date with GitHub's Dependabot</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/1d7f55963d5138f41c452a946f7cca7e0b6ee8b2"><code>1d7f559</code></a>
Fix or remove URLs that are causing docs tests to fail</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/6a5af8e85b8242ac815f33e26adf9068f5f0ebc3"><code>6a5af8e</code></a>
Update changelog.</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/d9fe8dfed15023d3410dd299c5092e755b8981c2"><code>d9fe8df</code></a>
Switch to furo. Closes <a
href="https://github.com/pytest-dev/pytest-cov/issues/618">#618</a>.</li>
<li>Additional commits viewable in <a
href="https://github.com/pytest-dev/pytest-cov/compare/v2.10.1...v5.0.0">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pytest-cov&package-manager=pip&previous-version=2.10.1&new-version=5.0.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simondeziel simondeziel simondeziel approved these changes

@markylaing markylaing Awaiting requested review from markylaing

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hamistao @simondeziel