Improve APIManager status #549
Conversation
| return false | ||
| } | ||
|
|
||
| // TODO should we sort the DCs? when we set the status section we are |
There was a problem hiding this comment.
See this TODO
There was a problem hiding this comment.
I've replaced the TODO by a comment indicating that at that point the DCs should be sorted already
|
missing error reporting. Are you implementing it in this PR? |
Do you mean |
|
|
||
| statusResult, err := r.reconcileAPIManagerStatus(instance) | ||
| if err != nil { | ||
| statusResult, statusErr := r.reconcileAPIManagerStatus(instance, err) |
There was a problem hiding this comment.
I'm unsure on the requeue/received errors management at this point between logic and status reconciliation. Let's iterate from this.
There was a problem hiding this comment.
For now if there's a requeue or error in the logic reconciler we'll reconcile at that point. Same when status scenario is reached.
6a38650 to
77d8e50
Compare
I'll open an issue to investigate how we could improve in that aspect. I understand the idea is to further improve the status by checking usual errors that might happen (imagestreams cannot pull the image, etc...) |
|
From https://issues.redhat.com/browse/THREESCALE-4753, this "state machine" would be a nice to have: |
e9963a5 to
df5c188
Compare
|
When testing the functionality the following scenario has arisen: We check the readiness of the OCP Routes. Due to Zync-related routes are not created by the APIManager they don't have an APIManager OwnerReference. This means that we are not able to detect changes on routes, and this implies that the reconciler is not able to detect them and the state calculation cannot be done appropriately. The following scenario happens:
The following solutions have been thought and are not feasible:
Possible solutions that might be feasible:
Maybe the best idea for now is to just take into account Deployments only and not consider routes. In this way we have something already and we can continue thinking about whether we want to include Routes and how. I'm not 100% sure though as I'm a little bit concerned that introducing this would be more problematic that if we don't due to users might report that "routes are not available but the 'Available' flag is set to true". At the end it will depend on how we define the Available flag and what expectations we set to the users / users have. |
|
I'm currently exploring the following possible solution that I commented in my previous reply: |
fccd270 to
5ab0662
Compare
|
New changes:
|
|
@eguzki I think maybe this could be good enough already? Regarding your previous comment #549 (comment), depending on what's the objective we want to achieve with that it might not be needed: The Condition provides the field |
|
Examples: APIManager status conditions when it is not available: APIManager status conditions when it is Available: |
|
About the generation of k8s events on: The state can be the It can be done in another PR. |
| if routeFound { | ||
| routeReady := helper.IsRouteReady(matchedRoute) | ||
| if !routeReady { | ||
| allDefaultRoutesReady = false |
There was a problem hiding this comment.
you can break the loop, right?
In the future, we can build a list or errors (errorList) to report which route is not ready. The allDefaultRoutesReady would be when the length of the errList is 0.
There was a problem hiding this comment.
You are correct.
The reason I didn't break the loop is exactly what you said. I left it prepared in case we want to add the list of routes.
|
Looking good. Added some feedback |
Add 'Available' field to APIManager status section.
We define the Available condition to true when ALL of the following scenarios are true:
· All expected DeploymentConfigs to be deployed exist and have the Available condition set to true
· All 3scale default OpenShift routes exist and have the Admitted condition set to true. The default routes are:
· Master route
· Backend Listener route
· Default tenant admin route, developer route, APIcast staging and production routes beloinging to the default tenant
Additionally status reconciliation has been refactored into its own reconciler
…APIManager events
5ab0662 to
a718df6
Compare
|
Code Climate has analyzed commit a718df6 and detected 15 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
Changes applied. Ready for a new review |
|
looking good |
This PR adds a new condition in APIManager's Status section named
Available.We define the
Availablecondition totruewhen ALL of the following scenarios are true:Availablecondition set to trueAdmittedcondition set to true. The default routes are:Design decisions / notable changes:
APIManagerStatusReconcilerreconciler has been created that is now used in the APIManager controllercommon.Conditionstype that we have available. This is a change in schema but if I'm not wrong this change in type has implied the addition of fields to theconditionsfield so this specific change should not be breaking compatibility.Pending: