govmm: Use it from our own repo #3496

fidencio · 2022-01-19T17:06:57Z

On #3468 we brought the govmm project into our own repo, now it's time to use it.

We also need to ensure CI is correctly passing and in order to do so I've:

Changed the license header's format
Added some //nolint to the code, as those were not being checked as part of the GoVMM repo

Let's stop using govmm from kata-containers/govmm and let's start using it from our own repo. Fixes: kata-containers#3495 Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>

govmm, from now on, should follow the same guidelines from contributing, copying, and etc as kata-containers does. The go.mod is not needed anymore as the project lives inside the runtime. Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>

govet checks have been ignored on govmm repo, but those are enabled on kata-containers one. So, in order to avoid failing our CIs let's just keep ignoring the checks for the govmm structs and have an issue opened for fixing it whenever someone has cycles to do it. The important bit here is, we're not making anything worse that it already is. :-) Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>

Both projects follow the same license, Apache-2.0, but the header saying that comes from govmm is different from the one expected for the tests present on the kata-containers repo. Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>

fidencio · 2022-01-19T17:07:13Z

/test

devimc

thanks @fidencio

fidencio · 2022-01-19T19:13:11Z

@Jakob-Naucke, It seems we didn't have the tests running against s390x on GoVMM before and that's the reason the issue I'm facing here was never ever caught. :-)

+++ golangci-lint run -c /home/jenkins/workspace/kata-containers-2.0-ubuntu-s390x-PR/go/src/github.com/kata-containers/tests/.ci/.golangci.yml .
qemu_test.go:528:21: undeclared name: `devicePCIBridgeStringReserved` (typecheck)
	testAppend(bridge, devicePCIBridgeStringReserved, t)
	                   ^

EDITED: #3500

fidencio · 2022-01-19T21:04:32Z

/test-s390x

fidencio · 2022-01-19T21:05:08Z

If we can make it pass on s390x, we are good to call /test and have it tested on all other arches (which already passed on a previous run).

fidencio · 2022-01-19T21:10:01Z

/test-s390x

fidencio · 2022-01-19T21:12:00Z

/test-s390x

fidencio · 2022-01-19T21:14:34Z

/test

fidencio · 2022-01-19T22:29:30Z

/test-s390x

fidencio · 2022-01-19T22:54:43Z

/retest-s390x

fidencio · 2022-01-19T22:56:49Z

/retest-s390x

For now a bunch of tests are simply not working. Let's skip them all, and re-enable them once kata-containers/issues/3500 gets fixed. Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>

fidencio · 2022-01-20T00:05:32Z

/test

bergwolf

Thanks!

amshinde · 2022-01-20T03:32:54Z

@fidencio s390 seems to be still failing.

fidencio · 2022-01-20T07:36:50Z

/retest-s390x

fidencio · 2022-01-20T07:47:41Z

@fidencio s390 seems to be still failing.

Yep, but the last failures were the "normal" processes being left behind, rather than something specific to GoVMM.

fidencio · 2022-01-20T08:14:18Z

clh-crio:

not ok 1 Security context
# (in test file k8s-security-context.bats, line 22)
#   `kubectl wait --for=condition=Ready --timeout=$timeout pod "$pod_name"' failed
# [bats-exec-test:32] INFO: k8s configured to use runtimeclass
# pod/security-context-test created
# error: timed out waiting for the condition on pods/security-context-test
# Name:         security-context-test
# Namespace:    default
# Priority:     0
# Node:         ubuntu1804-azure0bbb10/10.0.0.6
# Start Time:   Thu, 20 Jan 2022 08:11:29 +0000
# Labels:       <none>
# Annotations:  <none>
# Status:       Pending
# IP:           
# IPs:          <none>
# Containers:
#   sec-text:
#     Container ID:  
#     Image:         quay.io/prometheus/busybox:latest
#     Image ID:      
#     Port:          <none>
#     Host Port:     <none>
#     Command:
#       tail
#       -f
#       /dev/null
#     State:          Waiting
#       Reason:       ContainerCreating
#     Ready:          False
#     Restart Count:  0
#     Environment:    <none>
#     Mounts:
#       /var/run/secrets/kubernetes.io/serviceaccount from kube-api-access-d9xqn (ro)
# Conditions:
#   Type              Status
#   Initialized       True 
#   Ready             False 
#   ContainersReady   False 
#   PodScheduled      True 
# Volumes:
#   kube-api-access-d9xqn:
#     Type:                    Projected (a volume that contains injected data from multiple sources)
#     TokenExpirationSeconds:  3607
#     ConfigMapName:           kube-root-ca.crt
#     ConfigMapOptional:       <nil>
#     DownwardAPI:             true
# QoS Class:                   BestEffort
# Node-Selectors:              <none>
# Tolerations:                 node.kubernetes.io/not-ready:NoExecute op=Exists for 300s
#                              node.kubernetes.io/unreachable:NoExecute op=Exists for 300s
# Events:
#   Type    Reason     Age   From               Message
#   ----    ------     ----  ----               -------
#   Normal  Scheduled  90s   default-scheduler  Successfully assigned default/security-context-test to ubuntu1804-azure0bbb10
#   Normal  Pulling    87s   kubelet            Pulling image "quay.io/prometheus/busybox:latest"
#   Normal  Pulled     86s   kubelet            Successfully pulled image "quay.io/prometheus/busybox:latest" in 219.462682ms
#   Normal  Created    86s   kubelet            Created container sec-text
#   Normal  Started    86s   kubelet            Started container sec-text
# pod "security-context-test" deleted

fidencio · 2022-01-20T08:42:18Z

@Jakob-Naucke, s390x is constantly failing with:

[cleanup_env.sh:54] INFO: Check no kata processes are left behind after reseting kubernetes
1109985
1110230
1117531
1118679
1118680
1120687
1123381
1124330
[cleanup_env.sh:129] ERROR: Found unexpected /usr/local/bin/containerd-shim-kata-v2 present
make: *** [Makefile:57: kubernetes] Error 1

That's not related to this PR and I will go ahead as soon as the clh-crio CI passes to unblock things here.

Bevisy · 2022-01-20T09:05:20Z

src/runtime/pkg/govmm/qemu/qemu.go

@@ -250,6 +239,7 @@ const (
 )

 // Object is a qemu object representation.
+// nolint: govet


@fidencio By the way, do we need to deal with the problem that arises here, i.e. field alignment?

At some point we may need to, indeed.

One thing to keep in mind is that this specific check was never ever made on GoVMM repo. The reason we see this failure on our repo is because we, however, do check for that.

So, right now, we're not making anything worse, we're just using GoVMM as we've always been using it, without any change.

I also mentioned this in the commit itself:

govmm: Ignore govet checks, at least for now govet checks have been ignored on govmm repo, but those are enabled on kata-containers one. So, in order to avoid failing our CIs let's just keep ignoring the checks for the govmm structs and have an issue opened for fixing it whenever someone has cycles to do it. The important bit here is, we're not making anything worse that it already is. :-) Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>

Did I answer your question, @Bevisy?

Also, pleasel if that's something you'd be willing to work on, feel free to jump on it! With the caveat that with the transition to the rust shim, GoVMM may become ... dead, in the near future.

Thank you for your reply, I know what I need to do next 😄

Our e2e's use either waitForProcess() or `kubectl wait` for waiting resources on the test cluster. Both rely on timeouts, being the later set to 30s by default and the former requires the user to pass wait_time and sleep_time parameters. Some tests leave the defaults, others set a value, and a bunch relies on $timeout (variable defined in .ci/lib.sh). It isn't uncommon to see tests failing on CI because they hit a timeout. This changed all the tests to use a mininum of 90s (current value of $timeout) on wait operatios, with the hope those fails will become really rare. Fixes kata-containers#3496 Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> (cherry picked from commit 66c96a3)

fidencio added 4 commits January 19, 2022 18:02

govmm: Use govmm from our own pkg

ec6655a

Let's stop using govmm from kata-containers/govmm and let's start using it from our own repo. Fixes: kata-containers#3495 Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>

fidencio requested a review from a team as a code owner January 19, 2022 17:06

fidencio added no-backport-needed labels Jan 19, 2022

devimc approved these changes Jan 19, 2022

View reviewed changes

jcvenegas approved these changes Jan 19, 2022

View reviewed changes

fidencio force-pushed the wip/use-govmm-from-kata branch from 1fcc14a to 4d0bb58 Compare January 19, 2022 21:09

fidencio force-pushed the wip/use-govmm-from-kata branch from 4d0bb58 to 24fc84e Compare January 19, 2022 21:11

fidencio force-pushed the wip/use-govmm-from-kata branch from 24fc84e to 2eed94e Compare January 19, 2022 22:29

fidencio force-pushed the wip/use-govmm-from-kata branch from 2eed94e to c482a1e Compare January 19, 2022 22:54

fidencio force-pushed the wip/use-govmm-from-kata branch from c482a1e to 53787d8 Compare January 19, 2022 22:56

govmm: s390x: Skip broken tests

5ce9011

For now a bunch of tests are simply not working. Let's skip them all, and re-enable them once kata-containers/issues/3500 gets fixed. Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>

fidencio force-pushed the wip/use-govmm-from-kata branch from 53787d8 to 5ce9011 Compare January 20, 2022 00:05

bergwolf approved these changes Jan 20, 2022

View reviewed changes

amshinde approved these changes Jan 20, 2022

View reviewed changes

fidencio mentioned this pull request Jan 20, 2022

static checker is not testing against the current PR? kata-containers/tests#4388

Open

fidencio merged commit 1a59c57 into kata-containers:main Jan 20, 2022

fidencio mentioned this pull request Jan 20, 2022

runtime: update runc and image-spec dependencies #3476

Merged

Bevisy reviewed Jan 20, 2022

View reviewed changes

This was referenced Jan 20, 2022

govmm: Bring the project in #3468

Merged

Document the force-skip-ci usage kata-containers/community#248

Open

jodh-intel mentioned this pull request Feb 16, 2022

CCv0: Merge main into CCv0 branch #3665

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

govmm: Use it from our own repo #3496

govmm: Use it from our own repo #3496

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

devimc left a comment

fidencio commented Jan 19, 2022 •

edited

Loading

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 20, 2022

bergwolf left a comment

amshinde commented Jan 20, 2022

fidencio commented Jan 20, 2022

fidencio commented Jan 20, 2022

fidencio commented Jan 20, 2022

fidencio commented Jan 20, 2022

Bevisy Jan 20, 2022

fidencio Jan 20, 2022

Bevisy Jan 20, 2022 •

edited

Loading

govmm: Use it from our own repo #3496

govmm: Use it from our own repo #3496

Conversation

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

devimc left a comment

Choose a reason for hiding this comment

fidencio commented Jan 19, 2022 • edited Loading

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 19, 2022

fidencio commented Jan 20, 2022

bergwolf left a comment

Choose a reason for hiding this comment

amshinde commented Jan 20, 2022

fidencio commented Jan 20, 2022

fidencio commented Jan 20, 2022

fidencio commented Jan 20, 2022

fidencio commented Jan 20, 2022

Bevisy Jan 20, 2022

Choose a reason for hiding this comment

fidencio Jan 20, 2022

Choose a reason for hiding this comment

Bevisy Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

fidencio commented Jan 19, 2022 •

edited

Loading

Bevisy Jan 20, 2022 •

edited

Loading