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

Bump k8s.io/uitls to 8e7ff06 #71047

Merged
merged 1 commit into from Nov 29, 2018

Conversation

@hoegaarden
Copy link
Member

hoegaarden commented Nov 14, 2018

What type of PR is this?

/kind bug

What this PR does / why we need it:

The reason for the bump is the new functionality of the
k8s.io/utils/exec package which allows

  • to get a hold of the process' std{out,err} as io.Readers
  • to Start a process and Wait for it

This should help on addressing #70890 by allowing to wrap std{out,err}
of the process to be wrapped with a io.limitedReader.

Does this PR introduce a user-facing change?:

Update k8s.io/utils to allow for asynchronous process control
@hoegaarden

This comment has been minimized.

Copy link
Member Author

hoegaarden commented Nov 14, 2018

/hold

Bump k8s.io/uitls to 8e7ff06
The reason for the bump is the new functionality of the
k8s.io/utils/exec package which allows
- to get a hold of the process' std{out,err} as `io.Reader`s
- to `Start` a process and `Wait` for it

This should help on addressing #70890 by allowing to wrap std{out,err}
of the process to be wrapped with a `io.limitedReader`.

It also updates
- k8s.io/kubernetes/pkg/probe/exec.FakeCmd
- k8s.io/kubernetes/pkg/kubelet/prober.execInContainer
- k8s.io/kubernetes/cmd/kubeadm/app/phases/kubelet.fakeCmd
to implement the changed interface.

The dependency on 'k8s.io/utils/pointer' to the new version has also
been bumped in some staging repos:
- apiserver
- kube-controller-manager
- kube-scheduler

@hoegaarden hoegaarden force-pushed the pivotal-k8s:bump-k8s-utils branch from 6e78511 to 0d4b5c9 Nov 15, 2018

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 15, 2018

@kubernetes/sig-node-bugs
sig-node, please add a priority label for this bug.

@hoegaarden tomorrow is code freeze and if this is not critical we cannot add it.

@hoegaarden

This comment has been minimized.

Copy link
Member Author

hoegaarden commented Nov 15, 2018

@neolit123 I feel it is not too urgent, so I was not targeting 1.13 really. If people feel otherwise that's also fine with me.

@fedebongio

This comment has been minimized.

Copy link
Contributor

fedebongio commented Nov 15, 2018

/assign @lavalamp

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Nov 15, 2018

/lgtm
/approve
/milestone v1.13
/priority important-soon

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 15, 2018

@k8s-ci-robot k8s-ci-robot added this to the v1.13 milestone Nov 15, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 15, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hoegaarden, lavalamp

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

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Nov 15, 2018

If you're happy to wait, I won't put the important label on.

@nikopen

This comment has been minimized.

Copy link
Member

nikopen commented Nov 19, 2018

@hoegaarden as this has been approved, is it ready to merge to 1.13?

if it won't cause any instability to CI then it can go in no problem

@hoegaarden

This comment has been minimized.

Copy link
Member Author

hoegaarden commented Nov 19, 2018

@nikopen

I read lavalamp's comment as:

If it's not super urgent, let's better keep it out of v1.13 -- but if it's urgent I am happy to add the important label and pull it in for v1.13.

As I don't consider that super urgent I am happy to bring it in only after v1.14. If I have misinterpreted lavalamp's comment I am also happy if we bring it in to 1.13. I do not think it would destabilise anything, as it just introduces new things and does not change any of the existing APIs.

To give some context: This is just the stepping stone for the fix of #70890 and I am not sure if I have the bandwidth to fix the actual thing for v1.13 (and I am not aware of anyone else working on that). So given that the fix for #70890 will probably only land in v1.14 anyway it might be just fine to also have this PR only land in v1.14. Having said that, I am also happy if this changes makes it into v1.13 and the actual fix in v1.14.

TL;DR: I'll leave it up to you if this should be merged for v1.13 :)

@nikopen

This comment has been minimized.

Copy link
Member

nikopen commented Nov 19, 2018

thanks!

/milestone v1.14

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.13, v1.14 Nov 19, 2018

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Nov 19, 2018

I'm fine either way :)

@k8s-ci-robot k8s-ci-robot merged commit 409bfc4 into kubernetes:master Nov 29, 2018

18 checks passed

cla/linuxfoundation hoegaarden authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@jberkus

This comment has been minimized.

Copy link

jberkus commented Dec 4, 2018

Hey, even if this is not a user-facing change, because it's a version update, we should have a (very short) release note.

@hoegaarden

This comment has been minimized.

Copy link
Member Author

hoegaarden commented Dec 6, 2018

@jberkus I updated the PR's description and added release notes. Is that good enough or is there anything more to do?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment