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

Automatically add node labels beta.kubernetes.io/{os,arch} #23684

Merged
merged 2 commits into from
May 13, 2016

Conversation

luxas
Copy link
Member

@luxas luxas commented Mar 31, 2016

Proposal: #17981
As discussed in #22623:

@davidopp: #9044 says cloud provider but can also cover platform stuff.

Adds a label beta.kubernetes.io/platform to kubelet that informs about the os/arch it's running on.
Makes it easy to specify nodeSelectors for different arches in multi-arch clusters.

$ kubectl get no --show-labels
NAME        STATUS    AGE       LABELS
127.0.0.1   Ready     1m        beta.kubernetes.io/platform=linux-amd64,kubernetes.io/hostname=127.0.0.1
$ kubectl describe no
Name:           127.0.0.1
Labels:         beta.kubernetes.io/platform=linux-amd64,kubernetes.io/hostname=127.0.0.1
CreationTimestamp:  Thu, 31 Mar 2016 20:39:15 +0300

@davidopp @vishh @fgrzadkowski @thockin @wojtek-t @ixdy @bgrant0607 @dchen1107 @preillyme

@davidopp
Copy link
Member

davidopp commented Apr 3, 2016

Would it be better to split it into two labels, one for OS and one for arch?

@luxas
Copy link
Member Author

luxas commented Apr 3, 2016

Do you have a specific case where it would be useful?
To me, it doesn't matter so much, but I think we're fine with one, cause in most cases, you must specify both os and arch

@luxas
Copy link
Member Author

luxas commented Apr 4, 2016

@davidopp Is it ok as-is or should I change something?
I would like to hear what you think.

I lean towards just one label as implemented here.
It would make less noise, and it's only one label to specify in nodeSelector
And I honestly can't come up with a case where you only would specify os or arch.

WDYT about os-arch vs os_arch then?
os/arch (which I would have preferred) wasn't allowed as a value.

@vishh
Copy link
Contributor

vishh commented Apr 4, 2016 via email

@ixdy
Copy link
Member

ixdy commented Apr 4, 2016

Note prior art: with #19905, the server version string includes a field "platform" which is fmt.Sprintf("%s/%s", runtime.GOOS, runtime.GOARCH).

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 14, 2016
@luxas
Copy link
Member Author

luxas commented Apr 14, 2016

@davidopp @vishh Updated to two labels for reasons no one knows today but may prove useful in the future.
PTAL

@davidopp
Copy link
Member

LGTM

@davidopp davidopp added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-merge labels Apr 14, 2016
@vishh
Copy link
Contributor

vishh commented Apr 14, 2016

@davidopp @dchen1107 @bgrant0607 What is the purpose of NodeStatus.NodeSystemInfo? Is that supposed to be a super set of all the node related labels that are exposed by the kubelet? Will users be able to use the fields in NodeSystemInfo for scheduling in the future?

@davidopp davidopp added ok-to-merge release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Apr 14, 2016
@davidopp davidopp changed the title Automatically add a platform node label: GOOS-GOARCH Automatically add node labels beta.kubernetes.io/{os,arch} Apr 14, 2016
@vishh
Copy link
Contributor

vishh commented Apr 14, 2016

@davidopp can we hold on to this PR until #23684 (comment) is resolved?

@davidopp davidopp removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2016
@luxas
Copy link
Member Author

luxas commented May 6, 2016

ping @davidopp @lavalamp
I think it'll pass now
PTAL

@davidopp
Copy link
Member

davidopp commented May 6, 2016

LGTM

@davidopp davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2016
@luxas
Copy link
Member Author

luxas commented May 7, 2016

@k8s-bot unit test this please github issue: #IGNORE

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2016
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 10, 2016
@luxas
Copy link
Member Author

luxas commented May 10, 2016

@davidopp Any clue what's failing in integration?
FAIL k8s.io/kubernetes/pkg/kubelet 0.768s

Removing ok-to-merge as it isn't needed anymore

@luxas luxas removed the ok-to-merge label May 10, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2016
@luxas
Copy link
Member Author

luxas commented May 10, 2016

Adding LGTM back now when the rebase/flake is fixed.

@luxas luxas added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2016
@luxas
Copy link
Member Author

luxas commented May 13, 2016

@david-mcmahon FYI, I'd really like this to get merged before v1.3.0-alpha.4 so we can test the changes out.
But I don't know if I should add priority just for that (right now 7th in the queue)
Please keep in mind before cutting the release :)

@luxas
Copy link
Member Author

luxas commented May 13, 2016

@jlowdermilk FYI, this has been ín the queue now three times when the submit-queue stopped and started again. Now it got kicked out of the queue because of what I think is a flake:

FAIL    k8s.io/kubernetes/contrib/mesos/pkg/offers  5.889s

That hasn't showed up earlier when these commits have been tested, so it have to be a flake.
Will retest unit.

@luxas
Copy link
Member Author

luxas commented May 13, 2016

@k8s-bot unit test this please github issue: #IGNORE

@luxas luxas added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 13, 2016
@luxas
Copy link
Member Author

luxas commented May 13, 2016

Adding P1 given that we want it merged before v1.3.0-alpha.4 and because it was on the top of the queue some hours ago but was kicked out due to flakes/unstable queue

@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 May 13, 2016

GCE e2e build/test passed for commit 0b6b723.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 399b086 into kubernetes:master May 13, 2016
@davidopp
Copy link
Member

@luxas if you want these also published as labels, you're running low on time to send a PR, as the code freeze for 1.3 is this Friday.

@luxas
Copy link
Member Author

luxas commented May 16, 2016

@davidopp I do not really understand what you mean, os/arch is already exposed as labels on every node:

$ hack/local-up-cluster.sh
...
$ _output/local/bin/linux/amd64/kubectl describe no
Name:           127.0.0.1
Labels:         beta.kubernetes.io/arch=amd64
            beta.kubernetes.io/os=linux
            kubernetes.io/hostname=127.0.0.1
CreationTimestamp:  Mon, 16 May 2016 08:56:22 +0300
Phase:          
Conditions:
  Type      Status  LastHeartbeatTime           LastTransitionTime          Reason              Message
  ----      ------  -----------------           ------------------          ------              -------
  OutOfDisk     False   Mon, 16 May 2016 08:57:26 +0300     Mon, 16 May 2016 08:56:22 +0300     KubeletHasSufficientDisk    kubelet has sufficient disk space available
  Ready     True    Mon, 16 May 2016 08:57:26 +0300     Mon, 16 May 2016 08:56:23 +0300     KubeletReady            kubelet is posting ready status
Addresses:  127.0.0.1,127.0.0.1
Capacity:
 alpha.kubernetes.io/nvidia-gpu:    0
 cpu:                   4
 memory:                16359708Ki
 pods:                  110
System Info:
 Machine ID:            dce29be2dcd94c1b958052dc77e08e6f
 System UUID:           3C4DC303-9C6C-11E4-A26A-F0761C62F136
 Boot ID:           5d120efb-67d6-4e91-9ae7-4c666fa4ab42
 Kernel Version:        4.2.0-25-generic
 OS Image:          Ubuntu 15.10
 Operating System:      linux
 Architecture:          amd64
 Container Runtime Version: docker://1.9.1
 Kubelet Version:       v1.3.0-alpha.1.1829+aada051b20c9e4-dirty
 Kube-Proxy Version:        v1.3.0-alpha.1.1829+aada051b20c9e4-dirty
ExternalID:         127.0.0.1
Non-terminated Pods:        (0 in total)
...

But thanks for notifying anyway, I'll have to be quick with some other things

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.