Skip to content

Expose modules to support creation of CDP DW Database Catalog and Virtual Warehouses #19

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

Expose modules to support creation of CDP DW Database Catalog and Virtual Warehouses #19

wants to merge 8 commits into from

Conversation

raju-saravanan
Copy link
Contributor

@raju-saravanan raju-saravanan commented Jul 15, 2021 •
edited
Loading

Changes in this PR:

  • Support private load balancers while creating a DW cluster.
  • Support the creation of DW database catalog.
  • Support the creation of DW virtual warehouse.

Changes in this PR are done in conjunction with the following PRs:

This was referenced Jul 15, 2021
Draft
Closed
Merged
Chaffelson
Chaffelson suggested changes Jul 15, 2021
View reviewed changes
Chaffelson
Chaffelson suggested changes Jul 22, 2021
View reviewed changes
plugins/modules/dw_vw.py Outdated
cluster_id=dict(required=True, type='str', aliases=['cluster_id']),
dbc_id=dict(required=False, type='str', aliases=['dbc_id']),
vw_type = dict(required=False, type='str', aliases=['vw_type']),
name = dict(required=True, type='str', aliases=['name']),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be 'name' with an alias of 'name'

plugins/modules/dw_dbc.py Outdated
self.target = self.cdpy.sdk.wait_for_state(
describe_func=self.cdpy.dw.describe_dbc,
params=dict(cluster_id=self.cluster_id,dbc_id=self.name),
state='Running', delay=self.delay, timeout=self.timeout
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state should be self.cdpy.sdk.STARTED_STATES

plugins/modules/dw_vw.py Outdated
self.target = self.cdpy.sdk.wait_for_state(
describe_func=self.cdpy.dw.delete_vw,
params=dict(cluster_id=self.cluster_id, vw_id=self.name),
state='Running', delay=self.delay, timeout=self.timeout
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.cdpy.sdk.STARTED_STATES

plugins/modules/dw_vw.py Outdated
self.target = self.cdpy.sdk.wait_for_state(
describe_func=self.cdpy.dw.describe_vw,
params=dict(cluster_id=self.cluster_id, vw_id=self.name),
state='Running', delay=self.delay, timeout=self.timeout
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.cdpy.sdk.STARTED_STATES

@raju-saravanan raju-saravanan force-pushed the cdw-level-2 branch 2 times, most recently from 7e43f33 to 898d275 Compare July 22, 2021 11:48
Chaffelson
Chaffelson suggested changes Jul 22, 2021
View reviewed changes
wmudge
wmudge requested changes Jul 22, 2021
View reviewed changes
Copy link
Member

@wmudge wmudge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I would like to clarify the module names and return values. In addition, the documentation sections need review, esp. the dw_vw.py module. The contents do not reflect the code, and I suspect the format of applicationConfig, for example, needs updating, too.

plugins/modules/dw_dbc.py Outdated
)

result = DwDbc(module)
output = dict(changed=False, dbcs=result.dbcs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dbcs -> data_catalogs

plugins/modules/dw_vw.py Outdated
description: Maximum number of available nodes for Virtual Warehouse autoscaling.
type: int
required: False
config:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no config parameter group in the code -- commonConfigs, applicationConfigs, ldapGroups, and enableSSO are all "top level" module parameters in the code. Moreover, the parameters are lowercase with underscores.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, will fix this.

@raju-saravanan raju-saravanan force-pushed the cdw-level-2 branch 2 times, most recently from 0ac1f4e to a647c61 Compare July 23, 2021 09:55
@raju-saravanan
Copy link
Contributor Author

@wmudge : Address your comments, can you take another look ?

wmudge
wmudge requested changes Jul 23, 2021
View reviewed changes
Copy link
Member

@wmudge wmudge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation and module specification sections need alignment and further detail.

plugins/modules/dw_virtual_warehouse.py Outdated
description: Configurations that are applied to every application in the service.
type: dict
required: False
contains:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, contains has now been replaced by suboptions -- for example: https://github.com/ansible-collections/azure/blob/dev/plugins/modules/azure_rm_securitygroup.py#L49-L52

Full docs are here: https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#documentation-block

plugins/modules/dw_virtual_warehouse.py
required: False
contains:
configBlocks: List of ConfigBlocks for the application.
type: list
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lists require elements, which would be dict in this case, IIRC

plugins/modules/dw_virtual_warehouse.py Outdated
required: False
content:
description: Contents of a ConfigBlock.
type: obj
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dict

plugins/modules/dw_virtual_warehouse.py Outdated
contains:
keyValues:
description: Key-value type configurations.
type: obj
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dict

plugins/modules/dw_virtual_warehouse.py
- cloudera.cloud.cdp_auth_options
'''

EXAMPLES = r'''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an example or two of using the various nested configurations?

plugins/modules/dw_virtual_warehouse.py Outdated
def main():
module = AnsibleModule(
argument_spec=CdpModule.argument_spec(
id=dict(required=False, type='str', default=None),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only use default when necessary. The _get_param() should handle a non-existent parameter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with required -- only needed if True

plugins/modules/dw_virtual_warehouse.py Outdated
autoscaling_min_nodes=dict(required=False, type='int', default=None),
autoscaling_max_nodes=dict(required=False, type='int', default=None),
common_configs=dict(required=False, type='dict', default=None),
application_configs=dict(required=False, type='dict', default=None),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both common_configs and application_configs need to be expanded to match documentation rules above.

plugins/modules/dw_virtual_warehouse.py Outdated
)

result = DwVw(module)
output = dict(changed=False, virtual_warehouses=result.virtual_warehouses)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed is invalid -- this module can make changes.

@raju-saravanan
Copy link
Contributor Author

@wmudge : I have expanded the commonConfigs in argument_spec, but applicationConfig have some dynamic keys in the uppermost level so, couldn't expand beyond 'dict'. Eg :

 "applicationConfigs": {
        "das-webapp": {
        "configBlocks": [
           {
               "id": "hive-kerberos-config",
               "format": "TEXT",
               "content": {
                "text": "\n[libdefaults]\n\trenew_lifetime = 7d"
              }
          }
        ] 
      }   
    }

Here "das-webapp" is dynamic key, we can have configs for other services as well.

@wmudge wmudge mentioned this pull request Aug 19, 2021
Merged
@wmudge
Copy link
Member

wmudge commented Oct 6, 2021

Superseded by #29

@wmudge wmudge closed this Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wmudge wmudge wmudge requested changes

@Chaffelson Chaffelson Chaffelson approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@raju-saravanan @wmudge @Chaffelson