CODENVY-957 Implement resources redistribution between suborganizations #1141

sleshchenko · 2016-11-14T11:03:04Z

What does this PR do?

Added ability to redistribute resources between suborganizations.

Introduced new state for account's resources, it's called reserved resources. It means that account's license has resources but it can't use it for consumtion by workpace starting, creation etc.

Implemented new organization resources API that allows to define resources that should be provided for usage by suborganization from its parent organization. Only users who have manageResources permissions can grant suborganization to use parent organization's resources. Resources become unavailable for his parent organization when user allows suborganization to use them, so they become reserved resources. By default all suborganizations don't have any resources limit and it means that they are not able to use resources of their parent organizations.

What issues does this PR fix or reference?

#957

New Behavior

Organization can provide own resources to usage by their suborganizations.

Tests written?

Yes

Docs requirements?

Include the content for all the docs changes required.

API changes
User docs changes

sleshchenko · 2016-11-15T09:28:31Z

@skabashnyuk @akorneta @evoevodin @mshaposhnik Please take a look my changes

sleshchenko · 2016-11-22T09:45:35Z

These changes should be merged with changes for #1198

voievodin · 2016-11-25T12:49:33Z

...n/src/main/java/com/codenvy/organization/api/resource/OrganizationResourcesLimitManager.java

+     * @throws ServerException
+     *         when any other error occurs
+     */
+    public void setResourcesLimit(OrganizationResourcesLimit resourcesLimit) throws NotFoundException,


The name of the method looks inconsistent with get, i mean it's should be either

setResourcesLimit getResourcesLimit

or(i like this one)

setLimit getLimit

or even

set get

but not

setResourcesLimit get

voievodin · 2016-11-25T13:08:35Z

...n/src/main/java/com/codenvy/organization/api/resource/OrganizationResourcesLimitManager.java

+            if (existed != null) {
+                try {
+                    final Resource deducted = resourceAggregator.deduct(existed, newResource);
+                    // existed resource amount is more than new one


is greater than

voievodin · 2016-11-30T09:25:49Z

...y/organization/api/permissions/OrganizationResourceDistributionServicePermissionsFilter.java

+            case RESET_DISTRIBUTED_RESOURCES:
+                //we should check permissions on parent organization level
+                String suborganizationId = (String)arguments[0];
+                final String parentId = organizationManager.getById(suborganizationId).getParent();


i don't see any purpose from having 2 more variables.
e.g the following code looks also ok to me:

organizationId = organizationManager.getById((String)arguments[0]).getParent(); if (organizationid == null) { return; } break;

voievodin · 2016-11-30T09:34:30Z

...y/organization/api/permissions/OrganizationResourceDistributionServicePermissionsFilter.java

+        }
+
+
+        if (!currentSubject.hasPermission(DOMAIN_ID, organizationId, OrganizationDomain.MANAGE_RESOURCES)) {


I would use class name along with DOMAIN_ID, from the code it's not clear what domain is identified with this id.
Moreover you use SystemDomain.DOMAIN_ID above, does it make any sense to you?

Also you use MANAGE_ORGANIZATION_ACTION as static import

import static com.codenvy.organization.api.permissions.OrganizationPermissionsFilter.MANAGE_ORGANIZATIONS_ACTION;

but you don't do it with OrganizationDomain.MANAGE_RESOURCES is there any regularity i don't see?

Nice catch. I removed static imports for domains' identifiers and action to make more clear their locations

voievodin · 2016-11-30T09:47:07Z

...src/main/java/com/codenvy/organization/api/resource/OrganizationResourceLockKeyProvider.java

+ */
+@Singleton
+public class OrganizationResourceLockKeyProvider implements ResourceLockKeyProvider {
+    private OrganizationManager organizationManager;


looks like final is missing

voievodin · 2016-11-30T09:48:59Z

...src/main/java/com/codenvy/organization/api/resource/OrganizationResourceLockKeyProvider.java

+            }
+            return organization.getId();
+        } catch (NotFoundException e) {
+            throw new ServerException(format("Organization with id '%s' was not found during resources locking.",


during resources locking

looks like you just trying to get a lock key, so i think the message should be a bit different

voievodin · 2016-11-30T09:51:47Z

...ain/java/com/codenvy/organization/api/resource/OrganizationResourcesDistributionService.java

+    }
+
+    @POST
+    @Path("{suborganizationId}")


reset method path starts with / while this path doesn't, i find it a bit inconsistent don't you?

voievodin · 2016-11-30T09:52:28Z

...ain/java/com/codenvy/organization/api/resource/OrganizationResourcesDistributionService.java

+    @ApiOperation(value = "Distribute resources for suborganization.",
+                  notes = "Distributed resources is unavailable for usage by parent organization." +
+                          "It should reset resources distribution before consuming.",
+                  response = OrganizationDistributedResourcesDto.class)


lie, this method returns nothing

voievodin · 2016-11-30T10:05:12Z

...ain/java/com/codenvy/organization/api/resource/OrganizationResourcesDistributionService.java

+    @Path("/{organizationId}")
+    @ApiOperation(value = "Reset resources distribution.",
+                  notes = "After distribution resetting suborganization won't be able to use parent resources.")
+    @ApiResponses({@ApiResponse(code = 201, message = "Resources distribution successfully reset"),


why 201, it's 204 i think

voievodin · 2016-11-30T10:09:53Z

...on/src/main/java/com/codenvy/organization/api/resource/OrganizationResourcesDistributor.java

+    }
+
+    /**
+     * TODO Revise doc


do it! 🚀

voievodin

It seems that you did a pretty big amount of work, good job! Some of the futures are context specific so it's hard to review all of them, the code in general looks okay to me.

akorneta · 2016-11-30T11:49:22Z

...denvy-sql-schema/src/main/resources/codenvy-schema/5.0.0-M8/1.2__add_org_resources_limit.sql

+-- from Codenvy S.A..
+--
+
+-- Organization distributed resources  ))---------------------------------------------------------


delete the brackets please

akorneta · 2016-11-30T12:24:03Z

...nization/src/main/java/com/codenvy/organization/spi/OrganizationDistributedResourcesDao.java

+ */
+public interface OrganizationDistributedResourcesDao {
+    /**
+     * Stores distributed resources for suborganization.


I guess it will be helpful to describe here how to store resources when there are already exist resources for the organization.

musienko-maxim · 2016-12-09T10:00:26Z

After running the selenium tests new regression has not been found

sleshchenko force-pushed the CODENVY-957 branch from 45ed0d4 to 32662b6 Compare November 15, 2016 08:31

sleshchenko force-pushed the CODENVY-957 branch 5 times, most recently from 98619bb to fff6a09 Compare November 18, 2016 10:40

sleshchenko force-pushed the CODENVY-957 branch 2 times, most recently from 9cafe1d to 04dfa49 Compare November 24, 2016 16:14

voievodin reviewed Nov 25, 2016

View reviewed changes

sleshchenko mentioned this pull request Nov 28, 2016

Move StripedLocks to commons-lang module eclipse-che/che#3187

Merged

sleshchenko force-pushed the CODENVY-957 branch from 29b1b48 to 900b5cb Compare November 28, 2016 13:22

voievodin reviewed Nov 30, 2016

View reviewed changes

voievodin approved these changes Nov 30, 2016

View reviewed changes

akorneta reviewed Nov 30, 2016

View reviewed changes

akorneta approved these changes Nov 30, 2016

View reviewed changes

skabashnyuk approved these changes Nov 30, 2016

View reviewed changes

sleshchenko changed the title ~~[WIP] CODENVY-957 Implement resources redistribution between suborganizations~~ CODENVY-957 Implement resources redistribution between suborganizations Dec 1, 2016

sleshchenko force-pushed the CODENVY-957 branch 2 times, most recently from f5d23e2 to 9ef1706 Compare December 8, 2016 07:58

sleshchenko force-pushed the CODENVY-957 branch 5 times, most recently from f3f9ac1 to 461842f Compare December 8, 2016 09:49

ashumilova added this to the 5.0.0-M9 milestone Dec 8, 2016

sleshchenko force-pushed the CODENVY-957 branch from 461842f to e39a9fc Compare December 8, 2016 11:51

sleshchenko force-pushed the CODENVY-957 branch from e39a9fc to 7e14a2e Compare December 12, 2016 09:00

CODENVY-957 Implement resources distribution between suborganizations

a3c529a

sleshchenko force-pushed the CODENVY-957 branch from 7e14a2e to a3c529a Compare December 12, 2016 09:36

sleshchenko merged commit 6393a43 into codenvy:master Dec 12, 2016

sleshchenko deleted the CODENVY-957 branch December 12, 2016 11:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CODENVY-957 Implement resources redistribution between suborganizations #1141

CODENVY-957 Implement resources redistribution between suborganizations #1141

sleshchenko commented Nov 14, 2016 •

edited

Loading

sleshchenko commented Nov 15, 2016 •

edited

Loading

sleshchenko commented Nov 22, 2016

voievodin Nov 25, 2016 •

edited by sleshchenko

Loading

voievodin Nov 25, 2016

voievodin Nov 30, 2016

voievodin Nov 30, 2016

voievodin Nov 30, 2016

sleshchenko Nov 30, 2016

voievodin Nov 30, 2016

voievodin Nov 30, 2016 •

edited

Loading

voievodin Nov 30, 2016

voievodin Nov 30, 2016

voievodin Nov 30, 2016

voievodin Nov 30, 2016

voievodin left a comment

akorneta Nov 30, 2016

akorneta Nov 30, 2016

musienko-maxim commented Dec 9, 2016

		}


		if (!currentSubject.hasPermission(DOMAIN_ID, organizationId, OrganizationDomain.MANAGE_RESOURCES)) {

CODENVY-957 Implement resources redistribution between suborganizations #1141

CODENVY-957 Implement resources redistribution between suborganizations #1141

Conversation

sleshchenko commented Nov 14, 2016 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

New Behavior

Tests written?

Docs requirements?

sleshchenko commented Nov 15, 2016 • edited Loading

sleshchenko commented Nov 22, 2016

voievodin Nov 25, 2016 • edited by sleshchenko 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

voievodin Nov 30, 2016 • 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

voievodin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musienko-maxim commented Dec 9, 2016

sleshchenko commented Nov 14, 2016 •

edited

Loading

sleshchenko commented Nov 15, 2016 •

edited

Loading

voievodin Nov 25, 2016 •

edited by sleshchenko

Loading

voievodin Nov 30, 2016 •

edited

Loading