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

Actually test that logs are flowing to sd-log #1087

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Jun 17, 2024

Status

Ready for review

Description of Changes

Instead of checking that a folder exists, we now trigger a log entry in all VMs and then look for that log entry in the logs.

We also verify that sd-gpg, sd-log and sd-whonix logs are not flowing.

Testing

  • CI passes
  • Visual review

Deployment

Any special considerations for deployment? n/a

Checklist

  • All tests (make test) pass in dom0

Comment on lines 42 to 53
# TODO: test a sd-viewer based dispVM
for vm_name in ["sd-app", "sd-devices", "sd-proxy", "sd-gpg"]:
# The sudo call will make it into syslog
subprocess.check_call(["qvm-run", vm_name, f"sudo echo {token}"])

# Now look in the log files to find the token
for vm_name in ["sd-app", "sd-devices", "sd-proxy"]:
contents = self._get_file_contents(f"/home/user/QubesIncomingLogs/{vm_name}/syslog.log")
self.assertIn(token, contents)

# But nothing for sd-gpg
self.assertFalse(self._fileExists("/home/user/QubesIncomingLogs/sd-gpg/syslog.log"))
Copy link
Member

Choose a reason for hiding this comment

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

Do you see any point in parameterizing this now, such that either:

  1. we have sets logs_expected_from and logs_not_expected_from that we enforce; or even
  2. we have a set log_expected_from that we check strictly against all the VMs on the system?

(2) might well be overkill, but these are the two possibilities that occurred to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think that's a good idea, let me see...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! There's one todo about sd-viewer, because we can't actually run/start sd-viewer, we have to use a disposable based on it. There's a workaround for it in the sd-viewer tests, but it's not reusable yet. Once we're pytest and can have easily shared fixtures, it should be trivial.

@cfm cfm self-assigned this Jun 18, 2024
@legoktm legoktm force-pushed the logs-flow-downstream branch 5 times, most recently from 7d0d41a to a0027c4 Compare June 18, 2024 18:45
@legoktm
Copy link
Member Author

legoktm commented Jun 18, 2024

The test failure looks legit...I suspect there could easily be a race condition since we run the command and then immediately look for it in the logs.

@legoktm
Copy link
Member Author

legoktm commented Jun 18, 2024

I've switched it back to the original style of one loop to run the command, and then another loop to check the results, which hopefully avoids the race condition (I must've ran this 10-15 times when originally writing the tests with no issues). It's possible CI is worse because the VM has less resources than a physical machine.

If this doesn't solve it, we could 1) add a small time.sleep(5) or something, 2) some procedure in which we check the timestamp in the last log entry and wait for it to be after the time we trigger our command.

@cfm
Copy link
Member

cfm commented Jun 25, 2024

Looks like this is now conflicted, @legoktm, but I'll be happy to re-review and merge tomorrow.

Instead of checking that a folder exists, we now trigger a log entry
in all VMs and then look for that log entry in the logs.

We also verify that sd-gpg, sd-log and sd-whonix logs are *not* flowing.
@legoktm
Copy link
Member Author

legoktm commented Sep 23, 2024

Rebased.

Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

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

Thanks, @legoktm.

@cfm cfm added this pull request to the merge queue Oct 1, 2024
Merged via the queue into main with commit 94d36d6 Oct 1, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants