Remove 'iam' projects by khos2ow · Pull Request #2817 · apache/cloudstack

khos2ow · 2018-08-20T21:48:09Z

Description

Following up after #2613 services/iam project is obsolete and has been disabled since ba84808, and technically it should be removed. This will be discussed here on and on the ML to see if there's anyone wants to revive/maintain this.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

GitHub Issue/PRs

Fixes: #2772

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

DaanHoogland · 2018-08-21T07:39:37Z

big cleanup; heavy testing (will review)
@blueorangutan package

blueorangutan · 2018-08-21T07:40:00Z

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

DaanHoogland

there are 6 VO refeerring tables as far as I can see. I don't see upgrade code to move those tables out of the way nor the removal of the upgrade code to create those tables. I think we should add these bits. (good cleanup otherwise @khos2ow )

blueorangutan · 2018-08-21T08:15:38Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2261

yadvr · 2018-08-21T08:16:03Z

-    public static final String IAM_POLICY_IDS = "policyids";
-    public static final String IAM_POLICIES = "policies";
-    public static final String IAM_APIS = "apis";
    public static final String IAM_GROUPS = "groups";


Remove IAM_GROUPS as well?

I removed it at first, but it is used in AccountResponse, and since that's an API response I moved it back.

yadvr

LGTM, subject to regression testing. Can you start a thread on dev+user ML about this?

yadvr · 2018-08-21T08:17:14Z

@blueorangutan test

blueorangutan · 2018-08-21T08:18:00Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

rafaelweingartner · 2018-08-21T10:43:45Z

I agree with @DaanHoogland, if we remove the *VO*, we should remove tables as well. I noticed that this component has a few API methods as well. Out of curiosity, does anybody know what IAM stands for? And what was the purpose of this component?

When I read IAM I tempted to think Identity and Access Management, which ACS could benefit from.

khos2ow · 2018-08-21T13:53:45Z

@DaanHoogland @rafaelweingartner I noticed that (the definition of tables in sql files) and I wanted to open the PR as it was and have a discussion around it here. I am completely for removing the tables too (schema-41120-41200-cleanup.sql probably).

khos2ow · 2018-08-21T13:56:16Z

@rafaelweingartner It stands for "Identity and Access Management" or like "I am" (as subject and verb)

rafaelweingartner · 2018-08-21T13:57:21Z

@khos2ow so it was some sort of integration with policy decision points that was never finished?

khos2ow · 2018-08-21T14:15:05Z

I would say so. Most probably started at around the same time AWS introduced their IAM and left alone here?

rafaelweingartner · 2018-08-21T14:16:10Z

Interesting, but sadly it was not finished.

khos2ow · 2018-08-21T14:32:41Z

@rhtyd started a thread in ML.

borisstoyanov · 2018-08-23T08:52:28Z

@blueorangutan test

blueorangutan · 2018-08-23T08:53:00Z

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2018-08-24T07:13:40Z

Trillian test result (tid-2956)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 72619 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2817-t2956-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_accounts.py
Intermitten failure detected: /marvin/tests/smoke/test_certauthority_root.py
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_deploy_vm_iso.py
Intermitten failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_public_ip_range.py
Intermitten failure detected: /marvin/tests/smoke/test_router_dhcphosts.py
Intermitten failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermitten failure detected: /marvin/tests/smoke/test_ssvm.py
Intermitten failure detected: /marvin/tests/smoke/test_templates.py
Intermitten failure detected: /marvin/tests/smoke/test_usage.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_volumes.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 59 look OK, 10 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestTemplateHierarchy>:setup	`Error`	1648.34	test_accounts.py
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_deploy_vm_from_iso	`Error`	1605.75	test_deploy_vm_iso.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	1295.96	test_privategw_acl.py
test_09_destroy_ssvm	`Failure`	932.87	test_ssvm.py
test_04_extract_template	`Failure`	128.41	test_templates.py
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
test_02_unsecure_vm_migration	`Error`	67.27	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	3646.38	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	3787.36	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	139.55	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	22.44	test_vm_life_cycle.py
test_06_download_detached_volume	`Failure`	137.80	test_volumes.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	2.90	test_hostha_kvm.py

borisstoyanov

LGTM, we fixed these tests failures last month.

yadvr · 2018-08-27T08:36:29Z

@khos2ow can you address @DaanHoogland 's comments? Please check and remove any db tables as part of the upgrade path and delete the VOs/Daos classes if any.

khos2ow · 2018-08-28T19:12:16Z

@rhtyd @DaanHoogland all the VO java files were inside the project services/iam/server which already has been deleted as part of the first commit of this PR, and I just pushed a schema cleanup to drop corresponding tables as well.

DaanHoogland · 2018-08-30T07:57:06Z

looks good @khos2ow. Just wondering if we should actually remove the upgrade code that adds them. someone installing a new system now goes through the steps of adding those tables only to end up removing them again at the initial start of the system.

yadvr · 2018-08-30T08:45:11Z

@blueorangutan package

blueorangutan · 2018-08-30T08:45:40Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-08-30T09:26:35Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-2275

yadvr · 2018-08-30T09:44:54Z

@blueorangutan package

blueorangutan · 2018-08-30T09:45:40Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-08-30T10:35:44Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2278

yadvr · 2018-08-30T11:07:01Z

@blueorangutan test

blueorangutan · 2018-08-30T11:07:42Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

khos2ow · 2018-08-30T15:55:18Z

@DaanHoogland that's true but I wouldn't remove the code that adds them in the first place. This will make the schema upgrade path not "immutable". (at least immutable by convention to never change anything that's been released)

blueorangutan · 2018-08-30T22:22:37Z

Trillian test result (tid-2977)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38455 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2817-t2977-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 66 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	1260.82	test_privategw_acl.py
test_01_secure_vm_migration	`Error`	138.06	test_vm_life_cycle.py
test_02_unsecure_vm_migration	`Error`	137.02	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	137.04	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	135.99	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	20.01	test_vm_life_cycle.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	2.63	test_hostha_kvm.py

borisstoyanov

LGTM, latest tests looks good

swill · 2018-09-11T15:53:34Z

Is this one ready to merge???

rafaelweingartner · 2018-09-11T15:57:58Z

I think so.

swill · 2018-09-11T16:03:06Z

Everything looks in order. What is the process of getting from this stage to merged? I could merge the PR (as I am a committer), but what are we doing now as a way to get through the valid PRs to merge?

rafaelweingartner · 2018-09-11T16:06:13Z

Click on "squash+merge" as soon as the PR has passed the tests and has been approved by reviewers...

swill · 2018-09-11T16:08:01Z

Ok, so basically manual inspection and merging by active members? Thanks...

rafaelweingartner · 2018-09-11T16:10:37Z

That is it. We are not merging PRs automatically yet.

Remove 'iam' projects

de76e9c

rafaelweingartner assigned khos2ow Aug 20, 2018

rafaelweingartner added the type:cleanup label Aug 20, 2018

rafaelweingartner added this to the 4.12.0.0 milestone Aug 20, 2018

DaanHoogland reviewed Aug 21, 2018

View reviewed changes

yadvr reviewed Aug 21, 2018

View reviewed changes

yadvr approved these changes Aug 21, 2018

View reviewed changes

borisstoyanov approved these changes Aug 24, 2018

View reviewed changes

Drop corresponding tables

2217a2e

rafaelweingartner approved these changes Aug 30, 2018

View reviewed changes

borisstoyanov approved these changes Sep 5, 2018

View reviewed changes

rafaelweingartner merged commit 56f9185 into apache:master Sep 11, 2018

khos2ow deleted the remove-iam branch September 11, 2018 16:19

Conversation

khos2ow commented Aug 20, 2018

Description

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

DaanHoogland commented Aug 21, 2018

Uh oh!

blueorangutan commented Aug 21, 2018

Uh oh!

DaanHoogland left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Aug 21, 2018

Uh oh!

yadvr Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

khos2ow Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Aug 21, 2018

Uh oh!

blueorangutan commented Aug 21, 2018

Uh oh!

rafaelweingartner commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khos2ow commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khos2ow commented Aug 21, 2018

Uh oh!

rafaelweingartner commented Aug 21, 2018

Uh oh!

khos2ow commented Aug 21, 2018

Uh oh!

rafaelweingartner commented Aug 21, 2018

Uh oh!

khos2ow commented Aug 21, 2018

Uh oh!

borisstoyanov commented Aug 23, 2018

Uh oh!

blueorangutan commented Aug 23, 2018

Uh oh!

blueorangutan commented Aug 24, 2018

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Aug 27, 2018

Uh oh!

khos2ow commented Aug 28, 2018

Uh oh!

DaanHoogland commented Aug 30, 2018

Uh oh!

yadvr commented Aug 30, 2018

Uh oh!

blueorangutan commented Aug 30, 2018

Uh oh!

blueorangutan commented Aug 30, 2018

Uh oh!

yadvr commented Aug 30, 2018

Uh oh!

blueorangutan commented Aug 30, 2018

Uh oh!

blueorangutan commented Aug 30, 2018

Uh oh!

yadvr commented Aug 30, 2018

Uh oh!

blueorangutan commented Aug 30, 2018

Uh oh!

DaanHoogland left a comment •

edited

Loading

khos2ow Aug 21, 2018 •

edited

Loading

rafaelweingartner commented Aug 21, 2018 •

edited

Loading

khos2ow commented Aug 21, 2018 •

edited

Loading