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

deprecate unused option deployment-controller-sync-period for deploym… #103538

Merged
merged 1 commit into from Aug 5, 2021
Merged

deprecate unused option deployment-controller-sync-period for deploym… #103538

merged 1 commit into from Aug 5, 2021

Conversation

Pingan2017
Copy link
Member

…ent controller

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

deployment-controller-sync-period option is unused in code, deprecate it.

grep -r 'DeploymentControllerSyncPeriod'
cmd/kube-controller-manager/app/options/options_test.go:				DeploymentControllerSyncPeriod: metav1.Duration{Duration: 45 * time.Second},
cmd/kube-controller-manager/app/options/options_test.go:				DeploymentControllerSyncPeriod: metav1.Duration{Duration: 45 * time.Second},
cmd/kube-controller-manager/app/options/deploymentcontroller.go:	fs.DurationVar(&o.DeploymentControllerSyncPeriod.Duration, "deployment-controller-sync-period", o.DeploymentControllerSyncPeriod.Duration, "Period for syncing the deployments.")
cmd/kube-controller-manager/app/options/deploymentcontroller.go:	cfg.DeploymentControllerSyncPeriod = o.DeploymentControllerSyncPeriod
_output/KUBE_violations.report:API rule violation: names_match,k8s.io/kube-controller-manager/config/v1alpha1,DeploymentControllerConfiguration,DeploymentControllerSyncPeriod
staging/src/k8s.io/kube-controller-manager/config/v1alpha1/zz_generated.deepcopy.go:	out.DeploymentControllerSyncPeriod = in.DeploymentControllerSyncPeriod
staging/src/k8s.io/kube-controller-manager/config/v1alpha1/types.go:	DeploymentControllerSyncPeriod metav1.Duration
pkg/controller/deployment/config/zz_generated.deepcopy.go:	out.DeploymentControllerSyncPeriod = in.DeploymentControllerSyncPeriod
pkg/controller/deployment/config/types.go:	DeploymentControllerSyncPeriod metav1.Duration
pkg/controller/deployment/config/v1alpha1/zz_generated.conversion.go:	out.DeploymentControllerSyncPeriod = in.DeploymentControllerSyncPeriod
pkg/controller/deployment/config/v1alpha1/zz_generated.conversion.go:	out.DeploymentControllerSyncPeriod = in.DeploymentControllerSyncPeriod
pkg/controller/deployment/config/v1alpha1/defaults.go:	if obj.DeploymentControllerSyncPeriod == zero {
pkg/controller/deployment/config/v1alpha1/defaults.go:		obj.DeploymentControllerSyncPeriod = metav1.Duration{Duration: 30 * time.Second}
pkg/generated/openapi/zz_generated.openapi.go:					"DeploymentControllerSyncPeriod": {
pkg/generated/openapi/zz_generated.openapi.go:				Required: []string{"ConcurrentDeploymentSyncs", "DeploymentControllerSyncPeriod"},
api/api-rules/violation_exceptions.list:API rule violation: names_match,k8s.io/kube-controller-manager/config/v1alpha1,DeploymentControllerConfiguration,DeploymentControllerSyncPeriod

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

the flag `--deployment-controller-sync-period` has no effect now, deprecate it and will be removed in v1.24.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 7, 2021
@k8s-ci-robot
Copy link
Contributor

@Pingan2017: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 7, 2021
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 7, 2021
@Pingan2017
Copy link
Member Author

/sig apps

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Jul 7, 2021
@Pingan2017
Copy link
Member Author

/assign @lavalamp

@caesarxuchao
Copy link
Member

/remove-sig api-machinery
/unassign @lavalamp
/sig apps

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 8, 2021
@caesarxuchao
Copy link
Member

/assign

@caesarxuchao
Copy link
Member

Agree that this option is not used in the code base.

/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 Jul 8, 2021
@caesarxuchao
Copy link
Member

/unassign
/assign @cheftako @sttts

Can you help approve? Thanks.

@k8s-ci-robot k8s-ci-robot assigned cheftako and sttts and unassigned caesarxuchao Jul 8, 2021
@lavalamp
Copy link
Member

lavalamp commented Jul 8, 2021

Should it be used, though? Maybe we should use it instead of removing it?

@Pingan2017
Copy link
Member Author

Pingan2017 commented Jul 9, 2021 •
edited

Should it be used, though? Maybe we should use it instead of removing it?

we didn't need it . It's used for list deployment period when we has no informer.

kubernetes/cmd/kube-controller-manager/app/controllermanager.go

Lines 351 to 355 in aec7b41

if containsResource(resources, "deployments") {
glog.Infof("Starting deployment controller")
deployment.New(kubeClient).
Run(s.DeploymentControllerSyncPeriod)
}

kubernetes/pkg/controller/deployment/deployment_controller.go

Lines 53 to 73 in aec7b41

func (d *DeploymentController) Run(syncPeriod time.Duration) {
go util.Until(func() {
errs := d.reconcileDeployments()
for _, err := range errs {
glog.Errorf("Failed to reconcile: %v", err)
}
}, syncPeriod, util.NeverStop)
}
func (d *DeploymentController) reconcileDeployments() []error {
list, err := d.expClient.Deployments(api.NamespaceAll).List(labels.Everything(), fields.Everything())
if err != nil {
return []error{fmt.Errorf("error listing deployments: %v", err)}
}
errs := []error{}
for _, deployment := range list.Items {
if err := d.reconcileDeployment(&deployment); err != nil {
errs = append(errs, fmt.Errorf("error in reconciling deployment %s: %v", deployment.Name, err))
}
}
return errs

@lavalamp
Copy link
Member

lavalamp commented Jul 9, 2021

OK.

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, lavalamp, Pingan2017

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 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 Jul 9, 2021
@lavalamp
Copy link
Member

lavalamp commented Jul 9, 2021

I would ask for an exception on this one, it should be extremely low risk.

@cheftako
Copy link
Member

/lgtm
Silly question. I realize its unused but the deprecation note lists it as being removed in 1.24. Do we want to remove it early?

@Pingan2017
Copy link
Member Author

/lgtm
Silly question. I realize its unused but the deprecation note lists it as being removed in 1.24. Do we want to remove it early?

It’s not clear if anyone has configured this option, It's better to leave enough time to remind users.

@k8s-ci-robot k8s-ci-robot merged commit f1c8176 into kubernetes:master Aug 5, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Aug 5, 2021
@Pingan2017 Pingan2017 deleted the deprecate-option-controller-0707 branch September 6, 2021 03:50
@SataQiu SataQiu mentioned this pull request Dec 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@resouer resouer Awaiting requested review from resouer

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees

@lavalamp lavalamp

@sttts sttts

@cheftako cheftako

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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Pingan2017 @k8s-ci-robot @caesarxuchao @lavalamp @cheftako @sttts