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

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

Merged
merged 2 commits into from Feb 12, 2018

Conversation

@khenidak
Contributor

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).
@khenidak

This comment has been minimized.

Contributor

khenidak commented Feb 5, 2018

/retest

@deads2k

This comment has been minimized.

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

This comment has been minimized.

Member

lavalamp commented Feb 5, 2018

/assign @cheftako

@lavalamp

This comment has been minimized.

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 Feb 5, 2018

@cheftako

/lgtm. Please make sure to clean up all of 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},

This comment has been minimized.

@cheftako

cheftako Feb 6, 2018

Member

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

This comment has been minimized.

@khenidak

khenidak Feb 6, 2018

Contributor

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

auto generated items
remove ServiceSyncPeriod from tests

fixing tests
@khenidak

This comment has been minimized.

Contributor

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 Feb 7, 2018

@khenidak

This comment has been minimized.

Contributor

khenidak commented Feb 7, 2018

/test pull-kubernetes-bazel-test

1 similar comment
@khenidak

This comment has been minimized.

Contributor

khenidak commented Feb 7, 2018

/test pull-kubernetes-bazel-test

@deads2k

This comment has been minimized.

Contributor

deads2k commented Feb 7, 2018

/lgtm
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 7, 2018

@khenidak

This comment has been minimized.

Contributor

khenidak commented Feb 7, 2018

/assign @lavalamp
/assign @mikedanese

Folks, please approve this one.

@mikedanese

This comment has been minimized.

Member

mikedanese commented Feb 12, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 12, 2018

[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

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 12, 2018

/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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 12, 2018

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

@khenidak

This comment has been minimized.

Contributor

khenidak commented Feb 12, 2018

/test pull-kubernetes-node-e2e

@khenidak

This comment has been minimized.

Contributor

khenidak commented Feb 12, 2018

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 12, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit f072871 into kubernetes:master Feb 12, 2018

13 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-node-e2e
Details
cla/linuxfoundation khenidak authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce-canary Skipped
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment