Skip to content

Tagging Hypershift clusters for tracking#157

Closed
krishvoor wants to merge 6 commits into
cloud-bulldozer:mainfrom
krishvoor:tagging
Closed

Tagging Hypershift clusters for tracking#157
krishvoor wants to merge 6 commits into
cloud-bulldozer:mainfrom
krishvoor:tagging

Conversation

@krishvoor

Copy link
Copy Markdown
Member

No description provided.

krishvoor and others added 2 commits June 7, 2023 19:43
@krishvoor krishvoor requested review from dry923 and morenod June 7, 2023 14:14
@vishnuchalla vishnuchalla self-requested a review June 7, 2023 14:36

@jtaleric jtaleric left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@krishvoor

Copy link
Copy Markdown
Member Author

When we run this wrapper from our forked repo: this will look like this:


2023-06-07 14:56:38.359 DEBUG rosa-hosted-wrapper - _build_cluster: Attempting cluster installation
2023-06-07 14:56:38.359 DEBUG rosa-hosted-wrapper - _build_cluster: Output directory set to /tmp/bea96cda-815c-4d0d-804e-dc5d44aa2b9b/chktag-njc-0001
2023-06-07 14:56:38.361 DEBUG rosa-hosted-wrapper - _build_cluster: ['/root/rosa/rosa', 'create', 'cluster', '--cluster-name', 'chktag-njc-0001', '--tags=User:krishvoor', '--replicas', '3', '--hosted-cp', '--sts', '--mode', 'auto', '-y', '--output', 'json', '--oidc-config-id', 'abcd', '--subnet-ids', 'subnet-XXX,subnet-YYY,subnet-ZZZ,subnet-xxx,subnet-yyy,subnet-zzz', '--properties', 'provision_shard_id:8c3bcd1f-xxxx', '--version', '4.12.15', '--operator-roles-prefix', 'chktag-njc']

@morenod

morenod commented Jun 7, 2023

Copy link
Copy Markdown
Contributor

You can do the same just passing the tag you want on the extra params.

This change requires that the command will be launched from a git based folder, which is not mandatory at this moment

@morenod morenod left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we want to add this, we need to consider the option of running wrapper from outside a git based folder.

Anyway, I still recommend to use the extra-params option instead of this

@krishvoor

krishvoor commented Jun 8, 2023

Copy link
Copy Markdown
Member Author

You can do the same just passing the tag you want on the extra params.

We need this tag <User:GITHUB_USERNAME> for better consolidation of cloud-expenses as defined here, and users will not have to pass tags for every-run

However, will implement supplying extra-tags via argsparse in case.

Comment thread rosa-hypershift/rosa-hosted-wrapper.py Outdated
Comment on lines +1113 to +1116
# Get the Git remote URL for the current working directory
old_wd = os.getcwd()
current_wd = os.path.dirname(os.path.abspath(__file__))
os.chdir(current_wd)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@morenod This tackles the issue: even if the user triggers the wrapper script outside the GitHub repo.

Comment thread rosa-hypershift/rosa-hosted-wrapper.py Outdated
else:
GITHUB_USERNAME = "cloud-bulldozer"

cluster_cmd = [rosa_cmnd, "create", "cluster", "--cluster-name", cluster_name, f"--tags=User:{GITHUB_USERNAME}", "--replicas", str(worker_nodes), "--hosted-cp", "--sts", "--mode", "auto", "-y", "--output", "json", "--oidc-config-id", oidc_config_id]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@krishvoor I honeslty don't think that the github user is the most useful tag to apply here. I'd rather we take in a string from the CLI to tag whatever we need it to be, ie some financial tracking, username, or whatever

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@krishvoor I honeslty don't think that the github user is the most useful tag to apply here.

@dry923 I hear you, today the cloud expenses are tracked via GitHub user details just like the one defined here via ROSA DAGs and have proved useful.

I'd rather we take in a string from the CLI to tag whatever we need it to be, ie some financial tracking, username, or whatever

Agreed, will implement changes through with this as an additional change.

@morenod

morenod commented Jun 9, 2023

Copy link
Copy Markdown
Contributor

Im not saying I want to tag, but if we are going to do it, we need to also add this tags on:

  • vpcs created by terraform
  • oidc-config objects
  • operator-roles
  • machinepool

@krishvoor

Copy link
Copy Markdown
Member Author

Im not saying I want to tag, but if we are going to do it, we need to also add this tags on:

  • vpcs created by terraform
  • oidc-config objects
  • operator-roles
  • machinepool

Thanks, @morenod this would mean it will be easy to find all "resources" in the AWS belonging to the same cluster.

krishvoor and others added 2 commits June 14, 2023 18:48
@krishvoor

krishvoor commented Jun 15, 2023

Copy link
Copy Markdown
Member Author

@morenod @dry923: Based on the discussions above, I have implemented
--extra-tags parameter will also tag operator-roles, machinepool, oidc-config objects & vpcs.
as w/ --wildcard_options we will only be limited to tag resources created during the _build_cluster() function.

Signed-off-by: Krishna Harsha Voora <krvoora@redhat.com>
@vishnuchalla

vishnuchalla commented Jun 15, 2023

Copy link
Copy Markdown

@krishvoor Can you please attach the steps in detail on how these changes were tested and verified? I am somehow unable to reproduce the steps mentioned in README.md

logging.debug("Creating cluster on VPC %s, with subnets: %s" % (vpc_info[0], vpc_info[1]))
try:
thread = threading.Thread(target=_build_cluster, args=(ocm_cmnd, rosa_cmnd, cluster_name_seed, args.must_gather_all, args.provision_shard, args.create_vpc, vpc_info, args.workers_wait_time, args.add_cluster_load, args.cluster_load_duration, jobs, workers, my_path, my_uuid, loop_counter, es, args.es_url, args.es_index, args.es_index_retry, service_cluster, sc_kubeconfig_path, all_clusters_installed, oidc_config_id, args.workload_type, args.kube_burner_version, args.e2e_git_details, args.git_branch, operator_roles_prefix))
thread = threading.Thread(target=_build_cluster, args=(ocm_cmnd, rosa_cmnd, cluster_name_seed, args.must_gather_all, args.provision_shard, args.create_vpc, vpc_info, args.workers_wait_time, args.add_cluster_load, args.cluster_load_duration, jobs, workers, my_path, my_uuid, loop_counter, es, args.es_url, args.es_index, args.es_index_retry, service_cluster, sc_kubeconfig_path, all_clusters_installed, oidc_config_id, args.workload_type, args.kube_burner_version, args.e2e_git_details, args.git_branch, operator_roles_prefix, tagging))

@vishnuchalla vishnuchalla Jun 15, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we can use **kwargs or dictionaries here instead of passing these many arguments in the params.

@krishvoor

Copy link
Copy Markdown
Member Author

Closing this in favor of #162

@krishvoor krishvoor closed this Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants