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

Remove the old docker-multinode files that were built into the hyperkube image #44555

Merged
merged 1 commit into from
Apr 20, 2017
Merged

Remove the old docker-multinode files that were built into the hyperkube image #44555

merged 1 commit into from
Apr 20, 2017

Conversation

luxas
Copy link
Member

@luxas luxas commented Apr 17, 2017 •
edited
Loading

What this PR does / why we need it:

ref: https://goo.gl/VxSaKx

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

Special notes for your reviewer:

Release note:

The hyperkube image has been slimmed down and no longer includes addon manifests and other various scripts. These were introduced for the now removed docker-multinode setup system.

cc @jbeda @brendandburns @bgrant0607 @justinsb @mikedanese

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

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 17, 2017
@roberthbailey
Copy link
Contributor

lgtm (not adding the slash to get relevant approvals before this gets merged)

@mikedanese
Copy link
Member

lgtm as well

/approve

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

luxas commented Apr 17, 2017

Cool, thanks! @jbeda is the last one to lgtm then before merge 👍

@jbeda
Copy link
Contributor

jbeda commented Apr 20, 2017

This LGTM but is a bit scary.

I'd put a bit more in the release note. Something like "The hyperkube image has been slimmed down and no longer includes addon manifests and other various scripts. These were introduced for the now removed docker-multinode setup system."

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2017
@jbeda
Copy link
Contributor

jbeda commented Apr 20, 2017

Sorry I didn't get this earlier! I thought I had covered all of these.

@luxas
Copy link
Member Author

luxas commented Apr 20, 2017

This LGTM but is a bit scary.

Yes, these were here only for the docker-multinode setup

The hyperkube image has been slimmed down and no longer includes addon manifests and other various scripts. These were introduced for the now removed docker-multinode setup system.

Proposal accepted ;)

Thanks!

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbeda, luxas, mikedanese

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 1413f2e into kubernetes:master Apr 20, 2017
@shashidharatd shashidharatd mentioned this pull request Apr 21, 2017
Closed
@nikhiljindal
Copy link
Contributor

I believe this PR broke federation build.

I see the following in logs:

I0420 10:48:18.314] make[1]: Entering directory '/var/lib/jenkins/workspace/ci-kubernetes-federation-build/go/src/k8s.io/kubernetes/cluster/images/hyperkube'
I0420 10:48:18.719] tar: /tmp/hyperkubeD9ftu6/cni-bin: Cannot open: No such file or directory
I0420 10:48:18.719] tar: Error is not recoverable: exiting now
I0420 10:48:18.720] curl: (23) Failed writing body (919 != 1379)
I0420 10:48:18.721] Makefile:62: recipe for target 'build' failed
I0420 10:48:18.721] make[1]: *** [build] Error 2
I0420 10:48:18.722] make[1]: Leaving directory '/var/lib/jenkins/workspace/ci-kubernetes-federation-build/go/src/k8s.io/kubernetes/cluster/images/hyperkube'
I0420 10:48:18.725] !!! [0420 10:48:18] Call tree:
I0420 10:48:18.727] !!! [0420 10:48:18]  1: ./develop/develop.sh:151 build_image(...)
I0420 10:48:18.727] !!! Error in ./develop/develop.sh:112
I0420 10:48:18.727]   Error in ./develop/develop.sh:112. '((i<3-1))' exited with status 2
I0420 10:48:18.727] Call stack:
I0420 10:48:18.727]   1: ./develop/develop.sh:112 build_image(...)
I0420 10:48:18.727]   2: ./develop/develop.sh:151 main(...)
I0420 10:48:18.727] Exiting with status 1
I0420 10:48:18.728] Makefile:50: recipe for target 'build_image' failed

Was removing cni-bin dir intentional?

nikhiljindal
nikhiljindal reviewed Apr 21, 2017
View reviewed changes
cluster/images/hyperkube/Makefile
@@ -60,43 +60,15 @@ ifndef VERSION
$(error VERSION is undefined)
endif
cp -r ./* ${TEMP_DIR}
mkdir -p ${TEMP_DIR}/cni-bin ${TEMP_DIR}/addons ${TEMP_DIR}/addons/singlenode ${TEMP_DIR}/addons/multinode
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there are other scripts relying on this cni-bin dir. Was removing this intentional?
It seems to have broken federation build.

@nikhiljindal nikhiljindal mentioned this pull request Apr 21, 2017
Merged
@nikhiljindal
Copy link
Contributor

Sent #44751 to fix the issue. Looking for someone in cluster/OWNERS to approve.

k8s-github-robot pushed a commit that referenced this pull request Apr 21, 2017
Merge pull request #44751 from nikhiljindal/fixBuild
136396c 
Automatic merge from submit-queue

Fixing build break for federation

#44555 (comment) is the culprit PR.
This reverts one line from that PR.

Our build is broken: https://k8s-testgrid.appspot.com/cluster-federation#build

cc @kubernetes/sig-federation-pr-reviews
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikhiljindal nikhiljindal nikhiljindal left review comments

Assignees

@jbeda jbeda

@mikedanese mikedanese

@roberthbailey roberthbailey

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@luxas @k8s-reviewable @roberthbailey @mikedanese @jbeda @k8s-github-robot @nikhiljindal @k8s-ci-robot