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

Remove controller-manager --service-sync-period flag #59359

Merged
merged 2 commits into from
Feb 12, 2018
Merged

Remove controller-manager --service-sync-period flag #59359

merged 2 commits into from
Feb 12, 2018

Conversation

khenidak
Copy link
Contributor

@khenidak khenidak commented Feb 5, 2018

What this PR does / why we need it:
This PR removes controller manager --service-sync-period flag which is not used anywhere in the code and is causing confusion

Which issue(s) this PR fixes
#58776

Special notes for your reviewer:
@deads2k this remove the flag as per the discussion on #58776
2 commits

  1. one for code change
  2. one for auto generated code

Release note:

1. Controller-manager --service-sync-period flag is removed (was never used in the code).

@k8s-ci-robot k8s-ci-robot added 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 5, 2018
@k8s-github-robot k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Feb 5, 2018
@jdumars jdumars added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Feb 5, 2018
@khenidak
Copy link
Contributor Author

khenidak commented Feb 5, 2018

/retest

@deads2k
Copy link
Contributor

deads2k commented Feb 5, 2018

This flag was disconnected at least as far back as 1.4. I'll leave this open for a couple days in case someone can find a way it could have worked in the past year and change. Lacking that, I plan to merge on Wednesday.

@khenidak please squash into two commits: one for work, one for generated.

/approve

@lavalamp
Copy link
Member

lavalamp commented Feb 5, 2018

/assign @cheftako

@lavalamp
Copy link
Member

lavalamp commented Feb 5, 2018

if anything this flag probably belongs in the cloud controller binary rather than the main one?

@lavalamp lavalamp removed their request for review February 5, 2018 19:21
cheftako
cheftako approved these changes Feb 6, 2018
View reviewed changes
Copy link
Member

@cheftako cheftako left a comment

Choose a reason for hiding this comment

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

/lgtm. Please make sure to clean up all of cmd/kube-controller-manager/app/options/options_test.go.

cmd/kube-controller-manager/app/options/options_test.go
@@ -138,7 +138,6 @@ func TestAddFlags(t *testing.T) {
EnableProfiling: false,
CIDRAllocatorType: "CloudAllocator",
NodeCIDRMaskSize: 48,
ServiceSyncPeriod: metav1.Duration{Duration: 2 * time.Minute},
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we clean up line 103 as well? ("--service-sync-period=2m",)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I am working on flushing out out of tests.

@khenidak
auto generated items
f9c8d7d 
remove ServiceSyncPeriod from tests

fixing tests
@khenidak
Copy link
Contributor Author

khenidak commented Feb 7, 2018

@deads2k squashed -- and all test passed except pull-kubernetes-unit which i believe is a false positive (please let me know if it is not).

@timothysc timothysc removed their request for review February 7, 2018 03:45
@khenidak
Copy link
Contributor Author

khenidak commented Feb 7, 2018 •
edited
Loading

/test pull-kubernetes-bazel-test

1 similar comment
@khenidak
Copy link
Contributor Author

khenidak commented Feb 7, 2018

/test pull-kubernetes-bazel-test

@deads2k
Copy link
Contributor

deads2k commented Feb 7, 2018

/lgtm
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2018
@khenidak
Copy link
Contributor Author

khenidak commented Feb 7, 2018

/assign @lavalamp
/assign @mikedanese

Folks, please approve this one.

@mikedanese
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, khenidak, mikedanese

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these OWNERS Files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2018
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@khenidak
Copy link
Contributor Author

/test pull-kubernetes-node-e2e

@khenidak
Copy link
Contributor Author

the e2e is flaky
W0212 20:53:45.956] F0212 20:53:45.956282 777 run_remote.go:233] Could not retrieve list of images based on image prefix "cos-stable-63-10032-71-0": Failed to list images in project "cos-cloud": Get https://www.googleapis.com/compute/beta/projects/cos-cloud/global/images?alt=json: oauth2: cannot fetch token: Post https://accounts.google.com/o/oauth2/token: net/http: TLS handshake timeout

I will try later tonight.

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f072871 into kubernetes:master Feb 12, 2018
This was referenced Feb 13, 2018
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheftako cheftako cheftako approved these changes

@markturansky markturansky Awaiting requested review from markturansky

Assignees

@lavalamp lavalamp

@cheftako cheftako

@mikedanese mikedanese

@deads2k deads2k

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. 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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@khenidak @deads2k @lavalamp @mikedanese @k8s-ci-robot @fejta-bot @k8s-github-robot @cheftako @jdumars