CHE-274 - Improve idling implementation #5512

mkuznyetsov · 2017-07-03T06:31:36Z

What does this PR do?

Enable workspace shutdown after a specified inactivity period.

What issues does this PR fix or reference?

This is a copy of PR #5386 with minor fixes

https://issues.jboss.org/browse/CHE-274

Changelog

Added workspace idling tracking, that will shutdown workspaces, which are inactive over a period of time, configured by "che.machine.ws.agent.inactive.stop.timeout.ms" property. Tracking is scheduled to perform in intervals, defined by "stop.workspace.scheduler.period" property.

Release Notes

Per default, a workspace will be stopped after one hour of inactivity.

Docs PR

eclipse-che/che-docs#253

See #5386

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>

codenvy-ci · 2017-07-03T07:44:39Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2973/

benoitf

I suppose there is a pending PR on codenvy repository as well ?

benoitf · 2017-07-03T07:54:57Z

linked to codenvy/codenvy#2297

benoitf · 2017-07-03T07:55:50Z

@slemeur I think there is some doc required

slemeur · 2017-07-03T08:24:23Z

added eclipse-che/che-docs#253

garagatyi · 2017-07-03T11:36:17Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties

+# Idle Timeout
+#     The system will suspend the workspace and snapshot it if the end user is idle for
+#     this length of time. Idleness is determined by the length of time that a user has
+#     not interacted with the workspace, meaning that one of our agents has not received


One of the agents or any?

removed this part

garagatyi · 2017-07-03T11:36:20Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties

@@ -115,6 +115,14 @@ che.workspace.agent.dev.ping_timeout_error_msg=Timeout. The Che server is unable
 che.agent.dev.max_start_time_ms=120000
 che.agent.dev.ping_delay_ms=2000

+# Idle Timeout
+#     The system will suspend the workspace and snapshot it if the end user is idle for
+#     this length of time. Idleness is determined by the length of time that a user has


I'd rather use amount of time instead of length of time

garagatyi · 2017-07-03T11:37:42Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties

+#     not interacted with the workspace, meaning that one of our agents has not received
+#     instructions. Leaving a browser window open counts as idleness time.
+che.machine.ws.agent.inactive.stop.timeout.ms=3600000
+stop.workspace.scheduler.period=60


Please prepend property name with che. and add measurement unit as a suffix of the property.

changed property name

garagatyi · 2017-07-03T11:39:09Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties

+#     this length of time. Idleness is determined by the length of time that a user has
+#     not interacted with the workspace, meaning that one of our agents has not received
+#     instructions. Leaving a browser window open counts as idleness time.
+che.machine.ws.agent.inactive.stop.timeout.ms=3600000


Part of a term that means single area are usually divided by underscores, e.g. che.machine.wsagent.inactive_stop_timeout_ms

changed property name

garagatyi · 2017-07-03T11:41:50Z

...-workspace/src/main/java/org/eclipse/che/api/agent/server/activity/LastAccessTimeFilter.java

+import java.io.IOException;
+
+/**
+ * Counts every request to the agent as a workspace activity


Does it count frames in case of WebSocket connection or only connection itself?

It would only react on HTTP requests going through the filter, updated Javadoc

garagatyi · 2017-07-03T11:43:38Z

...-workspace/src/main/java/org/eclipse/che/api/agent/server/activity/LastAccessTimeFilter.java

+ * @author Mihail Kuznyetsov
+ */
+@Singleton
+public class LastAccessTimeFilter implements Filter {


If this class is related to wsagent let's move it to wsagent related module

Moved to a separate module

garagatyi · 2017-07-03T11:45:29Z

...-workspace/src/main/java/org/eclipse/che/api/agent/server/activity/LastAccessTimeFilter.java

+ * @author Mihail Kuznyetsov
+ */
+@Singleton
+public class LastAccessTimeFilter implements Filter {


Please add tests

Added tests

garagatyi

Mostly OK, but, please, add tests and take a look on my comments

garagatyi · 2017-07-03T11:50:21Z

...space/src/main/java/org/eclipse/che/api/agent/server/activity/WorkspaceActivityNotifier.java

+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * Notifies master about activity in workspace, but not more often than once per minute.


Please, correct javadocs as actual notification threshold is configurable now.

Javadoc updated

garagatyi · 2017-07-03T11:51:25Z

...space/src/main/java/org/eclipse/che/api/agent/server/activity/WorkspaceActivityNotifier.java

+ * @author Anton Korneta
+ */
+@Singleton
+public class WorkspaceActivityNotifier {


If this class is related to wsagent let's move it to wsagent related module

Moved to separate module

garagatyi · 2017-07-03T12:06:16Z

...ce/src/main/java/org/eclipse/che/api/workspace/server/activity/WorkspaceActivityManager.java

+import com.google.inject.Inject;
+
+/**
+ * Stops the inactive workspaces by given expiration time.


As far as I can see this class not only stops workspaces but provides API for updating workspace activity timestamp. Can you rephrase javadocs in accordance with that?

Javadoc updated

garagatyi · 2017-07-03T12:18:03Z

...ce/src/main/java/org/eclipse/che/api/workspace/server/activity/WorkspaceActivityManager.java

+                    workspaceManager.updateWorkspace(workspaceId, workspace);
+                    workspaceManager.stopWorkspace(workspaceId);
+                } catch (NotFoundException e) {
+                    LOG.info("Workspace already stopped");


Please ignore this exception - there is no need to log it

codenvy-ci · 2017-07-10T15:56:32Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3028/

garagatyi · 2017-07-11T06:46:24Z

...ce/src/main/java/org/eclipse/che/api/workspace/server/activity/WorkspaceActivityManager.java

+import com.google.inject.Inject;
+
+/**
+ * Stops the inactive workspaces by given expiration time. Upon stopping, workspace attributes will be updated with


It also provides API for WS activity timestamp updates

garagatyi · 2017-07-11T06:47:10Z

...ce/src/main/java/org/eclipse/che/api/workspace/server/activity/WorkspaceActivityService.java

+import static org.eclipse.che.api.core.model.workspace.WorkspaceStatus.RUNNING;
+
+/**
+ * Monitors the activity of the runtime workspace.


It doesn't monitor activity itself, but rather provides API for WS activity timestamp updates

garagatyi · 2017-07-11T06:51:21Z

wsagent/activity/src/main/test/org/eclipse/che/api/activity/LastAccessTimeFilterTest.java

+    }
+
+    @Test
+    public void shouldLogExceptionUponActivityNotification() throws IOException, ServletException {


This test doesn't check logging - it ensures that an exception that can be thrown by activity call doesn't prevent the call of the chain. So, please, rename test and verify the call of the chain.

garagatyi · 2017-07-11T06:51:36Z

wsagent/activity/src/main/test/org/eclipse/che/api/activity/LastAccessTimeFilterTest.java

+        // when
+        filter.doFilter(request, response, chain);
+        // then
+        verify(workspaceActivityNotifier).onActivity();


Please, also verify the call of the chain.

garagatyi · 2017-07-11T06:52:12Z

...ce/src/main/java/org/eclipse/che/api/workspace/server/activity/WorkspaceActivityService.java

+    @ApiResponses(@ApiResponse(code = 204, message = "Activity counted"))
+    public void active(@ApiParam(value = "Workspace id")
+                       @PathParam("wsId") String wsId) throws ForbiddenException, NotFoundException, ServerException {
+        final WorkspaceImpl workspace = workspaceManager.getWorkspace(wsId);


Please, add tests

Added WorkspaceActivityServiceTest

garagatyi · 2017-07-11T06:53:48Z

wsagent/activity/src/main/java/org/eclipse/che/api/activity/WorkspaceActivityNotifier.java

+
+
+    @Inject
+    public WorkspaceActivityNotifier(HttpJsonRequestFactory httpJsonRequestFactory,


Please, add tests

Added WorkspaceActivityNotifierTest

garagatyi · 2017-07-11T06:57:41Z

.../src/main/java/org/eclipse/che/api/activity/websocket/WorkspaceWebsocketMessageReceiver.java

+ * @author Mihail Kuznyetsov
+ * @author Anton Korneta
+ */
+public class WorkspaceWebsocketMessageReceiver implements WSMessageReceiver {


Please, add tests

garagatyi · 2017-07-11T07:07:12Z

...ce/src/main/java/org/eclipse/che/api/workspace/server/activity/WorkspaceActivityManager.java

+ * @author Anton Korneta
+ */
+@Singleton
+public class WorkspaceActivityManager {


Please, add tests

Added WorkspaceActivityManagerTest

codenvy-ci · 2017-07-11T14:59:27Z

Build # 3037 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3037/ to view the results.

…ation

…plementation

…ling implementation

codenvy-ci · 2017-07-12T08:29:27Z

Build # 3044 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3044/ to view the results.

…rove idling implementation

codenvy-ci · 2017-07-12T09:37:30Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3045/

…4 - Improve idling implementation

codenvy-ci · 2017-07-12T14:35:09Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3052/

codenvy-ci · 2017-07-12T16:25:45Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3054/

* CHE-274 - Improve idling implementation Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com> * CHE-274 - Improve idling implementation * fixup! CHE-274 - Improve idling implementation * fixup! fixup! CHE-274 - Improve idling implementation * fixup! fixup! fixup! CHE-274 - Improve idling implementation * fixup! fixup! fixup! fixup! CHE-274 - Improve idling implementation * fixup! fixup! fixup! fixup! fixup! CHE-274 - Improve idling implementation * fixup! fixup! fixup! fixup! fixup! fixup! CHE-274 - Improve idling implementation * fixup! fixup! fixup! fixup! fixup! fixup! fixup! CHE-274 - Improve idling implementation * fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! CHE-274 - Improve idling implementation * fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! CHE-274 - Improve idling implementation * fixup

snjeza and others added 4 commits June 21, 2017 19:17

CHE-274 - Improve idling implementation

3533ef5

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>

Merge branch 'master' of github.com:eclipse/che into che-274a

8d4c554

Merge branch 'master' of github.com:eclipse/che into che-274a

7cabcf2

CHE-274 - Improve idling implementation

8958f42

mkuznyetsov requested review from skabashnyuk and garagatyi July 3, 2017 06:31

This was referenced Jul 3, 2017

CHE-274 - Improve idling implementation #5425

Closed

Reuse workspace activity managing components from Che codenvy/codenvy#2297

Merged

mkuznyetsov requested a review from benoitf July 3, 2017 07:08

mkuznyetsov changed the title ~~Che 274a~~ CHE-274 - Improve idling implementation Jul 3, 2017

benoitf approved these changes Jul 3, 2017

View reviewed changes

benoitf added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Jul 3, 2017

benoitf requested a review from slemeur July 3, 2017 07:55

mkuznyetsov requested a review from benoitf July 3, 2017 08:01

garagatyi reviewed Jul 3, 2017

View reviewed changes

garagatyi suggested changes Jul 3, 2017

View reviewed changes

ibuziuk mentioned this pull request Jul 5, 2017

Rebasing openshift-connector to upstream che master redhat-developer/rh-che#146

Closed

10 tasks

slemeur approved these changes Jul 5, 2017

View reviewed changes

mkuznyetsov added 2 commits July 10, 2017 17:41

fixup! CHE-274 - Improve idling implementation

2b26e78

fixup! fixup! CHE-274 - Improve idling implementation

1c83014

garagatyi suggested changes Jul 11, 2017

View reviewed changes

mkuznyetsov added 2 commits July 11, 2017 16:19

fixup! fixup! fixup! CHE-274 - Improve idling implementation

b2e1e22

fixup! fixup! fixup! fixup! CHE-274 - Improve idling implementation

aa1e796

garagatyi approved these changes Jul 11, 2017

View reviewed changes

garagatyi removed the request for review from skabashnyuk July 11, 2017 13:38

mkuznyetsov added 3 commits July 12, 2017 10:32

fixup! fixup! fixup! fixup! fixup! CHE-274 - Improve idling implement…

fb0a363

…ation

fixup! fixup! fixup! fixup! fixup! fixup! CHE-274 - Improve idling im…

fbebc79

…plementation

fixup! fixup! fixup! fixup! fixup! fixup! fixup! CHE-274 - Improve id…

fd347cd

…ling implementation

fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! CHE-274 - Imp…

36031c6

…rove idling implementation

benoitf approved these changes Jul 12, 2017

View reviewed changes

mkuznyetsov added 2 commits July 12, 2017 16:33

fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! CHE-27…

613f24c

…4 - Improve idling implementation

Merge branch 'master' of github.com:eclipse/che into che-274a

656fbc8

fixup

7e0a317

mkuznyetsov merged commit 0be04a6 into master Jul 12, 2017

mkuznyetsov deleted the che-274a branch July 12, 2017 15:35

l0rd mentioned this pull request Jul 13, 2017

Workspace idling is not working redhat-developer/rh-che#144

Closed

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 2, 2017



		@Inject
		public WorkspaceActivityNotifier(HttpJsonRequestFactory httpJsonRequestFactory,

CHE-274 - Improve idling implementation #5512

CHE-274 - Improve idling implementation #5512

Conversation

mkuznyetsov commented Jul 3, 2017 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

codenvy-ci commented Jul 3, 2017

benoitf left a comment

Choose a reason for hiding this comment

benoitf commented Jul 3, 2017

benoitf commented Jul 3, 2017

slemeur commented Jul 3, 2017

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

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

garagatyi left a 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

Choose a reason for hiding this comment

codenvy-ci commented Jul 10, 2017

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

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

codenvy-ci commented Jul 11, 2017

codenvy-ci commented Jul 12, 2017

codenvy-ci commented Jul 12, 2017

codenvy-ci commented Jul 12, 2017

codenvy-ci commented Jul 12, 2017

mkuznyetsov commented Jul 3, 2017 •

edited

Loading