runtime: Fix TestCheckHostIsVMContainerCapable unstablity issue

[justxuewei]

TestCheckHostIsVMContainerCapable removes sysModuleDir to simulate a
case that the kernel modules are not loaded. However,
checkKernelModules() executes modprobe <module> if a module not
found in that directory. Loading those modules is required to be denied
temporarily.

Fixes: #8390

Signed-off-by: Xuewei Niu niuxuewei.nxw@antgroup.com

[fidencio]

lgtm, thanks @justxuewei!

[studychao]

LGTM

[studychao]

/test

[gkurz]

Blocking this temporarily as it already has two approvals.

[gkurz]

I have two remarks here :

1. the "failure" test already removes the files on disk. This means they won't be loadable anymore and that we don't need to bother with a deny list,
2. making sure that the modules are unloaded from memory is rather a requirement of the "failure" test. For clarity, I'd move this block under the // remove the modules to force a failure comment.

[justxuewei]

Removing those simulated module files is not enough for passing the "failure" test. Let me give a more details about that:

1. sysModuleDir is replaced with a temp dir, e.g. "/tmp/tmp.xxGmSePqj2".
2. Call createModules(). The function just creates regular files to simulate system modules.

   kata-containers/src/runtime/cmd/kata-runtime/kata-check_test.go

   Lines 68 to 71 in 3b2fb6a

   	if !d.isDir {
   	err = createFile(d.path, d.contents)
   	assert.NoError(err)
   	}

3. Remove sysModuleDir under the "// remove the modules to force a failure" comment.
4. Call haveKernelModule(). This function checks existence of a kernel module. Three steps are involved:
   1. If a kernel module exists at sysModuleDir, return true. In this case, it won't return as the dir was removed.

      kata-containers/src/runtime/cmd/kata-runtime/kata-check.go

      Lines 139 to 142 in 3b2fb6a

      	path := filepath.Join(sysModuleDir, module)
      	if katautils.FileExists(path) {
      	return true
      	}

   2. Check if the current user is root. (Our test is started with sudo, it won't return here.)
   3. Try to load the module. Returns false if it fails. Please note that if a module was loaded in the memory or wasn't added to blacklist, loading module will be succeed.

      kata-containers/src/runtime/cmd/kata-runtime/kata-check.go

      Lines 152 to 156 in 3b2fb6a

      	cmd := exec.Command(modProbeCmd, module)
      	if output, err := cmd.CombinedOutput(); err != nil {
      	kmodLog.WithError(err).WithField("output", string(output)).Warnf("modprobe insert module failed")
      	return false
      	}

   4. Return true.

I admit this patch introduces dirty works. But I have no idea how to pass the test without this. Wdyt? @gkurz

[justxuewei]

Plus, the blacklist file is restored after the function is completed. It won't poison environment.

[gkurz]

@justxuewei any idea how this test can even pass without your fix ?

[justxuewei]

Things are getting complicated now. Commit dd530ba removes the failure test. Runtime's CI tests, make test and sudo -E PATH="$PATH" make test, will pass at the expense of declining coverage. This patch is still valuable to be reviewed and merged.

[fidencio]

Folks, right now we have all the PRs being blocked on this issue.
We have 2 options here, from my personal point of view.

1. Merge it, unblock other PRs, and figure out what's going on.
2. Mark the static-checks that are failing as non-required, and have this properly fixed on this PR.

I'm fine with both options, but I'd like to unblock main ASAP, so I need the opinion from the reviewers on what to do.

[gkurz]

Folks, right now we have all the PRs being blocked on this issue. We have 2 options here, from my personal point of view.

1. Merge it, unblock other PRs, and figure out what's going on.
2. Mark the static-checks that are failing as non-required, and have this properly fixed on this PR.

I'm fine with both options, but I'd like to unblock main ASAP, so I need the opinion from the reviewers on what to do.

Sorry for the delay. @justxuewei's clarifications are enough for now. Let's go with option 1 for the sake of unblocking everyone, but I really want this to be reworked again afterwards.

[gkurz]

Thanks for unblocking the CI @justxuewei !

[gkurz]

/test

[justxuewei]

/test