Feat: Added functionalities to remove duplicate parameters and overwrite existing ones. #527

DannyLiCom · 2025-07-11T02:39:39Z

Fixes / Features

Added functionality to delete conflicting parameters and overwrite duplicate ones, ensuring user-defined settings take precedence.

Testing / Documentation

Can be tested when creating a cluster using XPK.

[ y/n ] Tests pass
[ y/n ] Appropriate changes to documentation are included in the PR

…gement, and added functionalities to remove duplicate parameters and overwrite existing ones.

…Also, made minor code fixes.

…ub.com/AI-Hypercomputer/xpk into lidanny/feature/extended_subnet_ranges

pawloch00 · 2025-07-18T10:14:40Z

@sharabiani ptal

DannyLiCom · 2025-07-21T05:17:42Z

This PR is from #506 . @SujeethJinesh These errors likely aren't related to my modifications. They seem to be primarily about IP conflicts and the inability to get-credentials for the cluster.

pawloch00 · 2025-07-22T06:17:43Z

The tests are failing and I don't think fault is on the tests side this time @DannyLiCom

DannyLiCom · 2025-07-23T04:28:21Z

@SujeethJinesh This issue likely stems from conflicts caused by the default network and IP. To pass the checks, we might need to include the creation of networks, subnetworks, firewalls, and so on, and then change the defaults to the new network.(Refer to here) This would involve a lot of related changes and modifications.

DannyLiCom · 2025-07-24T07:39:21Z

@wstcliyu Might need to help Sujeeth discuss this with Piotr. Thanks!

pawloch00 · 2025-07-25T08:17:13Z

src/xpk/commands/cluster.py

+  # After shlex.split: Print the tokens list
+  xpk_print(f'Shlex-split tokens: {tokens}')
+  i = 0
+  while i < len(tokens):


Looking again at this, this is custom loop, handling string with arguments. I find it hard to mantain and debug. It should be replaced with usage of some library is possible

I tried to modify argparse before, but I don't think it's flexible enough. You have to pre-define all the parameters it needs to parse.
https://screenshot.googleplex.com/8fpLsUdZMrouXXt

pawloch00 · 2025-07-25T08:18:28Z

src/xpk/commands/cluster.py

+        del final_gcloud_args[opposite_key]
+      final_gcloud_args[key] = True
+    elif key.startswith('--enable-'):
+      opposite_key = f'--no-{key[2:]}'


how are we guaranteed that the only keys which will have opposite effect are formed with disable or no prefixes?

Our current processing logic is indeed based on the observed gcloud and our custom argument conventions, meaning keys with opposite effects use prefixes like --no-, --enable-, or --disable-. However, it is indeed a potential risk point.
https://screenshot.googleplex.com/dhSjkYf9LWhGupu
Currently, I'm only referring to this source.
https://cloud.google.com/sdk/gcloud/reference/container/clusters/create

pawloch00 · 2025-07-25T08:20:40Z

src/xpk/commands/cluster.py

-    command += f' --addons={addons_str}'
+    conditional_params['--addons'] = ','.join(addons)
+
+  for key, value in conditional_params.items():


this loop could be separate function

Added merge_conditional_params function.

pawloch00 · 2025-07-25T08:20:52Z

src/xpk/commands/cluster.py

+  process_gcloud_args(user_parsed_args, final_gcloud_args)
+
+  command_parts = ['gcloud beta container clusters create', args.cluster]
+  for key, value in final_gcloud_args.items():


this loop could be separate function

Added construct_gcloud_command_string function.

pawloch00 · 2025-07-25T08:21:49Z

src/xpk/commands/cluster.py

+  final_gcloud_args['--master-ipv4-cidr'] = '172.16.0.32/28'
+  # This value is from here https://cloud.google.com/vpc/docs/subnets
+  final_gcloud_args['--cluster-ipv4-cidr'] = '10.224.0.0/12'
+  final_gcloud_args['--enable-private-nodes'] = True


shouldn't this be False?

If you want to use --master-ipv4-cidr, you must also include --enable-private-nodes, otherwise an error will occur.

error: ERROR: (gcloud.beta.container.clusters.create) Cannot specify --master-ipv4-cidr without --enable-private-nodes. It seems I might need to add --enable-private-nodes to execute python3 xpk/xpk.py cluster create-pathways.

pawloch00 · 2025-07-25T08:23:21Z

What is the exact reason for this PR?

…functions

…ub.com/AI-Hypercomputer/xpk into lidanny/feature/extended_subnet_ranges

DannyLiCom · 2025-07-28T09:10:17Z

What is the exact reason for this PR?

This is a task @SujeethJinesh assigned to me. Here are Sujeeth's objectives.
https://b.corp.google.com/issues/423650971

…ub.com/AI-Hypercomputer/xpk into lidanny/feature/extended_subnet_ranges

DannyLiCom · 2025-08-04T06:15:59Z

@SujeethJinesh This issue likely stems from conflicts caused by the default network and IP. To pass the checks, we might need to include the creation of networks, subnetworks, firewalls, and so on, and then change the defaults to the new network.(Refer to here) This would involve a lot of related changes and modifications.

@SujeethJinesh This issue still exists.

scaliby · 2025-08-12T10:00:17Z

src/xpk/commands/cluster.py

+        else:
+          parsed_args[token] = True
+    elif token.startswith('-'):
+      pass


This if branch could be removed.

scaliby · 2025-08-12T10:07:12Z

src/xpk/commands/cluster.py

+      final_gcloud_args[key] = value
+
+
+def merge_conditional_params(conditional_params, final_gcloud_args):


Please refactor this to not modify the final_gcloud_args input parameter, but copy it and return a copy.

scaliby · 2025-08-12T10:07:21Z

src/xpk/commands/cluster.py

+  return parsed_args
+
+
+def process_gcloud_args(user_parsed_args, final_gcloud_args):


Please refactor this to not modify the final_gcloud_args input parameter, but copy it and return a copy.

DannyLiCom · 2025-08-13T08:51:44Z

@SujeethJinesh @pawloch00
Main task: https://b.corp.google.com/issues/423650971
The current problem is a conflict caused by the default network of the test environment and IP.

The reason my manual tests had no issues is that I would first create the network, subnetwork, firewall and so on as a prerequisite before running the test.
https://screenshot.googleplex.com/4BGSx4yGJqz6Xdh
Sources: https://docs.google.com/document/d/1gpcNDkqCgWAQYN_KOb7M5eiXd_bquSvLeKjA966LJbk/edit?tab=t.0#heading=h.glahk2wkse7w

But if I were to put the creation of networks, etc., inside the xpk code, it feels a bit illogical, and it would likely require a lot of modifications.

The solutions I can think of are to either change the original default network or, as we just discussed, to add a method for creating networks inside the code.

I might need some advice.

DannyLiCom added 7 commits June 18, 2025 01:40

feat: Adding a de-dupe function

91b3ae1

Refactored GKE cluster creation to use a dictionary for argument mana…

bc47601

…gement, and added functionalities to remove duplicate parameters and overwrite existing ones.

feat: Add process_gcloud_args to handle custom gcloud arguments.

85f0982

feat: Added two unit tests.

ab3cd89

Update test_gcloud_arg_processor.py

928f693

feat: Updated the functionality of the process_gcloud_args function. …

e95b91f

…Also, made minor code fixes.

Resolve lint issue

1b7442c

DannyLiCom self-assigned this Jul 11, 2025

DannyLiCom requested a review from Obliviour as a code owner July 11, 2025 02:39

DannyLiCom added the release-improvements label Jul 11, 2025

DannyLiCom requested review from 44past4, sharabiani, pawloch00, BluValor, gcie and RoshaniN as code owners July 11, 2025 02:39

DannyLiCom and others added 5 commits July 11, 2025 02:40

Merge branch 'develop' into lidanny/feature/extended_subnet_ranges

e743dee

Added --enable-private-nodes.

9466df4

Merge branch 'lidanny/feature/extended_subnet_ranges' of https://gith…

4edc813

…ub.com/AI-Hypercomputer/xpk into lidanny/feature/extended_subnet_ranges

Merge branch 'develop' into lidanny/feature/extended_subnet_ranges

a827858

Merge branch 'develop' into lidanny/feature/extended_subnet_ranges

8fd41ca

pawloch00 mentioned this pull request Jul 18, 2025

Feat: Added functionalities to remove duplicate parameters and overwrite existing ones. #506

Closed

DannyLiCom added 2 commits July 21, 2025 03:54

Modify the command issue.

03f1fd6

Add the enable-ip-alias flag.

9cac631

Merge branch 'develop' into lidanny/feature/extended_subnet_ranges

5370d17

Merge branch 'develop' into lidanny/feature/extended_subnet_ranges

721a8ac

Merge branch 'develop' into lidanny/feature/extended_subnet_ranges

f1ca15b

pawloch00 reviewed Jul 25, 2025

View reviewed changes

DannyLiCom added 2 commits July 28, 2025 08:50

Add the merge_conditional_params and construct_gcloud_command_string …

3d728e3

…functions

Merge branch 'lidanny/feature/extended_subnet_ranges' of https://gith…

91eb0c2

…ub.com/AI-Hypercomputer/xpk into lidanny/feature/extended_subnet_ranges

DannyLiCom added 4 commits July 29, 2025 07:33

Merge branch 'develop' into lidanny/feature/extended_subnet_ranges

b433738

fix pyink

2aae966

Merge branch 'lidanny/feature/extended_subnet_ranges' of https://gith…

a53bf91

…ub.com/AI-Hypercomputer/xpk into lidanny/feature/extended_subnet_ranges

resolve conflicts

497bdef

Merge branch 'develop' into lidanny/feature/extended_subnet_ranges

5922023

DannyLiCom requested a review from scaliby as a code owner August 8, 2025 01:40

scaliby requested changes Aug 12, 2025

View reviewed changes

		final_gcloud_args[key] = value


		def merge_conditional_params(conditional_params, final_gcloud_args):

		return parsed_args


		def process_gcloud_args(user_parsed_args, final_gcloud_args):

Feat: Added functionalities to remove duplicate parameters and overwrite existing ones. #527

Are you sure you want to change the base?

Feat: Added functionalities to remove duplicate parameters and overwrite existing ones. #527

Uh oh!

Conversation

DannyLiCom commented Jul 11, 2025

Fixes / Features

Testing / Documentation

Uh oh!

pawloch00 commented Jul 18, 2025

Uh oh!

DannyLiCom commented Jul 21, 2025

Uh oh!

pawloch00 commented Jul 22, 2025

Uh oh!

DannyLiCom commented Jul 23, 2025

Uh oh!

DannyLiCom commented Jul 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawloch00 commented Jul 25, 2025

Uh oh!

DannyLiCom commented Jul 28, 2025

Uh oh!

DannyLiCom commented Aug 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DannyLiCom commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DannyLiCom commented Aug 13, 2025 •

edited

Loading