Skip to content

Conversation

@sergio-correia
Copy link
Member

Enhancement:

Fix idempotence issue when binding fails to be added

Reason:

Sometimes, the role will not be able to add the required bindings, in which case it is expected to rollback and undo any change it has done.

In certain cases, the rollback was not performed correctly, and caused idempotence issues. We fix that by performing a backup of the LUKS header before doing the operations, so that we can properly restore it in the case the operation cannot be completed successfully.

Result:

The role now performs correctly and maintains the idempotence property in the cases where the binding failed to be added.

Issue Tracker Tickets (Jira or BZ if any): https://issues.redhat.com/browse/RHEL-84891

@sergio-correia sergio-correia force-pushed the idempotence branch 2 times, most recently from 4b58797 to bfe9bee Compare April 4, 2025 13:42
@sergio-correia sergio-correia changed the title Fix idempotence issue when binding fails to be added fix: fix idempotence issue when binding fails to be added Apr 4, 2025
@rjeffman
Copy link

rjeffman commented Apr 5, 2025

[citest]

@sergio-correia
Copy link
Member Author

[citest]

@sergio-correia sergio-correia marked this pull request as draft April 6, 2025 14:20
@sergio-correia sergio-correia changed the title fix: fix idempotence issue when binding fails to be added fix: idempotence issue when binding fails to be added Apr 14, 2025
@sergio-correia sergio-correia marked this pull request as ready for review April 14, 2025 20:30
@sergio-correia sergio-correia requested a review from richm April 14, 2025 21:33
richm
richm previously approved these changes Apr 14, 2025
Copy link
Contributor

@richm richm left a comment

Choose a reason for hiding this comment

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

lgtm - how will we test this to ensure idempotence?

@richm
Copy link
Contributor

richm commented Apr 28, 2025

lgtm - how will we test this to ensure idempotence?

@sergio-correia do you want to merge this now and work on testing later?

@richm
Copy link
Contributor

richm commented Jul 2, 2025

@sergio-correia what should we do with this PR?

@richm
Copy link
Contributor

richm commented Nov 11, 2025

for the dd command you'll need to add coreutils to __nbde_client_packages wherever it is defined, and to .ostree/packages-runtime.txt

otherwise, lgtm

Sometimes, the role will not be able to add the required bindings, in
which case it is expected to rollback and undo any change it has done.

In certain cases, the rollback was not performed correctly, and caused
idempotence issues. We fix that by performing a backup of the LUKS
header before doing the operations, so that we can properly restore it
in the case the operation cannot be completed successfully.

Signed-off-by: Sergio Correia <[email protected]>
@sergio-correia
Copy link
Member Author

for the dd command you'll need to add coreutils to __nbde_client_packages wherever it is defined, and to .ostree/packages-runtime.txt

otherwise, lgtm

Thanks for taking another look. I updated the vars to include coreutils, however I am a bit lost as to the .ostree/packages-runtime.txt -- also, it would be great if those qemu tests ran properly; are those failures expected?

@richm
Copy link
Contributor

richm commented Nov 12, 2025

for the dd command you'll need to add coreutils to __nbde_client_packages wherever it is defined, and to .ostree/packages-runtime.txt
otherwise, lgtm

Thanks for taking another look. I updated the vars to include coreutils, however I am a bit lost as to the .ostree/packages-runtime.txt

The format is one package per line in alphanum order - so just add it in a line between clevis-systemd and iproute

-- also, it would be great if those qemu tests ran properly; are those failures expected?

Yeah :-( working on it

@richm
Copy link
Contributor

richm commented Nov 12, 2025

[citest]

@richm
Copy link
Contributor

richm commented Nov 12, 2025

tests are failing because some test is not cleaning up after itself, causing the tests_bind_high_availability.yml to fail

@richm
Copy link
Contributor

richm commented Nov 12, 2025

@richm
Copy link
Contributor

richm commented Nov 12, 2025

[citest]

@richm
Copy link
Contributor

richm commented Nov 12, 2025

sergio-correia#2

@richm
Copy link
Contributor

richm commented Nov 12, 2025

[citest]

@richm
Copy link
Contributor

richm commented Nov 13, 2025

[citest]

@richm richm merged commit 2b3b4fc into linux-system-roles:main Nov 13, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants