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

test/ctr.bats: fixes and improvements #4253

Merged
merged 8 commits into from Oct 12, 2020

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Oct 9, 2020

What type of PR is this?

/kind bug

What this PR does / why we need it:

  • fix a few bugs found in ctr.bats tests.
  • make tests more compact (+394 −1,180 lines) and readable (mostly by removing excessive run use).
  • fix the code to pass shellcheck.
  • introduces edit_json helper to modify json using jq
    (the plan is to switch code using sed or python to this one).

The test fixed are:

1. "annotations passed through".

Commit 5f49b2c added this test case.

Obviously, run crictl do not output anything to stdout,
and run grep do not fail if grep exits with non-zero exit code.
So the test was not testing anything.

2. "privileged ctr -- check for rw mounts".

It was broken from the very beginning, since it was supposed to run a
privileged container and check that /sys/fs/cgroup is mounted rw.

Commit d3a50d8 ("privileged: set mounts to rw") added
test/testdata/container_config_privileged.json file, but it is not
used by the test (or any other tests). It also checks for ro mount
while it should check for rw (based on what I understood). Finally,
it used incorrect pod config while creating a container (not sure
if it makes any difference, just observing).

The test case was later amended to not check ro/rw bits since it was
failing on some configurations (see commit 78badc8).

This commit fixes the config used for container (and adds edit_json
function to which hopefully other tests will be eventually converted),
fixes the check for rw mount, and fixes the pod config used to create
container.

While at it, remove the never used container_config_privileged.json.

3. "ctr lifecycle", "ctr execsync should not overwrite initial spec args", "privileged ctr -- check for rw mounts".

Commits a2ec1d4 and 247d465 added a few checks using jq.
Alas, those checks do not work (and never worked); jq always succeeds.

This happened because

  1. in run cmd1 ... | cmd2 the part starting with the pipe
    character is not part of run statement;

  2. run eats cmd1 ... output (into $output variable);

so cmd2 is provided with empty input.

Now,

  1. jq with empty input does not run any filters and thus succeeds
    (even with -e, see [1]).

The fix is to add a separate check that the output is not empty.

While at it, remove run where it's not needed from the other places
in those three tests we fix.

[1] jqlang/jq#1628

Please see individual commits for more details.

Which issue(s) this PR fixes:

Fixes: #3662

Special notes for your reviewer:

Please review commit-by-commit.

Does this PR introduce a user-facing change?

None

@openshift-ci-robot openshift-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 9, 2020
@openshift-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci-robot openshift-ci-robot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. labels Oct 9, 2020
@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #4253 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4253   +/-   ##
=======================================
  Coverage   38.59%   38.59%           
=======================================
  Files         111      111           
  Lines        8893     8893           
=======================================
  Hits         3432     3432           
  Misses       5077     5077           
  Partials      384      384           

@kolyshkin kolyshkin marked this pull request as ready for review October 9, 2020 17:46
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 9, 2020
test/ctr.bats Outdated Show resolved Hide resolved
Fix issues like this one:

> In ctr.bats line 69:
> 	errorconfig=$(cat "$TESTDATA"/container_config.json | python -c 'import json,sys;obj=json.load(sys.stdin);obj["command"] = ["false"]; json.dump(obj, sys.stdout)')
>                           ^-- SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Test cleanup (including stopping+removing all containers and pods,
and stopping crio) is performed via teardown.

Remove the cleanup from those test cases where it's not clearly
part of the test itself.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Fixes warings like this one:

> In ctr.bats line 195:
>	if test $(stat -f -c%T /sys/fs/cgroup) != cgroup2fs; then
>                ^----------------------------^ SC2046: Quote this to prevent word splitting.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 5f49b2c added this test case.

Obviously, `run crictl` do not output anything to stdout,
and `run grep` do not fail if grep exits with non-zero exit code.
So the test was not testing anything.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kolyshkin, mrunalp

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 Oct 9, 2020
This [partially] fixes the test case.

It was broken from the very beginning, since it was supposed to run a
privileged container and check that /sys/fs/cgroup is mounted rw.

Commit d3a50d8 ("privileged: set mounts to rw") added
test/testdata/container_config_privileged.json file, but it is not
used by the test (or any other tests). It also checks for ro mount
while it should check for rw (based on what I understood). Finally,
it used incorrect pod config while creating a container (not sure
if it makes any difference, just observing).

The test case was later amended to not check ro/rw bits since it was
failing on some configurations (see commit 78badc8).

This commit fixes the config used for container (and adds `edit_json`
function to which hopefully other tests will be eventually converted),
fixes the check for rw mount, and fixes the pod config used to create
container.

While at it, remove the never used container_config_privileged.json.

The issues that are not fixed (will be addressed separately) are:
 - non-working check via jq;
 - excessive use of `run`.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commits a2ec1d4 and 247d465 added a few checks
using jq in the following test cases:

 * ctr lifecycle
 * ctr execsync should not overwrite initial spec args
 * privileged ctr -- check for rw mounts

Alas, those checks do not work (and never worked); jq always succeeds.

This happened because

1. in `run cmd1 ... | cmd2` the part starting with the pipe
   character is not part of `run` statement;

2. `run` eats `cmd1 ...` output (into `$output` variable);

so `cmd2` is provided with empty input.

Now,

3. `jq` with empty input does not run any filters and thus succeeds
   (even with `-e`, see [1]).

The fix is to add a separate check that the output is not empty.

While at it, remove `run` where it's not needed from the other places
in those three tests we fix.

[1] jqlang/jq#1628

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This commit is brought to you by

	shellcheck -f diff ctr.bats | patch -p1

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Collaborator Author

Not sure what is going on. I have amended and pushed this branch, but github is not picking it up here.

@kolyshkin kolyshkin force-pushed the test-ctr branch 2 times, most recently from d398e11 to 4ab4128 Compare October 9, 2020 22:26
@kolyshkin
Copy link
Collaborator Author

/retest

@mrunalp
Copy link
Member

mrunalp commented Oct 11, 2020

/lgtm

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

mrunalp commented Oct 12, 2020

/retest

@kolyshkin
Copy link
Collaborator Author

single failure with e2e_features_fedora (which I've seen before):

1/5226 Tests Failed.
expand_less
Kubernetes e2e suite: [sig-network] IngressClass [Feature:Ingress] should not set default value if no default IngressClass [Serial] expand_less	0s
/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/network/ingressclass.go:61
Oct 12 01:36:32.373: Unexpected error:
    <*errors.StatusError | 0xc003a5f9a0>: {
        ErrStatus: {
            TypeMeta: {Kind: "", APIVersion: ""},
            ListMeta: {
                SelfLink: "",
                ResourceVersion: "",
                Continue: "",
                RemainingItemCount: nil,
            },
            Status: "Failure",
            Message: "ingressclasses.networking.k8s.io \"ingressclass1\" already exists",
            Reason: "AlreadyExists",
            Details: {
                Name: "ingressclass1",
                Group: "networking.k8s.io",
                Kind: "ingressclasses",
                UID: "",
                Causes: nil,
                RetryAfterSeconds: 0,
            },
            Code: 409,
        },
    }
    ingressclasses.networking.k8s.io "ingressclass1" already exists
occurred
/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/network/ingressclass.go:63

There is an open issue (kubernetes/kubernetes#92147) and looks like the related fix (kubernetes/kubernetes#93427 is in our codebase but it's not enough).

@kolyshkin
Copy link
Collaborator Author

/retest

@kolyshkin
Copy link
Collaborator Author

/retest

@kolyshkin
Copy link
Collaborator Author

No logs for "failed" integration_crun, it seems it just went astray.

/retest

@openshift-merge-robot openshift-merge-robot merged commit de9ef40 into cri-o:master Oct 12, 2020
kolyshkin added a commit to kolyshkin/cri-o that referenced this pull request Oct 13, 2020
shfmt has recently added support for bats files formatting
(see mvdan/sh#600), so let's use it.
We have to use git HEAD for now since there was no release done
with this feature yet.

Unfortunately, shfmt introduces an alledged regression (see [1])
so we have to specify `-ln bash` explicitly in Makefile.

This commit has brought to you by

	go get 'mvdan.cc/sh/v3@master'
	go mod vendor
	go mod tidy

[1] cri-o#4253

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix check for rw mounts test
7 participants