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.

Sign up for GitHub

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

Jump to bottom

bump QEMU version to v2.9.1 #50597

Merged
merged 1 commit into from
Sep 3, 2017
Merged

bump QEMU version to v2.9.1 #50597

merged 1 commit into from
Sep 3, 2017

Conversation

dixudx
Copy link
Member

@dixudx dixudx commented Aug 14, 2017

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
xref #38067

Special notes for your reviewer:
/assign @luxas

Release note:

update QEMU version to v2.9.1

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 14, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @dixudx. Thanks for your PR.

I'm waiting for a 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.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 14, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 14, 2017
@dixudx dixudx changed the title WIP: update QEMU version to v2.9.1 update QEMU version to v2.9.1 Aug 14, 2017
@dixudx
Copy link
Member Author

dixudx commented Aug 14, 2017

/assign @jbeda @zmerlynn
I think it's ready for a review. PTAL. Thanks.

@dixudx dixudx mentioned this pull request Aug 14, 2017
Closed
mkumatag
mkumatag suggested changes Aug 14, 2017
View reviewed changes
test/images/Makefile
@@ -14,7 +14,7 @@

REGISTRY ?= gcr.io/kubernetes-e2e-test-images
GOARM=7
QEMUVERSION=v2.7.0
QEMUVERSION=v2.9.1
Copy link
Member

@mkumatag mkumatag Aug 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure whether we need to push all the relevant images under test/images with this new qemu binary. @luxas WDYT?

test/images/resource-consumer/BASEIMAGE Outdated
arm=gcr.io/google-containers/debian-base-arm:0.1
arm64=gcr.io/google-containers/debian-base-arm64:0.1
ppc64le=gcr.io/google-containers/debian-base-ppc64le:0.1
amd64=gcr.io/google-containers/debian-base-amd64:0.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may need to bump the version for resource-consumer image in test/images/resource-consumer/VERSION

vabc3
vabc3 reviewed Aug 14, 2017
View reviewed changes
build/root/WORKSPACE Outdated
@@ -54,6 +54,7 @@ http_file(
url = "https://storage.googleapis.com/kubernetes-release/network-plugins/cni-amd64-0799f5732f2a11b329d9e3d51b9c8f2e3759f2ff.tar.gz",
)

// TODO(dixudx): update the sha and tag (to v8) when the image get pushed
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use # for comments? Or spawn an issue for tracking

@dixudx dixudx force-pushed the qemu_upgrade_2.9.1 branch from bedffcc to e64ff22 Compare August 14, 2017 09:55
luxas
luxas reviewed Aug 14, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dixudx for your PR!

I don't think we need to bump the versions.

  1. It shouldn't affect the result (this is only syscall emulation)
  2. There is no direct benefit we get from using a newer QEMU that would need an image bump. This setting will just be picked up the next time the image is built/pushed.

The rationale for upgrading is to hopefully get more stable and reliable builds, have better emulation support for s390x and pick up performance improvements tha have been made in recent versions.

@luxas
Copy link
Member

luxas commented Aug 14, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 14, 2017
@luxas luxas added this to the v1.8 milestone Aug 14, 2017
@dixudx
Copy link
Member Author

dixudx commented Aug 14, 2017

/retest

1 similar comment
@dixudx
Copy link
Member Author

dixudx commented Aug 15, 2017

/retest

@dixudx dixudx force-pushed the qemu_upgrade_2.9.1 branch 2 times, most recently from 57cca0f to f0de8f3 Compare August 15, 2017 13:26
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 15, 2017
@dixudx dixudx force-pushed the qemu_upgrade_2.9.1 branch from f0de8f3 to e4c6337 Compare August 16, 2017 02:41
@dixudx
Copy link
Member Author

dixudx commented Aug 16, 2017

/retest

luxas
luxas reviewed Aug 16, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @ixdy; think we need to bump versions for this?

build/common.sh Outdated
@@ -85,6 +85,7 @@ readonly KUBE_CONTAINER_RSYNC_PORT=8730
#
# $1 - server architecture
kube::build::get_docker_wrapped_binaries() {
## TODO(dixudx): update the tag to v8 when the image get pushed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still valid; do we need to do this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the base image of debian-iptables, we'd better bump a new version.

@ixdy
Copy link
Member

ixdy commented Aug 18, 2017

QEMU is really only used when building the images, right? in that case, no, I don't think bumping the tags is needed.

@dixudx
Copy link
Member Author

dixudx commented Aug 19, 2017

QEMU is really only used when building the images, right?

Yes, we are just using it during building.

@ixdy Actually since the Dockerfile has been changed, will the docker images be updated and pushed to gcr.io? And If the images have been updated and pushed, but we still bump the old tag, users may not get noticed by the change. The image with the same tag they pulled before is actually different from that on current gcr.io.

@dixudx dixudx mentioned this pull request Aug 19, 2017
Merged
@dixudx dixudx force-pushed the qemu_upgrade_2.9.1 branch from e4c6337 to 22bdaec Compare August 19, 2017 06:16
@dixudx dixudx changed the title update QEMU version to v2.9.1 bump QEMU version to v2.9.1 Aug 19, 2017
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 19, 2017
@dixudx
Copy link
Member Author

dixudx commented Aug 19, 2017

@ixdy @luxas @mkumatag

QEMU is really only used when building the images, right? in that case, no, I don't think bumping the tags is needed.

Updated. I've only bumped new version to QEMU.

Since PR #50602 has to get bumped a new tag due to old stale base image. We just need to update it once.

@luxas
Copy link
Member

luxas commented Aug 19, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dixudx, luxas
We suggest the following additional approver: jbeda

Assign the PR to them by writing /assign @jbeda in a comment when ready.

Associated issue: 38067

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@dixudx
Copy link
Member Author

dixudx commented Aug 20, 2017

/retest

1 similar comment
@dixudx
Copy link
Member Author

dixudx commented Aug 23, 2017

/retest

@luxas
Copy link
Member

luxas commented Aug 23, 2017

@ixdy Please approve this. Thanks!

@dixudx
Copy link
Member Author

dixudx commented Aug 28, 2017

/retest

@dixudx
Copy link
Member Author

dixudx commented Aug 28, 2017

ping @ixdy Need your approval. PTAL. Thanks!

@luxas
Copy link
Member

luxas commented Sep 1, 2017

/test all

@dixudx
Copy link
Member Author

dixudx commented Sep 3, 2017

/retest

@luxas luxas added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 6b9ce5b into kubernetes:master Sep 3, 2017
@k8s-ci-robot
Copy link
Contributor

@dixudx: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 22bdaec link /test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@dixudx dixudx deleted the qemu_upgrade_2.9.1 branch September 9, 2017 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luxas luxas luxas left review comments

@vabc3 vabc3 vabc3 left review comments

@mkumatag mkumatag mkumatag approved these changes

Assignees

@jbeda jbeda

@zmerlynn zmerlynn

@luxas luxas

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.
9 participants
@dixudx @k8s-ci-robot @luxas @ixdy @k8s-github-robot @vabc3 @mkumatag @jbeda @zmerlynn