Adds support for VPC #200

prabhav-thali · 2025-01-09T06:31:17Z

Adds support for VPC resources. By default, make install-deployer-tf will build for powervs. To use vpc provider pass --provider vpc in kubetest2 tf command. This provider value is mapped to TargetProvider variable in deployer.go which allows to select vpc provider. By default, it is powervs.

ppc64le-cloud-bot · 2025-01-09T06:31:26Z

Welcome @prabhav-thali! It looks like this is your first PR to ppc64le-cloud/kubetest2-plugins 🎉

ppc64le-cloud-bot · 2025-01-09T06:31:28Z

Hi @prabhav-thali. Thanks for your PR.

I'm waiting for a ppc64le-cloud member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

mkumatag · 2025-01-09T06:40:45Z

can you please post the ansible code outside of this PR?

prabhav-thali · 2025-01-09T06:41:48Z

Yes, sure. I'll raise another PR to update ansible submodule

mkumatag · 2025-01-09T06:52:50Z

Makefile

@@ -14,7 +15,7 @@ INSTALL_DIR?=$(shell $(REPO_ROOT)/hack/goinstalldir.sh)
 # the output binary name, overridden when cross compiling
 BINARY_NAME=kubetest2-tf
 BINARY_PATH=./kubetest2-tf
-BUILD_FLAGS=-trimpath -ldflags="-buildid= -X=github.com/ppc64le-cloud/kubetest2-plugins/kubetest2-tf/deployer.GitTag=$(COMMIT)"
+BUILD_FLAGS=-trimpath -ldflags="-buildid= -X=github.com/ppc64le-cloud/kubetest2-plugins/kubetest2-tf/deployer.GitTag=$(COMMIT) -X=github.com/ppc64le-cloud/kubetest2-plugins/kubetest2-tf/deployer.TargetProvider=$(PROVIDER)"


already build mechanism is not that great for this project, lets not add one more to it, lets decide the provider based some flag runtime instead of buildtime.

I will the code post this change.

Let me check how we can handle this during runtime instead of buildtime.

mkumatag

overall lgtm except few minor comments

mkumatag · 2025-01-10T07:01:54Z

kubetest2-tf/deployer/deployer.go

@@ -81,6 +82,7 @@ type deployer struct {
 	Playbook              string            `desc:"name of ansible playbook to be run"`
 	ExtraVars             map[string]string `desc:"Passes extra-vars to ansible playbook, enter a string of key=value pairs"`
 	SetKubeconfig         bool              `desc:"Flag to set kubeconfig"`
+	TargetProvider		  string			`json:"provider value to be used"`


Suggested change

TargetProvider string `json:"provider value to be used"`

TargetProvider string `desc:"provider value to be used(powervs, vpc)"`

Implemented this

mkumatag · 2025-01-10T07:04:48Z

kubetest2-tf/deployer/deployer.go

+	if targetProvider == "" {
+		targetProvider = os.Getenv("TARGET_PROVIDER")
+	}
+	if targetProvider == "" {
+		return fmt.Errorf("provider not specified. Use --provider or set the TARGET_PROVIDER environment variable")
+	}


lets set the default value as powervs and remove this check and also lets avoid reading from env for now.

Yes, kept powervs as default

mkumatag · 2025-01-10T07:05:33Z

kubetest2-tf/deployer/deployer.go

@@ -142,6 +159,7 @@ func New(opts types.Options) (types.Deployer, *pflag.FlagSet) {
 	}
 	klog.InitFlags(nil)
 	flagSet.AddGoFlagSet(goflag.CommandLine)
+	flagSet.StringVar(&d.TargetProvider, "target-provider", "powervs", "The provider to use (vpc or powervs)")


this is not required if you use desc as above

instead add an entry here https://github.com/mkumatag/kubetest2-plugins/blob/df90d8a23d4dadc5fa18cc18ba3e191f181472e4/kubetest2-tf/deployer/deployer.go#L137 to set the default as follows

SetKubeconfig: true, TargetProvider: "powervs"

Implemented the change.

prabhav-thali · 2025-01-10T07:20:59Z

Thanks for the suggestions. I will post an update here once review comments are implemented and testing is complete.

mkumatag · 2025-01-10T07:22:55Z

Adds support for VPC resources. By default, make install-deployer-tf will build for powervs. To build the tool for VPC usage, pass PROVIDER=vpc in the make command make install-deployer-tf PROVIDER=vpc. This PROVIDER value is mapped to TargetProvider variable in deployer.go which helps to decide on vpc or powervs.

change this description as well in the end

mkumatag · 2025-01-10T07:23:07Z

/ok-to-test

mkumatag · 2025-01-10T11:09:19Z

kubetest2-tf/deployer/deployer.go

@@ -81,6 +82,7 @@ type deployer struct {
 	Playbook              string            `desc:"name of ansible playbook to be run"`
 	ExtraVars             map[string]string `desc:"Passes extra-vars to ansible playbook, enter a string of key=value pairs"`
 	SetKubeconfig         bool              `desc:"Flag to set kubeconfig"`
+	TargetProvider		  string			`flag:"~target-provider" desc:"provider value to be used(powervs, vpc)"`


I think flag tag is not required, can you check ones?

and also for all the go files you have added

Removed the flag tag.

mkumatag · 2025-01-10T11:09:47Z

kubetest2-tf/deployer/deployer.go

@@ -135,6 +143,7 @@ func New(opts types.Options) (types.Deployer, *pflag.FlagSet) {
 		RetryOnTfFailure: 1,
 		Playbook:         "install-k8s.yml",
 		SetKubeconfig:    true,
+		TargetProvider: "powervs",


please run the go fmt on this file

Yes formatted the files.

mkumatag

/lgtm
/hold
for @Rajalakshmi-Girish @Prajyot-Parab 's review

mkumatag · 2025-01-10T13:08:23Z

/cc @Prajyot-Parab

Prajyot-Parab · 2025-01-16T11:27:15Z

/cc @Prajyot-Parab

will review by today.

Prajyot-Parab · 2025-01-16T18:01:52Z

@prabhav-thali

Please run terraform fmt to format the tf files.
Also the folder structure has vpc dir inside vpc (vpc -> vpc), maybe try to rename the parent folder or (if possible see if you can optimize the overall vpc code at same level)

Rajalakshmi-Girish

As @Prajyot-Parab mentioned, it might be better to avoid a folder structure like having a 'vpc' directory inside another 'vpc'. Otherwise, it looks good to me based on my basic understanding of Terraform, considering the structure and resource creation in the Terraform files at a high level.
/lgtm

ppc64le-cloud-bot · 2025-01-17T09:59:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mkumatag, prabhav-thali, Rajalakshmi-Girish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mkumatag]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mkumatag · 2025-01-17T14:28:19Z

@prabhav-thali

Please run terraform fmt to format the tf files.

Also the folder structure has vpc dir inside vpc (vpc -> vpc), maybe try to rename the parent folder or (if possible see if you can optimize the overall vpc code at same level)

Please fix these comments, will merge the PR

ppc64le-cloud-bot requested review from mkumatag and Rajalakshmi-Girish January 9, 2025 06:31

ppc64le-cloud-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 9, 2025

ppc64le-cloud-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 9, 2025

prabhav-thali force-pushed the add-s390x branch from 8d159c4 to 873805b Compare January 9, 2025 06:46

mkumatag requested changes Jan 9, 2025

View reviewed changes

mkumatag requested changes Jan 10, 2025

View reviewed changes

ppc64le-cloud-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 10, 2025

prabhav-thali force-pushed the add-s390x branch from f64abd9 to cfc45ab Compare January 10, 2025 10:15

mkumatag requested changes Jan 10, 2025

View reviewed changes

add s390x support

3a4f035

prabhav-thali force-pushed the add-s390x branch from cfc45ab to 3a4f035 Compare January 10, 2025 12:38

mkumatag approved these changes Jan 10, 2025

View reviewed changes

ppc64le-cloud-bot assigned mkumatag Jan 10, 2025

ppc64le-cloud-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 10, 2025

ppc64le-cloud-bot requested a review from Prajyot-Parab January 10, 2025 13:08

Rajalakshmi-Girish approved these changes Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for VPC #200

Adds support for VPC #200

prabhav-thali commented Jan 9, 2025 •

edited

Loading

ppc64le-cloud-bot commented Jan 9, 2025

ppc64le-cloud-bot commented Jan 9, 2025

mkumatag commented Jan 9, 2025

prabhav-thali commented Jan 9, 2025

mkumatag Jan 9, 2025

mkumatag Jan 9, 2025

prabhav-thali Jan 9, 2025

mkumatag left a comment

mkumatag Jan 10, 2025

prabhav-thali Jan 10, 2025

mkumatag Jan 10, 2025

prabhav-thali Jan 10, 2025

mkumatag Jan 10, 2025

mkumatag Jan 10, 2025

prabhav-thali Jan 10, 2025

prabhav-thali commented Jan 10, 2025

mkumatag commented Jan 10, 2025

mkumatag commented Jan 10, 2025

mkumatag Jan 10, 2025

mkumatag Jan 10, 2025

prabhav-thali Jan 10, 2025

mkumatag Jan 10, 2025

prabhav-thali Jan 10, 2025

mkumatag left a comment

mkumatag commented Jan 10, 2025

Prajyot-Parab commented Jan 16, 2025

Prajyot-Parab commented Jan 16, 2025

Rajalakshmi-Girish left a comment •

edited

Loading

ppc64le-cloud-bot commented Jan 17, 2025

mkumatag commented Jan 17, 2025

	TargetProvider string `json:"provider value to be used"`
	TargetProvider string `desc:"provider value to be used(powervs, vpc)"`

Adds support for VPC #200

Are you sure you want to change the base?

Adds support for VPC #200

Conversation

prabhav-thali commented Jan 9, 2025 • edited Loading

ppc64le-cloud-bot commented Jan 9, 2025

ppc64le-cloud-bot commented Jan 9, 2025

mkumatag commented Jan 9, 2025

prabhav-thali commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkumatag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prabhav-thali commented Jan 10, 2025

mkumatag commented Jan 10, 2025

mkumatag commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkumatag left a comment

Choose a reason for hiding this comment

mkumatag commented Jan 10, 2025

Prajyot-Parab commented Jan 16, 2025

Prajyot-Parab commented Jan 16, 2025

Rajalakshmi-Girish left a comment • edited Loading

Choose a reason for hiding this comment

ppc64le-cloud-bot commented Jan 17, 2025

mkumatag commented Jan 17, 2025

prabhav-thali commented Jan 9, 2025 •

edited

Loading

Rajalakshmi-Girish left a comment •

edited

Loading