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.

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 flaky wireguard key rotation test #6090

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

Serock3
Copy link
Contributor

@Serock3 Serock3 commented Apr 9, 2024

Fixes DES-774.


This change is Reviewable

Copy link

linear bot commented Apr 9, 2024

@Serock3 Serock3 marked this pull request as ready for review April 9, 2024 08:27
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2, 21 of 21 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Serock3)


test/test-manager/src/tests/account.rs line 298 at r2 (raw file):

        .expect("Could not stop system service");

    log::info!("Mutating device.json change created field to more than 7 days ago");

This log message is a bit weird, maybe shorten it a bit?

log::info!("Changing created field of `device.json` to more than 7 days ago");

Code quote:

log::info!("Mutating device.json change created field to more than 7 days ago");

test/test-manager/src/tests/account.rs line 318 at r3 (raw file):

    // Check if the key rotation has already occurred when connected to the daemon, otherwise
    // listen for device daemon events until we se the change. We have to register the event

we see the change*

or

we observe the change*

Code quote:

we se the change

test/test-manager/src/tests/account.rs line 331 at r3 (raw file):

    // Current key not yet updated
    if new_key == old_key {

Should this comment be somewhere else?:)

Code quote:

    // Current key not yet updated
    if new_key == old_key {

Copy link
Contributor Author

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 21 of 22 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)


test/test-manager/src/tests/account.rs line 318 at r3 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

we see the change*

or

we observe the change*

Done.


test/test-manager/src/tests/account.rs line 331 at r3 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Should this comment be somewhere else?:)

Done. Rephrased the comment.

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)


test/test-manager/src/tests/account.rs line 332 at r4 (raw file):

    if new_key == old_key {
        log::info!("Listening for device daemon event");
        // Verify rotation has happened within a minute

100 != a minute

@dlon
Copy link
Member

dlon commented Apr 10, 2024

I suspect that this problem is actually caused by API overrides not being cleaned up. As with the API access method tests.

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)

The test was flaky because if a race condition which made the key
rotation missable.
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)

@MarkusPettersson98 MarkusPettersson98 merged commit fa0e78a into main Apr 11, 2024
49 of 50 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the integration-test-fixes branch April 11, 2024 12:07
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