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

refactor cluster policy controller and NamespaceSCCAllocationController #65

Merged
merged 5 commits into from Jul 23, 2021
Merged

refactor cluster policy controller and NamespaceSCCAllocationController #65

merged 5 commits into from Jul 23, 2021

Conversation

atiratree
Copy link
Member

@atiratree atiratree commented Jul 5, 2021 •
edited

  • I have refactored the main cluster-policy-controller server logic and RunNamespaceSecurityAllocationController.
  • RunResourceQuotaManager controller is imported from kubernetes
  • RunClusterQuotaReconciliationController seemed more complex and unsuitable for the factory methods. But I could try to redesign the factory methods if needed

TODO:

@openshift-ci openshift-ci bot requested review from deads2k and sttts July 5, 2021 23:15
atiratree
atiratree commented Jul 5, 2021
View reviewed changes
pkg/cmd/cluster-policy-controller/cmd.go Outdated
// leader election and other "normal" behaviors.
// The context passed will be passed down to controller loops and observers and cancelled on SIGTERM and SIGINT signals.
// copied from controllercmd.ControllerCommandConfig
func (c *ClusterPolicyControllerCommandConfig) NewCommandWithContext(ctx context.Context) *cobra.Command {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is copy pasted logic for now, since we need a custom logic in StartController and access to the internal variables.

I will make a PR against library-go so this could be customizable and we could avoid copy paste

@atiratree atiratree mentioned this pull request Jul 6, 2021
Closed
@atiratree
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2021
@atiratree atiratree mentioned this pull request Jul 7, 2021
Merged
@soltysh
Copy link
Member

soltysh commented Jul 7, 2021

you will want to wait for #61 to land, it's also bumping all deps

@atiratree
Copy link
Member Author

you will want to wait for #61 to land, it's also bumping all deps

will rebase once it merges, but it is missing the bump in replaces which is included here

soltysh
soltysh reviewed Jul 7, 2021
View reviewed changes
Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

Leaving some comments, but I'll have another look once the other merges and this gets rebased. Also please put all the vendor changes into single commit.

pkg/cmd/cluster-policy-controller/cmd.go
// for metrics
_ "github.com/openshift/library-go/pkg/controller/metrics"

clusterpolicyversion "github.com/openshift/cluster-policy-controller/pkg/version"
Copy link
Member

Choose a reason for hiding this comment

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

Keep openshift imports in a single group above.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear: are we splitting openshift imports from this repo vs other repos into the groups or not? (am not sure if you are pointing out just the metrics)

pkg/cmd/cluster-policy-controller/cmd.go Outdated Show resolved Hide resolved
pkg/cmd/cluster-policy-controller/cmd.go Outdated
KubeConfigFile string
func NewClusterPolicyControllerCommand(name string) *cobra.Command {
cmd := NewClusterPolicyControllerCommandConfig("cluster-policy-controller", clusterpolicyversion.Get(), RunClusterPolicyController).
NewCommandWithContext(context.TODO())
Copy link
Member

Choose a reason for hiding this comment

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

context.Background() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am little bit confused by It is never canceled: https://golang.org/pkg/context/#Background . Nevertheless, I updated the code.

pkg/cmd/cluster-policy-controller/policy_controller.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2021
@atiratree
Copy link
Member Author

@soltysh the code was simplified to use only the default config . There are some changes to the operator that need to be merged openshift/cluster-kube-controller-manager-operator#545 .

What is the order of merging when we need both of these changes at the same time?

@atiratree atiratree force-pushed the move-controllers branch 2 times, most recently from 4040f2a to 65e972a Compare July 8, 2021 15:36
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2021
@atiratree
update go to 1.16 and vendor folder
d4c6c84 
- k8s to v0.21.2
- library-go
@atiratree
Copy link
Member Author

rebased and using WithComponentOwnerReference @soltysh

@atiratree
Copy link
Member Author

/retest

soltysh
soltysh approved these changes Jul 23, 2021
View reviewed changes
Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

pkg/security/controller/namespace_scc_allocation_controller.go
@@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"github.com/openshift/library-go/pkg/operator/events"
Copy link
Member

Choose a reason for hiding this comment

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

nit: imports order

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 23, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, soltysh

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2021
@atiratree
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2021
@openshift-merge-robot openshift-merge-robot merged commit 4de9b83 into openshift:master Jul 23, 2021
This was referenced Jul 23, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh approved these changes

@deads2k deads2k Awaiting requested review from deads2k

@sttts sttts Awaiting requested review from sttts

Assignees

@soltysh soltysh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@atiratree @soltysh @openshift-merge-robot