Vendor cleanup #2124

vrothberg · 2019-03-12T07:56:57Z

This PR cleans up the vendored dependencies by:

forcing make vendor to always fetch the latest version of vndr to avoid divergence
pin dependencies that used a branch to releases where possible or commits
align the K8s dependencies to the appropriate releases
add remaining file changes from vndr which have been forgotten in commits prior to this PR
add a hack/tree_status.sh file to check the status of the local git tree
enforce clean dependencies in Travis by first running make vendor followed by ./hack/tree_status.sh
update libpod and its transitive dependencies
fix Travis build

k8s-ci-robot · 2019-03-12T07:57:11Z

Hi @vrothberg. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vrothberg · 2019-03-12T07:58:02Z

@mrunalp @runcom @umohnani8 @haircommander @rhatdan PTAL

vrothberg · 2019-03-12T10:32:01Z

The CI seems to go wild on other PRs as well. However, Travis seems happy with the vendor changes. I'd appreciate if we can this PR in before the others to avoid potentially very hairy rebase conflicts.

oci/oci_unix.go

TomSweeneyRedHat

The changes LGTM, best of luck getting Travis happy.

vrothberg · 2019-03-12T10:52:21Z

The changes LGTM, best of luck getting Travis happy.

Thanks, Tom! This PR was a bit of a challenge as so many things had to be touched.

vrothberg · 2019-03-12T11:07:58Z

/hold

vrothberg · 2019-03-12T14:05:37Z

One step further. We're now hitting a runtime error caused by a conflict between go-criu and run:

checkpoint-restore/go-criu#7

saschagrunert

👍 This cleanup is very great and important in my POV, so I would like to push the topic forward. There are just some small comments/suggestions from my side. Is it still on hold?

saschagrunert · 2019-03-21T08:01:29Z

lib/container_server_linux.go

@@ -9,12 +9,16 @@ import (
 )

 func (c *ContainerServer) addSandboxPlatform(sb *sandbox.Sandbox) {
-	c.state.processLevels[selinux.NewContext(sb.ProcessLabel())["level"]]++
+	// NewContext() always returns nil, so we can safely ignore the error
+	ctx, _ := selinux.NewContext(sb.ProcessLabel())


I would still go for the error handling here since I have the feeling that it will be missed when the day comes that selinux.NewContext returns a real error.

Agreed, from a maintainability perspective we should catch if there ever becomes an error

I think that @rhatdan wants to get rid of those entirely by letting containers/storage handle all label issue but I can update the code now to be on the safe side.

I've had a look at the call stack and don't think it's worth the effort. We had to change a whole bunch of APIs up to the ContainerServer which does not really implement error handling (even for {Add,Remove}Container).

I agree that errors shouldn't be ignored but given this affects large parts of the API, I prefer to defer this to a separate PR.

saschagrunert · 2019-03-21T08:02:58Z

lib/container_server_linux.go

 }

 func (c *ContainerServer) removeSandboxPlatform(sb *sandbox.Sandbox) {
 	processLabel := sb.ProcessLabel()
-	level := selinux.NewContext(processLabel)["level"]
+	// NewContext() always returns nil, so we can safely ignore the error
+	ctx, _ := selinux.NewContext(processLabel)


The same for here. Another option would be to revert the selinux-go related changes and put it in a separate PR like this: #2127 WDYT? (If you disagree I will close my PR)

vendor.conf

vrothberg · 2019-03-21T09:11:03Z

This cleanup is very great and important in my POV, so I would like to push the topic forward. There are just some small comments/suggestions from my side. Is it still on hold?

Thanks. Yes, it's currently on hold for #2123.

Regarding updating more deps: I prefer deferring that to another PR. This one is quite big already and I consider the first step of a series of commits. Once that's in, the deps are stable and CI will catch regressions.

vrothberg · 2019-03-25T08:44:18Z

Resuming work. May God have mercy on me while rebasing.

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

vrothberg · 2019-03-26T10:59:54Z

/test kata-containers

vrothberg · 2019-03-26T11:28:54Z

/test kata-containers

vrothberg · 2019-03-26T11:54:42Z

@mrunalp any idea what's going on? Kata seems to go south on other PRs as well.

rhatdan · 2019-03-26T11:57:29Z

Lets leave the SELinux issues to the other PR that @umohnani8 is working on. It is close.

LGTM

vrothberg · 2019-03-26T12:01:14Z

/test kata-containers

saschagrunert · 2019-03-26T12:55:09Z

LGTM, awesome cleanup!

haircommander · 2019-03-26T14:09:09Z

/retest

vrothberg · 2019-03-26T14:37:23Z

/test kata-containers

vrothberg · 2019-03-26T15:40:25Z

@sboeuf, do you know if the kata job has a hiccup at the moment? Other PRs show similar issues.

sboeuf · 2019-03-26T15:58:56Z

/cc @chavafg

k8s-ci-robot · 2019-03-26T15:58:57Z

@sboeuf: GitHub didn't allow me to request PR reviews from the following users: chavafg.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @chavafg

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chavafg · 2019-03-26T16:23:12Z

the error looks pretty weird...

15:09:21 RUNTIME=kata-runtime ./integration/cri-o/cri-o.sh
15:09:21 Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?

Has something change in the way the cri-o tests are run? It is looking for the docker daemon to be running, which we actually stop it before running the tests.

https://github.com/kata-containers/tests/blob/master/integration/cri-o/cri-o.sh#L95-L100

vrothberg · 2019-03-26T16:26:53Z

Has something change in the way the cri-o tests are run? It is looking for the docker daemon to be running, which we actually stop it before running the tests.

Not to my knowledge. @saschagrunert, as you're the test master, do you have a suspicion?

saschagrunert · 2019-03-26T16:48:22Z

On the cri-o side nothing should have changed. @chavafg could the issue related to that part of the logs:

14:58:51 Set to test rootfs image
14:58:51 Add kata-runtime as a new/default Docker runtime.
14:58:51 manage_ctr_mgr.sh - INFO: docker_info: version: 18.06.3-ce
14:58:51 manage_ctr_mgr.sh - INFO: docker_info: default runtime: runc
14:58:51 manage_ctr_mgr.sh - INFO: docker_info: package name: docker-ce
14:58:52 <13>Mar 26 14:58:52 manage_ctr_mgr.sh: configure_docker: configuring kata-runtime as default docker runtime
14:58:52 ls: cannot access '/etc/systemd/system/docker.service.d/': No such file or directory
14:58:52 ls: cannot access '/etc/systemd/system/docker.service.d/': No such file or directory
14:58:52 Stopping the docker service
14:58:53 Removing /var/lib/docker
14:58:53 Changing docker service configuration
14:58:53 [Service]
14:58:53 Environment=""
14:58:53 Environment=""
14:58:53 ExecStart=
14:58:53 ExecStart=/usr/bin/dockerd -D --add-runtime kata-runtime=/usr/local/bin/kata-runtime --default-runtime=kata-runtime --storage-driver=overlay2
14:58:53 Reloading unit files and starting docker service
14:59:10 Script finished

I think the docker service does not start correctly but is listed as systemd state active. So shutting down docker does not work in that case.

chavafg · 2019-03-26T16:57:53Z

sorry, found the issue. One of our functions in .ci/lib.sh was trying to access docker, but it was already stopped. I removed the part where we were stopping the docker service on our jenkins job.
Should be fixed now, can you restart the job?

haircommander · 2019-03-26T17:00:07Z

Thanks @chavafg

haircommander · 2019-03-26T17:00:48Z

/retest

vrothberg · 2019-03-26T17:17:38Z

Thanks a lot folks :)

haircommander · 2019-03-26T18:15:07Z

LGTM @umohnani8 @mrunalp PTAL

vrothberg · 2019-03-27T10:46:20Z

/hold cancel

giuseppe · 2019-03-27T10:47:49Z

/lgtm

giuseppe · 2019-03-27T11:01:40Z

/approve

k8s-ci-robot · 2019-03-27T11:01:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, vrothberg

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:

~~OWNERS~~ [giuseppe]

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

vrothberg requested review from mrunalp and runcom as code owners March 12, 2019 07:57

k8s-ci-robot requested review from sboeuf and umohnani8 March 12, 2019 07:57

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes labels Mar 12, 2019

TomSweeneyRedHat reviewed Mar 12, 2019

View reviewed changes

oci/oci_unix.go Show resolved Hide resolved

TomSweeneyRedHat reviewed Mar 12, 2019

View reviewed changes

vrothberg force-pushed the vendor-cleanup branch from 51f07a9 to 26140d1 Compare March 12, 2019 10:59

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2019

vrothberg force-pushed the vendor-cleanup branch 3 times, most recently from 0612c65 to f9aec36 Compare March 12, 2019 12:08

vrothberg mentioned this pull request Mar 12, 2019

go-criu and runc cause runtime errors checkpoint-restore/go-criu#7

Closed

vrothberg force-pushed the vendor-cleanup branch 3 times, most recently from 04bfdac to 2bb143a Compare March 13, 2019 12:29

vrothberg mentioned this pull request Mar 13, 2019

[WIP] Sync vendor and add validation #2041

Closed

4 tasks

saschagrunert mentioned this pull request Mar 14, 2019

Update error handling paths for sandbox add and removal #2127

Merged

saschagrunert reviewed Mar 21, 2019

View reviewed changes

make vendor: always use latest vndr

3be74ff

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2019

k8s-ci-robot assigned giuseppe Mar 27, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2019

k8s-ci-robot merged commit f4c2ffb into cri-o:master Mar 27, 2019

vrothberg deleted the vendor-cleanup branch March 27, 2019 11:03

cblecker unassigned giuseppe Apr 3, 2019

Vendor cleanup #2124

Vendor cleanup #2124

Conversation

vrothberg commented Mar 12, 2019 • edited

k8s-ci-robot commented Mar 12, 2019

vrothberg commented Mar 12, 2019

vrothberg commented Mar 12, 2019

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

vrothberg commented Mar 12, 2019

vrothberg commented Mar 12, 2019

vrothberg commented Mar 12, 2019

saschagrunert left a comment

Choose a reason for hiding this comment

saschagrunert Mar 21, 2019

Choose a reason for hiding this comment

haircommander Mar 25, 2019

Choose a reason for hiding this comment

vrothberg Mar 25, 2019

Choose a reason for hiding this comment

vrothberg Mar 25, 2019

Choose a reason for hiding this comment

saschagrunert Mar 21, 2019

Choose a reason for hiding this comment

vrothberg commented Mar 21, 2019

vrothberg commented Mar 25, 2019

vrothberg commented Mar 26, 2019

vrothberg commented Mar 26, 2019

vrothberg commented Mar 26, 2019

rhatdan commented Mar 26, 2019

vrothberg commented Mar 26, 2019

saschagrunert commented Mar 26, 2019

haircommander commented Mar 26, 2019

vrothberg commented Mar 26, 2019

vrothberg commented Mar 26, 2019

sboeuf commented Mar 26, 2019

k8s-ci-robot commented Mar 26, 2019

chavafg commented Mar 26, 2019

vrothberg commented Mar 26, 2019

saschagrunert commented Mar 26, 2019 • edited

chavafg commented Mar 26, 2019

haircommander commented Mar 26, 2019 • edited

haircommander commented Mar 26, 2019

vrothberg commented Mar 26, 2019

haircommander commented Mar 26, 2019

vrothberg commented Mar 27, 2019

giuseppe commented Mar 27, 2019

giuseppe commented Mar 27, 2019

k8s-ci-robot commented Mar 27, 2019

vrothberg commented Mar 12, 2019 •

edited

saschagrunert commented Mar 26, 2019 •

edited

haircommander commented Mar 26, 2019 •

edited