Added flag to select ProviderID format for Machine #580

Karthik-K-N · 2022-03-01T11:08:34Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

/area provider/ibmcloud

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

This PR introduce a flag providerid-format to select the ProviderID format to set to the machine

v1 version will have format, this is default provider id format: ibmpowervs://<cluster_name>/<vm_hostname>
v2 version will have format: ibmpowervs://<region>/<zone>/<service_instance_id>/<powervs_machine_id>

When we are setting the providerid-format to v2, We need to explicitly run the external cloud controller manager, which will take the responsibility of setting the ProviderID for node

netlify · 2022-03-01T11:08:41Z

✔️ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

🔨 Explore the source changes: c816b5b

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/621f841f0c2ae6000790cbbc

😎 Browse the preview: https://deploy-preview-580--kubernetes-sigs-cluster-api-ibmcloud.netlify.app

Karthik-K-N · 2022-03-01T11:09:21Z

/cc @mkumatag @Prajyot-Parab

mkumatag · 2022-03-01T12:02:39Z

api/v1beta1/ibmpowervsmachine_types.go

+	Region *string `json:"region,omitempty"`
+
+	// Zone specifies the Power VS Service instance zone
+	// +optional
+	Zone *string `json:"zone,omitempty"`


keeping them optional will be challenge because is user doesn't provide and then someone set v2 as provider version this will throw an NPE, wondering if we can make these params as required fields.

what other think? @Amulyam24 @Prajyot-Parab

@Karthik-K-N if required, lets setup a meeting to discuss this in detail

We can get the region and zone using the Power VS service instance id and currently its done like that we are not depending on user to provide it.

I think keeping it optional would be fine as the region and zone are always being set before setting the provider ID despite the provider version used.

if that's the case, I'm wondering if we can have these options in the status instead if it is never set by the user or if we don't take any action if user supplies them?

IMO, +1 to move it as part of status, as we have internal logic to deduce their values.

main.go

mkumatag · 2022-03-01T12:07:17Z

util/util.go

+var ProviderIDFormat string
+
+// ProviderIDFormatV2 will set provider id to machine as ibmpowervs://<region>/<zone>/<service_instance_id>/<powervs_machine_id>
+const ProviderIDFormatV2 = "v2"


wondering if we can move this to pkg/options package

mkumatag · 2022-03-01T14:06:40Z

main.go

+	fs.StringVar(
+		&options.PowerVSProviderIDFormat,
+		"powervs-provider-id-fmt",
+		"v1",
+		"ProviderID format is used set the Provider ID format for Machine",
+	)


this accepts any random value as of now, can we limit this to accept only v1 or v2

mkumatag · 2022-03-01T14:06:57Z

pkg/options/options.go

+var PowerVSProviderIDFormat string
+
+// PowerVSProviderIDFormatV2 will set provider id to machine as ibmpowervs://<region>/<zone>/<service_instance_id>/<powervs_machine_id>
+const PowerVSProviderIDFormatV2 = "v2"


can we add v1 constant as well?

Karthik-K-N · 2022-03-02T09:45:31Z

Review comments are being addressed in the latest commit and PR is ready for new review.

mkumatag

minor fmt issues, other lgtm

mkumatag · 2022-03-02T09:59:05Z

main.go

@@ -18,6 +18,8 @@ package main

 import (
 	"flag"
+	"fmt"
+	"k8s.io/klog/v2/klogr"


nit: move this k8s.io/klog/v2/klogr import to down below

mkumatag · 2022-03-02T10:00:31Z

main.go

@@ -68,6 +70,11 @@ func main() {

 	ctrl.SetLogger(klogr.New())

+	if err := validateFlags(); err != nil {
+		setupLog.Info("Flag validation failure", "error", err.Error())


how about using Error() if they have one

mkumatag · 2022-03-02T10:01:28Z

main.go

+	fs.StringVar(
+		&options.PowerVSProviderIDFormat,
+		"powervs-provider-id-fmt",
+		"v2",


can we use options.PowerVSProviderIDFormatV2 here instead of v2?

mkumatag · 2022-03-02T10:02:46Z

cloud/scope/powervs_machine.go

@@ -20,13 +20,12 @@ import (
 	"context"
 	"encoding/base64"
 	"fmt"
+	"sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/options"


nit: move this import to down

mkumatag

/lgtm
/approve
/retest
/hold for @Prajyot-Parab 's review

Prajyot-Parab · 2022-03-02T14:06:51Z

@Karthik-K-N do run make generate-go-conversions-core target command, let me know if it generates new changes to be pushed else PR changes lgtm!

Prajyot-Parab

/lgtm

k8s-ci-robot · 2022-03-02T15:13:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Karthik-K-N, mkumatag, Prajyot-Parab

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 · 2022-03-02T15:14:50Z

/unhold

k8s-ci-robot added area/provider/ibmcloud Issues or PRs related to ibmcloud provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 1, 2022

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 1, 2022

k8s-ci-robot requested review from mkumatag and relyt0925 March 1, 2022 11:08

k8s-ci-robot requested a review from Prajyot-Parab March 1, 2022 11:09

mkumatag requested changes Mar 1, 2022

View reviewed changes

mkumatag requested changes Mar 2, 2022

View reviewed changes

Karthik-K-N force-pushed the add-providerid-flag branch from 337878c to a363bf5 Compare March 2, 2022 10:27

Added flag to select ProviderID format for Machine

a363bf5

mkumatag approved these changes Mar 2, 2022

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2022

k8s-ci-robot assigned mkumatag Mar 2, 2022

k8s-ci-robot added lgtm "Looks good to me", 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 Mar 2, 2022

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2022

Added generated files

c816b5b

Prajyot-Parab approved these changes Mar 2, 2022

View reviewed changes

k8s-ci-robot assigned Prajyot-Parab Mar 2, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2022

mkumatag added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 2, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2022

k8s-ci-robot merged commit 78b9d0e into kubernetes-sigs:main Mar 2, 2022

Karthik-K-N deleted the add-providerid-flag branch July 12, 2022 06:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added flag to select ProviderID format for Machine #580

Added flag to select ProviderID format for Machine #580

Karthik-K-N commented Mar 1, 2022

netlify bot commented Mar 1, 2022 •

edited

Loading

Karthik-K-N commented Mar 1, 2022

mkumatag Mar 1, 2022

mkumatag Mar 1, 2022

Karthik-K-N Mar 1, 2022

Amulyam24 Mar 1, 2022

mkumatag Mar 1, 2022

Prajyot-Parab Mar 1, 2022

mkumatag Mar 1, 2022

mkumatag Mar 1, 2022

mkumatag Mar 1, 2022

Karthik-K-N commented Mar 2, 2022

mkumatag left a comment

mkumatag Mar 2, 2022

mkumatag Mar 2, 2022

mkumatag Mar 2, 2022

mkumatag Mar 2, 2022

mkumatag left a comment

Prajyot-Parab commented Mar 2, 2022

Prajyot-Parab left a comment

k8s-ci-robot commented Mar 2, 2022

mkumatag commented Mar 2, 2022

Added flag to select ProviderID format for Machine #580

Added flag to select ProviderID format for Machine #580

Conversation

Karthik-K-N commented Mar 1, 2022

netlify bot commented Mar 1, 2022 • edited Loading

Karthik-K-N commented Mar 1, 2022

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

Choose a reason for hiding this comment

Karthik-K-N commented Mar 2, 2022

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

mkumatag left a comment

Choose a reason for hiding this comment

Prajyot-Parab commented Mar 2, 2022

Prajyot-Parab left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 2, 2022

mkumatag commented Mar 2, 2022

netlify bot commented Mar 1, 2022 •

edited

Loading