Skip to content

Add controllerset for controlling an apiserver deployment#667

Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
stlaz:apiservercontrollers
Jan 20, 2020
Merged

Add controllerset for controlling an apiserver deployment#667
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
stlaz:apiservercontrollers

Conversation

@stlaz
Copy link
Contributor

@stlaz stlaz commented Jan 16, 2020

Add controller for NS finalization of a running API server deployment, controller handling an API server deployment's APIServices, and a controllerset which bundles these controllers + some others that do the API server housekeeping

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 16, 2020
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 16, 2020
@stlaz stlaz force-pushed the apiservercontrollers branch 2 times, most recently from 47ad796 to cfb43b7 Compare January 16, 2020 13:49
@stlaz stlaz mentioned this pull request Jan 16, 2020
Closed
@deads2k
Copy link
Contributor

deads2k commented Jan 16, 2020

straigth move? if so, it can be lgtmd

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2020
sttts
sttts reviewed Jan 20, 2020
View reviewed changes
pkg/operator/nsfinalizercontroller/finalizer_controller.go
// so it deletes the rest and requeues. The ns controller starts again and does a complete discovery and.... fails. The
// failure means it refuses to complete the cleanup. Now, we don't actually want to delete the resoruces from our
// aggregated API, only the server plus config if we remove the apiservices to unstick it, GC will start cleaning
// everything. For now, we can unbork 4.0, but clearing the finalizer after the pod and daemonset we created are gone.
Copy link
Contributor

@sttts sttts Jan 20, 2020

Choose a reason for hiding this comment

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

with so much apiserver specifics, why not put everything into pkg/operator/apiserver/controller/namespacefinalizer, and the others in similar places?

sttts
sttts reviewed Jan 20, 2020
View reviewed changes
pkg/operator/apiservicecontroller/apiservice_controller.go Outdated
@@ -0,0 +1,234 @@
package apiservicecontroller
Copy link
Contributor

@sttts sttts Jan 20, 2020

Choose a reason for hiding this comment

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

pkg/operator/apiserver/controllers/apiserver

sttts
sttts reviewed Jan 20, 2020
View reviewed changes
pkg/operator/apiservercontrollerset/apiservercontrollerset.go
@@ -0,0 +1,199 @@
package apiservercontrollerset
Copy link
Contributor

@sttts sttts Jan 20, 2020

Choose a reason for hiding this comment

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

pkg/operator/apiserver/controllerset

@stlaz stlaz force-pushed the apiservercontrollers branch from cfb43b7 to a366955 Compare January 20, 2020 08:54
@stlaz
Copy link
Contributor Author

stlaz commented Jan 20, 2020 •
edited
Loading

@sttts thanks, that makes sense, added the suggested changes.

@deads2k it is a clean move (with the repackaging now) + a fix to use controllers directly in the slice https://github.com/openshift/library-go/pull/667/files#diff-0c281fce8ed3b258f8b6fb4cec7b5ea7R196-R198 in case the goroutine triggered after the loop variable changed to point to the next controller.

@stlaz
Copy link
Contributor Author

stlaz commented Jan 20, 2020

/hold
I can see there were changes to the code in COA-o

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2020
p0lyn0mial
p0lyn0mial reviewed Jan 20, 2020
View reviewed changes
pkg/operator/apiserver/controller/apiservice/apiservice_controller.go
queue workqueue.RateLimitingInterface
}

func NewAPIServiceController(
Copy link
Contributor

@p0lyn0mial p0lyn0mial Jan 20, 2020

Choose a reason for hiding this comment

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

this has slightly changed in openshift/cluster-openshift-apiserver-operator#294

worth picking as it made it more generic.

@stlaz stlaz force-pushed the apiservercontrollers branch from a366955 to 79d55b8 Compare January 20, 2020 15:08
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2020
@stlaz stlaz force-pushed the apiservercontrollers branch from 79d55b8 to 2ce1427 Compare January 20, 2020 15:10
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2020
@stlaz
Copy link
Contributor Author

stlaz commented Jan 20, 2020

/hold cancel
this should now include latest changes done in COA-o

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2020
@stlaz stlaz force-pushed the apiservercontrollers branch from 2ce1427 to 1ad3a14 Compare January 20, 2020 15:13
@p0lyn0mial
Copy link
Contributor

p0lyn0mial commented Jan 20, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2020
@openshift-merge-robot openshift-merge-robot merged commit 906409a into openshift:master Jan 20, 2020
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jan 20, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, p0lyn0mial, stlaz

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

The pull request process is described here

Details 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

bertinatto pushed a commit to bertinatto/library-go that referenced this pull request Jul 2, 2020
@openshift-merge-robot
Merge pull request openshift#667 from stlaz/apiservercontrollers
33a8109 
Add controllerset for controlling an apiserver deployment
@ingvagabund ingvagabund mentioned this pull request Jun 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@p0lyn0mial p0lyn0mial p0lyn0mial left review comments

@deads2k deads2k Awaiting requested review from deads2k

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

+1 more reviewer

@sttts sttts sttts left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@p0lyn0mial p0lyn0mial

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@stlaz @deads2k @p0lyn0mial @openshift-ci-robot @sttts @openshift-merge-robot