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

system tests: enable sdnotify tests #7317

Merged
merged 1 commit into from
Aug 17, 2020
Merged

system tests: enable sdnotify tests #7317

merged 1 commit into from
Aug 17, 2020

Conversation

edsantiago
Copy link
Member

Oops. PR #6693 (sdnotify) added tests, but they were disabled
due to broken crun on f31. I tried for three weeks to get a
magic CI:IMG PR to update crun on the CI VMs ... but in that
time I forgot to actually enable those new tests.

This PR removes a 'skip'. It also changes the test image,
from fedora:latest to :31, because (sigh) :latest removed
the systemd-notify tool.

WARNING WARNING WARNING: the symptom of a missing systemd-notify
is that podman will hang forever, not even stopped by the timeout
command in podman_run! (Filed: #7316). This means that if the
sdnotify-in-container test ever fails, the symptom will be that
Cirrus itself will time out (2 hours?). This is horrible. I
don't know what to do about it other than push for a fix for 7316.

Signed-off-by: Ed Santiago [email protected]

@rhatdan
Copy link
Member

rhatdan commented Aug 13, 2020

LGTM

1 similar comment
@TomSweeneyRedHat
Copy link
Member

LGTM

@edsantiago
Copy link
Member Author

Note: @vrothberg's #7312 mucks with NOTIFY_SOCKET, and I pushed this PR before that one merged. I did, of course, test (root, rootless; my f32 laptop and a rawhide VM) that both work together. I can rebase and push if anyone is nervous.

@edsantiago edsantiago mentioned this pull request Aug 14, 2020
Closed
@edsantiago
system tests: enable sdnotify tests
60ab5f3 
Oops. PR containers#6693 (sdnotify) added tests, but they were disabled
due to broken crun on f31. I tried for three weeks to get a
magic CI:IMG PR to update crun on the CI VMs ... but in that
time I forgot to actually enable those new tests.

This PR removes a 'skip', replacing it with a check that systemd
is running plus one more to make sure our runtime is crun. It
looks like sdnotify just doesn't work on Ubuntu (it hangs), and
my guess is that it's a crun/runc issue.

I also changed the test image from fedora:latest to :31, because,
sigh, fedora:latest removed the systemd-notify tool.

WARNING WARNING WARNING: the symptom of a missing systemd-notify
is that podman will hang forever, not even stopped by the timeout
command in podman_run! (Filed: containers#7316). This means that if the
sdnotify-in-container test ever fails, the symptom will be that
Cirrus itself will time out (2 hours?). This is horrible. I
don't know what to do about it other than push for a fix for 7316.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago
Copy link
Member Author

What is verify_test_built_images_task and why is it running instead of all the other things like testing_task? @cevich?

@edsantiago
Copy link
Member Author

@cevich this verify_test_built_images thing, could it have to do with this change in 576ce0f ?

 # Update metadata on VM images referenced by this repository state
 meta_task:

-    depends_on:
-        - "gating"
-        - "vendor"
-        - "varlink_api"
-        - "build_each_commit"
-        - "build_without_cgo"
-

vrothberg
vrothberg reviewed Aug 17, 2020
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2020
@rhatdan
Copy link
Member

rhatdan commented Aug 17, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2020
@openshift-merge-robot openshift-merge-robot merged commit 1db18bf into containers:master Aug 17, 2020
@edsantiago edsantiago mentioned this pull request Aug 18, 2020
Merged
@cevich
Copy link
Member

cevich commented Aug 19, 2020

What is verify_test_built_images_task and why is it running instead of all the other things like testing_task? @cevich?

@edsantiago that should only run when building new VM images, (i.e. CI:IMG magic). I don't believe it will function properly in any other context.

@edsantiago edsantiago mentioned this pull request Aug 19, 2020
Closed
@edsantiago
Copy link
Member Author

@cevich if you click 'View details' on this PR (next to openshift-merge-robot), you'll see a slew of verify-* jobs.

Aw, crap. I bet they triggered because I include CI:IMG in the commit message body. This is why a bad PR got merged: CI never actually ran. Filed #7374. Thanks for the insight, that's what I was missing.

@edsantiago edsantiago mentioned this pull request Aug 19, 2020
Merged
edsantiago added a commit to edsantiago/libpod that referenced this pull request Aug 19, 2020
@edsantiago
Cirrus: special-case CI colon-IMG and colon-DOCS only in subject
71adbcc 
As of a few minutes ago (relative to this commit), Cirrus
defines the CIRRUS_CHANGE_TITLE envariable as "First line
of CIRRUS_CHANGE_MESSAGE"[1]. Replace all conditionals
accordingly.

 [1] cirruslabs/cirrus-ci-docs@f8d2530

Reasoning: up until this PR, the presence of CI:IMG
or CI:DOCS *in the body* of the commit message would trigger
those magic CI code flows. This violates POLA, and actually
led to a bad PR (containers#7317) being merged because CI never ran.

Fixes: containers#7374

Signed-off-by: Ed Santiago <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg left review comments

@baude baude Awaiting requested review from baude

Assignees

@vrothberg vrothberg

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@edsantiago @rhatdan @TomSweeneyRedHat @openshift-ci-robot @cevich @vrothberg @openshift-merge-robot