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

Revision 8 bump #3652

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Revision 8 bump #3652

merged 1 commit into from
Nov 8, 2024

Conversation

jbaublitz
Copy link
Member

Blocked by #3637

@jbaublitz jbaublitz requested a review from mulkieran July 18, 2024 18:38
@jbaublitz jbaublitz self-assigned this Jul 18, 2024
@jbaublitz jbaublitz force-pushed the r8 branch 3 times, most recently from 95f55e0 to df15ee7 Compare July 22, 2024 22:01
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit df15ee7. @martinpitt, @jelly, @mvollmer please check.

@jbaublitz jbaublitz force-pushed the r8 branch 2 times, most recently from 86a5b29 to 7fcb04f Compare July 26, 2024 20:57
@jbaublitz jbaublitz force-pushed the r8 branch 6 times, most recently from a1056d3 to 03363d7 Compare August 15, 2024 14:21
@jbaublitz jbaublitz force-pushed the r8 branch 3 times, most recently from 85a5e0b to a205492 Compare August 21, 2024 18:00
@packit-as-a-service Packit-as-a-Service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/stratis-storage-stratisd-3652
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@mulkieran
Copy link
Member

@jbaublitz Could you refresh this in whatever way is necessary?

@jbaublitz
Copy link
Member Author

@mulkieran Feel free to review again, but I think I moved everything around appropriately after looking at what was released in 3.7.0.

@mulkieran
Copy link
Member

@mulkieran Feel free to review again, but I think I moved everything around appropriately after looking at what was released in 3.7.0.

Ok. I'll handle the stratis-cli and testing repo failures.

mulkieran
mulkieran requested changes Nov 4, 2024
View reviewed changes
Copy link
Member

@mulkieran mulkieran left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The test errors TypeError: Fewer items found in D-Bus signature than in Python arguments are because the r8 StartPool method does not take a file descriptor for the passphrase but this change was introduced to support the metadata rework. I think that the thing to do is to port that change from the r7 revision to the r8 revision and remove it from the r7 revision. I think doing it in a separate commit will help keep that step clear.

Correction: I had this backward. I believe that it is the testing repo that needs to be updated and that the StartPool failures are what should be expected.

mulkieran
mulkieran requested changes Nov 4, 2024
View reviewed changes
src/dbus_api/pool/mod.rs Outdated
.add_m(pool_3_0::rebind_clevis_method(&f))
.add_m(pool_3_0::rename_method(&f))
.add_m(pool_3_3::grow_physical_device_method(&f))
.add_m(pool_3_7::get_metadata_method(&f))
Copy link
Member

Choose a reason for hiding this comment

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

There's one method here that is missing, get_fs_metadata_method. Added in r7.

@mulkieran
Copy link
Member

mulkieran commented Nov 7, 2024 •
edited
Loading

In the dbus monitor tests I'm finding that, for r7 only, the updated value of the D-Bus StoppedPools property has the following extra chunks, while the final value does not. Most likely this means, that, for r7 only, a signal was put on the D-Bus for the enriched, r8, value of StoppedPools. At the end of the check, though, when the GetManagedObjects() call is made, the StoppedPools value associated with r7 is the correct one, without the metadata_version, etc. fields.

                    dbus.String("metadata_version"): dbus.Struct(
                        (dbus.Boolean(True), dbus.UInt64(2)),
                        signature=None,
                        variant_level=1,
                    ),
                    dbus.String("features"): dbus.Struct(
                        (
                            dbus.Boolean(True),
                            dbus.Dictionary({}, signature=dbus.Signature("sb")),
                        ),
                        signature=None,
                        variant_level=1,
                    ),

Reference: https://github.com/stratis-storage/stratisd/actions/runs/11714462886/job/32629190208?pr=3545

mulkieran
mulkieran requested changes Nov 7, 2024
View reviewed changes
src/dbus_api/pool/mod.rs
@@ -407,6 +440,22 @@ pub fn get_pool_properties(
consts::POOL_OVERPROV_PROP => shared::pool_overprov_enabled(pool),
consts::POOL_NO_ALLOCABLE_SPACE_PROP => shared::pool_no_alloc_space(pool),
consts::POOL_METADATA_VERSION_PROP => shared::pool_metadata_version(pool)
},
Copy link
Member

Choose a reason for hiding this comment

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

I believe that you should remove POOL_METADATA_VERSION_PROP from the 3_7 interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, done.

@jbaublitz jbaublitz force-pushed the r8 branch 2 times, most recently from 0958f3c to 41dd06e Compare November 7, 2024 15:12
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 0958f3c. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/stratis-storage-stratisd-3652
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

2 similar comments
@packit-as-a-service Packit-as-a-Service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/stratis-storage-stratisd-3652
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@packit-as-a-service Packit-as-a-Service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/stratis-storage-stratisd-3652
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@mulkieran
Copy link
Member

merging this, stratis-cli, and testing PRs, and then all should be back to normal.

@mulkieran mulkieran merged commit 677593e into stratis-storage:master Nov 8, 2024
48 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mulkieran mulkieran mulkieran approved these changes

Assignees

@jbaublitz jbaublitz

Labels
None yet
Projects
No open projects
2024November
Status: Done(1)
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jbaublitz @mulkieran