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

[Federation][Kubefed] Bug fix to enable disabling federation controllers through override args #44209

Merged
merged 1 commit into from
Apr 11, 2017

Conversation

irfanurrehman
Copy link
Contributor

@irfanurrehman irfanurrehman commented Apr 7, 2017

Targets #42761

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

Special notes for your reviewer:
@marun @perotinus @nikhiljindal

Release note:

NONE 

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

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 7, 2017
@irfanurrehman
Copy link
Contributor Author

@k8s-bot cvm gce e2e test this

@perotinus
Copy link
Contributor

I'm not super enthused that this could break people's scripted usages of kubefed, since it's now in beta. That said, I'm not sure that I can see a better way to do this, except revamping the whole system to use some sort of configuration file rather than command-line parameters. Which, incidentally, is how kubeadm does this: https://kubernetes.io/docs/admin/kubeadm/#config-file

I'm wondering if we should move from a flag to doing this the way kubeadm does. Of course, this is complicated by the fact that kubefed is in beta now.

@marun
Copy link
Contributor

marun commented Apr 7, 2017

Is this potential complexity worth it? Or might it be preferable to expose any multivariate configuration option for apiserver or controllermanager as a first-class argument to kubefed init?

@madhusudancs
Copy link
Contributor

I'm not super enthused that this could break people's scripted usages of kubefed, since it's now in beta. That said, I'm not sure that I can see a better way to do this, except revamping the whole system to use some sort of configuration file rather than command-line parameters. Which, incidentally, is how kubeadm does this: https://kubernetes.io/docs/admin/kubeadm/#config-file

I'm wondering if we should move from a flag to doing this the way kubeadm does. Of course, this is complicated by the fact that kubefed is in beta now.

Yes. We have discussed this before. Someone should bite the bullet. But note that we want to go the ComponentConfig route. SIG-node and SIG-scheduling are working on that. So it might be worth discussing with them. I had a chat with @mtaufen long back. I don't know the current status though.

Is this potential complexity worth it? Or might it be preferable to expose any multivariate configuration option for apiserver or controllermanager as a first-class argument to kubefed init?

How is that less complex?

@irfanurrehman
Copy link
Contributor Author

Should I limit this PR to only update the marshaling of arguments such that = in the values don't break us?
It would at least be possible to disable one controller in near term?

@madhusudancs
Copy link
Contributor

Yeah, I think we should do that to begin with and cherry-pick that to v1.6.x branch before we resolve the other issues with syntax changes for 1.7

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

updated, PTAL!

@marun
Copy link
Contributor

marun commented Apr 10, 2017

How is that less complex?

More complex in terms of UX, not implementation. I think it would be simpler for the user to provide a first-class argument to kubefed vs having to remember kubefed-specific formatting for multivalued arguments.

@madhusudancs
Copy link
Contributor

More complex in terms of UX, not implementation. I think it would be simpler for the user to provide a first-class argument to kubefed vs having to remember kubefed-specific formatting for multivalued arguments.

There are pros and cons. There are the advantages that you mention here. But on the other hand

  1. Every user, even the ones who don't care about these flags, has to sift through the ton of documentation that is generated for these flags.
  2. Each such flag has to be dealt on a case-by-case basis. If we miss a flag in a release, there will be a window where users will have to wait for that flag to be available in kubefed.
  3. Adds to the confusion. There are now multiple ways of providing override args.
  4. Name collision. You might have a flag with a same name for different components that mean different things. Now you need to namespace them or qualify them with the component name. In either case those names are not exactly the same as the one taken by the components and users need to learn this.

@perotinus
Copy link
Contributor

What's here LGTM, for the issue with marshaling multiple =. Perhaps we should continue any discussion about the higher-level problem in #42761?

@madhusudancs
Copy link
Contributor

Perhaps we should continue any discussion about the higher-level problem in #42761?

Agreed.

@madhusudancs
Copy link
Contributor

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irfanurrehman, madhusudancs

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 k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2017
@madhusudancs
Copy link
Contributor

@irfanurrehman please also cherry-pick this to release-1.6 branch.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 44286, 44209)

@k8s-github-robot k8s-github-robot merged commit 2976cb8 into kubernetes:master Apr 11, 2017
k8s-github-robot pushed a commit that referenced this pull request Apr 11, 2017
Automatic merge from submit-queue

[Federation][Kubefed] Bug fix to enable disabling federation controllers through override args

Targets #42761

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

**Special notes for your reviewer**:
PR #44209 is merged to master.
This fix needs to be cherry-picked to release-1.6 also.

**Release note**:

```release-note
Fix for [disabling federation controllers through override args](#42761).
```
mintzhao pushed a commit to mintzhao/kubernetes that referenced this pull request Jun 1, 2017
…lease-1.6

Automatic merge from submit-queue

[Federation][Kubefed] Bug fix to enable disabling federation controllers through override args

Targets kubernetes#42761

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

**Special notes for your reviewer**:
PR kubernetes#44209 is merged to master.
This fix needs to be cherry-picked to release-1.6 also.

**Release note**:

```release-note
Fix for [disabling federation controllers through override args](kubernetes#42761).
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants