CHE-2369: add a mechanism which checks state of the ws agent #2530
Conversation
| dialogFactory.createMessageDialog(infoWindowTitle, | ||
| "Your workspace is not responding. To fix the problem, verify you have a good " + | ||
| "network connection and restart the workspace.", | ||
| new ConfirmCallback() { |
There was a problem hiding this comment.
you can use use null instead empty ConfirmCallback
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/459/ |
| Log.warn(getClass(), "Parse root resources failed."); | ||
| } | ||
|
|
||
| if (object != null && object.containsKey("rootResources")) { |
There was a problem hiding this comment.
if you move this if-block inside the upper try-block
checking object != null will be unnecessary
| dialogFactory.createMessageDialog(infoWindowTitle, | ||
| "Your workspace has stopped responding. To fix the problem, " + | ||
| "restart the workspace in the dashboard.", | ||
| new ConfirmCallback() { |
There was a problem hiding this comment.
ConfirmCallback is nullable so no need to create this ConfirmCallback that does nothing
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/465/ |
|
@garagatyi pls review |
| AgentHealthStateDto withAgentId(String agentId); | ||
|
|
||
| /** Returns id of the agent */ | ||
| String getAgentId(); |
There was a problem hiding this comment.
some id of agent or reference, what problem here ?
There was a problem hiding this comment.
My concern is in addition of new terminology -agent ID
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * Interface which describes mechanism for checking agent's state. |
There was a problem hiding this comment.
It is not mandatory to explain purpose of interface with interface term
| * Verify if the agent is alive. | ||
| * | ||
| * @param devMachine | ||
| * development machine instance |
There was a problem hiding this comment.
yep good catch, it can be any machine
There was a problem hiding this comment.
The purpose and life cycle of it has to be properly documented.
In particular:
- why it needed
- what and when calls check method
| * if internal server error occurred | ||
| * @throws ForbiddenException | ||
| * if the user is not workspace owner | ||
| * @throws BadRequestException |
There was a problem hiding this comment.
Please add explanation for this exception
| * @throws UnauthorizedException | ||
| * if the user is not authorized | ||
| * @throws IOException | ||
| * @throws ConflictException |
There was a problem hiding this comment.
Please add explanation for these exceptions
| */ | ||
| @Api(value = "/workspace-agent-health", description = "Workspace Agent Health Checker") | ||
| @Path("/workspace-agent-health") | ||
| public class AgentHealthCheckerService extends Service { |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/504/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/510/ |
|
Build # 514 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/514/ to view the results. |
|
Build # 515 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/515/ to view the results. |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/517/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/522/ |
|
Build # 528 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/528/ to view the results. |
| import static org.eclipse.che.ide.api.machine.WsAgentState.STARTED; | ||
| import static org.eclipse.che.ide.api.machine.WsAgentState.STOPPED; | ||
| import static org.eclipse.che.ide.rest.HTTPHeader.ACCEPT; | ||
|
|
||
| /** | ||
| * @author Roman Nikitenko | ||
| * @author Valeriy Svydenko | ||
| */ | ||
| @Singleton |
There was a problem hiding this comment.
No single word about purpose of this class, the same for public methods.
Please add java docs.
| } | ||
| } | ||
|
|
||
| private void checkWsAgentState() { | ||
| final String url = restContext + "/workspace/" + devMachine.getWorkspace() + "/check"; |
There was a problem hiding this comment.
Can not we put and take this URL from DevMachine?
Such a string concatenation looks bad.
BTW, DevMachine is initialized not in constructor, what if DevMachine == null by any reason?
|
|
||
| WsAgentHealthStateDto withCode(int code); | ||
|
|
||
| void setReason(String reason); |
There was a problem hiding this comment.
Could not we remove set methods in DTO (in general)?
| } | ||
|
|
||
| final WsAgentHealthStateDto check = agentHealthChecker.check(devMachine); | ||
|
|
| final WsAgentHealthStateDto check = agentHealthChecker.check(devMachine); | ||
|
|
||
| check.setWorkspaceStatus(workspace.getStatus()); | ||
|
|
|
|
||
| void setCode(int code); | ||
|
|
||
| /** Returns status code. */ |
There was a problem hiding this comment.
Where to see possible values for the code?
(java docs)
There was a problem hiding this comment.
it just HTTP status code
There was a problem hiding this comment.
Why not to say it is java-docs and name the parameter accordingly?
| * well configured proxy server or other problems which are not related to our responsibility. | ||
| * | ||
| * @author Vitalii Parfonov | ||
| */ |
There was a problem hiding this comment.
Will we really have several implementations of it?
There was a problem hiding this comment.
Yes for codenvy it require authorization token
There was a problem hiding this comment.
I doubt it worth another implementation. I'd say we need a policy for it - like if there are no token it is taken as "allowed". I suppose Codenvy always includes token and Che always does not. Is not it?
There was a problem hiding this comment.
Yes we include token in other implementation that provided in codenvy repository. Or i don't understand your notes
| Server wsAgent = null; | ||
| for (Server server : servers.values()) { | ||
| if (WSAGENT_REFERENCE.equals(server.getRef())) { | ||
| wsAgent = server; |
There was a problem hiding this comment.
can we stop iterations here ?
| @Produces(APPLICATION_JSON) | ||
| @ApiOperation(value = "Get state of the workspace agent by the workspace id and agent id") | ||
| @ApiResponses({@ApiResponse(code = 200, message = "The response contains requested workspace entity"), | ||
| @ApiResponse(code = 404, message = "The workspace with specified id does not exist"), |
There was a problem hiding this comment.
PLs add status codes for BadRequest (400), Conflict(409), Unauthrorized(401) (btw who is throwing unauthorized?)
There was a problem hiding this comment.
ok for 400 and I think don't need to add for other two (409 and 401),we don't have situations where it can be threw.
| UnauthorizedException, | ||
| IOException, | ||
| ConflictException { | ||
| validateKey(key); |
There was a problem hiding this comment.
This check is not needed if we accept oly ID's (and no composite keys)
|
Build # 536 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/536/ to view the results. |
| @@ -89,6 +91,8 @@ protected void configure() { | |||
| new org.eclipse.che.api.machine.server.model.impl.ServerConfImpl(Constants.WSAGENT_DEBUG_REFERENCE, "4403/tcp", "http", | |||
| null)); | |||
|
|
|||
| bind(WsAgentHealthChecker.class).to(WsAgentHealthCheckerImpl.class); | |||
| * @throws ServerException | ||
| * if internal server error occurred | ||
| * @throws ForbiddenException | ||
| * if the user is not workspace owner |
| * if internal server error occurred | ||
| * @throws ForbiddenException | ||
| * if the user is not workspace owner | ||
| * @throws BadRequestException |
There was a problem hiding this comment.
This exception should not be used outside of REST services.
There was a problem hiding this comment.
There was a problem hiding this comment.
It is not reason to expose that exception
| * if the user is not workspace owner | ||
| * @throws BadRequestException | ||
| * if has invalid parameters | ||
| * @throws UnauthorizedException |
There was a problem hiding this comment.
How it it possible? Is this functionality behind the login filter?
There was a problem hiding this comment.
There was a problem hiding this comment.
It is not reason to expose that exception
| * if has invalid parameters | ||
| * @throws UnauthorizedException | ||
| * if the user is not authorized | ||
| * @throws ConflictException |
There was a problem hiding this comment.
Can you explain in which situation this exception might be thrown?
There was a problem hiding this comment.
There was a problem hiding this comment.
It is not reason to expose that exception
| ForbiddenException, | ||
| BadRequestException, | ||
| UnauthorizedException, | ||
| IOException, |
There was a problem hiding this comment.
IOException should not be exposed here
| ServerException, | ||
| ForbiddenException, | ||
| BadRequestException, | ||
| UnauthorizedException, |
There was a problem hiding this comment.
There was a problem hiding this comment.
It should not be thrown by REST this service, so should not be exposed in this method
| @PathParam("id") String key) throws NotFoundException, | ||
| ServerException, | ||
| ForbiddenException, | ||
| BadRequestException, |
There was a problem hiding this comment.
Don't see the case when this exception can be thrown
There was a problem hiding this comment.
There was a problem hiding this comment.
It should not be thrown by REST this service, so should not be exposed in this method
| BadRequestException, | ||
| UnauthorizedException, | ||
| IOException, | ||
| ConflictException { |
There was a problem hiding this comment.
In which case this exception can be thrown?
There was a problem hiding this comment.
There was a problem hiding this comment.
It should not be thrown by REST this service, so should not be exposed in this method
| return newDto(WsAgentHealthStateDto.class) | ||
| .withWorkspaceStatus(workspace.getStatus()) | ||
| .withCode(NOT_FOUND.getStatusCode()) | ||
| .withReason("Workspace Agent isn't available if Dev machine isn't RUNNING"); |
There was a problem hiding this comment.
Please check if term dev-machine is blessed for error messages
There was a problem hiding this comment.
I think the most modern term ws-machine, but I'm not sure. @eivantsov can you help with that?
There was a problem hiding this comment.
dev-machine is what's it's called in consoles panel right now
| final HttpJsonRequest pingRequest = createPingRequest(machine); | ||
| final HttpJsonResponse response = pingRequest.request(); | ||
| agentHealthStateDto.withCode(response.getResponseCode()) | ||
| .withReason(response.asString()); |
There was a problem hiding this comment.
Do we want to have a reason as strigified json?
|
Build # 546 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/546/ to view the results. |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/549/ |
| @@ -15,6 +15,7 @@ | |||
| import com.google.inject.name.Names; | |||
| import com.google.inject.persist.jpa.JpaPersistModule; | |||
|
|
|||
| import org.eclipse.che.api.agent.server.WsAgentHealthChecker; | |||
|
|
||
| @Override | ||
| public WsAgentHealthStateDto check(Machine machine) throws NotFoundException, ServerException { | ||
| final Map<String, ? extends Server> servers = machine.getRuntime().getServers(); |
There was a problem hiding this comment.
Consider moving of this line to getWsAgent method since it is not used anywhere else.
|
@garagatyi I've applied your remarks, take a look please again! |
| String wsAgentPingUrl = wsAgent.getUrl(); | ||
| // since everrest mapped on the slash in case of it absence | ||
| // we will always obtain not found response | ||
| if (!wsAgentPingUrl.endsWith("/")) { |
There was a problem hiding this comment.
It is theoretically possible that URL will be null, so please add check that prevents NPE
| @Produces(APPLICATION_JSON) | ||
| @ApiOperation(value = "Get state of the workspace agent by the workspace id and agent id") | ||
| @ApiResponses({@ApiResponse(code = 200, message = "The response contains requested workspace entity"), | ||
| @ApiResponse(code = 400, message = "Missed required parameters, parameters are not valid"), |
There was a problem hiding this comment.
This method doesn't throw 400 anymore
| @ApiOperation(value = "Get state of the workspace agent by the workspace id and agent id") | ||
| @ApiResponses({@ApiResponse(code = 200, message = "The response contains requested workspace entity"), | ||
| @ApiResponse(code = 400, message = "Missed required parameters, parameters are not valid"), | ||
| @ApiResponse(code = 404, message = "The workspace with specified id does not exist"), |
There was a problem hiding this comment.
Another case is connected to ws agent check - please mention that also
There was a problem hiding this comment.
in this case it's internal server error
| @GET | ||
| @Path("/{id}/check") | ||
| @Produces(APPLICATION_JSON) | ||
| @ApiOperation(value = "Get state of the workspace agent by the workspace id and agent id") |
| } | ||
| try { | ||
| final HttpJsonRequest pingRequest = createPingRequest(machine, wsAgent); | ||
| final HttpJsonResponse response = pingRequest.request(); |
There was a problem hiding this comment.
I don't understand why this method throws NotFoundException. If request to ws-agent returns 404 it looks like internal server error. WDYT?
There was a problem hiding this comment.
Agree with internal server error
| * @throws ServerException | ||
| * if internal server error occurred | ||
| */ | ||
| WsAgentHealthStateDto check(Machine machine) throws NotFoundException, ServerException; |
There was a problem hiding this comment.
please remove not found exception from method declaration
| }).catchError(new Operation<PromiseError>() { | ||
| @Override | ||
| public void apply(PromiseError arg) throws OperationException { | ||
| Log.error(getClass(), arg.getMessage()); |
There was a problem hiding this comment.
Will it handle internal server error in appropriate way?
| } | ||
| try { | ||
| final HttpJsonRequest pingRequest = createPingRequest(machine, wsAgent); | ||
| final HttpJsonResponse response = pingRequest.request(); |
There was a problem hiding this comment.
This pinging miss super new feature of internal/external urls of agents contributed by Mario. So if it is possible please create single component for pinging and use it here and in WsAgentlauncherImpl.
|
Build # 560 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/560/ to view the results. |
|
Build # 567 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/567/ to view the results. |
|
Build # 621 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/621/ to view the results. |
Signed-off-by: Vitalii Parfonov <[email protected]>
|
Build # 625 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/625/ to view the results. |
What does this PR do?
Adds a service which periodically pings ws agent and notify a user when something is happened.
What issues does this PR fix or reference?
#2369
Previous behavior
When ws agent stops responding a user don't get any response about this and don't know what is happened.
PR type
Minor change checklist
@vparfonov please take a look