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

Move more things into docker label, and add label test #17234

Merged
merged 1 commit into from
Nov 21, 2015
Merged

Move more things into docker label, and add label test #17234

merged 1 commit into from
Nov 21, 2015
+129 −36

Conversation

Random-Liu
Copy link
Member

  1. Add new label Container Name and Container Hash.
  2. Implement all getter functions
  3. Add label test

/cc @yujuhong

(For issue #15089)

@Random-Liu Random-Liu added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Nov 13, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 13, 2015
Random-Liu
Random-Liu reviewed Nov 13, 2015
View reviewed changes
pkg/kubelet/dockertools/labels.go

return labels
}

// TODO: (random-liu) When old containers are deprecated, we should change the following `Info` log to `Error` log, or return error here
Copy link
Member Author

Choose a reason for hiding this comment

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

Question: Is some kind of aggregation function better here? Such as getPodInfoFromLabel()

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I think it'd be better to convert all the kubelet-managed labels and return a single object.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I'll do it. :)

@k8s-bot
Copy link

k8s-bot commented Nov 13, 2015

GCE e2e build/test failed for commit ee02b98843b9babb5974365515b73a654c757a95.

@k8s-bot
Copy link

k8s-bot commented Nov 13, 2015

GCE e2e build/test failed for commit a79466e70622245c3933601b3f000c1016725098.

@yujuhong
Copy link
Contributor

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Nov 13, 2015

GCE e2e build/test failed for commit ee02b98843b9babb5974365515b73a654c757a95.

@Random-Liu Random-Liu closed this Nov 13, 2015
@Random-Liu Random-Liu reopened this Nov 13, 2015
@Random-Liu
Copy link
Member Author

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Nov 13, 2015

GCE e2e build/test failed for commit ee02b98843b9babb5974365515b73a654c757a95.

@k8s-bot
Copy link

k8s-bot commented Nov 13, 2015

GCE e2e build/test failed for commit 47cefca53f5cdfe56f20691beb6338587da4c45a.

@Random-Liu
Copy link
Member Author

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Nov 14, 2015

GCE e2e build/test failed for commit 47cefca53f5cdfe56f20691beb6338587da4c45a.

@Random-Liu Random-Liu force-pushed the move-more-info-to-docker-label branch from b0fc8dd to bec3fa2 Compare November 14, 2015 01:23
@k8s-bot
Copy link

k8s-bot commented Nov 14, 2015

GCE e2e build/test failed for commit bec3fa2b87983c1294e35b474897ae633286c1c0.

@Random-Liu
Copy link
Member Author

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Nov 14, 2015

GCE e2e test build/test passed for commit bec3fa2b87983c1294e35b474897ae633286c1c0.

@thockin thockin changed the title Move more things into docke label, and add label test Move more things into docker label, and add label test Nov 14, 2015
@zhengguoyong
Copy link
Contributor

i see some files were changed from 0644 to 0755,
maybe you want to change it back.
:)

@Random-Liu
Copy link
Member Author

@zhengguoyong Thank you, :)
In fact, someone else changed manager_test.go to 755 before, I just change it back to 644 in this PR.

@zhengguoyong
Copy link
Contributor

oh, yes.
you are right, its my fault.
forget it.
:)
@Random-Liu

@Random-Liu
Copy link
Member Author

@zhengguoyong Never mind~ :D

yujuhong
yujuhong reviewed Nov 17, 2015
View reviewed changes
pkg/kubelet/dockertools/labels.go
return terminationMessagePath
func getStringValueFromLabel(labels map[string]string, label string) string {
if value, found := labels[label]; found {
return value
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove else since it's redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. :)

@yujuhong
Copy link
Contributor

looks good overall with some nits.

@k8s-bot
Copy link

k8s-bot commented Nov 18, 2015

GCE e2e test build/test passed for commit 36a0f6ac8b38d97c61f5383d8c2dfa13e68ebddd.

@yujuhong
Copy link
Contributor

LGTM. Please squash :)

@Random-Liu Random-Liu force-pushed the move-more-info-to-docker-label branch from 36a0f6a to ad8dd59 Compare November 18, 2015 03:40
@Random-Liu
Copy link
Member Author

Squashed, :)

@k8s-bot
Copy link

k8s-bot commented Nov 18, 2015

GCE e2e test build/test passed for commit ad8dd59859f34ceaee001a8a5c4b9ef13282381a.

@k8s-bot
Copy link

k8s-bot commented Nov 18, 2015

GCE e2e test build/test passed for commit 956a94f7af3b6d31bbbd53739f1b63176f8bac18.

@Random-Liu
Copy link
Member Author

@k8s-bot unit test this please.

@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2015
@Random-Liu Random-Liu force-pushed the move-more-info-to-docker-label branch from 956a94f to f080975 Compare November 19, 2015 07:17
@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2015
@Random-Liu Random-Liu added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2015
@Random-Liu
Copy link
Member Author

Add LGTM again because I just did a squash. :)

@k8s-bot
Copy link

k8s-bot commented Nov 19, 2015

GCE e2e test build/test passed for commit f080975.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Nov 21, 2015

GCE e2e test build/test passed for commit f080975.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Nov 21, 2015
Merge pull request #17234 from Random-Liu/move-more-info-to-docker-label
8a306a7 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 8a306a7 into kubernetes:master Nov 21, 2015
@Random-Liu Random-Liu deleted the move-more-info-to-docker-label branch November 22, 2015 02:51
@2opremio 2opremio mentioned this pull request Dec 4, 2015
Closed
@emmanuel emmanuel mentioned this pull request Feb 20, 2016
Closed
@bgrant0607 bgrant0607 mentioned this pull request Mar 17, 2016
Closed
@yujuhong yujuhong mentioned this pull request Apr 7, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@yujuhong yujuhong

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Random-Liu @k8s-github-robot @k8s-bot @yujuhong @zhengguoyong @k8s-teamcity-mesosphere @googlebot