Skip to content

ci: fix delete.bats for GHA#3542

Merged
hqhq merged 1 commit intoopencontainers:mainfrom
kolyshkin:ci-fix-vs-azsec
Dec 16, 2022
Merged

ci: fix delete.bats for GHA#3542
hqhq merged 1 commit intoopencontainers:mainfrom
kolyshkin:ci-fix-vs-azsec

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin commented Jul 28, 2022

TL;DR: this is a forward-port of f46c0da (part of #3538) to main branch, fixing a CI flake caused by a GHA CI env peculiarity.

A couple of test cases in delete.bats check that a particular cgroup
exists (or doesn't exist) using find. This is now resulting in errors
like these:

    find: ‘/sys/fs/cgroup/blkio/azsec’: Permission denied
    find: ‘/sys/fs/cgroup/blkio/azsec_clamav’: Permission denied
    find: ‘/sys/fs/cgroup/cpu,cpuacct/azsec’: Permission denied
    find: ‘/sys/fs/cgroup/cpu,cpuacct/azsec_clamav’: Permission denied
    find: ‘/sys/fs/cgroup/memory/azsec’: Permission denied
    find: ‘/sys/fs/cgroup/memory/azsec_clamav’: Permission denied

leading to test case failures.

Apparently, GHA runs something else on a test box, so we get this.

To fix, ignore non-zero exit code from find, and redirect its stderr
to /dev/null.

AkihiroSuda
AkihiroSuda previously approved these changes Aug 13, 2022
@kolyshkin
Copy link
Copy Markdown
Contributor Author

@thaJeztah PTAL

thaJeztah
thaJeztah previously approved these changes Nov 1, 2022
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

Should we disable this check? Not sure if it's really tenable to require all PR's to be up-to-date;
Screenshot 2022-11-01 at 16 29 17

@kolyshkin
Copy link
Copy Markdown
Contributor Author

Should we disable this check? Not sure if it's really tenable to require all PR's to be up-to-date

This actually helps to ensure that two PRs that both took a long time to merge won't break one another. I've seen a few cases of that in the past, and now we have a mechanism (update with rebase button) which rebases the PR without losing LGTMs.

So, I am going to rebase it and, if CI is all nice and green, merge it.

@kolyshkin
Copy link
Copy Markdown
Contributor Author

This actually helps to ensure that two PRs that both took a long time to merge won't break one another. I've seen a few cases of that in the past, and now we have a mechanism (update with rebase button) which rebases the PR without losing LGTMs.

So, I am going to rebase it and, if CI is all nice and green, merge it.

Overall, this is an inherent issue with GitHub. There's no way to see what happened in between force pushes, so we can't have "leave LGTMs after force-push" enabled.

For this and many other reasons, I'd switch to gerrit, but I'm not sure other @opencontainers/runc-maintainers concur.

@kolyshkin
Copy link
Copy Markdown
Contributor Author

and now we have a mechanism (update with rebase button) which rebases the PR without losing LGTMs.

Hmm, either something has changed or I did a mistake a few minutes ago, but this is no longer working the way it's supposed to (meaning, I can rebase the branch without losing LGTMs). Oh my :(

@kolyshkin
Copy link
Copy Markdown
Contributor Author

OTOH it just worked for #3635, so maybe if there are some noticeable changes during the rebase, LGTMs are lost, and if not, they are retained.

mrunalp
mrunalp previously approved these changes Nov 2, 2022
AkihiroSuda
AkihiroSuda previously approved these changes Nov 2, 2022
mrunalp
mrunalp previously approved these changes Nov 3, 2022
@kolyshkin
Copy link
Copy Markdown
Contributor Author

@thaJeztah @AkihiroSuda PTAL

@kolyshkin
Copy link
Copy Markdown
Contributor Author

@opencontainers/runc-maintainers this is an easy-to-review CI-only fix which sits here since July. Anything I can do to move this forward?

@kolyshkin kolyshkin added this to the 1.2.0 milestone Dec 15, 2022
A couple of test cases in delete.bats check that a particular cgroup
exists (or doesn't exist) using find. This is now resulting in errors
like these:

        find: ‘/sys/fs/cgroup/blkio/azsec’: Permission denied
        find: ‘/sys/fs/cgroup/blkio/azsec_clamav’: Permission denied
        find: ‘/sys/fs/cgroup/cpu,cpuacct/azsec’: Permission denied
        find: ‘/sys/fs/cgroup/cpu,cpuacct/azsec_clamav’: Permission denied
        find: ‘/sys/fs/cgroup/memory/azsec’: Permission denied
        find: ‘/sys/fs/cgroup/memory/azsec_clamav’: Permission denied

leading to test case failures.

Apparently, GHA runs something else on a test box, so we get this.

To fix, ignore non-zero exit code from find, and redirect its stderr
to /dev/null.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@hqhq hqhq merged commit 7c14308 into opencontainers:main Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants