Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove 'iam' projects #2817

Merged
merged 2 commits into from Sep 11, 2018

Conversation

@khos2ow
Copy link
Contributor

commented Aug 20, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

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

@blueorangutan

This comment has been minimized.

Copy link

commented Aug 21, 2018

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

@DaanHoogland
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link

commented Aug 21, 2018

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

public static final String IAM_PARENT_POLICY_NAME = "parentpolicyname";
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";

This comment has been minimized.

Copy link
@rhtyd

rhtyd Aug 21, 2018

Member

Remove IAM_GROUPS as well?

This comment has been minimized.

Copy link
@khos2ow

khos2ow Aug 21, 2018

Author Contributor

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

@rhtyd

rhtyd approved these changes Aug 21, 2018

Copy link
Member

left a comment

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

@rhtyd

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

@blueorangutan

This comment has been minimized.

Copy link

commented Aug 21, 2018

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

@rafaelweingartner

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

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

@rafaelweingartner

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

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

@khos2ow

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

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

@rafaelweingartner

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

Interesting, but sadly it was not finished.

@khos2ow

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

@rhtyd started a thread in ML.

@borisstoyanov

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

@blueorangutan

This comment has been minimized.

Copy link

commented Aug 23, 2018

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

@blueorangutan

This comment has been minimized.

Copy link

commented Aug 24, 2018

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
Copy link
Contributor

left a comment

LGTM, we fixed these tests failures last month.

@rhtyd

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2018

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.

@rhtyd

This comment has been minimized.

Copy link
Member

commented Aug 30, 2018

@blueorangutan package

@blueorangutan

This comment has been minimized.

Copy link

commented Aug 30, 2018

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

@blueorangutan

This comment has been minimized.

Copy link

commented Aug 30, 2018

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

@rhtyd

This comment has been minimized.

Copy link
Member

commented Aug 30, 2018

@blueorangutan package

@blueorangutan

This comment has been minimized.

Copy link

commented Aug 30, 2018

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

@blueorangutan

This comment has been minimized.

Copy link

commented Aug 30, 2018

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

@rhtyd

This comment has been minimized.

Copy link
Member

commented Aug 30, 2018

@blueorangutan

This comment has been minimized.

Copy link

commented Aug 30, 2018

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

@khos2ow

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2018

@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

This comment has been minimized.

Copy link

commented Aug 30, 2018

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
Copy link
Contributor

left a comment

LGTM, latest tests looks good

@swill

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

Is this one ready to merge???

@rafaelweingartner

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

I think so.

@swill

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

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

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

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

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

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rafaelweingartner rafaelweingartner added this to To do in Master (4.12.0.0) via automation Sep 11, 2018

@swill

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

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

@rafaelweingartner

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

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

@khos2ow khos2ow deleted the khos2ow:remove-iam branch Sep 11, 2018

@rhtyd rhtyd moved this from To do to Done in Master (4.12.0.0) Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.