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][Kubefed] Flag cleanup #41335

Merged
merged 2 commits into from Mar 1, 2017
Merged

[Federation][Kubefed] Flag cleanup #41335

merged 2 commits into from Mar 1, 2017

Conversation

irfanurrehman
Copy link
Contributor

@irfanurrehman irfanurrehman commented Feb 13, 2017 •
edited

This PR is for the issue #41333

Special notes for your reviewer:
@marun @madhusudancs

Release note:

Kubefed init unlearned the following flags:
--storage-backend

Users should instead use the following flag to pass additional arguments:
--apiserver-arg-overrides to api server

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 13, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 13, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@marun
Copy link
Contributor

marun commented Feb 13, 2017

--dns-provider cannot be deferred to --controllermanager-arg-overrides because it is not acceptable going forward to set a default (as per #40757).

@spxtr
Copy link
Contributor

spxtr commented Feb 13, 2017

/approve

@marun
Copy link
Contributor

marun commented Feb 13, 2017

@spxtr Why are you trying to approve this?

@spxtr
Copy link
Contributor

spxtr commented Feb 13, 2017

@marun I only approved changes to hack/, someone else will need to approve the rest.

@marun
Copy link
Contributor

marun commented Feb 13, 2017

@spxtr Thank you for explaining. I hadn't fully grokked the path-based nature of the new approval mechanism.

@madhusudancs madhusudancs self-assigned this Feb 16, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2017
@madhusudancs
Copy link
Contributor

@irfanurrehman Since federation doesn't work, at least the service controller doesn't work, without a valid --dns-zone-name value, I think we should leave that flag in kubefed init to give people an easy way to configure dns zone name. That flag is kind of mandatory for now to get a working federation control plane. I also think we should leave --dns-provider flag in there for the reason @marun mentioned.

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 24, 2017
@irfanurrehman
Copy link
Contributor Author

@madhusudancs updated PTAL!

@irfanurrehman
Copy link
Contributor Author

@k8s-bot kops aws e2e test this

@irfanurrehman
Copy link
Contributor Author

@k8s-bot cvm gce e2e test this

marun
marun reviewed Feb 24, 2017
View reviewed changes
federation/cluster/federation-up.sh Outdated
@@ -82,7 +82,8 @@ function init() {
"${FEDERATION_NAME}" \
--host-cluster-context="${HOST_CLUSTER_CONTEXT}" \
--dns-zone-name="${DNS_ZONE_NAME}" \
--image="${kube_registry}/hyperkube-amd64:${kube_version}"
--image="${kube_registry}/hyperkube-amd64:${kube_version}" \
--apiserver-arg-overrides="--storage-backend=etcd2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that kubefed init is no longer specifying etcd2 as the default, a user will get the underlying default of etcd3 unless they specify the same override as appears here. Why is it desirable that federation-up.sh not use etcd3 as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure, if enough analysis or update (if there is any that is needed) is done in the federation API server code to move to etcd3.
Given the previous, trying to be cautious not to break something.
as far as I know @madhusudancs and @shashidharatd found some issues with using etcd3 backend; probably they can comment better.

Copy link
Contributor

@marun marun Feb 24, 2017 •
edited

Choose a reason for hiding this comment

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

Then why is this PR proposing to default kubefed to etcd3? That would seem to risk breaking users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would it break a user who is running kubefed init? kubefed init brings up a new control plane and people who bring up new control planes can choose etcd3. We don't have upgrade support yet.

Should we also test with etcd3? Yes. And that's coming. I am asking people to not make too many disruptive changes at this point. We are trying really hard to get the CI tests to a green state.

@madhusudancs
Copy link
Contributor

/lgtm
/approve

Reviewed 1 of 7 files at r1, 1 of 4 files at r2, 3 of 7 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 25, 2017
@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 Feb 27, 2017
@irfanurrehman
Copy link
Contributor Author

@madhusudancs has to rebase to remove conflicts, need the lgtm again. thanks!

@madhusudancs
Copy link
Contributor

@irfanurrehman unfortunately, it needs a rebase again. Please ping me on IM as soon as you rebase this.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2017
@irfanurrehman
Copy link
Contributor Author

@madhusudancs rebased, PTAL!

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2017
@madhusudancs
Copy link
Contributor

/lgtm

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2017
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2017
@irfanurrehman
Copy link
Contributor Author

@madhusudancs had to rebase once more.. :(, PTAL!

@irfanurrehman
Copy link
Contributor Author

@k8s-bot kops aws e2e test this

@madhusudancs
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: irfanurrehman, k8s-merge-robot, madhusudancs, spxtr

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

@madhusudancs madhusudancs added this to the v1.6 milestone Feb 28, 2017
@madhusudancs
Copy link
Contributor

@kubernetes/release-admins @idvoretskyi @calebamiles We added a new flag in 1.6 cycle that we want to remove before the release goes out. This PR removes the flag. The PR was reviewed and ready before code freeze but was struck in the rebase hell. I also forgot to add the label before. Could we get an exception for this one?

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 42200, 39535, 41708, 41487, 41335)

@k8s-github-robot k8s-github-robot merged commit aaaa7e4 into kubernetes:master Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madhusudancs madhusudancs madhusudancs left review comments

@marun marun marun left review comments

Assignees

@madhusudancs madhusudancs

@nikhiljindal nikhiljindal

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@irfanurrehman @k8s-reviewable @marun @spxtr @madhusudancs @k8s-github-robot @nikhiljindal @k8s-ci-robot