- 
                Notifications
    You must be signed in to change notification settings 
- Fork 28
Support creation of CDP DW Database Catalog and Virtual Warehouses #27
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
Conversation
a2026a1    to
    0078f9d      
    Compare
  
    8217872    to
    b290255      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. there's a minor hiccup with DW teardown but that is a bug already fixed in the purge PR, which is also due to be merged.
a4f0c27    to
    8bd5738      
    Compare
  
    8bd5738    to
    079ae61      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review. I'm going to continue testing and review, esp. with the changes/updates in the underlying cloudera.cloud.dw_* modules.
| name: "{{ item.1.name | default([run__namespace, run__dw_vw_suffix ,__dw_dbc_index] | join('-')) }}" | ||
| dbc_name: "{{ item.0.name }}" | ||
| use_default_dbc: "{{ item.0.use_default_dbc }}" | ||
| type: "{{ item.1.type | default('hive') }}" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a variable set at the dw: global key level (with defaults)
| dbc_name: "{{ item.0.name }}" | ||
| use_default_dbc: "{{ item.0.use_default_dbc }}" | ||
| type: "{{ item.1.type | default('hive') }}" | ||
| template: "{{ item.1.template | default('xsmall') }}" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a variable set at the dw: global key level (with defaults)
| use_default_dbc: "{{ item.0.use_default_dbc }}" | ||
| type: "{{ item.1.type | default('hive') }}" | ||
| template: "{{ item.1.template | default('xsmall') }}" | ||
| autoscaling_min_nodes: "{{ item.1.autoscaling.min_nodes | default(None) }}" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use omit() than pass None or an empty config dict for lines 194-199
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am referencing these parameters in setup_aws file. So, if I use omit() here I would have to use omit() in setup_aws as well. If you are ok with this, I can make the changes
| loop: "{{ run__dw_dbc_configs | subelements('virtual_warehouses')}}" | ||
| loop_control: | ||
| index_var: __dw_dbc_index | ||
| label: "{{ item.0.name }}" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also have a loop_var to avoid using item
| run__dw_dbc_configs: "{{ run__dw_dbc_configs | default([]) | union([config]) }}" | ||
| vars: | ||
| include: "{{ lookup('template', __dw_config.include | default('experiences_config_placeholder.j2')) | from_yaml }}" | ||
| use_default_dbc: "{{ True if __dw_config.name is not defined else False | bool }}" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be inlined at line 177, also __dw_config.name is undefined | bool works?
| - name: Assign AWS Private Subnet IDs in not assinged | ||
| when: run__datahub_private_subnet_ids is undefined | ||
| ansible.builtin.set_fact: | ||
| run__datahub_private_subnet_ids: "{{ [] }}" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run__datahub_private_subnet_ids: "{{ run__datahub_private_subnet_ids | default([]) }}" and remove the when
| - name: Create CDP DW Database catalogs | ||
| when: not __dw_dbc_config.use_default_dbc | ||
| cloudera.cloud.dw_database_catalog: | ||
| cluster_id : "{{ run__dw_list.clusters[0].id }}" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps pull out into a separate task that can ensure the cluster 0 exists, etc. -- the set_fact variable could be __default_dbc etc.
        
          
                roles/runtime/tasks/setup_aws.yml
              
                Outdated
          
        
      | - name: Set CDP DW Database catalog name to id map | ||
| when: __dw_dbc_build_async.database_catalogs is defined | ||
| ansible.builtin.set_fact: | ||
| run__dw_dbc_ids: "{{ run__dw_dbc_ids | default({}) | combine({ __dw_dbc_build_async.database_catalogs[0].name : __dw_dbc_build_async.database_catalogs[0].id}) }}" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Output of dw_database_catalog now returns a dict directly, not a list.
        
          
                roles/runtime/tasks/setup_aws.yml
              
                Outdated
          
        
      | - name: Create CDP DW Virtual warehouse | ||
| cloudera.cloud.dw_virtual_warehouse: | ||
| cluster_id: "{{ run__dw_list.clusters[0].id }}" | ||
| dbc_id: "{{ run__dw_dbc_ids[__dw_vw_config.dbc_name] if not __dw_vw_config.use_default_dbc else run__dw_list.clusters[0].dbcs[0].id}}" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be streamlined via the __default_dbc etc., also could use ternary as well
        
          
                roles/runtime/tasks/setup_aws.yml
              
                Outdated
          
        
      | type: "{{ __dw_vw_config.type }}" | ||
| name: "{{ __dw_vw_config.name }}" | ||
| template: "{{ __dw_vw_config.template }}" | ||
| autoscaling_min_nodes: "{{ __dw_vw_config.autoscaling_min_nodes | int }}" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the omit() that would be set in initialize task list should carry through here to the module call.
| 
 | 
f98c32d    to
    ea4323d      
    Compare
  
    | @Chaffelson: Rebased the PR and made changes to make it work with webster's PR for Cloudera cloud. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor change
| @wmudge : Can we get this PR merged ? Is there anything else we are waiting for ? | 
| 
 Not to my knowledge, provided the .cloud and cdpy dependencies are also clear. | 
| @raju-saravanan Just wanted to get the other changes into  | 
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
ea4323d    to
    84576b9      
    Compare
  
    Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
3765202    to
    bf69eed      
    Compare
  
    Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
31bc9e7    to
    ababc1e      
    Compare
  
    Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
2cce0be    to
    a85ea21      
    Compare
  
    | @wmudge: Added support for partial teardown. The user would have to declare 'dw.teardown' tag to delete a subset of database_catalogs or virtual_warehouse (this will not delete the cdw cluster itself), if this is not declared we will do a cascade delete of the entire cdw cluster. eg: You can mention vw and dbc to be deleted with its id or name as shown above. Found a small bug in dw_database_catalog.py raised a pr for this here (cloudera-labs/cloudera.cloud#33), which needs to be merged before this PR. | 
| Replaced by #102 | 
Changes in this PR:
Changes in this PR are done in conjunction with the following PRs: