[OCPCLOUD-1307] Refactor CCCMO cluster-operator resource handling #152
Conversation
|
/retest |
1 similar comment
|
/retest |
| if err := r.setCloudControllerOwnerCondition(ctx); err != nil { | ||
| // Set the CloudControllerOwner condition to true | ||
| if err := r.setClusterOperatorCondition(ctx, cloudControllerOwnershipCondition, configv1.ConditionTrue, ReasonAsExpected, | ||
| fmt.Sprintf("Cluster Cloud Controller Manager Operator owns cloud controllers at %s", r.ReleaseVersion)); err != nil { |
There was a problem hiding this comment.
klog.Infof IMO is better fit there.
There was a problem hiding this comment.
We need to provide a formatted string value to the function setClusterOperatorCondition. klog.Infof doesn't do this, but just prints the string to the log.
There was a problem hiding this comment.
Oh, sorry. Misread this line... Dunno why, but i thought its a log message
pkg/controllers/status.go
Outdated
| @@ -30,10 +31,17 @@ const ( | |||
| defaultManagementNamespace = "openshift-cloud-controller-manager-operator" | |||
| ) | |||
|
|
|||
| type ClusterOperatorStatus struct { | |||
There was a problem hiding this comment.
I'm thinking about better name for this struct...
ClusterOperatorStatusClient or smth like BaseClusterOperatorReconciler maybe.
Naming of this thing is a bit confusing IMHO. We are using this for embedding k8s client into reconcilers.
There was a problem hiding this comment.
ClusterOperatorStatusClient works for me.
|
/retest |
Fedosin
force-pushed
the
cluster_operator_status
branch
from
40896cb
to
e1b2991
Compare
Fedosin
force-pushed
the
cluster_operator_status
branch
3 times, most recently
from
46ba321
to
ff7e631
Compare
|
/hold we need to merge #153 first |
openshift-ci
bot
added
the
do-not-merge/hold
This commit moves logic from ClusterOperatorReconciler related to the status management into its own type - ClusterOperatorStatus. It allows to reuse it in other controllers.
Fedosin
force-pushed
the
cluster_operator_status
branch
from
ff7e631
to
44ec015
Compare
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
This commit injects ClusterOperatorStatus struct into config sync controller, which allows it to set the operator status.
This commit injects ClusterOperatorStatus struct into trusted ca bundle controller, which allows it to set the operator status.
Before deploying CCM resources we need to check if dependant controllers are available, and degrade the operator if any of them doesn't work properly.
Fedosin
force-pushed
the
cluster_operator_status
branch
from
44ec015
to
776c006
Compare
|
/test unit |
This commit adds missing deletions of the cluster operator resource after each test.
Fedosin
force-pushed
the
cluster_operator_status
branch
from
776c006
to
6306895
Compare
|
/retest |
|
/test e2e-azure-ccm |
|
/retest |
|
@Fedosin: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| @@ -30,10 +31,17 @@ const ( | |||
| defaultManagementNamespace = "openshift-cloud-controller-manager-operator" | |||
| ) | |||
|
|
|||
| type ClusterOperatorStatusClient struct { | |||
There was a problem hiding this comment.
I find it odd that this is exported, even though none of the methods are. Perhaps instead of constructing this in a constructor in each controller?
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed 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 |
openshift-ci
bot
added
the
approved
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
This PR moves logic from ClusterOperatorReconciler related to the status management into its own type - ClusterOperatorStatus. It allows to reuse it in other controllers.