Bug 2012069: Refactoring Status controller #498

rluders · 2021-09-14T08:39:05Z

This PR changes the status.go organizing the logic into smaller pieces making it a little bit better.

Sample Archive

None

Documentation

None

Unit Tests

None

Privacy

Yes. There are no sensitive data in the newly collected information.

Changelog

Breaking Changes

No

References

https://issues.redhat.com/browse/CCXDEV-5350
https://bugzilla.redhat.com/show_bug.cgi?id=2012069
https://access.redhat.com/solutions/???

openshift-ci · 2021-09-14T08:39:06Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

pkg/controller/status/status.go

tremes · 2021-09-14T12:21:37Z

I think I would move related objects settings to separate function

tremes · 2021-09-14T12:25:32Z

Some inspiration can be found in the cluster-monitoring-operator :-)

rluders · 2021-09-14T13:41:14Z

Some inspiration can be found in the cluster-monitoring-operator :-)

LoL... I was doing something very similar. Thanks for the inspiration.

tremes · 2021-09-17T06:14:48Z

/retest

pkg/controller/status/controller.go

rluders · 2021-10-04T09:52:30Z

pkg/controller/status/controller.go

@@ -131,6 +131,9 @@ func (c *Controller) merge(clusterOperator *configv1.ClusterOperator) *configv1.
 		clusterOperator = newClusterOperator(c.name, nil)
 	}

+	// make sure to start a clean status controller
+	c.ctrlStatus.reset()


@tremes So, after a few tests (and fighting with the e2e tests), it seems that this is the best solution. We keep an instance to the status controller into the controller (controller everywhere) and then it encapsulates the logic to handle the statuses. But, it needs to be reset every time that the merge is executed, otherwise, the operator status would be wrong.

yes I am looking at it and it seem we are reseting everything (also the line https://github.com/openshift/insights-operator/pull/498/files#diff-10c33b0f428af9470aef2747e42fc97d97150d338b151e836d9cdebbf4fbef42R149) eveyrtime....

Yeh, but in this case it is correct. We are "resetting" 'cause the merge method receives the clusterOperator when it is called.

pkg/controller/status/conditions.go

pkg/controller/status/conditions_test.go

tremes · 2021-10-06T06:31:59Z

pkg/controller/status/conditions_test.go

+	}
+	type args struct {
+		condition configv1.ClusterStatusConditionType
+	}


this type is little bit redundant IMO

I think it's not necessary to wrap condition configv1.ClusterStatusConditionType to a new type. IOW you can use directly the configv1.ClusterStatusConditionType IMO

I see... I prefer to keep the args and fields wrapped. It makes the code more standardized test structures.

fields and why pls? I can't see any benefit TBH. It adds only unnecessary parts to the test IMO. Using conditions you can omit the https://github.com/openshift/insights-operator/pull/498/files#diff-6cd7b045851c73c1120b823f3d07a763bfb7e20aa20e241ac5d3e27e862d57eaR119-R121 and just do tt.conditions.findCondition(tt.args.condition) then.

pkg/controller/status/status.go

pkg/controller/status/status_test.go

natiiix

Aside from the commend from @tremes about "The operator is healthy" being logged right after logging an error, I see nothing wrong. The code is alright and at least somewhat commented. I tested this version of IO locally and it ran just fine. Now giving a final approval yet to give others a chance to further review the PR.

natiiix

After the fixes that have been made and a discussion with other team members, I believe this PR is now ready.
/lgtm

openshift-ci · 2021-10-07T09:27:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natiiix, rluders

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:

~~OWNERS~~ [natiiix,rluders]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-10-07T10:34:43Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-07T10:58:41Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-07T12:50:42Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-07T15:13:42Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-07T19:33:55Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-07T22:09:29Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-08T01:08:44Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-10-08T08:06:26Z

@rluders: This pull request references Bugzilla bug 2012069, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (dmisharo@redhat.com), skipping review request.

In response to this:

Bug 2012069: Refactoring Status controller

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.

openshift-ci · 2021-10-08T08:06:30Z

@rluders: This pull request references Bugzilla bug 2012069, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (dmisharo@redhat.com), skipping review request.

In response to this:

Bug 2012069: Refactoring Status controller

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.

openshift-bot · 2021-10-08T10:18:46Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-08T10:44:45Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-10-08T12:33:45Z

@rluders: All pull requests linked via external trackers have merged:

openshift/insights-operator#498

Bugzilla bug 2012069 has been moved to the MODIFIED state.

In response to this:

Bug 2012069: Refactoring Status controller

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.

Ricardo Lüders added 2 commits September 13, 2021 16:27

refactor: simplifying the logic for merge method

bec2914

refactor: extracting methods

28986b2

openshift-ci bot requested review from natiiix and tisnik September 14, 2021 08:39

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2021

tremes reviewed Sep 14, 2021

View reviewed changes

pkg/controller/status/status.go Outdated Show resolved Hide resolved

tremes reviewed Sep 14, 2021

View reviewed changes

pkg/controller/status/status.go Outdated Show resolved Hide resolved

refactor: improving variable names

a32b4db

Ricardo Lüders added 3 commits September 14, 2021 16:58

refactor: improves condition handling

3cf6071

refactor: rename status*.go to controller*.go

f32abb3

refactor: introduce controller status and status messages

c6fd846

rluders requested a review from tremes September 16, 2021 09:29

rluders marked this pull request as ready for review September 16, 2021 09:31

rluders marked this pull request as draft September 16, 2021 09:55

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2021

Ricardo Lüders added 2 commits September 16, 2021 12:45

style: fixing linting issues

2814df4

fix: error handling to status.go

c4755bb

rluders marked this pull request as ready for review September 16, 2021 14:08

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2021

tremes reviewed Sep 17, 2021

View reviewed changes

pkg/controller/status/controller.go Outdated Show resolved Hide resolved

tremes reviewed Sep 17, 2021

View reviewed changes

pkg/controller/status/controller.go Outdated Show resolved Hide resolved

tremes reviewed Sep 17, 2021

View reviewed changes

pkg/controller/status/controller.go Outdated Show resolved Hide resolved

tremes reviewed Sep 17, 2021

View reviewed changes

pkg/controller/status/controller.go Outdated Show resolved Hide resolved

Ricardo Lüders added 2 commits September 17, 2021 10:08

style: converting TODOS to issues

5ee0837

refactor: smaller fixes

cc26439

rluders commented Oct 4, 2021

View reviewed changes

fix: check healthy operator status

53b5d39

Serhii1011010 reviewed Oct 5, 2021

View reviewed changes

pkg/controller/status/conditions.go Outdated Show resolved Hide resolved

tremes reviewed Oct 6, 2021

View reviewed changes

pkg/controller/status/conditions_test.go Show resolved Hide resolved

tremes reviewed Oct 6, 2021

View reviewed changes

pkg/controller/status/status.go Outdated Show resolved Hide resolved

tremes reviewed Oct 6, 2021

View reviewed changes

pkg/controller/status/status_test.go Show resolved Hide resolved

tremes reviewed Oct 6, 2021

View reviewed changes

pkg/controller/status/status_test.go Show resolved Hide resolved

natiiix reviewed Oct 6, 2021

View reviewed changes

fix: improving code based on review

69a4731

natiiix approved these changes Oct 7, 2021

View reviewed changes

openshift-ci bot assigned natiiix Oct 7, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2021

rluders changed the title ~~Refactoring Status controller~~ Bug 2012069: Refactoring Status controller Oct 8, 2021

openshift-ci bot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Oct 8, 2021

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Oct 8, 2021

openshift-merge-robot merged commit 94660b7 into openshift:master Oct 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2012069: Refactoring Status controller #498

Bug 2012069: Refactoring Status controller #498

rluders commented Sep 14, 2021 •

edited

Loading

openshift-ci bot commented Sep 14, 2021

tremes commented Sep 14, 2021

tremes commented Sep 14, 2021

rluders commented Sep 14, 2021

tremes commented Sep 17, 2021

rluders Oct 4, 2021

tremes Oct 5, 2021

rluders Oct 5, 2021

tremes Oct 6, 2021

rluders Oct 6, 2021

tremes Oct 6, 2021

rluders Oct 6, 2021

tremes Oct 6, 2021

natiiix left a comment

natiiix left a comment

openshift-ci bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 8, 2021

openshift-ci bot commented Oct 8, 2021

openshift-ci bot commented Oct 8, 2021

openshift-bot commented Oct 8, 2021

openshift-bot commented Oct 8, 2021

openshift-ci bot commented Oct 8, 2021

Bug 2012069: Refactoring Status controller #498

Bug 2012069: Refactoring Status controller #498

Conversation

rluders commented Sep 14, 2021 • edited Loading

Categories

Sample Archive

Documentation

Unit Tests

Privacy

Changelog

Breaking Changes

References

openshift-ci bot commented Sep 14, 2021

tremes commented Sep 14, 2021

tremes commented Sep 14, 2021

rluders commented Sep 14, 2021

tremes commented Sep 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natiiix left a comment

Choose a reason for hiding this comment

natiiix left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 7, 2021

openshift-bot commented Oct 8, 2021

openshift-ci bot commented Oct 8, 2021

openshift-ci bot commented Oct 8, 2021

openshift-bot commented Oct 8, 2021

openshift-bot commented Oct 8, 2021

openshift-ci bot commented Oct 8, 2021

rluders commented Sep 14, 2021 •

edited

Loading