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

Update hcsshim v0.8.7 and containerd v1.3.2 #86975

Open
wants to merge 37 commits into
base: master
from

Conversation

@dims
Copy link
Member

dims commented Jan 8, 2020

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
We use containerd 1.3.x in cluster-api / kind already, it's time to move up to newer dependencies.

Need reviews for/from:

sig-windows:

  • github.com/Microsoft/go-winio
  • github.com/Microsoft/hcsshim
  • github.com/konsorten/go-windows-terminal-sequences

sig-node:

  • github.com/containerd/cgroups
  • github.com/containerd/console
  • github.com/containerd/containerd
  • github.com/containerd/ttrpc
  • github.com/godbus/dbus
  • github.com/google/cadvisor
  • github.com/mistifyio/go-zfs
  • github.com/opencontainers/runtime-spec

sig-apimachinery:

  • github.com/gogo/protobuf
  • github.com/golang/groupcache
  • github.com/google/go-cmp
  • github.com/hashicorp/golang-lru
  • google.golang.org/genproto
  • google.golang.org/grpc

sig-instrumentation:

  • github.com/prometheus/procfs (sig-instrumentation)
  • go.opencensus.io (sig-instrumentation)

Miscellaneous:

  • github.com/onsi/gomega (sig-testing)
  • github.com/vishvananda/netns (sig-network)
  • google.golang.org/appengine (sig-auth)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@dims

This comment has been minimized.

Copy link
Member Author

dims commented Jan 9, 2020

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-kind-ipv6

@dims

This comment has been minimized.

Copy link
Member Author

dims commented Jan 9, 2020

/test pull-kubernetes-e2e-kind-ipv6

dims added 6 commits Jan 16, 2020
…30031844-723cc1e459b8
…0.20190919025122-fc70bd9a86b5
@dims dims force-pushed the dims:update-hcsshim-v0.8.7-and-containerd-v1.3.2 branch from 5325092 to a9f2122 Jan 16, 2020
@dims dims force-pushed the dims:update-hcsshim-v0.8.7-and-containerd-v1.3.2 branch from a9f2122 to b476974 Jan 16, 2020
@dims

This comment has been minimized.

Copy link
Member Author

dims commented Jan 16, 2020

/test pull-kubernetes-bazel-test
/test pull-kubernetes-e2e-gce

@dims

This comment has been minimized.

Copy link
Member Author

dims commented Jan 16, 2020

/hold cancel

github.com/cespare/prettybench => github.com/cespare/prettybench v0.0.0-20150116022406-03b8cfe5406c
github.com/chai2010/gettext-go => github.com/chai2010/gettext-go v0.0.0-20160711120539-c6fed771bfd5
github.com/checkpoint-restore/go-criu => github.com/checkpoint-restore/go-criu v0.0.0-20181120144056-17b0214f6c48 // 17b0214f6c48 is the SHA for git tag 3.11

This comment has been minimized.

Copy link
@dims

dims Jan 16, 2020

Author Member

This change did not stick. 2 steps forward and one step back!

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 16, 2020

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@dims

This comment has been minimized.

Copy link
Member Author

dims commented Jan 16, 2020

/test pull-kubernetes-e2e-gce

@dims

This comment has been minimized.

Copy link
Member Author

dims commented Jan 16, 2020

/assign @liggitt

Jordan,
this is ready. all green and got acks from all the sigs.

case 3:
for {

This comment has been minimized.

Copy link
@liggitt

liggitt Jan 16, 2020

Member

this change looks surprising... it looks like an entire block of logic was dropped

@@ -260,11 +265,13 @@ replace (
github.com/flynn/go-shlex => github.com/flynn/go-shlex v0.0.0-20150515145356-3f9db97f8568
github.com/fogleman/gg => github.com/fogleman/gg v1.2.1-0.20190220221249-0403632d5b90
github.com/fsnotify/fsnotify => github.com/fsnotify/fsnotify v1.4.7
github.com/garyburd/redigo => github.com/garyburd/redigo v0.0.0-20150301180006-535138d7bcd7

This comment has been minimized.

Copy link
@liggitt

liggitt Jan 16, 2020

Member

this is a sunset repository used only by cadvisor redis storage. we should find a way to isolate storage and container integrations in cadvisor so we don't pull this into our dependency tree if we don't need to

github.com/hashicorp/hcl => github.com/hashicorp/hcl v1.0.0
github.com/heketi/heketi => github.com/heketi/heketi v9.0.1-0.20190917153846-c2e2a4ab7ab9+incompatible
github.com/heketi/tests => github.com/heketi/tests v0.0.0-20151005000721-f3775cbcefd6
github.com/hpcloud/tail => github.com/hpcloud/tail v1.0.0
github.com/imdario/mergo => github.com/imdario/mergo v0.3.5
github.com/inconshreveable/mousetrap => github.com/inconshreveable/mousetrap v1.0.0
github.com/influxdb/influxdb => github.com/influxdb/influxdb v0.9.6-0.20151125225445-9eab56311373

This comment has been minimized.

Copy link
@liggitt

liggitt Jan 16, 2020

Member

this pulls in a lot of additional transitive dependencies and is only used by cadvisor influxdb storage (which we don't use). we should find a way to isolate storage and container integrations in cadvisor so we don't pull this into our dependency tree if we don't need to

github.com/opencontainers/selinux => github.com/opencontainers/selinux v1.3.1-0.20190929122143-5215b1806f52
github.com/pborman/uuid => github.com/pborman/uuid v0.0.0-20150824212802-cccd189d45f7

This comment has been minimized.

Copy link
@liggitt
@@ -520,6 +543,7 @@ replace (
gopkg.in/inf.v0 => gopkg.in/inf.v0 v0.9.1
gopkg.in/mcuadros/go-syslog.v2 => gopkg.in/mcuadros/go-syslog.v2 v2.2.1
gopkg.in/natefinch/lumberjack.v2 => gopkg.in/natefinch/lumberjack.v2 v2.0.0
gopkg.in/olivere/elastic.v2 => gopkg.in/olivere/elastic.v2 v2.0.12

This comment has been minimized.

Copy link
@liggitt

liggitt Jan 16, 2020

Member

this is only used by cadvisor elasticsearch storage (which we don't use). we should find a way to isolate storage and container integrations in cadvisor so we don't pull this into our dependency tree if we don't need to

github.com/Rican7/retry => github.com/Rican7/retry v0.1.1-0.20160712041035-272ad122d6e5
github.com/SeanDolphin/bqschema => github.com/SeanDolphin/bqschema v0.0.0-20150424181127-f92a08f515e1
github.com/Shopify/sarama => github.com/Shopify/sarama v1.8.0
github.com/Shopify/toxiproxy => github.com/Shopify/toxiproxy v2.1.4+incompatible

This comment has been minimized.

Copy link
@liggitt

liggitt Jan 16, 2020

Member

these pull in a lot of additional transitive dependencies and are only used by cadvisor kafka storage integration (which we don't use). we should find a way to isolate storage and container integrations in cadvisor so we don't pull this into our dependency tree if we don't need to

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 16, 2020

@dims @dashpole since the last time we updated cadvisor, it added a go.mod file, which exposes all its transitive dependency version opinions, whether we make use of them or not. That means that all the container runtime (containerd, crio, docker, libcontainer, mesos, systemd) and storage integrations (bigquery, elasticsearch, influxdb, kafka, redis, statsd) that cadvisor has affect our dependency tree, even if we don't actually vendor the code in and link to it.

Unless there is an urgent reason to pull this in (e.g. fixing critical bugs), I'd like to see if we can isolate the cadvisor dependencies needed for the cadvisor cmd so downstream projects don't have to pull in the world like this. I'm experimenting with an approach locally in the cadvisor component and can sync up with you and @dashpole on it.

Update: @dims, @dashpole and I are meeting on 1/20 to work on a plan to resolve this issue

@dims

This comment has been minimized.

Copy link
Member Author

dims commented Jan 16, 2020

@liggitt no rush other than the normal "try to get this stuff into master early in the cycle" so we can bake stuff in ci

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 16, 2020

It looks like github.com/Microsoft/hcsshim@v0.8.7 also has a declared dependency on k8s.io/kubernetes@v1.13.0, which is problematic

https://github.com/microsoft/hcsshim/blob/master/go.mod#L36

go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
Copy link

kazimsarikaya left a comment

zfs-go related problems (#87341 ) are solved with this pr

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 20, 2020

@dims: PR needs rebase.

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.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 21, 2020

It looks like github.com/Microsoft/hcsshim@v0.8.7 also has a declared dependency on k8s.io/kubernetes@v1.13.0, which is problematic

https://github.com/microsoft/hcsshim/blob/master/go.mod#L36

That is depending on cri-api, which is published to a staging module now. https://github.com/liggitt/hcsshim/commits/bump-k8s easily updates hcsshim to use the staging module, but that would mean we could not depend on that version of hcsshim (we do not allow k8s.io/kubernetes -> vendor -> staging cycles). The only way I see to unblock that is for k8s.io/cri-api to graduate from staging and be a standalone repo.

@dims

This comment has been minimized.

Copy link
Member Author

dims commented Jan 23, 2020

/area code-organization

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.