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

[Federation] Implement kubefed command. #35495

Merged
merged 2 commits into from
Nov 2, 2016
Merged

[Federation] Implement kubefed command. #35495

merged 2 commits into from
Nov 2, 2016

Conversation

madhusudancs
Copy link
Contributor

@madhusudancs madhusudancs commented Oct 25, 2016 •
edited
Loading

Supersedes PR #35154.

Please review only the last commit here. This is based on PR #35494 which will be reviewed independently.

I will add a release note separately for this entire feature, so please don't worry too much about the release note here in the PR.

Design Doc: PR #34484

cc @kubernetes/sig-cluster-federation @quinton-hoole @mwielgus

This change is Reviewable

@madhusudancs madhusudancs added area/cluster-federation release-note-none Denotes a PR that doesn't merit a release note. labels Oct 25, 2016
@madhusudancs madhusudancs added this to the v1.5 milestone Oct 25, 2016
@madhusudancs madhusudancs mentioned this pull request Oct 25, 2016
Closed
@k8s-github-robot k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 25, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 0dc50b23bd9f9b43c068e9da0ced0e5c559c4b82. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 74fb6c2. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@madhusudancs madhusudancs mentioned this pull request Oct 26, 2016
Merged
@nikhiljindal
Copy link
Contributor

Looked at the last commit only. LGTM

ghost
ghost reviewed Oct 27, 2016
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Minor nits, then LGTM. Feel free to apply the label once addressed.

federation/cmd/kubefed/app/kubefed.go
"os"

"k8s.io/kubernetes/federation/pkg/kubefed"
_ "k8s.io/kubernetes/pkg/client/metrics/prometheus" // for client metric registration
Copy link

Choose a reason for hiding this comment

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

Do prometheus client metrics apply for federation? It's not clear to me that you need this import here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a client metric, nothing specific to federation. It measures how long a request takes as seen by the client. I put it there because I thought it would be useful when it is necessary.

federation/cmd/kubefed/app/kubefed.go
_ "k8s.io/kubernetes/pkg/client/metrics/prometheus" // for client metric registration
cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
"k8s.io/kubernetes/pkg/util/logs"
_ "k8s.io/kubernetes/pkg/version/prometheus" // for version metric registration
Copy link

Choose a reason for hiding this comment

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

Same comment as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above :)

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2016
@madhusudancs
[Federation][unjoin-00] Implement kubefed unjoin command.
90d46c8 
Also, add unit tests for `kubefed unjoin`.
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 2, 2016
@madhusudancs madhusudancs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 02990249ace4bc7ab9eb0927668cf7dafa9001cd. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@madhusudancs
[Federation] Implement the kubefed command.
3c692eb 
Also:
1. Add it to the client build targets list.
2. Register `kubefed join` and `kubefed unjoin` commands.
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2016
@madhusudancs madhusudancs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2016
@madhusudancs
Copy link
Contributor Author

Adding retest-not-required label because it only touches federation.

@k8s-github-robot k8s-github-robot merged commit 3c692eb into kubernetes:master Nov 2, 2016
k8s-github-robot pushed a commit that referenced this pull request Nov 2, 2016
Merge pull request #35592 from madhusudancs/federation-kubefed-04
9a7ad2e 
Automatic merge from submit-queue

[Federation][(Un)join-01] Refactor common functions and structs into a util package.

Please review only the last commit here. This is based on PR #35495 which will be reviewed independently.

Design Doc: PR #34484

cc @kubernetes/sig-cluster-federation @quinton-hoole @nikhiljindal
@madhusudancs madhusudancs deleted the federation-kubefed-03 branch November 2, 2016 21:58
@madhusudancs madhusudancs mentioned this pull request Nov 17, 2016
Closed
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
Assignees

@nikhiljindal nikhiljindal

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@madhusudancs @k8s-ci-robot @nikhiljindal @googlebot @k8s-github-robot