Add ConsoleLink Creation/Remove #462

myeung18 · 2020-09-04T16:02:10Z

Signed-off-by: Marco Yeung myeung@redhat.com

Added ConsoleLink Creation/Remove and upgraded client-go/RHsyseng libraries for OCP4.3 or above

myeung18 · 2020-09-04T19:18:09Z

This new PR is to replace #448.

deploy/cluster_role_binding.yaml

deploy/cluster_role.yaml

go.mod

pkg/apis/addtoscheme_apps_v1alpha1.go

eguzki · 2020-09-08T15:43:45Z

pkg/controller/apimanager/apimanager_controller.go

@@ -101,6 +105,11 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 		return err
 	}

+	err = c.Watch(&source.Kind{Type: &routev1.Route{}}, &handler.EnqueueRequestForObject{})


As watched routes are not managed by the apimanager controller, I think it makes sense to create a separate controller for this logic. Note that this apimanager controller always tries to read APIManager type out of reconcile request object (

3scale-operator/pkg/controller/apimanager/apimanager_controller.go

Line 143 in c941a20

instance, err := r.apiManagerInstance(request.NamespacedName)

)

added a new controller

pkg/controller/apimanager/apimanager_controller.go

eguzki · 2020-09-08T15:48:18Z

pkg/controller/apimanager/apimanager_controller.go

 			return nil, nil
 		}
 		return nil, err
 	}
+	if err := helper.ConsoleLinkSupported(); err == nil {


I do not think apimanager controller is the one appropriate for this logic. But if it was, this would not be the right place. It would be inside the reconcileAPIManagerLogic method

moved to the new controller

pkg/helper/webconsole.go

eguzki · 2020-09-08T15:55:20Z

pkg/helper/webconsole.go

+func CreateConsoleLink(ctx context.Context, c client.Client, apimanager *appsv1alpha1.APIManager) {
+	route := getRoute(ctx, c, apimanager)
+	if route != nil {
+		consoleLinkName := fmt.Sprintf("%s-%s", apimanager.ObjectMeta.Name, apimanager.Namespace)


Try to create ReconcileConsoleLink method of the BaseAPIManagerLogicReconciler type in base_apimanager_logic_reconciler.go. You should have everything you need and the implementation should be easy.

moved to the new controller

eguzki · 2020-09-16T15:29:51Z

Waiting for your input. Let us know.

myeung18 · 2020-09-16T21:58:51Z

Waiting for your input. Let us know

sorry of the delay, now I'm back.

myeung18 · 2020-09-18T13:28:17Z

Waiting for your input. Let us know.

ready for further review

myeung18 · 2020-09-28T16:00:50Z

@eguzki I just did a rebase, built and tested in my lab, 3scale worked ok.
I do see those ci test failures, but they seem to appear on other PRs as well.
Is there anything else I should on this PR?

miguelsorianod · 2020-09-29T10:16:53Z

Hi @myeung18,

Yesterday we applied some changes to fix some issues that we had in our CI tests (#480). Could you rebase again from master so the tests are run with the new changes?

Thank you.

miguelsorianod · 2020-09-29T10:21:52Z

pkg/helper/webconsole.go

+}
+func getRoute(ctx context.Context, c client.Client, req *reconcile.Request) *routev1.Route {
+	// here the loop tries the best to collect a route, the reconcile event may comes
+	// before the route resources get created/updated


I'm not sure why this loop is needed.

Why would the reconcile event come before the route gets created/updated?
In the controller code there's a watch for Route resources. An event will be triggered when a route is created/updated/deleted

I can't reproduce the case I saw before.
I removed the loop, and it also works.

miguelsorianod · 2020-09-29T10:22:49Z

pkg/helper/webconsole.go

+		time.Sleep(time.Duration(100) * time.Microsecond)
+		deployedRoutes, err := reader.List(&routev1.RouteList{})
+		if err == nil {
+			for _, route := range deployedRoutes { //look for 3scale admin routes, capture route object


Outdated comment

miguelsorianod · 2020-09-29T10:27:09Z

pkg/helper/webconsole.go

+		ObjectMeta: metav1.ObjectMeta{
+			Name: consoleLinkName,
+			Labels: map[string]string{
+				"3scale.net/name": req.Name,


Would 3scale.net/route-name make more sense than 3scale.net/name?

myeung18 · 2020-09-29T16:58:19Z

@eguzki I just did a rebase, built and tested in my lab, 3scale worked ok.
I do see those ci test failures, but they seem to appear on other PRs as well.
Is there anything else I should on this PR?

eguzki · 2020-09-29T17:20:24Z

I just hit re-run. If it keeps failing, it needs to be debugged. The issue is that 3scale did not deploy as expected. Maybe the expectation is what needs to be worked out. We will check.

myeung18 · 2020-09-29T17:52:44Z

Hi @myeung18,

Yesterday we applied some changes to fix some issues that we had in our CI tests (#480). Could you rebase again from master so the tests are run with the new changes?

Thank you.

rebased, all changes are ready for review

miguelsorianod · 2020-09-30T12:43:17Z

pkg/helper/webconsole.go

+		err := c.Get(ctx, types.NamespacedName{Name: consoleLinkName}, consoleLink)
+		if err != nil && apierrors.IsNotFound(err) {
+			consoleLink = getConsoleLink(consoleLinkName, route, req)
+			if err := c.Create(ctx, consoleLink); err != nil {


If creation fails we should also retry apart from logging

added requeue

miguelsorianod · 2020-09-30T12:43:30Z

pkg/helper/webconsole.go

+				logger.Info("Console link has been created:", consoleLinkName)
+			}
+		} else if err == nil && consoleLink != nil {
+			updateConsoleLink(ctx, route, consoleLink, c)


If update fails we should also retry apart from logging

added requeue

miguelsorianod · 2020-09-30T12:46:40Z

pkg/helper/webconsole.go

+func ReconcileConsoleLink(ctx context.Context, c client.Client, req *reconcile.Request) error {
+	route := getRoute(ctx, c, req)
+	if route == nil {
+		removeConsoleLink(ctx, c, req)


Here if it fails to remove we should also requeue. There's an exception in this case: If it fails to remove because it does not exist anymore then we don't need to requeue

added requeue

eguzki · 2020-09-30T13:40:12Z

Good, all checks passed.

miguelsorianod · 2020-10-01T11:54:59Z

pkg/controller/webconsole/webconsole_controller.go

+	logger.Info("Reconciling ReconcileWebConsole")
+
+	if err := helper.ConsoleLinkSupported(); err == nil {
+		if err = helper.ReconcileConsoleLink(r.Context(), r.Client(), &request); err != nil {


It would be nice to log the error here

miguelsorianod · 2020-10-01T11:57:04Z

pkg/helper/webconsole.go

+// ReconcileConsoleLink creates/deletes/updates a ConsoleLink based on the Route
+func ReconcileConsoleLink(ctx context.Context, c client.Client, req *reconcile.Request) error {
+	route := getRoute(ctx, c, req)
+	if route == nil {


Probably a good idea to check the error and return the error to retry if it fails (the exception is if the error is not found of course)

miguelsorianod · 2020-10-01T11:59:05Z

pkg/helper/webconsole.go

+	consoleLink := &consolev1.ConsoleLink{}
+	consoleLinkName := fmt.Sprintf("%s-%s", ConsoleLinkNamePrefix, req.Namespace)
+	err := c.Get(ctx, types.NamespacedName{Name: consoleLinkName}, consoleLink)
+	if err == nil && consoleLink != nil {


This checking seems odd.

If err is nil ConsoleLink shouldn't be nil by definition right? 🤔

Additionally, if the error is not nil and it is not the isnotfound error then the error should be returned so it can be retried

eguzki · 2020-10-01T16:49:43Z

Created PR myeung18#1 for some refactoring

codeclimate · 2020-10-01T18:50:29Z

Code Climate has analyzed commit d0baac2 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1
Duplication	1

View more on Code Climate.

myeung18 · 2020-10-01T18:55:02Z

refactors are applied and tested.

miguelsorianod · 2020-10-02T09:22:47Z

Looks good to me 👍

eguzki · 2020-10-02T13:07:52Z

Congratulations @myeung18 for the very good job. 🎖️

Thank you very much for the contribution

eguzki assigned eguzki and miguelsorianod Sep 4, 2020

eguzki requested review from miguelsorianod and eguzki September 4, 2020 16:20

myeung18 force-pushed the consolelink branch 2 times, most recently from fc531c7 to ad21da0 Compare September 4, 2020 19:14

eguzki requested changes Sep 8, 2020

View reviewed changes

myeung18 force-pushed the consolelink branch 2 times, most recently from 3b2f46c to 243c132 Compare September 9, 2020 22:17

myeung18 force-pushed the consolelink branch 3 times, most recently from 7f63a39 to da55461 Compare September 16, 2020 21:48

myeung18 force-pushed the consolelink branch 7 times, most recently from 97319fd to d7c334c Compare September 18, 2020 13:26

myeung18 force-pushed the consolelink branch 2 times, most recently from 4c077f3 to 6b4965f Compare September 28, 2020 13:46

miguelsorianod reviewed Sep 29, 2020

View reviewed changes

myeung18 closed this Sep 29, 2020

myeung18 reopened this Sep 29, 2020

myeung18 force-pushed the consolelink branch 4 times, most recently from f3ab53a to f137358 Compare September 29, 2020 17:48

miguelsorianod reviewed Sep 30, 2020

View reviewed changes

myeung18 force-pushed the consolelink branch from f137358 to a77c4eb Compare September 30, 2020 17:03

miguelsorianod reviewed Oct 1, 2020

View reviewed changes

myeung18 force-pushed the consolelink branch from 86d393c to c8f4fde Compare October 1, 2020 18:43

rashelrr and others added 2 commits October 1, 2020 14:45

Add ConsoleLink Create/Remove

d961c21

refactor webconsole controller

d0baac2

myeung18 force-pushed the consolelink branch from c8f4fde to d0baac2 Compare October 1, 2020 18:49

miguelsorianod approved these changes Oct 2, 2020

View reviewed changes

eguzki approved these changes Oct 2, 2020

View reviewed changes

eguzki merged commit e35bb84 into 3scale:master Oct 2, 2020

miguelsorianod mentioned this pull request Oct 2, 2020

Update development documentation to include clusterrole creation/cleanup #485

Merged

Add ConsoleLink Creation/Remove #462

Add ConsoleLink Creation/Remove #462

Conversation

myeung18 commented Sep 4, 2020 • edited Loading

myeung18 commented Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myeung18 Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myeung18 Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

eguzki commented Sep 16, 2020

myeung18 commented Sep 16, 2020

myeung18 commented Sep 18, 2020

myeung18 commented Sep 28, 2020

miguelsorianod commented Sep 29, 2020

Choose a reason for hiding this comment

myeung18 Sep 29, 2020 • edited Loading

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

myeung18 commented Sep 29, 2020

eguzki commented Sep 29, 2020

myeung18 commented Sep 29, 2020

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

eguzki commented Sep 30, 2020

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

eguzki commented Oct 1, 2020

codeclimate bot commented Oct 1, 2020

myeung18 commented Oct 1, 2020

miguelsorianod commented Oct 2, 2020

eguzki commented Oct 2, 2020

myeung18 commented Sep 4, 2020 •

edited

Loading

myeung18 commented Sep 4, 2020 •

edited

Loading

myeung18 Sep 16, 2020 •

edited

Loading

myeung18 Sep 16, 2020 •

edited

Loading

myeung18 Sep 29, 2020 •

edited

Loading