[GitHub] cloudstack pull request: utils: Use non-blocking SSL handshake in ...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59671416
--- Diff: utils/src/main/java/com/cloud/utils/nio/Link.java ---
@@ -453,115 +449,150 @@ public static SSLContext initSSLContext(boolean
isClient) throws GeneralSecurity
return sslContext;
}
-public static void doHandshake(SocketChannel ch, SSLEngine sslEngine,
boolean isClient) throws IOException {
-if (s_logger.isTraceEnabled()) {
-s_logger.trace("SSL: begin Handshake, isClient: " + isClient);
+public static ByteBuffer enlargeBuffer(ByteBuffer buffer, final int
sessionProposedCapacity) {
+if (sessionProposedCapacity > buffer.capacity()) {
+buffer = ByteBuffer.allocate(sessionProposedCapacity);
+} else {
+buffer = ByteBuffer.allocate(buffer.capacity() * 2);
}
+return buffer;
--- End diff --
Yes
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: utils: Use non-blocking SSL handshake in ...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1493#issuecomment-209809491 @jburwell fixed all issues --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Re: Strange XenServer SR behavior when deploying system VMs in Basic Zone on 4.9
Mike, did the iso copy process not complete as expected. Sound like they are a remanence of some task ending in an exception. Probably a silently ignored one ;| On Thu, Apr 14, 2016 at 2:49 AM, Tutkowski, Mike wrote: > Just an FYI, but when I kicked off my first VM in this cloud, the VR > happened to get deployed to XenServer-6.5-3 (which was one of my XenServer > hosts that had an un-expected shared SR pointing at secondary storage > beforehand). > > Once the process of copying the system template down to local storage > completed, the shared SR pointing at secondary storage went away (as you > would expect). > > This leaves me now with one un-expected shared SR pointing at secondary > storage on XenServer-6.5-1. > > > From: Tutkowski, Mike > Sent: Wednesday, April 13, 2016 5:10 PM > To: dev@cloudstack.apache.org > Subject: Strange XenServer SR behavior when deploying system VMs in Basic > Zone on 4.9 > > Hi, > > > Has anyone recently observed the following behavior: > > > http://imgur.com/8ALJmWb > > > As you can see in the image, I have three 6.5 XenServer hosts in a > resource pool. > > > I just used them when creating a basic zone and the system VMs were > deployed just fine. However, there are SRs pointing to secondary storage on > my XenServer-6.5-1 and XenServer-6.5-3 hosts still (there used to be one on > my XenServer-6.5-2 host, but it went away once the system VMs started up on > that host). > > > Thoughts? > > > Thanks, > > Mike > -- Daan
[GitHub] cloudstack pull request: CLOUDSTACK-9334: Support jenv and pyenv t...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/1460#issuecomment-209822016 so it was a timeout: ``` [INFO] Apache CloudStack Plugin - Quota Service ... SUCCESS [ 44.634 s] [INFO] Apache CloudStack Framework - Spring Module SUCCESS [ 38.721 s] [INFO] Apache CloudStack Secondary Storage Controller . FAILURE [10:04 min] [INFO] Apache CloudStack Client UI SKIPPED [INFO] Apache CloudStack Console Proxy - RDP Client ... SKIPPED [INFO] Apache CloudStack Console Proxy SKIPPED [INFO] Apache CloudStack Console Proxy - Server ... SKIPPED [INFO] Apache CloudStack Framework - QuickCloud ... SKIPPED [INFO] [INFO] BUILD FAILURE [INFO] [INFO] Total time: 01:25 h [INFO] Finished at: 2016-04-13T22:08:13+00:00 [INFO] Final Memory: 249M/4518M [INFO] [ERROR] Failed to execute goal org.codehaus.mojo:findbugs-maven-plugin:3.0.1:findbugs (findbugs) on project cloud-controller-secondary-storage: Execution findbugs of goal org.codehaus.mojo:findbugs-maven-plugin:3.0.1:findbugs failed: Timeout: killed the sub-process -> [Help 1] [ERROR] [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch. [ERROR] Re-run Maven using the -X switch to enable full debug logging. [ERROR] [ERROR] For more information about the errors and possible solutions, please read the following articles: [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginExecutionException [ERROR] [ERROR] After correcting the problems, you can resume the build with the command [ERROR] mvn -rf :cloud-controller-secondary-storage Build step 'Invoke top-level Maven targets' marked build as failure [CHECKSTYLE] Skipping publisher since build result is FAILURE [FINDBUGS] Skipping publisher since build result is FAILURE [PMD] Skipping publisher since build result is FAILURE [DRY] Skipping publisher since build result is FAILURE Archiving artifacts Skipping Cobertura coverage report as build was not UNSTABLE or better ... Publishing Clover coverage report... No Clover report will be published due to a Build Failure Putting comment on the pull request Finished: FAILURE ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Fixes regarding VOLUME_DELETE events resu...
Github user koushik-das commented on the pull request: https://github.com/apache/cloudstack/pull/1491#issuecomment-209830019 @ProjectMoon On latest master during VM deployment no VOLUME.CREATE is getting generated for ROOT volume. Also during destroy there is no VOLUME.DELETE event. On the event table I only see events for VM creation/destroy. Is ROOT volume events broken in general? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Master nuage
GitHub user nlivens opened a pull request: https://github.com/apache/cloudstack/pull/1494 Master nuage You can merge this pull request into a Git repository by running: $ git pull https://github.com/nlivens/cloudstack master_nuage Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1494.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1494 commit b738ab0e7a6039e0aec9658bab6a25acf645715f Author: Nick Livens Date: 2016-03-02T12:53:47Z CLOUDSTACK-9242 : Remodel Nuage VSP plugin commit 1e98c86cf2f73a49f71443e68f37420c647645df Author: Nick Livens Date: 2016-03-16T11:07:50Z CLOUDSTACK-9294 : Make sure to remove VR from VSD when removing the VPC --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1492#issuecomment-209856283 @ProjectMoon we'll need more information on why you're doing this, why we should have it, what is fixes and will it guarantee backward resource-name compatibility (for example, vmware vms have this strictly tie up with internal ACS vm name, such config are set in each vmware's VM's annotations) and upgrade paths --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1489#issuecomment-209856388 thanks @DaanHoogland --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
Github user ProjectMoon commented on the pull request: https://github.com/apache/cloudstack/pull/1492#issuecomment-209875708 > @ProjectMoon correct resource naming is critical to the proper operation of the management server. We have had a significant bugs and production issues caused by subtle changes to resource naming strategies between releases where CloudStack suddenly can't find a resource on the device it is attempting to control. > Have you performed any upgrade testing for this PR? If so, what tests have you performed in which configurations? We have not specifically performed any upgrade testing. Our current stable version is based on 4.7.1, and essentially our "upgrade testing" has consisted of deploying 4.7.1 before and after our development of this feature is complete. The configuration has been tested with KVM, VmWare, and the simulator. > Also, could you please add an FS to the wiki and start a conversation on dev@? Given the importance of resource naming, it would be extremely helpful to have an explanation of the design and its operation. As far as I know, I don't have access to the wiki. Otherwise I would add to it. Do I need to register a separate account or can I use my Apache JIRA account? > @ProjectMoon we'll need more information on why you're doing this, why we should have it, what is fixes and will it guarantee backward resource-name compatibility (for example, vmware vms have this strictly tie up with internal ACS vm name, such config are set in each vmware's VM's annotations) and upgrade paths * **Why we are doing this**: we implement our own naming scheme for the supported resources. On our 4.2 branch we hacked this in, but now we want to present a framework that we can extend, and open the possibilities to other. * **Why should mainline have this**: More flexibility for developers, easier testing (static classes notoriously cause problems), a unified way to generate names (DRY principle). * **What it fixes**: It doesn't _fix_ anything per se, but the refactoring helps move us towards a cleaner codebase. * **Backwards compatibility**: The default plugin generates names and UUIDs in the exact same way as before, so yes. VmWare is an interesting case though. Can you describe in more detail/point me to where this stuff happens so I can verify that custom naming plugins will not break it? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Feature proposal: Resource naming policies
Yesterday, we submitted this pull request: https://github.com/apache/cloudstack/pull/1492 This originally grew out of making the VirtualMachineName class non-static (original PR is mentioned in the above link). We're presenting this as a refactoring of the existing code to enable more extensibility and flexibility, make unit testing easier, and unify the way CloudStack generates resource names. There is an associated JIRA ticket at CLOUDSTACK-9003. I will be writing up a design document for the CS wiki in the next few days. jburwell wanted me to open a discussion on the developer list about the PR and how it is implemented: There is now a ResourceNamingPolicyManager and a bunch of ResourceNamingPolicies. The manager exposes a method to get a policy for a type of resource, and the naming policies generate UUIDs + resource names. The default naming policies generate names exactly the same way as they are created now. This is in a new module called default-naming-policies. By excluding this module and loading our own naming policies, we gain the ability to change how names are generated.
[GitHub] cloudstack pull request: Remodeling of Nuage VSP Plugin + CLOUDSTA...
Github user remibergsma commented on the pull request: https://github.com/apache/cloudstack/pull/1494#issuecomment-209893539 @nlivens Please add a description because this confuses me ;-) You guys go the non-oss way? :-( --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1493#issuecomment-209896417 - Tested against KVM, mgmt server - KVM links and clustered management server - NioTest modified to have multiple clients against a server instance with just one worker and 10 malicious clients (they simply do a secure connect to the server and don't do anything else) trying to connect server per valid client - Ran Marvin smoke tests successfully against KVM --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Remodeling of Nuage VSP Plugin + CLOUDSTA...
Github user nlivens commented on the pull request: https://github.com/apache/cloudstack/pull/1494#issuecomment-209912550 @remibergsma, I've added description :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Fixes regarding VOLUME_DELETE events resu...
Github user nnesic commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1491#discussion_r59707938
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -761,6 +774,17 @@ protected boolean cleanupAccount(AccountVO account,
long callerUserId, Account c
s_logger.error("Unable to expunge vm: " + vm.getId());
accountCleanupNeeded = true;
}
+else if
(!vm.getState().equals(VirtualMachine.State.Destroyed)) {
--- End diff --
The check on line 777 makes sure that the volume wasn't already destroyed
before expunging. In that case, the delete event has already been emitted
somewhere else.
The expunge thread does emit other usage events correctly (we tested
deleting an account having active VMs, datadisks, allocated IP's, templates,
and snapshots, and upon deleting the account, "delete" usage events were
emitted for each of them). The emission of the ROOT volume's "delete" event is
handled on a more case-by-case basis (In UserVmManager for regular VM destroys,
in VolumeOrchestrator for failed vm creation, etc), and the case where the root
volume gets deleted as part of account deletion was not being handled.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9180: Optimize concurrent VM d...
Github user bvbharatk commented on the pull request: https://github.com/apache/cloudstack/pull/1251#issuecomment-209922105 ### ACS CI BVT Run **Sumarry:** Build Number 175 Hypervisor xenserver NetworkType Advanced Passed=34 Failed=21 Skipped=2 _Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 **Failed tests:** * test_affinity_groups_projects.py * ContextSuite context=TestDeployVmWithAffinityGroup>:teardown Failed * test_vpc_vpn.py * ContextSuite context=TestVpcRemoteAccessVpn>:setup Failing since 5 runs * ContextSuite context=TestVpcSite2SiteVpn>:setup Failing since 3 runs * test_scale_vm.py * ContextSuite context=TestScaleVm>:teardown Failed * test_routers_iptables_default_policy.py * test_01_single_VPC_iptables_policies Failed * ContextSuite context=TestVPCIpTablesPolicies>:teardown Failed * test_quota.py * test_01_quota Failing since 3 runs * test_02_quota Failing since 3 runs * test_03_quota Failing since 3 runs * test_04_quota Failing since 3 runs * test_05_quota Failing since 3 runs * test_06_quota Failing since 3 runs * test_07_quota Failing since 3 runs * test_loadbalance.py * ContextSuite context=TestLoadBalance>:setup Failing since 2 runs * test_network.py * test_delete_account Failing since 2 runs * ContextSuite context=TestPortForwarding>:setup Failing since 2 runs * ContextSuite context=TestPublicIP>:setup Failed * test_reboot_router Failing since 2 runs * test_releaseIP Failing since 2 runs * ContextSuite context=TestRouterRules>:setup Failing since 2 runs * test_password_server.py * test_isolate_network_password_server Failing since 2 runs **Skipped tests:** test_vm_nic_adapter_vmxnet3 test_deploy_vgpu_enabled_vm **Passed test suits:** test_deploy_vm_with_userdata.py test_over_provisioning.py test_guest_vlan_range.py test_service_offerings.py test_reset_vm_on_reboot.py test_deploy_vms_with_varied_deploymentplanners.py test_deploy_vm_iso.py test_public_ip_range.py test_multipleips_per_nic.py test_regions.py test_resource_detail.py test_secondary_storage.py test_vm_life_cycle.py --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Fixes regarding VOLUME_DELETE events resu...
Github user nnesic commented on the pull request: https://github.com/apache/cloudstack/pull/1491#issuecomment-209922376 @koushik-das We are mostly working with 4.7 version, where other volume usage events get emitted correctly. We haven't investigated the behavior on master yet. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[discuss] CloudStack logging
Hi All I think that we can all agree that CloudStack logs are very difficult to read, especially for operational staff. I believe that the primary reason for this is that a large amount of what should be INFO level events are categorised as DEBUG. The logging therefore must be set at DEBUG for the logs to capture these events. By defaulting the logging to DEBUG we include a lot of detail which is counter-productive when using the logs to diagnose operational issues. I think the situation can be greatly improved by reviewing the categorisation of logged events and where necessary re-categorise then such that INFO, WARN and ERROR logging would be sufficient for an 'operational' admin. I think a phased approach where the default logging level remains at DEBUG while re-categorisation occurs would mean that nothing materially changes until we are happy with the categorisations. Then we can default the logging level to INFO. Thoughts everyone? Kind regards, Paul Angus Regards, Paul Angus paul.an...@shapeblue.com www.shapeblue.com 53 Chandos Place, Covent Garden, London WC2N 4HSUK @shapeblue
Re: [discuss] CloudStack logging
I largely agree with just one sneer to the operators amongst us: the level can be set on a per package and even class basis. So you can work through disabling anoying logging step by step. Makes sense to keep looking at the source in the meanwhile to make sure sensible diagnostics remains available. On Thu, Apr 14, 2016 at 2:49 PM, Paul Angus wrote: > Hi All > > I think that we can all agree that CloudStack logs are very difficult to > read, especially for operational staff. > > I believe that the primary reason for this is that a large amount of what > should be INFO level events are categorised as DEBUG. The logging > therefore must be set at DEBUG for the logs to capture these events. By > defaulting the logging to DEBUG we include a lot of detail which is > counter-productive when using the logs to diagnose operational issues. > > I think the situation can be greatly improved by reviewing the > categorisation of logged events and where necessary re-categorise then such > that INFO, WARN and ERROR logging would be sufficient for an 'operational' > admin. > > I think a phased approach where the default logging level remains at DEBUG > while re-categorisation occurs would mean that nothing materially changes > until we are happy with the categorisations. Then we can default the > logging level to INFO. > > > > Thoughts everyone? > > > > > Kind regards, > > Paul Angus > > > Regards, > > Paul Angus > > paul.an...@shapeblue.com > www.shapeblue.com > 53 Chandos Place, Covent Garden, London WC2N 4HSUK > @shapeblue > -- Daan
Re: Feature proposal: Resource naming policies
sounds usefull for companies that for instance want to enforce uuid in name or include some user string in it, same would be true for networks. look forward to your design. On Thu, Apr 14, 2016 at 1:40 PM, Jeff Hair wrote: > Yesterday, we submitted this pull request: > https://github.com/apache/cloudstack/pull/1492 > > This originally grew out of making the VirtualMachineName class non-static > (original PR is mentioned in the above link). We're presenting this as a > refactoring of the existing code to enable more extensibility and > flexibility, make unit testing easier, and unify the way CloudStack > generates resource names. > > There is an associated JIRA ticket at CLOUDSTACK-9003. I will be writing up > a design document for the CS wiki in the next few days. > > jburwell wanted me to open a discussion on the developer list about the PR > and how it is implemented: > > There is now a ResourceNamingPolicyManager and a bunch of > ResourceNamingPolicies. The manager exposes a method to get a policy for a > type of resource, and the naming policies generate UUIDs + resource names. > > The default naming policies generate names exactly the same way as they are > created now. This is in a new module called default-naming-policies. By > excluding this module and loading our own naming policies, we gain the > ability to change how names are generated. > -- Daan
[GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
Github user DaanHoogland commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1492#discussion_r59717124 --- Diff: plugins/naming-policies/src/com/cloud/naming/DefaultConsoleProxyNamingPolicy.java --- @@ -0,0 +1,58 @@ +package com.cloud.naming; --- End diff -- license missing --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/1492#issuecomment-209945535 @ProjectMoon I will run through the code after I read the detailed design. one remark so far. This is a nice feature to have. please add integration tests for it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Re: [discuss] CloudStack logging
Is there an easy way to do that Daan, or is it a tedious task you just have to power through? I agree this initiative would be helpful. On Apr 14, 2016 9:04 AM, "Daan Hoogland" wrote: > I largely agree with just one sneer to the operators amongst us: the level > can be set on a per package and even class basis. So you can work through > disabling anoying logging step by step. Makes sense to keep looking at the > source in the meanwhile to make sure sensible diagnostics remains > available. > > On Thu, Apr 14, 2016 at 2:49 PM, Paul Angus > wrote: > > > Hi All > > > > I think that we can all agree that CloudStack logs are very difficult to > > read, especially for operational staff. > > > > I believe that the primary reason for this is that a large amount of what > > should be INFO level events are categorised as DEBUG. The logging > > therefore must be set at DEBUG for the logs to capture these events. By > > defaulting the logging to DEBUG we include a lot of detail which is > > counter-productive when using the logs to diagnose operational issues. > > > > I think the situation can be greatly improved by reviewing the > > categorisation of logged events and where necessary re-categorise then > such > > that INFO, WARN and ERROR logging would be sufficient for an > 'operational' > > admin. > > > > I think a phased approach where the default logging level remains at > DEBUG > > while re-categorisation occurs would mean that nothing materially changes > > until we are happy with the categorisations. Then we can default the > > logging level to INFO. > > > > > > > > Thoughts everyone? > > > > > > > > > > Kind regards, > > > > Paul Angus > > > > > > Regards, > > > > Paul Angus > > > > paul.an...@shapeblue.com > > www.shapeblue.com > > 53 Chandos Place, Covent Garden, London WC2N 4HSUK > > @shapeblue > > > > > > -- > Daan >
[GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
Github user DaanHoogland commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1492#discussion_r59717381 --- Diff: engine/schema/src/com/cloud/naming/ConsoleProxyNamingPolicy.java --- @@ -0,0 +1,9 @@ +package com.cloud.naming; --- End diff -- license missing --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Re: [discuss] CloudStack logging
On Thu, Apr 14, 2016 at 3:41 PM, Will Stevens wrote: > Is there an easy way to do that Daan, or is it a tedious task you just have > to power through? > It is hard work, mostly tedious, sometimes hilarious and most definitely devops! It certainly wouldn't classify as development. -- Daan
Re: [discuss] CloudStack logging
Do you have to recompile in order to turn off the logging for a specific package or class? If yes, that is a show stopper for almost everyone... If it only requires a management server restart, that is more realistic. *Will STEVENS* Lead Developer *CloudOps* *| *Cloud Solutions Experts 420 rue Guy *|* Montreal *|* Quebec *|* H3J 1S6 w cloudops.com *|* tw @CloudOps_ On Thu, Apr 14, 2016 at 9:48 AM, Daan Hoogland wrote: > On Thu, Apr 14, 2016 at 3:41 PM, Will Stevens > wrote: > > > Is there an easy way to do that Daan, or is it a tedious task you just > have > > to power through? > > > It is hard work, mostly tedious, sometimes hilarious and most definitely > > devops! It certainly wouldn't classify as development. > > > > > -- > Daan >
Re: [discuss] CloudStack logging
> Op 14 april 2016 om 14:49 schreef Paul Angus : > > > Hi All > > I think that we can all agree that CloudStack logs are very difficult to read, > especially for operational staff. > > I believe that the primary reason for this is that a large amount of what > should be INFO level events are categorised as DEBUG. The logging therefore > must be set at DEBUG for the logs to capture these events. By defaulting the > logging to DEBUG we include a lot of detail which is counter-productive when > using the logs to diagnose operational issues. > Yes! > I think the situation can be greatly improved by reviewing the categorisation > of logged events and where necessary re-categorise then such that INFO, WARN > and ERROR logging would be sufficient for an 'operational' admin. > > I think a phased approach where the default logging level remains at DEBUG > while re-categorisation occurs would mean that nothing materially changes > until we are happy with the categorisations. Then we can default the logging > level to INFO. > I created a issue for this a while ago: https://issues.apache.org/jira/browse/CLOUDSTACK-8645 Short: Yes. Just change this where needed. Wido > > > Thoughts everyone? > > > > > Kind regards, > > Paul Angus > > > Regards, > > Paul Angus > > paul.an...@shapeblue.com > www.shapeblue.com > 53 Chandos Place, Covent Garden, London WC2N 4HSUK > @shapeblue
RE: [discuss] CloudStack logging
Grabbing a couple of examples - these should be debug - I can't 'do' anything with this information. INFO [o.a.c.f.j.i.AsyncJobManagerImpl] (AsyncJobMgr-Heartbeat-1:ctx-4119d5bc) Begin cleanup expired async-jobs INFO [o.a.c.f.j.i.AsyncJobManagerImpl] (AsyncJobMgr-Heartbeat-1:ctx-4119d5bc) End cleanup expired async-jobs INFO [c.c.h.v.r.VmwareResource] (DirectAgentCronJob-2:ctx-3209f65c) Scan hung worker VM to recycle INFO [c.c.h.v.r.VmwareResource] (DirectAgentCronJob-113:ctx-56d9f9f2) Scan hung worker VM to recycle Whereas this should be WARN. I wouldn't want to lose this by switching off DEBUG DEBUG [c.c.c.ClusterManagerImpl] (Cluster-Heartbeat-1:ctx-91226be7) Detected management node left, id:2, nodeIP:10.2.0.6 DEBUG [c.c.c.ClusterManagerImpl] (Cluster-Heartbeat-1:ctx-0bccd078) Detected management node left, id:2, nodeIP:10.2.0.6 Kind regards, Paul Angus Regards, Paul Angus paul.an...@shapeblue.com www.shapeblue.com 53 Chandos Place, Covent Garden, London WC2N 4HSUK @shapeblue -Original Message- From: Wido den Hollander [mailto:w...@widodh.nl] Sent: 14 April 2016 15:04 To: Paul Angus ; dev@cloudstack.apache.org Subject: Re: [discuss] CloudStack logging > Op 14 april 2016 om 14:49 schreef Paul Angus : > > > Hi All > > I think that we can all agree that CloudStack logs are very difficult > to read, especially for operational staff. > > I believe that the primary reason for this is that a large amount of > what should be INFO level events are categorised as DEBUG. The > logging therefore must be set at DEBUG for the logs to capture these > events. By defaulting the logging to DEBUG we include a lot of detail > which is counter-productive when using the logs to diagnose operational > issues. > Yes! > I think the situation can be greatly improved by reviewing the > categorisation of logged events and where necessary re-categorise then > such that INFO, WARN and ERROR logging would be sufficient for an > 'operational' admin. > > I think a phased approach where the default logging level remains at > DEBUG while re-categorisation occurs would mean that nothing > materially changes until we are happy with the categorisations. Then > we can default the logging level to INFO. > I created a issue for this a while ago: https://issues.apache.org/jira/browse/CLOUDSTACK-8645 Short: Yes. Just change this where needed. Wido > > > Thoughts everyone? > > > > > Kind regards, > > Paul Angus > > > Regards, > > Paul Angus > > paul.an...@shapeblue.com > www.shapeblue.com > 53 Chandos Place, Covent Garden, London WC2N 4HSUK @shapeblue
[GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
Github user ProjectMoon commented on the pull request: https://github.com/apache/cloudstack/pull/1492#issuecomment-209963503 @DaanHoogland Do you have any kind of integration tests in mind? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Re: [discuss] CloudStack logging
I'm all for this as well. Training our systems folks on how to work through the logs is a pain. Focusing in on the most important items is more useful and also makes it easier to automate log fetching and categorization. - Si From: Paul Angus Sent: Thursday, April 14, 2016 9:06 AM To: Wido den Hollander; dev@cloudstack.apache.org Subject: RE: [discuss] CloudStack logging Grabbing a couple of examples - these should be debug - I can't 'do' anything with this information. INFO [o.a.c.f.j.i.AsyncJobManagerImpl] (AsyncJobMgr-Heartbeat-1:ctx-4119d5bc) Begin cleanup expired async-jobs INFO [o.a.c.f.j.i.AsyncJobManagerImpl] (AsyncJobMgr-Heartbeat-1:ctx-4119d5bc) End cleanup expired async-jobs INFO [c.c.h.v.r.VmwareResource] (DirectAgentCronJob-2:ctx-3209f65c) Scan hung worker VM to recycle INFO [c.c.h.v.r.VmwareResource] (DirectAgentCronJob-113:ctx-56d9f9f2) Scan hung worker VM to recycle Whereas this should be WARN. I wouldn't want to lose this by switching off DEBUG DEBUG [c.c.c.ClusterManagerImpl] (Cluster-Heartbeat-1:ctx-91226be7) Detected management node left, id:2, nodeIP:10.2.0.6 DEBUG [c.c.c.ClusterManagerImpl] (Cluster-Heartbeat-1:ctx-0bccd078) Detected management node left, id:2, nodeIP:10.2.0.6 Kind regards, Paul Angus Regards, Paul Angus paul.an...@shapeblue.com www.shapeblue.com 53 Chandos Place, Covent Garden, London WC2N 4HSUK @shapeblue -Original Message- From: Wido den Hollander [mailto:w...@widodh.nl] Sent: 14 April 2016 15:04 To: Paul Angus ; dev@cloudstack.apache.org Subject: Re: [discuss] CloudStack logging > Op 14 april 2016 om 14:49 schreef Paul Angus : > > > Hi All > > I think that we can all agree that CloudStack logs are very difficult > to read, especially for operational staff. > > I believe that the primary reason for this is that a large amount of > what should be INFO level events are categorised as DEBUG. The > logging therefore must be set at DEBUG for the logs to capture these > events. By defaulting the logging to DEBUG we include a lot of detail > which is counter-productive when using the logs to diagnose operational > issues. > Yes! > I think the situation can be greatly improved by reviewing the > categorisation of logged events and where necessary re-categorise then > such that INFO, WARN and ERROR logging would be sufficient for an > 'operational' admin. > > I think a phased approach where the default logging level remains at > DEBUG while re-categorisation occurs would mean that nothing > materially changes until we are happy with the categorisations. Then > we can default the logging level to INFO. > I created a issue for this a while ago: https://issues.apache.org/jira/browse/CLOUDSTACK-8645 Short: Yes. Just change this where needed. Wido > > > Thoughts everyone? > > > > > Kind regards, > > Paul Angus > > > Regards, > > Paul Angus > > paul.an...@shapeblue.com > www.shapeblue.com > 53 Chandos Place, Covent Garden, London WC2N 4HSUK @shapeblue
Re: [discuss] CloudStack logging
Really agree with this one. With a toolchain like ELK, it isn't as bad
because you can filter out the things you don't want, but generally
speaking, the log levels need to be reevaluated. I would also add that many
log statements would benefit from additional context, but that is a whole
different story.
Another example:
INFO [com.cloud.agent.transport.Request] (StatsCollector-3:ctx-de473a88)
not building log message for '[{}]', _cmds.length == 1
It even says "not building log message".
On Thu, Apr 14, 2016 at 10:18 AM, Simon Weller wrote:
> I'm all for this as well.
>
> Training our systems folks on how to work through the logs is a pain.
> Focusing in on the most important items is more useful and also makes it
> easier to automate log fetching and categorization.
>
> - Si
>
>
> From: Paul Angus
> Sent: Thursday, April 14, 2016 9:06 AM
> To: Wido den Hollander; dev@cloudstack.apache.org
> Subject: RE: [discuss] CloudStack logging
>
> Grabbing a couple of examples - these should be debug - I can't 'do'
> anything with this information.
>
> INFO [o.a.c.f.j.i.AsyncJobManagerImpl]
> (AsyncJobMgr-Heartbeat-1:ctx-4119d5bc) Begin cleanup expired async-jobs
> INFO [o.a.c.f.j.i.AsyncJobManagerImpl]
> (AsyncJobMgr-Heartbeat-1:ctx-4119d5bc) End cleanup expired async-jobs
> INFO [c.c.h.v.r.VmwareResource] (DirectAgentCronJob-2:ctx-3209f65c) Scan
> hung worker VM to recycle
> INFO [c.c.h.v.r.VmwareResource] (DirectAgentCronJob-113:ctx-56d9f9f2)
> Scan hung worker VM to recycle
>
> Whereas this should be WARN. I wouldn't want to lose this by switching off
> DEBUG
>
> DEBUG [c.c.c.ClusterManagerImpl] (Cluster-Heartbeat-1:ctx-91226be7)
> Detected management node left, id:2, nodeIP:10.2.0.6
> DEBUG [c.c.c.ClusterManagerImpl] (Cluster-Heartbeat-1:ctx-0bccd078)
> Detected management node left, id:2, nodeIP:10.2.0.6
>
>
>
> Kind regards,
>
> Paul Angus
>
> Regards,
>
> Paul Angus
>
> paul.an...@shapeblue.com
> www.shapeblue.com
> 53 Chandos Place, Covent Garden, London WC2N 4HSUK
> @shapeblue
>
> -Original Message-
> From: Wido den Hollander [mailto:w...@widodh.nl]
> Sent: 14 April 2016 15:04
> To: Paul Angus ; dev@cloudstack.apache.org
> Subject: Re: [discuss] CloudStack logging
>
>
> > Op 14 april 2016 om 14:49 schreef Paul Angus :
> >
> >
> > Hi All
> >
> > I think that we can all agree that CloudStack logs are very difficult
> > to read, especially for operational staff.
> >
> > I believe that the primary reason for this is that a large amount of
> > what should be INFO level events are categorised as DEBUG. The
> > logging therefore must be set at DEBUG for the logs to capture these
> > events. By defaulting the logging to DEBUG we include a lot of detail
> > which is counter-productive when using the logs to diagnose operational
> issues.
> >
>
> Yes!
>
> > I think the situation can be greatly improved by reviewing the
> > categorisation of logged events and where necessary re-categorise then
> > such that INFO, WARN and ERROR logging would be sufficient for an
> 'operational' admin.
> >
> > I think a phased approach where the default logging level remains at
> > DEBUG while re-categorisation occurs would mean that nothing
> > materially changes until we are happy with the categorisations. Then
> > we can default the logging level to INFO.
> >
>
> I created a issue for this a while ago:
> https://issues.apache.org/jira/browse/CLOUDSTACK-8645
>
> Short: Yes. Just change this where needed.
>
> Wido
>
> >
> >
> > Thoughts everyone?
> >
> >
> >
> >
> > Kind regards,
> >
> > Paul Angus
> >
> >
> > Regards,
> >
> > Paul Angus
> >
> > paul.an...@shapeblue.com
> > www.shapeblue.com
> > 53 Chandos Place, Covent Garden, London WC2N 4HSUK @shapeblue
>
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user pdube commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59730081
--- Diff: api/src/org/apache/cloudstack/acl/RoleType.java ---
@@ -16,18 +16,90 @@
// under the License.
package org.apache.cloudstack.acl;
+import com.cloud.user.Account;
+import com.google.common.base.Enums;
+import com.google.common.base.Strings;
+
// Enum for default roles in CloudStack
public enum RoleType {
-Admin(1), ResourceAdmin(2), DomainAdmin(4), User(8), Unknown(0);
+Admin(1L, Account.ACCOUNT_TYPE_ADMIN, 1),
+ResourceAdmin(2L, Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN, 2),
+DomainAdmin(3L, Account.ACCOUNT_TYPE_DOMAIN_ADMIN, 4),
+User(4L, Account.ACCOUNT_TYPE_NORMAL, 8),
+Unknown(-1L, (short) -1, 0);
+private long id;
+private short accountType;
private int mask;
-private RoleType(int mask) {
+RoleType(final long id, final short accountType, final int mask) {
+this.id = id;
+this.accountType = accountType;
this.mask = mask;
}
-public int getValue() {
+public long getId() {
+return id;
+}
+
+public short getAccountType() {
+return accountType;
+}
+
+public int getMask() {
return mask;
}
-}
+public static RoleType fromString(final String name) {
+if (!Strings.isNullOrEmpty(name)
+&& Enums.getIfPresent(RoleType.class, name).isPresent()) {
+return RoleType.valueOf(name);
+}
+return null;
+}
+
+public static RoleType fromMask(int mask) {
+for (RoleType roleType : RoleType.values()) {
+if (roleType.getMask() == mask) {
+return roleType;
+}
+}
+return Unknown;
+}
+
+public static RoleType getByAccountType(final short accountType) {
+RoleType roleType = RoleType.Unknown;
+switch (accountType) {
+case Account.ACCOUNT_TYPE_ADMIN:
+roleType = RoleType.Admin;
+break;
+case Account.ACCOUNT_TYPE_DOMAIN_ADMIN:
+roleType = RoleType.DomainAdmin;
+break;
+case Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN:
+roleType = RoleType.ResourceAdmin;
--- End diff --
What is this role type? Is there any documentation on it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user pdube commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59730334
--- Diff: api/src/org/apache/cloudstack/acl/RoleType.java ---
@@ -16,18 +16,90 @@
// under the License.
package org.apache.cloudstack.acl;
+import com.cloud.user.Account;
+import com.google.common.base.Enums;
+import com.google.common.base.Strings;
+
// Enum for default roles in CloudStack
public enum RoleType {
-Admin(1), ResourceAdmin(2), DomainAdmin(4), User(8), Unknown(0);
+Admin(1L, Account.ACCOUNT_TYPE_ADMIN, 1),
+ResourceAdmin(2L, Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN, 2),
+DomainAdmin(3L, Account.ACCOUNT_TYPE_DOMAIN_ADMIN, 4),
+User(4L, Account.ACCOUNT_TYPE_NORMAL, 8),
+Unknown(-1L, (short) -1, 0);
+private long id;
+private short accountType;
private int mask;
-private RoleType(int mask) {
+RoleType(final long id, final short accountType, final int mask) {
+this.id = id;
+this.accountType = accountType;
this.mask = mask;
}
-public int getValue() {
+public long getId() {
+return id;
+}
+
+public short getAccountType() {
+return accountType;
+}
+
+public int getMask() {
return mask;
}
-}
+public static RoleType fromString(final String name) {
+if (!Strings.isNullOrEmpty(name)
+&& Enums.getIfPresent(RoleType.class, name).isPresent()) {
+return RoleType.valueOf(name);
+}
+return null;
+}
+
+public static RoleType fromMask(int mask) {
+for (RoleType roleType : RoleType.values()) {
+if (roleType.getMask() == mask) {
+return roleType;
+}
+}
+return Unknown;
+}
+
+public static RoleType getByAccountType(final short accountType) {
+RoleType roleType = RoleType.Unknown;
+switch (accountType) {
+case Account.ACCOUNT_TYPE_ADMIN:
+roleType = RoleType.Admin;
+break;
+case Account.ACCOUNT_TYPE_DOMAIN_ADMIN:
+roleType = RoleType.DomainAdmin;
+break;
+case Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN:
+roleType = RoleType.ResourceAdmin;
+break;
+case Account.ACCOUNT_TYPE_NORMAL:
+roleType = RoleType.User;
+break;
+}
+return roleType;
+}
+
+public static Long getRoleByAccountType(final Long roleId, final Short
accountType) {
--- End diff --
Why are you passing in a roleId, if you are getting a role id by account
type?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
Re: [discuss] CloudStack logging
Based on the namespace log level can be set. It needs to be configured in the log4j. Check client/tomcatconf/log4j-cloud.xml.in in the source code and you will get an idea. -Koushik From: williamstev...@gmail.com on behalf of Will Stevens Sent: Thursday, April 14, 2016 7:29 PM To: dev@cloudstack.apache.org Subject: Re: [discuss] CloudStack logging Do you have to recompile in order to turn off the logging for a specific package or class? If yes, that is a show stopper for almost everyone... If it only requires a management server restart, that is more realistic. *Will STEVENS* Lead Developer *CloudOps* *| *Cloud Solutions Experts 420 rue Guy *|* Montreal *|* Quebec *|* H3J 1S6 w cloudops.com *|* tw @CloudOps_ On Thu, Apr 14, 2016 at 9:48 AM, Daan Hoogland wrote: > On Thu, Apr 14, 2016 at 3:41 PM, Will Stevens > wrote: > > > Is there an easy way to do that Daan, or is it a tedious task you just > have > > to power through? > > > It is hard work, mostly tedious, sometimes hilarious and most definitely > > devops! It certainly wouldn't classify as development. > > > > > -- > Daan > DISCLAIMER == This e-mail may contain privileged and confidential information which is the property of Accelerite, a Persistent Systems business. It is intended only for the use of the individual or entity to which it is addressed. If you are not the intended recipient, you are not authorized to read, retain, copy, print, distribute or use this message. If you have received this communication in error, please notify the sender and delete all copies of this message. Accelerite, a Persistent Systems business does not accept any liability for virus infected mails.
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user pdube commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59730927
--- Diff: api/src/org/apache/cloudstack/acl/RoleService.java ---
@@ -0,0 +1,43 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.acl;
+
+import org.apache.cloudstack.framework.config.ConfigKey;
+
+import java.util.List;
+
+public interface RoleService {
+
+ConfigKey EnableDynamicApiChecker = new ConfigKey<>("Hidden",
Boolean.class, "dynamic.apichecker.enabled", "false",
+"If set to true, this enables the dynamic role-based api
access checker and disables the default static role-based api access checker.",
+true);
+
+boolean isEnabled();
+Role findRole(final Long id);
+Role createRole(final String name, final RoleType roleType, final
String description);
+boolean updateRole(final Role role, final String name, final RoleType
roleType, final String description);
+boolean deleteRole(final Role role);
+
+RolePermission findRolePermission(final Long id);
+RolePermission createRolePermission(final Role role, final Rule rule,
final RolePermission.Permission permission, final String description);
+boolean updateRolePermission(final RolePermission rolePermission,
final Rule rule, final RolePermission.Permission permission, final String
description);
+boolean deleteRolePermission(final RolePermission rolePermission);
+
+List findAllRolesBy(final Long id, final String name, final
RoleType roleType);
--- End diff --
I would have to agree with @DaanHoogland. Splitting into separate methods
would be clearer and probably simplify the business logic per method as well
(so it would be simpler to unit test).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Add perl-modules as install dependency fo...
GitHub user sverrirab opened a pull request: https://github.com/apache/cloudstack/pull/1495 Add perl-modules as install dependency for cloudstack-agent Required to run perl scripts that configure networking for VMs. You can merge this pull request into a Git repository by running: $ git pull https://github.com/greenqloud/cloudstack pr-install-perl-modules-on-agent Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1495.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1495 commit 6d6bd510c2f5dc3fe076183379388521a55f95fe Author: Sverrir Berg Date: 2016-04-13T16:48:31Z Add perl-modules as install dependency for cloudstack-agent Required to run perl scripts that configure networking for VMs. That script fails silently if this is not installed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
RE: [discuss] CloudStack logging
I am confused about the configuration file there two files one is named Log4j-cloud.xml The other is Log4j-cloud.xml.in What is the difference between these two files ? I hope someone can help me clear this confusion Original message From: Koushik Das Date: 14/04/2016 6:49 PM (GMT+04:00) To: dev@cloudstack.apache.org Subject: Re: [discuss] CloudStack logging Based on the namespace log level can be set. It needs to be configured in the log4j. Check client/tomcatconf/log4j-cloud.xml.in in the source code and you will get an idea. -Koushik From: williamstev...@gmail.com on behalf of Will Stevens Sent: Thursday, April 14, 2016 7:29 PM To: dev@cloudstack.apache.org Subject: Re: [discuss] CloudStack logging Do you have to recompile in order to turn off the logging for a specific package or class? If yes, that is a show stopper for almost everyone... If it only requires a management server restart, that is more realistic. *Will STEVENS* Lead Developer *CloudOps* *| *Cloud Solutions Experts 420 rue Guy *|* Montreal *|* Quebec *|* H3J 1S6 w cloudops.com *|* tw @CloudOps_ On Thu, Apr 14, 2016 at 9:48 AM, Daan Hoogland wrote: > On Thu, Apr 14, 2016 at 3:41 PM, Will Stevens > wrote: > > > Is there an easy way to do that Daan, or is it a tedious task you just > have > > to power through? > > > It is hard work, mostly tedious, sometimes hilarious and most definitely > > devops! It certainly wouldn't classify as development. > > > > > -- > Daan > DISCLAIMER == This e-mail may contain privileged and confidential information which is the property of Accelerite, a Persistent Systems business. It is intended only for the use of the individual or entity to which it is addressed. If you are not the intended recipient, you are not authorized to read, retain, copy, print, distribute or use this message. If you have received this communication in error, please notify the sender and delete all copies of this message. Accelerite, a Persistent Systems business does not accept any liability for virus infected mails.
RE: [discuss] CloudStack logging
The file in a running installation you want is: /etc/cloudstack/management/ log4j-cloud.xml It is generally dynamic - you don't have to restart mgmt. server, it just takes a few seconds to kick in. If you're looking at the source code, various components use logging - SystemVMs, KVM agents, CloudStack mgmt. servers. So there'll be multiple occurrences As Koushik says, I think ./client/tomcatconf/log4j-cloud.xml.in is the one for the mgmt. server. Kind regards, Paul Angus Regards, Paul Angus paul.an...@shapeblue.com www.shapeblue.com 53 Chandos Place, Covent Garden, London WC2N 4HSUK @shapeblue -Original Message- From: Chaz PC [mailto:dreeems4e...@hotmail.com] Sent: 14 April 2016 16:01 To: dev@cloudstack.apache.org Subject: RE: [discuss] CloudStack logging I am confused about the configuration file there two files one is named Log4j-cloud.xml The other is Log4j-cloud.xml.in What is the difference between these two files ? I hope someone can help me clear this confusion Original message From: Koushik Das Date: 14/04/2016 6:49 PM (GMT+04:00) To: dev@cloudstack.apache.org Subject: Re: [discuss] CloudStack logging Based on the namespace log level can be set. It needs to be configured in the log4j. Check client/tomcatconf/log4j-cloud.xml.in in the source code and you will get an idea. -Koushik From: williamstev...@gmail.com on behalf of Will Stevens Sent: Thursday, April 14, 2016 7:29 PM To: dev@cloudstack.apache.org Subject: Re: [discuss] CloudStack logging Do you have to recompile in order to turn off the logging for a specific package or class? If yes, that is a show stopper for almost everyone... If it only requires a management server restart, that is more realistic. *Will STEVENS* Lead Developer *CloudOps* *| *Cloud Solutions Experts 420 rue Guy *|* Montreal *|* Quebec *|* H3J 1S6 w cloudops.com *|* tw @CloudOps_ On Thu, Apr 14, 2016 at 9:48 AM, Daan Hoogland wrote: > On Thu, Apr 14, 2016 at 3:41 PM, Will Stevens > > wrote: > > > Is there an easy way to do that Daan, or is it a tedious task you > > just > have > > to power through? > > > It is hard work, mostly tedious, sometimes hilarious and most > definitely > > devops! It certainly wouldn't classify as development. > > > > > -- > Daan > DISCLAIMER == This e-mail may contain privileged and confidential information which is the property of Accelerite, a Persistent Systems business. It is intended only for the use of the individual or entity to which it is addressed. If you are not the intended recipient, you are not authorized to read, retain, copy, print, distribute or use this message. If you have received this communication in error, please notify the sender and delete all copies of this message. Accelerite, a Persistent Systems business does not accept any liability for virus infected mails.
[GitHub] cloudstack pull request: Taking fast and efficient volume snapshot...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/1403#issuecomment-210009483 This does not compile: ``` [ERROR] COMPILATION ERROR : [INFO] - [ERROR] /data/git/cs1/cloudstack/dist/rpmbuild/BUILD/cloudstack-4.9.0-SNAPSHOT/plugins/api/solidfire-intg-test/src/org/apache/cloudstack/solidfire/ApiSolidFireServiceImpl.java:[93,68] error: cannot find symbol [INFO] 1 error ``` It fails here: ``` [INFO] Apache CloudStack Plugin - Storage Volume default provider SUCCESS [2.494s] [INFO] Apache CloudStack Plugin - Storage Volume SolidFire Provider SUCCESS [6.477s] [INFO] Apache CloudStack Plugin - API SolidFire .. FAILURE [2.601s] [INFO] Apache CloudStack Plugin - API Discovery .. SKIPPED [INFO] Apache CloudStack Plugin - ACL Static Role Based .. SKIPPED ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Taking fast and efficient volume snapshot...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/1403#issuecomment-210012980 Looking closer at this code. It look like `primaryStoreDriver` could easily be `null` in this case, so that return statement could potentially cause a null pointer exception... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Add perl-modules as install dependency fo...
Github user eriweb commented on the pull request: https://github.com/apache/cloudstack/pull/1495#issuecomment-210016535 Quite horribly that we need those modules for a simple perl script of 58 lines (including license header and empty lines). Not related to your PR though, but a good candidate to be rewritten in bash or python --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Add perl-modules as install dependency fo...
Github user jburwell commented on the pull request: https://github.com/apache/cloudstack/pull/1495#issuecomment-210018290 @sverrirab how much effort would it be to port to Python? That seems far preferable for two reasons. First, as @eriweb rightly points out, it is a large dependency to take on for such a small script. Second, Python and bash are our scripting languages of choice. It does not seem sustainable to add runtimes/libraries for every scripting language people might want to use. Such sprawl will not only lead to bloat, but a system that fewer and fewer people can maintain. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Github user rafaelweingartner commented on the pull request: https://github.com/apache/cloudstack/pull/1331#issuecomment-210018309 @syed, I totally understand that you do not have much experience with Java development and that you were following the practices already put in place. The problem is that we have so many cases of what not to do. I find it very good that you are willing to learn and help us improve the ACS code base. So letâs go to the work. Sure I changed the variable âloggerâ to protected, I needed to access it on a test case, so I changed it to an access delimiter that I could use; you should always try to use the most restrictive one, but at the end in Java those access delimiters can always be bypassed using Reflections. The protected delimiter states that childrenâs of that class and classes at the same package will have access to that variable/method. I also removed the âfinalâ because I needed to change that variable to the test with a mocked one. The final delimiter would not allow that; therefore, I had to remove it. That is why I would normally avoid the use of final everywhere, it can complicate things. I would only use final where I really want to be strict and not allow a variable change on the fly. I removed the static to transform that variable into an âinstanceâ attribute; those objects are spring beans, and they are working as singletons, there will be no more than one instance of them. That is why I see no reason to use that attribute as a class field. Now, you could go over all of the classes that extend âNfsSecondaryStorageResourceâ, they would have their own âloggerâ variable, then you could remove the âs_loggerâ they are using and use the one inherited. Additionally, you would be using a more common name for that variable that is âloggerâ. For your second batch of questions, I prefer to use the most restrictive delimiter access possible. I always start with private and then I go opening them on a need basis. I have no idea why someone would do different, such as using âpublic static finalâ when it is not needed. ACS has a lot of code that should not be taken as reference. In doubt, you can always call for help; there are folks here that are great devs. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Re: Feature proposal: Resource naming policies
Are these the names that are sent to HV? For e.g. VM name in the format i-xx-yy-VM. Currently there are functionalities that rely on the internal naming convention. All such functionalities needs to be handled appropriately. -Koushik From: Jeff Hair Sent: Thursday, April 14, 2016 5:10 PM To: dev@cloudstack.apache.org Subject: Feature proposal: Resource naming policies Yesterday, we submitted this pull request: https://github.com/apache/cloudstack/pull/1492 This originally grew out of making the VirtualMachineName class non-static (original PR is mentioned in the above link). We're presenting this as a refactoring of the existing code to enable more extensibility and flexibility, make unit testing easier, and unify the way CloudStack generates resource names. There is an associated JIRA ticket at CLOUDSTACK-9003. I will be writing up a design document for the CS wiki in the next few days. jburwell wanted me to open a discussion on the developer list about the PR and how it is implemented: There is now a ResourceNamingPolicyManager and a bunch of ResourceNamingPolicies. The manager exposes a method to get a policy for a type of resource, and the naming policies generate UUIDs + resource names. The default naming policies generate names exactly the same way as they are created now. This is in a new module called default-naming-policies. By excluding this module and loading our own naming policies, we gain the ability to change how names are generated. DISCLAIMER == This e-mail may contain privileged and confidential information which is the property of Accelerite, a Persistent Systems business. It is intended only for the use of the individual or entity to which it is addressed. If you are not the intended recipient, you are not authorized to read, retain, copy, print, distribute or use this message. If you have received this communication in error, please notify the sender and delete all copies of this message. Accelerite, a Persistent Systems business does not accept any liability for virus infected mails.
Re: Feature proposal: Resource naming policies
With this feature, it is possible to change the names that get sent to the hypervisor, yes. In 4.2 we actually had to fix an issue with the security groups because they weren't parsing properly. That isn't in the commit yet. We will put it in. It would be useful to have a list of all the places that rely on the internal naming convention. security_groups.py and friends is the only one i know of off the top of my head. On Thu, Apr 14, 2016 at 4:01 PM, Koushik Das wrote: > Are these the names that are sent to HV? For e.g. VM name in the format > i-xx-yy-VM. Currently there are functionalities that rely on the internal > naming convention. All such functionalities needs to be handled > appropriately. > > -Koushik > > > From: Jeff Hair > Sent: Thursday, April 14, 2016 5:10 PM > To: dev@cloudstack.apache.org > Subject: Feature proposal: Resource naming policies > > Yesterday, we submitted this pull request: > https://github.com/apache/cloudstack/pull/1492 > > This originally grew out of making the VirtualMachineName class non-static > (original PR is mentioned in the above link). We're presenting this as a > refactoring of the existing code to enable more extensibility and > flexibility, make unit testing easier, and unify the way CloudStack > generates resource names. > > There is an associated JIRA ticket at CLOUDSTACK-9003. I will be writing up > a design document for the CS wiki in the next few days. > > jburwell wanted me to open a discussion on the developer list about the PR > and how it is implemented: > > There is now a ResourceNamingPolicyManager and a bunch of > ResourceNamingPolicies. The manager exposes a method to get a policy for a > type of resource, and the naming policies generate UUIDs + resource names. > > The default naming policies generate names exactly the same way as they are > created now. This is in a new module called default-naming-policies. By > excluding this module and loading our own naming policies, we gain the > ability to change how names are generated. > > > DISCLAIMER > == > This e-mail may contain privileged and confidential information which is > the property of Accelerite, a Persistent Systems business. It is intended > only for the use of the individual or entity to which it is addressed. If > you are not the intended recipient, you are not authorized to read, retain, > copy, print, distribute or use this message. If you have received this > communication in error, please notify the sender and delete all copies of > this message. Accelerite, a Persistent Systems business does not accept any > liability for virus infected mails. >
Re: [GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
On Thu, Apr 14, 2016 at 4:14 PM, ProjectMoon wrote: > Github user ProjectMoon commented on the pull request: > > https://github.com/apache/cloudstack/pull/1492#issuecomment-209963503 > > @DaanHoogland Do you have any kind of integration tests in mind? > I could imagine a policy that keeps a static counter and name a resource bla- and then checks if the next created resource has the right name for each newly created resource; bla-? > > > --- > If your project is set up for it, you can reply to this email and have your > reply appear on GitHub as well. If your project does not have this feature > enabled and wishes so, or if the feature is enabled but not working, please > contact infrastructure at infrastruct...@apache.org or file a JIRA ticket > with INFRA. > --- > -- Daan
[GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/1492#issuecomment-210052430 sorry for not answerring on github I could imagine a policy that keeps a static counter and name a resource bla- and then checks if the next created resource has the right name for each newly created resource; bla- --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
Github user jburwell commented on the pull request: https://github.com/apache/cloudstack/pull/1492#issuecomment-210055791 @DaanHoogland -1 to counters -- the require coordination across all nodes in the cluster. In multi-region configurations, they would require coordination across multi-regions. (Database sequences are incredibly expensive in clustered configurations) We have enough technical debt with our sequence based counters in the database. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/1492#issuecomment-210056555 @jburwell we are talking about integration test code here! not about a production policy. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1331#discussion_r59756105
--- Diff:
services/secondary-storage/server/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java
---
@@ -88,6 +93,19 @@ public void testMount() throws Exception {
}
}
+@Test
+public void testCleanupNfsStaging(){
+TemplateObjectTO templateMock =
Mockito.mock(TemplateObjectTO.class);
+Exception exception = new Exception();
+resource.logger = Mockito.mock(Logger.class);
+
+Mockito.doNothing().when(resource.logger).debug("Failed to clean
up staging area:", exception);
+
Mockito.when(resource.execute(any(DeleteCommand.class))).thenThrow(exception);
--- End diff --
Why are you using Mockito to mock the logger? Why not retrieve the logger
used by the class and add test logger to collect the log message? You can then
ask search the log messages in the appender to see if the log message you
expect is present. This avoids the overhead of Mockito, and much more
straightforward approach to observing that logging is behaving as expected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Taking fast and efficient volume snapshot...
Github user mike-tutkowski commented on the pull request: https://github.com/apache/cloudstack/pull/1403#issuecomment-210066399 @swill Weird, I've compiled ee53705 with the following command on two separate machines and I ran regression tests against that SHA for a few hours yesterday: mvn -P developer,systemvm clean install -D noredist Let me look into this - thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Github user jburwell commented on the pull request: https://github.com/apache/cloudstack/pull/1331#issuecomment-210066343 @rafaelweingartner all statics should ``final`` unless there is explicit handling for their value changing due to thread safety issues. For loggers, they should always be ``private`` and ``static``. Log4j provides the means to retrieve the same logger across threads and contexts by name. Therefore, if you want 5 classes to share the same logger, retrieve the logger for that class by name in each class to avoid the potential pitfalls of static shadowing (i.e. instead of declaring ``private static final Logger LOGGER = Logger.getLogger(MyClass.class)``, declare it as ``private static final Logger LOGGER = Logger.getLogger(CONSTANT_WITH_THE_NAME_OF_ COMMON_LOGGER)``). As I mentioned in a previous comment, these type of compiler gymnastics are not necessary to test log output. Log4j provides the means to attach a custom appender to the logger so that you can monitor and test output. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Taking fast and efficient volume snapshot...
Github user mike-tutkowski commented on the pull request: https://github.com/apache/cloudstack/pull/1403#issuecomment-210072035 @swill This is pretty strange. The code that is failing to compile for you is actually not part of the codebase for ee53705: https://github.com/mike-tutkowski/cloudstack/tree/xs-snapshots/plugins/api All API plug-ins are in the location above and the one failing for you is not in there. What do you think, Will? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Taking fast and efficient volume snapshot...
Github user mike-tutkowski commented on the pull request: https://github.com/apache/cloudstack/pull/1403#issuecomment-210072697 This might be a better link (as it includes the SHA): https://github.com/apache/cloudstack/tree/ee5370536ac3f87457d5f74adbbb8c6af88da449/plugins/api --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1331#discussion_r59762357
--- Diff:
services/secondary-storage/server/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java
---
@@ -88,6 +93,19 @@ public void testMount() throws Exception {
}
}
+@Test
+public void testCleanupNfsStaging(){
+TemplateObjectTO templateMock =
Mockito.mock(TemplateObjectTO.class);
+Exception exception = new Exception();
+resource.logger = Mockito.mock(Logger.class);
+
+Mockito.doNothing().when(resource.logger).debug("Failed to clean
up staging area:", exception);
+
Mockito.when(resource.execute(any(DeleteCommand.class))).thenThrow(exception);
--- End diff --
I didn't use that other approach because I think it adds more
complications. I find it easier to use the Mockito and then just checking if
the method was used. It feels more natural.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Github user rafaelweingartner commented on the pull request: https://github.com/apache/cloudstack/pull/1331#issuecomment-210078579 @jburwell I agree with you that all static can be final. I disagree that loggers should be static, but that is a matter of taste. I understand the cache methods of Log4J, but still I do not like much of using static variables. It feels more natural to use the instance attribute when logging. I find it much more complications to add an appender and then use it to test if some method has been used or some message logged. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
Github user jburwell commented on the pull request: https://github.com/apache/cloudstack/pull/1492#issuecomment-210078861 @DaanHoogland I apologize -- I missed the context. Yes, for an integration test, a counter would be perfectly acceptable, and likely, the best choice to ensure determinism. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [CLOUDSTACK-9003] Resource Naming Policie...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/1492#issuecomment-210081172 np, imagined that --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Taking fast and efficient volume snapshot...
Github user mike-tutkowski commented on the pull request: https://github.com/apache/cloudstack/pull/1403#issuecomment-210081474 @swill I just pushed a new commit to rename the SolidFire integration testing project (which you saw as failing). While that should not have anything to do with a failure, I'd be curious to have you re-run your build because ApiSolidFireServiceImpl (which was failing) should not even be a part of the codebase in that SHA. Thanks! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Github user jburwell commented on the pull request: https://github.com/apache/cloudstack/pull/1331#issuecomment-210081688 @rafaelweingartner The issue is not one of style but performance and availability. The garbage collector skips reachability evaluations of ``final static`` fields where it must perform those for instance variables. This cost adds up when a large number of object is allocated and deallocated. Second, you cannot log from a static method. Our standard in the codebase is to have ``final static`` loggers, and I don't see a compelling reason to step away from it. In terms of the Mockito approach, I wouldn't have an opinion if the unit test where not requiring us to change the class. The test should always be the servant of the system not the other way around. Personally, I find mocks to be an obfuscation especially when the thing being mocked provides a standard mechanism to observe side-effects. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Taking fast and efficient volume snapshot...
Github user mike-tutkowski commented on the pull request: https://github.com/apache/cloudstack/pull/1403#issuecomment-210084608 @swill I think I might know what happened. There used to be a ApiSolidFireServiceImpl.java, but I renamed it in 940d5b. It seems the file is likely still sitting on your filesystem and causing you this trouble. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Taking fast and efficient volume snapshot...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/1403#issuecomment-210085510 i will blow away my git repo and start from scratch and we will go from there. :) thx... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Taking fast and efficient volume snapshot...
Github user mike-tutkowski commented on the pull request: https://github.com/apache/cloudstack/pull/1403#issuecomment-210087782 No problem. :) I had forgotten that I had renamed that file at one point. So, when I saw that you had a file by that name, I was really confused how you would have had access to a file that I had not yet checked into the repo (the current file with that name is for a project I'm still developing and I haven't opened a PR on it yet). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Github user rafaelweingartner commented on the pull request: https://github.com/apache/cloudstack/pull/1331#issuecomment-210086309 I see your point in regards to the garbage collector; but, that class is a singleton, it is not going to be allocated and deallocated. About the point about of using the log in static method; well, that class is a singleton and I believe that they should not have static method, we are using a dependency injection framework, we should wire things up using the lifecycle of the framework we choose and not try to work things out with static methods. Actually I see that the need for unit tests reflects on the code. Code with 50, 100 and even more lines cannot be properly tested. The TDD approach affects a lot on the design and writing of code. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59773107
--- Diff: api/src/org/apache/cloudstack/acl/RoleType.java ---
@@ -16,18 +16,90 @@
// under the License.
package org.apache.cloudstack.acl;
+import com.cloud.user.Account;
+import com.google.common.base.Enums;
+import com.google.common.base.Strings;
+
// Enum for default roles in CloudStack
public enum RoleType {
-Admin(1), ResourceAdmin(2), DomainAdmin(4), User(8), Unknown(0);
+Admin(1L, Account.ACCOUNT_TYPE_ADMIN, 1),
+ResourceAdmin(2L, Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN, 2),
+DomainAdmin(3L, Account.ACCOUNT_TYPE_DOMAIN_ADMIN, 4),
+User(4L, Account.ACCOUNT_TYPE_NORMAL, 8),
+Unknown(-1L, (short) -1, 0);
+private long id;
+private short accountType;
private int mask;
-private RoleType(int mask) {
+RoleType(final long id, final short accountType, final int mask) {
+this.id = id;
+this.accountType = accountType;
this.mask = mask;
}
-public int getValue() {
+public long getId() {
+return id;
+}
+
+public short getAccountType() {
+return accountType;
+}
+
+public int getMask() {
return mask;
}
-}
+public static RoleType fromString(final String name) {
+if (!Strings.isNullOrEmpty(name)
+&& Enums.getIfPresent(RoleType.class, name).isPresent()) {
+return RoleType.valueOf(name);
+}
+return null;
+}
+
+public static RoleType fromMask(int mask) {
+for (RoleType roleType : RoleType.values()) {
+if (roleType.getMask() == mask) {
+return roleType;
+}
+}
+return Unknown;
+}
+
+public static RoleType getByAccountType(final short accountType) {
+RoleType roleType = RoleType.Unknown;
+switch (accountType) {
+case Account.ACCOUNT_TYPE_ADMIN:
+roleType = RoleType.Admin;
+break;
+case Account.ACCOUNT_TYPE_DOMAIN_ADMIN:
+roleType = RoleType.DomainAdmin;
+break;
+case Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN:
+roleType = RoleType.ResourceAdmin;
--- End diff --
All four default cloudstack roles -- Admin, DomainAdmin, ResourceAdmin and
User. It's just an enum class, that maps these role types to account types.
This method previously existed in account mgr impl, I moved it here.
They've always existed. I could not find a previous FS on this, this is the
oldest document on wiki referring to RoleTypes
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Annotations+use+in+the+API
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1493#issuecomment-210103368 I've created two commits to show: (1) test to prove denial of service behavior due to blocking main IO loop, (2) the fix (as mentioned earlier long term fix would require migration to a better framework). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59773313
--- Diff: api/src/org/apache/cloudstack/acl/RoleType.java ---
@@ -16,18 +16,90 @@
// under the License.
package org.apache.cloudstack.acl;
+import com.cloud.user.Account;
+import com.google.common.base.Enums;
+import com.google.common.base.Strings;
+
// Enum for default roles in CloudStack
public enum RoleType {
-Admin(1), ResourceAdmin(2), DomainAdmin(4), User(8), Unknown(0);
+Admin(1L, Account.ACCOUNT_TYPE_ADMIN, 1),
+ResourceAdmin(2L, Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN, 2),
+DomainAdmin(3L, Account.ACCOUNT_TYPE_DOMAIN_ADMIN, 4),
+User(4L, Account.ACCOUNT_TYPE_NORMAL, 8),
+Unknown(-1L, (short) -1, 0);
+private long id;
+private short accountType;
private int mask;
-private RoleType(int mask) {
+RoleType(final long id, final short accountType, final int mask) {
+this.id = id;
+this.accountType = accountType;
this.mask = mask;
}
-public int getValue() {
+public long getId() {
+return id;
+}
+
+public short getAccountType() {
+return accountType;
+}
+
+public int getMask() {
return mask;
}
-}
+public static RoleType fromString(final String name) {
+if (!Strings.isNullOrEmpty(name)
+&& Enums.getIfPresent(RoleType.class, name).isPresent()) {
+return RoleType.valueOf(name);
+}
+return null;
+}
+
+public static RoleType fromMask(int mask) {
+for (RoleType roleType : RoleType.values()) {
+if (roleType.getMask() == mask) {
+return roleType;
+}
+}
+return Unknown;
+}
+
+public static RoleType getByAccountType(final short accountType) {
+RoleType roleType = RoleType.Unknown;
+switch (accountType) {
+case Account.ACCOUNT_TYPE_ADMIN:
+roleType = RoleType.Admin;
+break;
+case Account.ACCOUNT_TYPE_DOMAIN_ADMIN:
+roleType = RoleType.DomainAdmin;
+break;
+case Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN:
+roleType = RoleType.ResourceAdmin;
+break;
+case Account.ACCOUNT_TYPE_NORMAL:
+roleType = RoleType.User;
+break;
+}
+return roleType;
+}
+
+public static Long getRoleByAccountType(final Long roleId, final Short
accountType) {
--- End diff --
This is for doing backward compatiblity in create APIs when roleId passed
is null, but an account type is passed. This does the translation using above
method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user pdube commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59777518
--- Diff: api/src/org/apache/cloudstack/acl/RoleType.java ---
@@ -16,18 +16,90 @@
// under the License.
package org.apache.cloudstack.acl;
+import com.cloud.user.Account;
+import com.google.common.base.Enums;
+import com.google.common.base.Strings;
+
// Enum for default roles in CloudStack
public enum RoleType {
-Admin(1), ResourceAdmin(2), DomainAdmin(4), User(8), Unknown(0);
+Admin(1L, Account.ACCOUNT_TYPE_ADMIN, 1),
+ResourceAdmin(2L, Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN, 2),
+DomainAdmin(3L, Account.ACCOUNT_TYPE_DOMAIN_ADMIN, 4),
+User(4L, Account.ACCOUNT_TYPE_NORMAL, 8),
+Unknown(-1L, (short) -1, 0);
+private long id;
+private short accountType;
private int mask;
-private RoleType(int mask) {
+RoleType(final long id, final short accountType, final int mask) {
+this.id = id;
+this.accountType = accountType;
this.mask = mask;
}
-public int getValue() {
+public long getId() {
+return id;
+}
+
+public short getAccountType() {
+return accountType;
+}
+
+public int getMask() {
return mask;
}
-}
+public static RoleType fromString(final String name) {
+if (!Strings.isNullOrEmpty(name)
+&& Enums.getIfPresent(RoleType.class, name).isPresent()) {
+return RoleType.valueOf(name);
+}
+return null;
+}
+
+public static RoleType fromMask(int mask) {
+for (RoleType roleType : RoleType.values()) {
+if (roleType.getMask() == mask) {
+return roleType;
+}
+}
+return Unknown;
+}
+
+public static RoleType getByAccountType(final short accountType) {
+RoleType roleType = RoleType.Unknown;
+switch (accountType) {
+case Account.ACCOUNT_TYPE_ADMIN:
+roleType = RoleType.Admin;
+break;
+case Account.ACCOUNT_TYPE_DOMAIN_ADMIN:
+roleType = RoleType.DomainAdmin;
+break;
+case Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN:
+roleType = RoleType.ResourceAdmin;
--- End diff --
I meant the ResourceAdmin specifically. What is this role? Is there any
documentation on it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59785020
--- Diff: api/src/org/apache/cloudstack/acl/RoleType.java ---
@@ -16,18 +16,90 @@
// under the License.
package org.apache.cloudstack.acl;
+import com.cloud.user.Account;
+import com.google.common.base.Enums;
+import com.google.common.base.Strings;
+
// Enum for default roles in CloudStack
public enum RoleType {
-Admin(1), ResourceAdmin(2), DomainAdmin(4), User(8), Unknown(0);
+Admin(1L, Account.ACCOUNT_TYPE_ADMIN, 1),
+ResourceAdmin(2L, Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN, 2),
+DomainAdmin(3L, Account.ACCOUNT_TYPE_DOMAIN_ADMIN, 4),
+User(4L, Account.ACCOUNT_TYPE_NORMAL, 8),
+Unknown(-1L, (short) -1, 0);
+private long id;
+private short accountType;
private int mask;
-private RoleType(int mask) {
+RoleType(final long id, final short accountType, final int mask) {
+this.id = id;
+this.accountType = accountType;
this.mask = mask;
}
-public int getValue() {
+public long getId() {
+return id;
+}
+
+public short getAccountType() {
+return accountType;
+}
+
+public int getMask() {
return mask;
}
-}
+public static RoleType fromString(final String name) {
+if (!Strings.isNullOrEmpty(name)
+&& Enums.getIfPresent(RoleType.class, name).isPresent()) {
+return RoleType.valueOf(name);
+}
+return null;
+}
+
+public static RoleType fromMask(int mask) {
+for (RoleType roleType : RoleType.values()) {
+if (roleType.getMask() == mask) {
+return roleType;
+}
+}
+return Unknown;
+}
+
+public static RoleType getByAccountType(final short accountType) {
+RoleType roleType = RoleType.Unknown;
+switch (accountType) {
+case Account.ACCOUNT_TYPE_ADMIN:
+roleType = RoleType.Admin;
+break;
+case Account.ACCOUNT_TYPE_DOMAIN_ADMIN:
+roleType = RoleType.DomainAdmin;
+break;
+case Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN:
+roleType = RoleType.ResourceAdmin;
--- End diff --
@pdube I think this was used to support cpbm-cloudstack integration, I
don't know any documentation; please google? For backward compatibility
reasons, all four roles types supported by static-role-based-checked (as in
commands.properties) need to be supported.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59786533
--- Diff: api/src/org/apache/cloudstack/acl/Rule.java ---
@@ -0,0 +1,42 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.acl;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.google.common.base.Strings;
+
+public final class Rule {
+private final String rule;
+private final static String ALLOWED_PATTERN = "^[a-zA-Z0-9*]+$";
+
+public Rule(final String rule) {
+validate(rule);
+this.rule = rule;
+}
+
+public String toString() {
+return rule;
+}
+
+public static boolean validate(final String rule) throws
InvalidParameterValueException {
+if (Strings.isNullOrEmpty(rule) || !rule.matches(ALLOWED_PATTERN))
{
--- End diff --
Extract this validation logic into an immutable value class, ``Rule``, to
encapsulate this validation logic and provide strong type specification in
lower layers.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59787000
--- Diff: api/src/org/apache/cloudstack/acl/Rule.java ---
@@ -0,0 +1,42 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.acl;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.google.common.base.Strings;
+
+public final class Rule {
+private final String rule;
+private final static String ALLOWED_PATTERN = "^[a-zA-Z0-9*]+$";
--- End diff --
Aren't wildcard characters were only allowed as the first or last character
in the rule specification? If so, then this regex needs to be tightened up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59787197
--- Diff:
api/src/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java
---
@@ -196,5 +204,8 @@ private void validateParams() {
if(StringUtils.isEmpty(getPassword())) {
throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Empty
passwords are not allowed");
}
+if (getAccountType() == null && getRoleId() == null) {
--- End diff --
Consider splitting these checks to give a more precise error message to the
end user. Also, shouldn't check that ``roleId`` is greater than ``0L``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59788370
--- Diff:
api/src/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java ---
@@ -0,0 +1,104 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.command.admin.acl;
+
+import com.cloud.user.Account;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.acl.Role;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.RoleResponse;
+import org.apache.cloudstack.context.CallContext;
+
+@APICommand(name = CreateRoleCmd.APINAME, description = "Creates a role",
responseObject = RoleResponse.class,
+requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
+since = "4.9.0",
+authorized = {RoleType.Admin})
+public class CreateRoleCmd extends BaseCmd {
+public static final String APINAME = "createRole";
+
+/
+ API parameters /
+/
+
+@Parameter(name = ApiConstants.NAME, type = CommandType.STRING,
required = true, description = "creates a role with this unique name")
+private String roleName;
+
+@Parameter(name = ApiConstants.TYPE, type = CommandType.STRING,
required = true, description = "The type of the role, valid options are: Admin,
ResourceAdmin, DomainAdmin, User")
+private String roleType;
+
+@Parameter(name = ApiConstants.DESCRIPTION, type = CommandType.STRING,
description = "The description of the role")
+private String roleDescription;
+
+/
+/// Accessors ///
+/
+
+public String getRoleName() {
+return roleName;
+}
+
+public RoleType getRoleType() {
+return RoleType.fromString(roleType);
+}
+
+public String getRoleDescription() {
+return roleDescription;
+}
+
+/
+/// API Implementation///
+/
+
+@Override
+public String getCommandName() {
+return APINAME.toLowerCase() + BaseCmd.RESPONSE_SUFFIX;
+}
+
+@Override
+public long getEntityOwnerId() {
+return Account.ACCOUNT_ID_SYSTEM;
+}
+
+@Override
+public void execute() {
+if (Strings.isNullOrEmpty(getRoleName())) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Empty
role name provided");
+}
+if (getRoleType() == null || getRoleType() == RoleType.Unknown) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
"Invalid role type provided");
+}
--- End diff --
Consider extracting this validation logic to a private method to make
things more readable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59788517
--- Diff:
api/src/org/apache/cloudstack/api/command/admin/acl/DeleteRoleCmd.java ---
@@ -0,0 +1,84 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.command.admin.acl;
+
+import com.cloud.user.Account;
+import org.apache.cloudstack.acl.Role;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.RoleResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.cloudstack.context.CallContext;
+
+@APICommand(name = DeleteRoleCmd.APINAME, description = "Deletes a role",
responseObject = SuccessResponse.class,
+requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
+since = "4.9.0",
+authorized = {RoleType.Admin})
+public class DeleteRoleCmd extends BaseCmd {
+public static final String APINAME = "deleteRole";
+
+/
+ API parameters /
+/
+
+@Parameter(name = ApiConstants.ID, type = BaseCmd.CommandType.UUID,
required = true, entityType = RoleResponse.class, description = "ID of the
role")
+private Long roleId;
+
+/
+/// Accessors ///
+/
+
+public Long getRoleId() {
+return roleId;
+}
+
+/
+/// API Implementation///
+/
+
+@Override
+public String getCommandName() {
+return APINAME.toLowerCase() + BaseCmd.RESPONSE_SUFFIX;
+}
+
+@Override
+public long getEntityOwnerId() {
+return Account.ACCOUNT_ID_SYSTEM;
+}
+
+@Override
+public void execute() {
+if (getRoleId() == null || getRoleId() < 1L) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
"Invalid role id provided");
+}
+Role role = roleService.findRole(getRoleId());
+if (role == null) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
"Invalid role id provided");
+}
--- End diff --
Consider extracting this validation logic to a private method to make
things more readable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59788566
--- Diff:
api/src/org/apache/cloudstack/api/command/admin/acl/DeleteRolePermissionCmd.java
---
@@ -0,0 +1,84 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.command.admin.acl;
+
+import com.cloud.user.Account;
+import org.apache.cloudstack.acl.RolePermission;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.RolePermissionResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.cloudstack.context.CallContext;
+
+@APICommand(name = DeleteRolePermissionCmd.APINAME, description = "Deletes
a role permission", responseObject = SuccessResponse.class,
+requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
+since = "4.9.0",
+authorized = {RoleType.Admin})
+public class DeleteRolePermissionCmd extends BaseCmd {
+public static final String APINAME = "deleteRolePermission";
+
+/
+ API parameters /
+/
+
+@Parameter(name = ApiConstants.ID, type = BaseCmd.CommandType.UUID,
required = true, entityType = RolePermissionResponse.class, description = "ID
of the role permission")
+private Long rolePermissionId;
+
+/
+/// Accessors ///
+/
+
+public Long getRolePermissionId() {
+return rolePermissionId;
+}
+
+/
+/// API Implementation///
+/
+
+@Override
+public String getCommandName() {
+return APINAME.toLowerCase() + BaseCmd.RESPONSE_SUFFIX;
+}
+
+@Override
+public long getEntityOwnerId() {
+return Account.ACCOUNT_ID_SYSTEM;
+}
+
+@Override
+public void execute() {
+if (getRolePermissionId() == null || getRolePermissionId() < 1L) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
"Invalid role permission id provided");
+}
+RolePermission rolePermission =
roleService.findRolePermission(getRolePermissionId());
+if (rolePermission == null) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
"Invalid role permission id provided");
+}
--- End diff --
Consider extracting this validation logic to a private method to make
things more readable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59790778
--- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java ---
@@ -19,146 +19,198 @@
package com.cloud.utils.testcase;
-import java.nio.channels.ClosedChannelException;
-import java.util.Random;
-
-import junit.framework.TestCase;
-
-import org.apache.log4j.Logger;
-import org.junit.Assert;
-
+import com.cloud.utils.concurrency.NamedThreadFactory;
import com.cloud.utils.exception.NioConnectionException;
import com.cloud.utils.nio.HandlerFactory;
import com.cloud.utils.nio.Link;
import com.cloud.utils.nio.NioClient;
import com.cloud.utils.nio.NioServer;
import com.cloud.utils.nio.Task;
import com.cloud.utils.nio.Task.Type;
+import org.apache.log4j.Logger;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
-/**
- *
- *
- *
- *
- */
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.Selector;
+import java.nio.channels.SocketChannel;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+public class NioTest {
-public class NioTest extends TestCase {
+private static final Logger LOGGER = Logger.getLogger(NioTest.class);
-private static final Logger s_logger = Logger.getLogger(NioTest.class);
+final private int totalTestCount = 10;
+private int completedTestCount = 0;
-private NioServer _server;
-private NioClient _client;
+private NioServer server;
+private List clients = new ArrayList<>();
+private List maliciousClients = new ArrayList<>();
-private Link _clientLink;
+private ExecutorService clientExecutor =
Executors.newFixedThreadPool(totalTestCount, new
NamedThreadFactory("NioClientHandler"));;
+private ExecutorService maliciousExecutor =
Executors.newFixedThreadPool(5*totalTestCount, new
NamedThreadFactory("MaliciousNioClientHandler"));;
-private int _testCount;
-private int _completedCount;
+private Random randomGenerator = new Random();
+private byte[] testBytes;
private boolean isTestsDone() {
boolean result;
synchronized (this) {
-result = _testCount == _completedCount;
+result = totalTestCount == completedTestCount;
}
return result;
}
-private void getOneMoreTest() {
-synchronized (this) {
-_testCount++;
-}
-}
-
private void oneMoreTestDone() {
synchronized (this) {
-_completedCount++;
+completedTestCount++;
}
}
-@Override
+@Before
public void setUp() {
-s_logger.info("Test");
+LOGGER.info("Setting up Benchmark Test");
-_testCount = 0;
-_completedCount = 0;
-
-_server = new NioServer("NioTestServer", , 5, new
NioTestServer());
-try {
-_server.start();
-} catch (final NioConnectionException e) {
-fail(e.getMessage());
-}
+completedTestCount = 0;
+testBytes = new byte[100];
+randomGenerator.nextBytes(testBytes);
-_client = new NioClient("NioTestServer", "127.0.0.1", , 5, new
NioTestClient());
+// Server configured with one worker
+server = new NioServer("NioTestServer", , 1, new
NioTestServer());
try {
-_client.start();
+server.start();
} catch (final NioConnectionException e) {
-fail(e.getMessage());
+Assert.fail(e.getMessage());
}
-while (_clientLink == null) {
-try {
-s_logger.debug("Link is not up! Waiting ...");
-Thread.sleep(1000);
-} catch (final InterruptedException e) {
-// TODO Auto-generated catch block
-e.printStackTrace();
+// 5 malicious clients per valid client
+for (int i = 0; i < totalTestCount; i++) {
+for (int j = 0; j < 5; j++) {
+final NioClient maliciousClient = new
NioMaliciousClient("NioMaliciousTestClient-" + i, "127.0.0.1", , 1, new
NioMaliciousTestClient());
+maliciousClients.add(maliciousClient);
+
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59791280
--- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java ---
@@ -19,146 +19,198 @@
package com.cloud.utils.testcase;
-import java.nio.channels.ClosedChannelException;
-import java.util.Random;
-
-import junit.framework.TestCase;
-
-import org.apache.log4j.Logger;
-import org.junit.Assert;
-
+import com.cloud.utils.concurrency.NamedThreadFactory;
import com.cloud.utils.exception.NioConnectionException;
import com.cloud.utils.nio.HandlerFactory;
import com.cloud.utils.nio.Link;
import com.cloud.utils.nio.NioClient;
import com.cloud.utils.nio.NioServer;
import com.cloud.utils.nio.Task;
import com.cloud.utils.nio.Task.Type;
+import org.apache.log4j.Logger;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
-/**
- *
- *
- *
- *
- */
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.Selector;
+import java.nio.channels.SocketChannel;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+public class NioTest {
-public class NioTest extends TestCase {
+private static final Logger LOGGER = Logger.getLogger(NioTest.class);
-private static final Logger s_logger = Logger.getLogger(NioTest.class);
+final private int totalTestCount = 10;
+private int completedTestCount = 0;
-private NioServer _server;
-private NioClient _client;
+private NioServer server;
+private List clients = new ArrayList<>();
+private List maliciousClients = new ArrayList<>();
-private Link _clientLink;
+private ExecutorService clientExecutor =
Executors.newFixedThreadPool(totalTestCount, new
NamedThreadFactory("NioClientHandler"));;
+private ExecutorService maliciousExecutor =
Executors.newFixedThreadPool(5*totalTestCount, new
NamedThreadFactory("MaliciousNioClientHandler"));;
-private int _testCount;
-private int _completedCount;
+private Random randomGenerator = new Random();
+private byte[] testBytes;
private boolean isTestsDone() {
boolean result;
synchronized (this) {
-result = _testCount == _completedCount;
+result = totalTestCount == completedTestCount;
}
return result;
}
-private void getOneMoreTest() {
-synchronized (this) {
-_testCount++;
-}
-}
-
private void oneMoreTestDone() {
synchronized (this) {
-_completedCount++;
+completedTestCount++;
}
}
-@Override
+@Before
public void setUp() {
-s_logger.info("Test");
+LOGGER.info("Setting up Benchmark Test");
-_testCount = 0;
-_completedCount = 0;
-
-_server = new NioServer("NioTestServer", , 5, new
NioTestServer());
-try {
-_server.start();
-} catch (final NioConnectionException e) {
-fail(e.getMessage());
-}
+completedTestCount = 0;
+testBytes = new byte[100];
+randomGenerator.nextBytes(testBytes);
-_client = new NioClient("NioTestServer", "127.0.0.1", , 5, new
NioTestClient());
+// Server configured with one worker
+server = new NioServer("NioTestServer", , 1, new
NioTestServer());
try {
-_client.start();
+server.start();
} catch (final NioConnectionException e) {
-fail(e.getMessage());
+Assert.fail(e.getMessage());
}
-while (_clientLink == null) {
-try {
-s_logger.debug("Link is not up! Waiting ...");
-Thread.sleep(1000);
-} catch (final InterruptedException e) {
-// TODO Auto-generated catch block
-e.printStackTrace();
+// 5 malicious clients per valid client
+for (int i = 0; i < totalTestCount; i++) {
+for (int j = 0; j < 5; j++) {
+final NioClient maliciousClient = new
NioMaliciousClient("NioMaliciousTestClient-" + i, "127.0.0.1", , 1, new
NioMaliciousTestClient());
+maliciousClients.add(maliciousClient);
+
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59792529
--- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java ---
@@ -19,146 +19,198 @@
package com.cloud.utils.testcase;
-import java.nio.channels.ClosedChannelException;
-import java.util.Random;
-
-import junit.framework.TestCase;
-
-import org.apache.log4j.Logger;
-import org.junit.Assert;
-
+import com.cloud.utils.concurrency.NamedThreadFactory;
import com.cloud.utils.exception.NioConnectionException;
import com.cloud.utils.nio.HandlerFactory;
import com.cloud.utils.nio.Link;
import com.cloud.utils.nio.NioClient;
import com.cloud.utils.nio.NioServer;
import com.cloud.utils.nio.Task;
import com.cloud.utils.nio.Task.Type;
+import org.apache.log4j.Logger;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
-/**
- *
- *
- *
- *
- */
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.Selector;
+import java.nio.channels.SocketChannel;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+public class NioTest {
-public class NioTest extends TestCase {
+private static final Logger LOGGER = Logger.getLogger(NioTest.class);
-private static final Logger s_logger = Logger.getLogger(NioTest.class);
+final private int totalTestCount = 10;
+private int completedTestCount = 0;
-private NioServer _server;
-private NioClient _client;
+private NioServer server;
+private List clients = new ArrayList<>();
+private List maliciousClients = new ArrayList<>();
-private Link _clientLink;
+private ExecutorService clientExecutor =
Executors.newFixedThreadPool(totalTestCount, new
NamedThreadFactory("NioClientHandler"));;
+private ExecutorService maliciousExecutor =
Executors.newFixedThreadPool(5*totalTestCount, new
NamedThreadFactory("MaliciousNioClientHandler"));;
-private int _testCount;
-private int _completedCount;
+private Random randomGenerator = new Random();
+private byte[] testBytes;
private boolean isTestsDone() {
boolean result;
synchronized (this) {
-result = _testCount == _completedCount;
+result = totalTestCount == completedTestCount;
}
return result;
}
-private void getOneMoreTest() {
-synchronized (this) {
-_testCount++;
-}
-}
-
private void oneMoreTestDone() {
synchronized (this) {
-_completedCount++;
+completedTestCount++;
}
}
-@Override
+@Before
public void setUp() {
-s_logger.info("Test");
+LOGGER.info("Setting up Benchmark Test");
-_testCount = 0;
-_completedCount = 0;
-
-_server = new NioServer("NioTestServer", , 5, new
NioTestServer());
-try {
-_server.start();
-} catch (final NioConnectionException e) {
-fail(e.getMessage());
-}
+completedTestCount = 0;
+testBytes = new byte[100];
+randomGenerator.nextBytes(testBytes);
-_client = new NioClient("NioTestServer", "127.0.0.1", , 5, new
NioTestClient());
+// Server configured with one worker
+server = new NioServer("NioTestServer", , 1, new
NioTestServer());
try {
-_client.start();
+server.start();
} catch (final NioConnectionException e) {
-fail(e.getMessage());
+Assert.fail(e.getMessage());
}
-while (_clientLink == null) {
-try {
-s_logger.debug("Link is not up! Waiting ...");
-Thread.sleep(1000);
-} catch (final InterruptedException e) {
-// TODO Auto-generated catch block
-e.printStackTrace();
+// 5 malicious clients per valid client
+for (int i = 0; i < totalTestCount; i++) {
+for (int j = 0; j < 5; j++) {
+final NioClient maliciousClient = new
NioMaliciousClient("NioMaliciousTestClient-" + i, "127.0.0.1", , 1, new
NioMaliciousTestClient());
+maliciousClients.add(maliciousClient);
+
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59795416
--- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java ---
@@ -19,146 +19,198 @@
package com.cloud.utils.testcase;
-import java.nio.channels.ClosedChannelException;
-import java.util.Random;
-
-import junit.framework.TestCase;
-
-import org.apache.log4j.Logger;
-import org.junit.Assert;
-
+import com.cloud.utils.concurrency.NamedThreadFactory;
import com.cloud.utils.exception.NioConnectionException;
import com.cloud.utils.nio.HandlerFactory;
import com.cloud.utils.nio.Link;
import com.cloud.utils.nio.NioClient;
import com.cloud.utils.nio.NioServer;
import com.cloud.utils.nio.Task;
import com.cloud.utils.nio.Task.Type;
+import org.apache.log4j.Logger;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
-/**
- *
- *
- *
- *
- */
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.Selector;
+import java.nio.channels.SocketChannel;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+public class NioTest {
-public class NioTest extends TestCase {
+private static final Logger LOGGER = Logger.getLogger(NioTest.class);
-private static final Logger s_logger = Logger.getLogger(NioTest.class);
+final private int totalTestCount = 10;
+private int completedTestCount = 0;
-private NioServer _server;
-private NioClient _client;
+private NioServer server;
+private List clients = new ArrayList<>();
+private List maliciousClients = new ArrayList<>();
-private Link _clientLink;
+private ExecutorService clientExecutor =
Executors.newFixedThreadPool(totalTestCount, new
NamedThreadFactory("NioClientHandler"));;
+private ExecutorService maliciousExecutor =
Executors.newFixedThreadPool(5*totalTestCount, new
NamedThreadFactory("MaliciousNioClientHandler"));;
-private int _testCount;
-private int _completedCount;
+private Random randomGenerator = new Random();
+private byte[] testBytes;
private boolean isTestsDone() {
boolean result;
synchronized (this) {
-result = _testCount == _completedCount;
+result = totalTestCount == completedTestCount;
}
return result;
}
-private void getOneMoreTest() {
-synchronized (this) {
-_testCount++;
-}
-}
-
private void oneMoreTestDone() {
synchronized (this) {
-_completedCount++;
+completedTestCount++;
}
}
-@Override
+@Before
public void setUp() {
-s_logger.info("Test");
+LOGGER.info("Setting up Benchmark Test");
-_testCount = 0;
-_completedCount = 0;
-
-_server = new NioServer("NioTestServer", , 5, new
NioTestServer());
-try {
-_server.start();
-} catch (final NioConnectionException e) {
-fail(e.getMessage());
-}
+completedTestCount = 0;
+testBytes = new byte[100];
+randomGenerator.nextBytes(testBytes);
-_client = new NioClient("NioTestServer", "127.0.0.1", , 5, new
NioTestClient());
+// Server configured with one worker
+server = new NioServer("NioTestServer", , 1, new
NioTestServer());
try {
-_client.start();
+server.start();
} catch (final NioConnectionException e) {
-fail(e.getMessage());
+Assert.fail(e.getMessage());
}
-while (_clientLink == null) {
-try {
-s_logger.debug("Link is not up! Waiting ...");
-Thread.sleep(1000);
-} catch (final InterruptedException e) {
-// TODO Auto-generated catch block
-e.printStackTrace();
+// 5 malicious clients per valid client
+for (int i = 0; i < totalTestCount; i++) {
+for (int j = 0; j < 5; j++) {
+final NioClient maliciousClient = new
NioMaliciousClient("NioMaliciousTestClient-" + i, "127.0.0.1", , 1, new
NioMaliciousTestClient());
+maliciousClients.add(maliciousClient);
+
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59797686
--- Diff:
api/src/org/apache/cloudstack/api/command/admin/acl/ListRolePermissionsCmd.java
---
@@ -0,0 +1,104 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.command.admin.acl;
+
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.user.Account;
+import org.apache.cloudstack.acl.Role;
+import org.apache.cloudstack.acl.RolePermission;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.ListResponse;
+import org.apache.cloudstack.api.response.RolePermissionResponse;
+import org.apache.cloudstack.api.response.RoleResponse;
+
+import java.util.ArrayList;
+import java.util.List;
+
+
+@APICommand(name = ListRolePermissionsCmd.APINAME, description = "Lists
role permissions", responseObject = RolePermissionResponse.class,
+requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
+since = "4.9.0",
+authorized = {RoleType.Admin})
+public class ListRolePermissionsCmd extends BaseCmd {
+public static final String APINAME = "listRolePermissions";
+
+/
+ API parameters /
+/
+
+@Parameter(name = ApiConstants.ROLE_ID, type = CommandType.UUID,
entityType = RoleResponse.class, description = "ID of the role")
+private Long roleId;
+
+/
+/// Accessors ///
+/
+
+public Long getRoleId() {
+return roleId;
+}
+
+/
+/// API Implementation///
+/
+
+@Override
+public String getCommandName() {
+return APINAME.toLowerCase() + BaseCmd.RESPONSE_SUFFIX;
+}
+
+@Override
+public long getEntityOwnerId() {
+return Account.ACCOUNT_ID_SYSTEM;
+}
+
+@Override
+public void execute() throws ResourceUnavailableException,
InsufficientCapacityException, ServerApiException {
+if (getRoleId() != null && getRoleId() < 1L) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
"Invalid role id provided");
+}
+List rolePermissions =
roleService.findAllPermissionsBy(getRoleId());
+
+ListResponse response = new
ListResponse<>();
+List rolePermissionResponses = new
ArrayList<>();
+for (RolePermission rolePermission : rolePermissions) {
+Role role = roleService.findRole(rolePermission.getRoleId());
--- End diff --
Am I correct in understanding that all the permissions in
``rolePerimssions`` will be for ``roleId``? If so, why do we retrieve the same
role for every permission evaluated? Why not retrieve the role once between
the for loop? A step further, why not model ``RolePermission`` to have an
association with ``Role`` and retrieve and associate it in
``roleService.findAllPermissionsBy``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59799743
--- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java ---
@@ -19,146 +19,198 @@
package com.cloud.utils.testcase;
-import java.nio.channels.ClosedChannelException;
-import java.util.Random;
-
-import junit.framework.TestCase;
-
-import org.apache.log4j.Logger;
-import org.junit.Assert;
-
+import com.cloud.utils.concurrency.NamedThreadFactory;
import com.cloud.utils.exception.NioConnectionException;
import com.cloud.utils.nio.HandlerFactory;
import com.cloud.utils.nio.Link;
import com.cloud.utils.nio.NioClient;
import com.cloud.utils.nio.NioServer;
import com.cloud.utils.nio.Task;
import com.cloud.utils.nio.Task.Type;
+import org.apache.log4j.Logger;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
-/**
- *
- *
- *
- *
- */
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.Selector;
+import java.nio.channels.SocketChannel;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+public class NioTest {
-public class NioTest extends TestCase {
+private static final Logger LOGGER = Logger.getLogger(NioTest.class);
-private static final Logger s_logger = Logger.getLogger(NioTest.class);
+final private int totalTestCount = 10;
+private int completedTestCount = 0;
-private NioServer _server;
-private NioClient _client;
+private NioServer server;
+private List clients = new ArrayList<>();
+private List maliciousClients = new ArrayList<>();
-private Link _clientLink;
+private ExecutorService clientExecutor =
Executors.newFixedThreadPool(totalTestCount, new
NamedThreadFactory("NioClientHandler"));;
+private ExecutorService maliciousExecutor =
Executors.newFixedThreadPool(5*totalTestCount, new
NamedThreadFactory("MaliciousNioClientHandler"));;
-private int _testCount;
-private int _completedCount;
+private Random randomGenerator = new Random();
+private byte[] testBytes;
private boolean isTestsDone() {
boolean result;
synchronized (this) {
-result = _testCount == _completedCount;
+result = totalTestCount == completedTestCount;
}
return result;
}
-private void getOneMoreTest() {
-synchronized (this) {
-_testCount++;
-}
-}
-
private void oneMoreTestDone() {
synchronized (this) {
-_completedCount++;
+completedTestCount++;
}
}
-@Override
+@Before
public void setUp() {
-s_logger.info("Test");
+LOGGER.info("Setting up Benchmark Test");
-_testCount = 0;
-_completedCount = 0;
-
-_server = new NioServer("NioTestServer", , 5, new
NioTestServer());
-try {
-_server.start();
-} catch (final NioConnectionException e) {
-fail(e.getMessage());
-}
+completedTestCount = 0;
+testBytes = new byte[100];
+randomGenerator.nextBytes(testBytes);
-_client = new NioClient("NioTestServer", "127.0.0.1", , 5, new
NioTestClient());
+// Server configured with one worker
+server = new NioServer("NioTestServer", , 1, new
NioTestServer());
try {
-_client.start();
+server.start();
} catch (final NioConnectionException e) {
-fail(e.getMessage());
+Assert.fail(e.getMessage());
}
-while (_clientLink == null) {
-try {
-s_logger.debug("Link is not up! Waiting ...");
-Thread.sleep(1000);
-} catch (final InterruptedException e) {
-// TODO Auto-generated catch block
-e.printStackTrace();
+// 5 malicious clients per valid client
+for (int i = 0; i < totalTestCount; i++) {
+for (int j = 0; j < 5; j++) {
+final NioClient maliciousClient = new
NioMaliciousClient("NioMaliciousTestClient-" + i, "127.0.0.1", , 1, new
NioMaliciousTestClient());
+maliciousClients.add(maliciousClient);
+
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59800096
--- Diff:
api/src/org/apache/cloudstack/api/command/admin/config/UpdateCfgCmd.java ---
@@ -117,6 +123,12 @@ public long getEntityOwnerId() {
@Override
public void execute() {
+if (Strings.isNullOrEmpty(getCfgName())) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Empty
configuration name provided");
+}
+if
(getCfgName().equalsIgnoreCase(RoleService.EnableDynamicApiChecker.key())) {
+throw new ServerApiException(ApiErrorCode.METHOD_NOT_ALLOWED,
"Restricted configuration update not allowed");
--- End diff --
Method Not Allowed (405) indicates the caller attempted to use an HTTP
method (i.e. GET, PUT, POST, DELETE) that is not allowed for the resource. In
this case, the method is allowed, but the parameters do not all the method to
be performed. Seems like ``PARAM_ERROR`` would be more appropriate.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59800354
--- Diff: api/test/org/apache/cloudstack/acl/RoleTypeTest.java ---
@@ -0,0 +1,92 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.acl;
+
+import com.cloud.user.Account;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.Arrays;
+
+public class RoleTypeTest {
+
+@Test
+public void testRoleTypeFromString() {
+Assert.assertEquals(RoleType.fromString(null), null);
+Assert.assertEquals(RoleType.fromString(""), null);
+Assert.assertEquals(RoleType.fromString("admin"), null);
+Assert.assertEquals(RoleType.fromString("12345%^&*"), null);
--- End diff --
Why doesn't ``fromString`` should either return ``Unknown`` or throw an
``IllegalStateException`` in these scenarios?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59800553
--- Diff: api/test/org/apache/cloudstack/acl/RoleTypeTest.java ---
@@ -0,0 +1,92 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.acl;
+
+import com.cloud.user.Account;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.Arrays;
+
+public class RoleTypeTest {
+
+@Test
+public void testRoleTypeFromString() {
+Assert.assertEquals(RoleType.fromString(null), null);
+Assert.assertEquals(RoleType.fromString(""), null);
+Assert.assertEquals(RoleType.fromString("admin"), null);
+Assert.assertEquals(RoleType.fromString("12345%^&*"), null);
+for (RoleType roleType : RoleType.values()) {
+Assert.assertEquals(RoleType.fromString(roleType.name()),
roleType);
+}
+}
+
+@Test
+public void testDefaultRoleMaskByValue() {
+Assert.assertEquals(RoleType.fromMask(1), RoleType.Admin);
+Assert.assertEquals(RoleType.fromMask(2), RoleType.ResourceAdmin);
+Assert.assertEquals(RoleType.fromMask(4), RoleType.DomainAdmin);
+Assert.assertEquals(RoleType.fromMask(8), RoleType.User);
+Assert.assertEquals(RoleType.fromMask(0), RoleType.Unknown);
--- End diff --
What happens if ``fromMask`` is passed unexpected integer values?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59800725
--- Diff: api/test/org/apache/cloudstack/acl/RoleTypeTest.java ---
@@ -0,0 +1,92 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.acl;
+
+import com.cloud.user.Account;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.Arrays;
+
+public class RoleTypeTest {
+
+@Test
+public void testRoleTypeFromString() {
+Assert.assertEquals(RoleType.fromString(null), null);
+Assert.assertEquals(RoleType.fromString(""), null);
+Assert.assertEquals(RoleType.fromString("admin"), null);
+Assert.assertEquals(RoleType.fromString("12345%^&*"), null);
+for (RoleType roleType : RoleType.values()) {
+Assert.assertEquals(RoleType.fromString(roleType.name()),
roleType);
+}
+}
+
+@Test
+public void testDefaultRoleMaskByValue() {
+Assert.assertEquals(RoleType.fromMask(1), RoleType.Admin);
+Assert.assertEquals(RoleType.fromMask(2), RoleType.ResourceAdmin);
+Assert.assertEquals(RoleType.fromMask(4), RoleType.DomainAdmin);
+Assert.assertEquals(RoleType.fromMask(8), RoleType.User);
+Assert.assertEquals(RoleType.fromMask(0), RoleType.Unknown);
+}
+
+@Test
+public void testGetByAccountType() {
+
Assert.assertEquals(RoleType.getByAccountType(Account.ACCOUNT_TYPE_NORMAL),
RoleType.User);
+
Assert.assertEquals(RoleType.getByAccountType(Account.ACCOUNT_TYPE_ADMIN),
RoleType.Admin);
+
Assert.assertEquals(RoleType.getByAccountType(Account.ACCOUNT_TYPE_DOMAIN_ADMIN),
RoleType.DomainAdmin);
+
Assert.assertEquals(RoleType.getByAccountType(Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN),
RoleType.ResourceAdmin);
+
Assert.assertEquals(RoleType.getByAccountType(Account.ACCOUNT_TYPE_PROJECT),
RoleType.Unknown);
+}
+
+@Test
+public void testGetRoleByAccountTypeWhenRoleIdIsProvided() {
+Assert.assertEquals(RoleType.getRoleByAccountType(123L,
Account.ACCOUNT_TYPE_ADMIN), Long.valueOf(123L));
+Assert.assertEquals(RoleType.getRoleByAccountType(1234L, null),
Long.valueOf(1234L));
+}
+
+@Test
+public void testGetRoleByAccountTypeForDefaultAccountTypes() {
+Assert.assertEquals(RoleType.getRoleByAccountType(null,
Account.ACCOUNT_TYPE_ADMIN), (Long) RoleType.Admin.getId());
--- End diff --
Passing ``null`` as a parameter to a method as a flag is an anti-pattern.
Create a two parameter override of ``getRoleByAccountType`` to cover this
scenario.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59800993
--- Diff: api/test/org/apache/cloudstack/acl/RuleTest.java ---
@@ -0,0 +1,39 @@
+package org.apache.cloudstack.acl;
+
+import com.cloud.exception.InvalidParameterValueException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Arrays;
+
+public class RuleTest {
+
+@Test
+public void testToString() throws Exception {
+Rule rule = new Rule("someString");
+Assert.assertEquals(rule.toString(), "someString");
+}
+
+@Test
+public void testValidateRuleWithValidData() throws Exception {
+for (String rule : Arrays.asList("a", "1", "someApi",
"someApi321", "123SomeApi",
+"prefix*", "*middle*", "*Suffix",
+"*", "**", "f***", "m0nk3yMa**g1c*")) {
+Assert.assertTrue(Rule.validate(rule));
+Assert.assertEquals(new Rule(rule).toString(), rule);
+}
+}
+
+@Test
+public void testValidateRuleWithInvalidData() throws Exception {
+for (String rule : Arrays.asList(null, "", " ", " ", "\n", "\t",
"\r", "\"", "\'",
+"^someApi$", "^someApi", "some$", "some-Api;", "some,Api",
+"^", "$", "^$", ".*", "\\w+", "r**l3rd0@Kr3",
"j@s1n|+|0È·",
+"[a-z0-9-]+",
"^([a-z0-9_\\.-]+)@([\\da-z\\.-]+)\\.([a-z\\.]{2,6})$")) {
+try {
+new Rule(rule);
+Assert.fail("Invalid rule, exception was expected");
+} catch (InvalidParameterValueException ignored) {}
+}
--- End diff --
This method can be simplified by removing the try-catch and added
``expected=InvalidParameterValueException.class`` to the ``@Test`` annotation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
Re: Feature proposal: Resource naming policies
Awesome and long awaited On 4/14/16 4:40 AM, Jeff Hair wrote: > Yesterday, we submitted this pull request: > https://github.com/apache/cloudstack/pull/1492 > > This originally grew out of making the VirtualMachineName class non-static > (original PR is mentioned in the above link). We're presenting this as a > refactoring of the existing code to enable more extensibility and > flexibility, make unit testing easier, and unify the way CloudStack > generates resource names. > > There is an associated JIRA ticket at CLOUDSTACK-9003. I will be writing up > a design document for the CS wiki in the next few days. > > jburwell wanted me to open a discussion on the developer list about the PR > and how it is implemented: > > There is now a ResourceNamingPolicyManager and a bunch of > ResourceNamingPolicies. The manager exposes a method to get a policy for a > type of resource, and the naming policies generate UUIDs + resource names. > > The default naming policies generate names exactly the same way as they are > created now. This is in a new module called default-naming-policies. By > excluding this module and loading our own naming policies, we gain the > ability to change how names are generated. >
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59801226
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,139 @@ public boolean supportsRollingUpgrade() {
@Override
public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id,
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d,
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.debug("Unable to insert mapping for role id:" +
roleId + " apiName: " + apiName);
--- End diff --
Why not log this condition to ``WARN``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59801354
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,139 @@ public boolean supportsRollingUpgrade() {
@Override
public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id,
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d,
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.debug("Unable to insert mapping for role id:" +
roleId + " apiName: " + apiName);
+}
+}
+
+private void addRoleColumnAndMigrateAccountTable(final Connection
conn, final RoleType[] roleTypes) {
+final String alterTableSql = "ALTER TABLE `cloud`.`account` ADD
COLUMN `role_id` bigint(20) unsigned COMMENT 'role id for this account' AFTER
`type`, " +
+"ADD KEY `fk_account__role_id` (`role_id`), " +
+"ADD CONSTRAINT `fk_account__role_id` FOREIGN KEY
(`role_id`) REFERENCES `roles` (`id`);";
+try (PreparedStatement pstmt =
conn.prepareStatement(alterTableSql)) {
+pstmt.executeUpdate();
+s_logger.info("Altered cloud.account table and added column
role_id");
+} catch (SQLException e) {
+if (e.getMessage().contains("role_id")) {
+s_logger.warn("cloud.account table already has the role_id
column, skipping altering table and migration of accounts");
+return;
+} else {
+throw new CloudRuntimeException("Unable to create column
quota_calculated in table cloud_usage.cloud_usage", e);
+}
+}
+migrateAccountsToDefaultRoles(conn, roleTypes);
+}
+
+private void migrateAccountsToDefaultRoles(final Connection conn,
final RoleType[] roleTypes) {
+try (PreparedStatement selectStatement =
conn.prepareStatement("SELECT `id`, `type` FROM `cloud`.`account`;");
+ ResultSet selectResultSet = selectStatement.executeQuery()) {
--- End diff --
The ``selectResultSet`` is a resource that needs to be closed. Please add
it to enclosing try with resources block.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59801445
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,139 @@ public boolean supportsRollingUpgrade() {
@Override
public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id,
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d,
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.debug("Unable to insert mapping for role id:" +
roleId + " apiName: " + apiName);
+}
+}
+
+private void addRoleColumnAndMigrateAccountTable(final Connection
conn, final RoleType[] roleTypes) {
+final String alterTableSql = "ALTER TABLE `cloud`.`account` ADD
COLUMN `role_id` bigint(20) unsigned COMMENT 'role id for this account' AFTER
`type`, " +
+"ADD KEY `fk_account__role_id` (`role_id`), " +
+"ADD CONSTRAINT `fk_account__role_id` FOREIGN KEY
(`role_id`) REFERENCES `roles` (`id`);";
+try (PreparedStatement pstmt =
conn.prepareStatement(alterTableSql)) {
+pstmt.executeUpdate();
+s_logger.info("Altered cloud.account table and added column
role_id");
+} catch (SQLException e) {
+if (e.getMessage().contains("role_id")) {
+s_logger.warn("cloud.account table already has the role_id
column, skipping altering table and migration of accounts");
+return;
+} else {
+throw new CloudRuntimeException("Unable to create column
quota_calculated in table cloud_usage.cloud_usage", e);
+}
+}
+migrateAccountsToDefaultRoles(conn, roleTypes);
+}
+
+private void migrateAccountsToDefaultRoles(final Connection conn,
final RoleType[] roleTypes) {
+try (PreparedStatement selectStatement =
conn.prepareStatement("SELECT `id`, `type` FROM `cloud`.`account`;");
+ ResultSet selectResultSet = selectStatement.executeQuery()) {
+while (selectResultSet.next()) {
+Long accountId = selectResultSet.getLong(1);
+Short accountType = selectResultSet.getShort(2);
+Long roleId = null;
+for (RoleType roleType : roleTypes) {
+if (roleType.getAccountType() == accountType) {
+roleId = roleType.getId();
+break;
+}
+}
+if (roleId == null) {
+continue;
+}
+try (PreparedStatement updateStatement =
conn.prepareStatement("UPDATE `cloud`.`account` SET role_id = ? WHERE id =
?;")) {
+updateStatement.setLong(1, roleId);
+updateStatement.setLong(2, accountId);
+updateStatement.executeUpdate();
+updateStatement.close();
+
+} catch (SQLException e) {
+s_logger.error("Failed to update cloud.account role_id
for account id:" + accountId + " with exception: " + e.getMessage());
+throw new CloudRuntimeException("Exception while
updating cloud.account role_id", e);
+}
+}
--- End diff --
Should these updates be wrapped in a transaction to prevent data
inconsistencies if there is an exception?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59801500
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,139 @@ public boolean supportsRollingUpgrade() {
@Override
public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id,
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d,
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.debug("Unable to insert mapping for role id:" +
roleId + " apiName: " + apiName);
+}
+}
+
+private void addRoleColumnAndMigrateAccountTable(final Connection
conn, final RoleType[] roleTypes) {
+final String alterTableSql = "ALTER TABLE `cloud`.`account` ADD
COLUMN `role_id` bigint(20) unsigned COMMENT 'role id for this account' AFTER
`type`, " +
+"ADD KEY `fk_account__role_id` (`role_id`), " +
+"ADD CONSTRAINT `fk_account__role_id` FOREIGN KEY
(`role_id`) REFERENCES `roles` (`id`);";
+try (PreparedStatement pstmt =
conn.prepareStatement(alterTableSql)) {
+pstmt.executeUpdate();
+s_logger.info("Altered cloud.account table and added column
role_id");
+} catch (SQLException e) {
+if (e.getMessage().contains("role_id")) {
+s_logger.warn("cloud.account table already has the role_id
column, skipping altering table and migration of accounts");
+return;
+} else {
+throw new CloudRuntimeException("Unable to create column
quota_calculated in table cloud_usage.cloud_usage", e);
+}
+}
+migrateAccountsToDefaultRoles(conn, roleTypes);
+}
+
+private void migrateAccountsToDefaultRoles(final Connection conn,
final RoleType[] roleTypes) {
+try (PreparedStatement selectStatement =
conn.prepareStatement("SELECT `id`, `type` FROM `cloud`.`account`;");
+ ResultSet selectResultSet = selectStatement.executeQuery()) {
+while (selectResultSet.next()) {
+Long accountId = selectResultSet.getLong(1);
+Short accountType = selectResultSet.getShort(2);
+Long roleId = null;
+for (RoleType roleType : roleTypes) {
+if (roleType.getAccountType() == accountType) {
+roleId = roleType.getId();
+break;
+}
+}
+if (roleId == null) {
+continue;
+}
+try (PreparedStatement updateStatement =
conn.prepareStatement("UPDATE `cloud`.`account` SET role_id = ? WHERE id =
?;")) {
+updateStatement.setLong(1, roleId);
+updateStatement.setLong(2, accountId);
+updateStatement.executeUpdate();
+updateStatement.close();
+
+} catch (SQLException e) {
+s_logger.error("Failed to update cloud.account role_id
for account id:" + accountId + " with exception: " + e.getMessage());
+throw new CloudRuntimeException("Exception while
updating cloud.account role_id", e);
+}
+}
+} catch (SQLException e) {
+throw new CloudRuntimeException("Exception while migrating
existing account table's role_id column to a role based on account type", e);
+}
+s_logger.debug("Done migrating existing accounts to use one of
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59801544
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,139 @@ public boolean supportsRollingUpgrade() {
@Override
public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id,
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d,
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.debug("Unable to insert mapping for role id:" +
roleId + " apiName: " + apiName);
+}
+}
+
+private void addRoleColumnAndMigrateAccountTable(final Connection
conn, final RoleType[] roleTypes) {
+final String alterTableSql = "ALTER TABLE `cloud`.`account` ADD
COLUMN `role_id` bigint(20) unsigned COMMENT 'role id for this account' AFTER
`type`, " +
+"ADD KEY `fk_account__role_id` (`role_id`), " +
+"ADD CONSTRAINT `fk_account__role_id` FOREIGN KEY
(`role_id`) REFERENCES `roles` (`id`);";
+try (PreparedStatement pstmt =
conn.prepareStatement(alterTableSql)) {
+pstmt.executeUpdate();
+s_logger.info("Altered cloud.account table and added column
role_id");
+} catch (SQLException e) {
+if (e.getMessage().contains("role_id")) {
+s_logger.warn("cloud.account table already has the role_id
column, skipping altering table and migration of accounts");
+return;
+} else {
+throw new CloudRuntimeException("Unable to create column
quota_calculated in table cloud_usage.cloud_usage", e);
+}
+}
+migrateAccountsToDefaultRoles(conn, roleTypes);
+}
+
+private void migrateAccountsToDefaultRoles(final Connection conn,
final RoleType[] roleTypes) {
+try (PreparedStatement selectStatement =
conn.prepareStatement("SELECT `id`, `type` FROM `cloud`.`account`;");
+ ResultSet selectResultSet = selectStatement.executeQuery()) {
+while (selectResultSet.next()) {
+Long accountId = selectResultSet.getLong(1);
+Short accountType = selectResultSet.getShort(2);
+Long roleId = null;
+for (RoleType roleType : roleTypes) {
+if (roleType.getAccountType() == accountType) {
+roleId = roleType.getId();
+break;
+}
+}
+if (roleId == null) {
+continue;
+}
+try (PreparedStatement updateStatement =
conn.prepareStatement("UPDATE `cloud`.`account` SET role_id = ? WHERE id =
?;")) {
+updateStatement.setLong(1, roleId);
+updateStatement.setLong(2, accountId);
+updateStatement.executeUpdate();
+updateStatement.close();
+
+} catch (SQLException e) {
+s_logger.error("Failed to update cloud.account role_id
for account id:" + accountId + " with exception: " + e.getMessage());
+throw new CloudRuntimeException("Exception while
updating cloud.account role_id", e);
+}
+}
+} catch (SQLException e) {
+throw new CloudRuntimeException("Exception while migrating
existing account table's role_id column to a role based on account type", e);
+}
+s_logger.debug("Done migrating existing accounts to use one of
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user pdube commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59812425
--- Diff: api/src/org/apache/cloudstack/acl/RoleType.java ---
@@ -16,18 +16,90 @@
// under the License.
package org.apache.cloudstack.acl;
+import com.cloud.user.Account;
+import com.google.common.base.Enums;
+import com.google.common.base.Strings;
+
// Enum for default roles in CloudStack
public enum RoleType {
-Admin(1), ResourceAdmin(2), DomainAdmin(4), User(8), Unknown(0);
+Admin(1L, Account.ACCOUNT_TYPE_ADMIN, 1),
+ResourceAdmin(2L, Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN, 2),
+DomainAdmin(3L, Account.ACCOUNT_TYPE_DOMAIN_ADMIN, 4),
+User(4L, Account.ACCOUNT_TYPE_NORMAL, 8),
+Unknown(-1L, (short) -1, 0);
+private long id;
+private short accountType;
private int mask;
-private RoleType(int mask) {
+RoleType(final long id, final short accountType, final int mask) {
+this.id = id;
+this.accountType = accountType;
this.mask = mask;
}
-public int getValue() {
+public long getId() {
+return id;
+}
+
+public short getAccountType() {
+return accountType;
+}
+
+public int getMask() {
return mask;
}
-}
+public static RoleType fromString(final String name) {
+if (!Strings.isNullOrEmpty(name)
+&& Enums.getIfPresent(RoleType.class, name).isPresent()) {
+return RoleType.valueOf(name);
+}
+return null;
+}
+
+public static RoleType fromMask(int mask) {
+for (RoleType roleType : RoleType.values()) {
+if (roleType.getMask() == mask) {
+return roleType;
+}
+}
+return Unknown;
+}
+
+public static RoleType getByAccountType(final short accountType) {
+RoleType roleType = RoleType.Unknown;
+switch (accountType) {
+case Account.ACCOUNT_TYPE_ADMIN:
+roleType = RoleType.Admin;
+break;
+case Account.ACCOUNT_TYPE_DOMAIN_ADMIN:
+roleType = RoleType.DomainAdmin;
+break;
+case Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN:
+roleType = RoleType.ResourceAdmin;
--- End diff --
Thanks. I had seen it in the code and looked for more information about it,
but didn't find anything conclusive
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
SHA bbe0fc
Hi, I noticed an issue in Marvin the other day and I tracked it to this commit: https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;a=commit;h=bbe0fc4be9527d51820b067a602886003991db4d The problem is that it assumes the "ispublic" parameter will be provided. If it is not, then an exception is thrown. I think we want code more like this: if "ispublic" in services: cmd.ispublic = services["ispublic"] I don't think we would want to require the "ispublic" parameter in Marvin. It's not required in our API: http://cloudstack.apache.org/api/apidocs-4.8/root_admin/createTemplate.html? Unless someone can think of a reason why this part of the code is the way it is now, I plan to open a PR to fix this soon. Thanks, Mike
Re: Strange XenServer SR behavior when deploying system VMs in Basic Zone on 4.9
I'm not sure, Daan. I plan to keep an eye on this behavior for a while when creating new clouds. From: Daan Hoogland Sent: Thursday, April 14, 2016 2:12 AM To: dev Subject: Re: Strange XenServer SR behavior when deploying system VMs in Basic Zone on 4.9 Mike, did the iso copy process not complete as expected. Sound like they are a remanence of some task ending in an exception. Probably a silently ignored one ;| On Thu, Apr 14, 2016 at 2:49 AM, Tutkowski, Mike wrote: > Just an FYI, but when I kicked off my first VM in this cloud, the VR > happened to get deployed to XenServer-6.5-3 (which was one of my XenServer > hosts that had an un-expected shared SR pointing at secondary storage > beforehand). > > Once the process of copying the system template down to local storage > completed, the shared SR pointing at secondary storage went away (as you > would expect). > > This leaves me now with one un-expected shared SR pointing at secondary > storage on XenServer-6.5-1. > > > From: Tutkowski, Mike > Sent: Wednesday, April 13, 2016 5:10 PM > To: dev@cloudstack.apache.org > Subject: Strange XenServer SR behavior when deploying system VMs in Basic > Zone on 4.9 > > Hi, > > > Has anyone recently observed the following behavior: > > > http://imgur.com/8ALJmWb > > > As you can see in the image, I have three 6.5 XenServer hosts in a > resource pool. > > > I just used them when creating a basic zone and the system VMs were > deployed just fine. However, there are SRs pointing to secondary storage on > my XenServer-6.5-1 and XenServer-6.5-3 hosts still (there used to be one on > my XenServer-6.5-2 host, but it went away once the system VMs started up on > that host). > > > Thoughts? > > > Thanks, > > Mike > -- Daan
Re: Strange XenServer SR behavior when deploying system VMs in Basic Zone on 4.9
Mike, what type of storage are you using? > On 15-Apr-2016, at 9:49 AM, Tutkowski, Mike wrote: > > I'm not sure, Daan. > > I plan to keep an eye on this behavior for a while when creating new clouds. > > > From: Daan Hoogland > Sent: Thursday, April 14, 2016 2:12 AM > To: dev > Subject: Re: Strange XenServer SR behavior when deploying system VMs in Basic > Zone on 4.9 > > Mike, did the iso copy process not complete as expected. Sound like they > are a remanence of some task ending in an exception. Probably a silently > ignored one ;| > > On Thu, Apr 14, 2016 at 2:49 AM, Tutkowski, Mike > wrote: > >> Just an FYI, but when I kicked off my first VM in this cloud, the VR >> happened to get deployed to XenServer-6.5-3 (which was one of my XenServer >> hosts that had an un-expected shared SR pointing at secondary storage >> beforehand). >> >> Once the process of copying the system template down to local storage >> completed, the shared SR pointing at secondary storage went away (as you >> would expect). >> >> This leaves me now with one un-expected shared SR pointing at secondary >> storage on XenServer-6.5-1. >> >> >> From: Tutkowski, Mike >> Sent: Wednesday, April 13, 2016 5:10 PM >> To: dev@cloudstack.apache.org >> Subject: Strange XenServer SR behavior when deploying system VMs in Basic >> Zone on 4.9 >> >> Hi, >> >> >> Has anyone recently observed the following behavior: >> >> >> http://imgur.com/8ALJmWb >> >> >> As you can see in the image, I have three 6.5 XenServer hosts in a >> resource pool. >> >> >> I just used them when creating a basic zone and the system VMs were >> deployed just fine. However, there are SRs pointing to secondary storage on >> my XenServer-6.5-1 and XenServer-6.5-3 hosts still (there used to be one on >> my XenServer-6.5-2 host, but it went away once the system VMs started up on >> that host). >> >> >> Thoughts? >> >> >> Thanks, >> >> Mike >> > > > > -- > Daan DISCLAIMER == This e-mail may contain privileged and confidential information which is the property of Accelerite, a Persistent Systems business. It is intended only for the use of the individual or entity to which it is addressed. If you are not the intended recipient, you are not authorized to read, retain, copy, print, distribute or use this message. If you have received this communication in error, please notify the sender and delete all copies of this message. Accelerite, a Persistent Systems business does not accept any liability for virus infected mails.
