[KOGITO-3736] - Reconciler error in Data index controller when Kogito… #643

vaibhavjainwiz · 2020-10-30T19:40:00Z

…Runtime is deployed

Jira issue : https://issues.redhat.com/browse/KOGITO-3736

Many thanks for submitting your Pull Request ❤️!

Analysis

As analysed provided logs, I have found that Data-index controller called to reconcile with name and namespace of KogitoRuntimeService. I happed because data-index controller is watching for config map whose owner is KogitoRuntime and handler is provided as of type `

EnqueueRequestForOwner`. Any controller should only watch object which it owns not others. But In this data-index controller is watching object which onwed by KogitoRuntime.

Solution:

KogitoRuntime should monitor configMap which it owns and update deployment object of data-index directly if protoBuf config map update. With this change there is no requirement to maintain seperate controller for data-index.

Please make sure that your PR meets the following requirements:

You have read the contributors' guide
Pull Request title is properly formatted: [KOGITO-XYZ] Subject
Pull Request contains a link to the JIRA issue
Pull Request contains a description of the issue
Pull Request does not include fixes for issues other than the main ticket
Your feature/bug fix has a unit test that verifies it
You've tested the new feature/bug fix in an actual OpenShift cluster

…Runtime is deployed

codecov · 2020-10-30T19:43:29Z

Codecov Report

Merging #643 into master will decrease coverage by 3.48%.
The diff coverage is 56.98%.

@@            Coverage Diff             @@
##           master     #643      +/-   ##
==========================================
- Coverage   42.01%   38.52%   -3.49%     
==========================================
  Files         169      150      -19     
  Lines        9012     6412    -2600     
==========================================
- Hits         3786     2470    -1316     
+ Misses       4812     3558    -1254     
+ Partials      414      384      -30

Flag	Coverage Δ
cli	`53.93% <56.98%> (-1.05%)`	⬇️
operator	`34.32% <ø> (-4.25%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/kogito/command/service/build_service.go	`0.00% <0.00%> (ø)`
cmd/kogito/command/service/runtime_service.go	`0.00% <0.00%> (ø)`
cmd/kogito/command/shared/default_services.go	`0.00% <0.00%> (ø)`
cmd/kogito/command/shared/install_services.go	`0.00% <0.00%> (ø)`
cmd/kogito/command/shared/resource_checks.go	`63.29% <0.00%> (-4.53%)`	⬇️
pkg/apis/app/v1alpha1/conditions.go	`93.10% <ø> (+3.62%)`	⬆️
pkg/apis/app/v1alpha1/kogitobuild_types.go	`14.28% <ø> (-85.72%)`	⬇️
pkg/apis/app/v1alpha1/kogitoinfra_types.go	`20.00% <ø> (-80.00%)`	⬇️
pkg/apis/app/v1alpha1/kogitoruntime_types.go	`10.00% <ø> (-2.50%)`	⬇️
pkg/apis/app/v1alpha1/kogitoservices_types.go	`0.00% <ø> (ø)`
... and 203 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c175d9b...c64613a. Read the comment docs.

ricardozanini

+1 to this refactoring!

@Kaitou786 @vaibhavjainwiz I remember you saying that we had to create another controller because of the unnecessary reconciliations in a scenario where we had lots of support services. I'm assuming that after this PR won't be a problem anymore?

EDIT: @vaibhavjainwiz's comment made this clear.

ricardozanini · 2020-10-30T19:47:19Z

/jenkins test

kie-ci-bot · 2020-10-30T19:48:44Z

Change detected in the PR, requesting reviews and running pipeline(if required) again

vaibhavjainwiz · 2020-10-30T19:58:45Z

+1 to this refactoring!

@Kaitou786 @vaibhavjainwiz I remember you saying that we had to create another controller because of the unnecessary reconciliations in a scenario where we had lots of supporting services. I'm assuming that after this PR won't be a problem any more?

I had tested it for couple of scenerios. Thing should work as expected. 🤞

msmagnanijr · 2020-10-31T10:27:27Z

/jenkins test

vaibhavjainwiz · 2020-10-31T13:42:16Z

/jenkins test

Kaitou786 · 2020-11-02T04:15:00Z

/jenkins test

Kaitou786 · 2020-11-03T05:47:05Z

/jenkins test

pkg/controller/kogitoruntime/kogitoruntime_controller.go

ricardozanini · 2020-11-04T21:25:52Z

@vaibhavjainwiz could you please rebase your PR and review the comments? Many thanks!

kie-ci-bot · 2020-11-05T07:40:35Z

Change detected in the PR, requesting reviews and running pipeline(if required) again

kie-ci-bot · 2020-11-05T08:33:37Z

Change detected in the PR, requesting reviews and running pipeline(if required) again

pkg/infrastructure/data_index.go

kie-ci-bot · 2020-11-05T10:54:20Z

Change detected in the PR, requesting reviews and running pipeline(if required) again

pkg/controller/kogitosupportingservice/kogitosupportingservice_controller.go

pkg/infrastructure/data_index.go

sutaakar · 2020-11-05T11:29:41Z

Otherwise the code looks ok, will try it on OCP instance.

kie-ci-bot · 2020-11-05T11:58:03Z

Change detected in the PR, requesting reviews and running pipeline(if required) again

kie-ci-bot · 2020-11-05T12:16:26Z

Change detected in the PR, requesting reviews and running pipeline(if required) again

kie-ci-bot · 2020-11-05T13:40:41Z

Change detected in the PR, requesting reviews and running pipeline(if required) again

ricardozanini · 2020-11-05T13:41:27Z

/jenkins test

@spolti @sutaakar can you guys please take a look? :)

sutaakar · 2020-11-05T17:29:52Z

pkg/controller/kogitosupportingservice/kogitodataindex_reconciler.go

+
+// Reconcile reconcile Data Index
+func (*DataIndexSupportingServiceResource) Reconcile(client *client.Client, instance *appv1alpha1.KogitoSupportingService, scheme *runtime.Scheme) (reconcileAfter time.Duration, err error) {
+	log.Infof("Reconciling KogitoDataIndex for %s in %s", instance.Name, instance.Namespace)


Seems to be a duplicit log entry, in the log I see:

{"level":"info","T":"2020-11-05T17:20:54.598Z","logger":"kogitosupportingservice_controller","msg":"Reconciling KogitoSupportingService for data-index in ksuta-test"} {"level":"info","T":"2020-11-05T17:20:54.608Z","logger":"kogitosupportingservice_controller","msg":"Reconciling KogitoDataIndex for data-index in ksuta-test"}

I think this log can be removed.

sutaakar · 2020-11-05T17:37:14Z

One general comment, I have noticed that *-protobuf-files config map is created even when there are no protobuf files in Kogito application. Would it have sense to skip creation of *-protobuf-files config map in that case?

ricardozanini · 2020-11-05T17:42:40Z

One general comment, I have noticed that *-protobuf-files config map is created even when there are no protobuf files in Kogito application. Would it have sense to skip creation of *-protobuf-files config map in that case?

In the old feature, we always had to create it because the operator wouldn't know if there were protobuf files or not in the service. Now that we fetch for this information, I would say that we could skip this creation. Can you file a JIRA to follow this up? :D

sutaakar · 2020-11-05T17:58:54Z

Reported as https://issues.redhat.com/browse/KOGITO-3779

sutaakar · 2020-11-05T18:03:21Z

pkg/infrastructure/data_index.go

+		// append volume mount if its not exists
+		deployment.Spec.Template.Spec.Containers[0].VolumeMounts =
+			append(deployment.Spec.Template.Spec.Containers[0].VolumeMounts, corev1.VolumeMount{
+				Name:      cm.Name,


Minor? In case config map contains more protobuf files the Name parameter here is duplicated for every volume mount entry for those protobuf files. Shouldn't it be rather unique? Though I haven't see any issue with it so far.

Just realized the name is used to pair volume mounts with volumes.

sutaakar

Seems to work as expected. Approving. One optional comment related to logging duplicity above.
Good job.

[KOGITO-3736] - Reconciler error in Data index controller when Kogito…

fd90149

…Runtime is deployed

kie-ci-bot bot requested review from ricardozanini and spolti October 30, 2020 19:40

kie-ci-bot bot added needs review 🔍 Pull Request that needs reviewers operator ☁️ Pull Request related to kogito-operator code base labels Oct 30, 2020

vaibhavjainwiz requested review from Kaitou786 and sutaakar October 30, 2020 19:40

ricardozanini approved these changes Oct 30, 2020

View reviewed changes

[KOGITO-3736] - Update release note

3d4cc71

Kaitou786 approved these changes Nov 3, 2020

View reviewed changes

spolti suggested changes Nov 3, 2020

View reviewed changes

pkg/controller/kogitoruntime/kogitoruntime_controller.go Show resolved Hide resolved

Incorporate review comments

de3d6c0

Merge branch 'master' into KOGITO-3736

0679beb

sutaakar reviewed Nov 5, 2020

View reviewed changes

pkg/infrastructure/data_index.go Outdated Show resolved Hide resolved

sutaakar reviewed Nov 5, 2020

View reviewed changes

pkg/infrastructure/data_index.go Outdated Show resolved Hide resolved

Incorporate review comments

3884a57

vaibhavjainwiz requested a review from sutaakar November 5, 2020 10:55

sutaakar reviewed Nov 5, 2020

View reviewed changes

pkg/controller/kogitosupportingservice/kogitosupportingservice_controller.go Outdated Show resolved Hide resolved

sutaakar reviewed Nov 5, 2020

View reviewed changes

pkg/controller/kogitosupportingservice/kogitosupportingservice_controller.go Outdated Show resolved Hide resolved

sutaakar reviewed Nov 5, 2020

View reviewed changes

pkg/infrastructure/data_index.go Outdated Show resolved Hide resolved

Incorporate review comments

9065bdc

Move protoBufConfigMapVolumeDefaultMode const to file_permissions file

18e6ebe

using octal instead

c64613a

ricardozanini added this to the v1.0.0 milestone Nov 5, 2020

sutaakar reviewed Nov 5, 2020

View reviewed changes

sutaakar approved these changes Nov 5, 2020

View reviewed changes

spolti approved these changes Nov 5, 2020

View reviewed changes

spolti added ready 🚀 PR is ready to be merged and removed needs review 🔍 Pull Request that needs reviewers labels Nov 5, 2020

spolti merged commit fc7e971 into apache:master Nov 5, 2020

vaibhavjainwiz deleted the KOGITO-3736 branch November 7, 2020 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KOGITO-3736] - Reconciler error in Data index controller when Kogito… #643

[KOGITO-3736] - Reconciler error in Data index controller when Kogito… #643

vaibhavjainwiz commented Oct 30, 2020 •

edited

codecov bot commented Oct 30, 2020 •

edited

ricardozanini left a comment •

edited

ricardozanini commented Oct 30, 2020

kie-ci-bot bot commented Oct 30, 2020

vaibhavjainwiz commented Oct 30, 2020

msmagnanijr commented Oct 31, 2020

vaibhavjainwiz commented Oct 31, 2020

Kaitou786 commented Nov 2, 2020

Kaitou786 commented Nov 3, 2020

ricardozanini commented Nov 4, 2020

kie-ci-bot bot commented Nov 5, 2020

kie-ci-bot bot commented Nov 5, 2020

kie-ci-bot bot commented Nov 5, 2020

sutaakar commented Nov 5, 2020

kie-ci-bot bot commented Nov 5, 2020

kie-ci-bot bot commented Nov 5, 2020

kie-ci-bot bot commented Nov 5, 2020

ricardozanini commented Nov 5, 2020

sutaakar Nov 5, 2020

sutaakar commented Nov 5, 2020

ricardozanini commented Nov 5, 2020

sutaakar commented Nov 5, 2020

sutaakar Nov 5, 2020 •

edited

sutaakar left a comment •

edited

[KOGITO-3736] - Reconciler error in Data index controller when Kogito… #643

[KOGITO-3736] - Reconciler error in Data index controller when Kogito… #643

Conversation

vaibhavjainwiz commented Oct 30, 2020 • edited

codecov bot commented Oct 30, 2020 • edited

Codecov Report

ricardozanini left a comment • edited

Choose a reason for hiding this comment

ricardozanini commented Oct 30, 2020

kie-ci-bot bot commented Oct 30, 2020

vaibhavjainwiz commented Oct 30, 2020

msmagnanijr commented Oct 31, 2020

vaibhavjainwiz commented Oct 31, 2020

Kaitou786 commented Nov 2, 2020

Kaitou786 commented Nov 3, 2020

ricardozanini commented Nov 4, 2020

kie-ci-bot bot commented Nov 5, 2020

kie-ci-bot bot commented Nov 5, 2020

kie-ci-bot bot commented Nov 5, 2020

sutaakar commented Nov 5, 2020

kie-ci-bot bot commented Nov 5, 2020

kie-ci-bot bot commented Nov 5, 2020

kie-ci-bot bot commented Nov 5, 2020

ricardozanini commented Nov 5, 2020

sutaakar Nov 5, 2020

Choose a reason for hiding this comment

sutaakar commented Nov 5, 2020

ricardozanini commented Nov 5, 2020

sutaakar commented Nov 5, 2020

sutaakar Nov 5, 2020 • edited

Choose a reason for hiding this comment

sutaakar left a comment • edited

Choose a reason for hiding this comment

vaibhavjainwiz commented Oct 30, 2020 •

edited

codecov bot commented Oct 30, 2020 •

edited

ricardozanini left a comment •

edited

sutaakar Nov 5, 2020 •

edited

sutaakar left a comment •

edited