Skip to content

channel program: test that encryption and encryptionroot properties are gettable #12335

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
Closed
wants to merge 1 commit into from
Closed

channel program: test that encryption and encryptionroot properties are gettable #12335

wants to merge 1 commit into from

Conversation

@grahamc
Copy link
Contributor

@grahamc grahamc commented Jul 8, 2021 •
edited
Loading

Motivation and Context

Test zfs.get_prop on encryption and encryptionroot. These tests are novel because today they appear to always return "off" and null, respectively. Reported in #12337

Description

See above.

How Has This Been Tested?

I'm hoping the GitHub CI will run them and verify they fail 😬

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@grahamc grahamc force-pushed the zcp-encryption branch 2 times, most recently from dc40f0b to 8c583a2 Compare July 8, 2021 14:00
@grahamc grahamc mentioned this pull request Jul 8, 2021
Open
@grahamc
channel program: test that encryption properties are gettable
a77fdf6 
It appears that the current behavior is broken:

* 'encryption' always being "off"
* 'encryptionroot' always being null

Signed-off-by: Graham Christensen <graham@determinate.systems>
gmelikov
gmelikov approved these changes Jul 9, 2021
View reviewed changes
Copy link
Member

@gmelikov gmelikov left a comment

Choose a reason for hiding this comment

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

LGTM, but we should merge it after fixes to remain CI green for others.

@jwk404
Copy link
Contributor

jwk404 commented Jul 12, 2021

LGTM also, and I agree with @gmelikov about landing the tests after the bug is fixed. @grahamc is that something you're working on?

@jwk404 jwk404 added the Status: Blocked Depends on another pending change label Jul 12, 2021
@grahamc
Copy link
Contributor Author

grahamc commented Jul 14, 2021

I took a look at it with Matt's suggestion of:

Looks like it is not handled there so it will zcp_get_system_prop() will fall back on get_zap_prop() which may not be doing exactly the right thing for this property.

but I didn't make any progress. I don't think I can fix this bug, sorry.

@jwk404
Copy link
Contributor

jwk404 commented Jul 22, 2021

I'm going to close out this PR, while adding a note in 12337 that we need to be sure to commit these tests once the bug is fixed.

@jwk404 jwk404 closed this Jul 22, 2021
@snajpa snajpa mentioned this pull request Apr 27, 2025
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gmelikov gmelikov gmelikov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jwk404 jwk404

Labels

Status: Blocked Depends on another pending change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@grahamc @jwk404 @gmelikov