Change NodePool CLI to have platform sub command #832

nirarg · 2022-01-10T07:30:04Z

Change the NodePool CLI to be platform aware
This change is done as part of the multy platform support,
to allow the CLI to provide platform specific UX where needed

nirarg · 2022-01-10T07:39:59Z

This PR is opened as continue to the discussion here #779 (comment)
@enxebre FYI

enxebre · 2022-01-10T18:01:41Z

cmd/nodepool/core/create.go

+	Render       bool
+}
+
+// ApplyPlatformSpecifics can be used to create platform specific values as well as enriching the fixure with additional values


This comment seems to refer to a different function.

enxebre · 2022-01-10T18:18:06Z

cmd/nodepool/aws/create.go

@@ -0,0 +1,114 @@
+package aws


please update the docs getting starting guide to reflect this changes

Updated the docs (added aws option)

enxebre · 2022-01-10T18:20:34Z

cmd/nodepool/aws/log.go

@@ -0,0 +1,7 @@
+package aws


I'm not sure where this pattern of having a single file for this variable is coming from?
I question we should keep perpetuating it? may @ironcladlou knows better where this is coming from/why we need it?

I think it's just a combination of organic changes and cargo culting. Quick analysis:

In the CLI we're relying on a package variable for the logging singleton (util.Log). Authors might use util.Log.Error(...) (for example) directly. Or alias it to a local so it would read more like log.Error(...) (what this PR is doing).

If we're okay fundamentally with the package variable for logger state (instead of e.g. passing logger instances around), we could move the package variable to a log package so that users get the concise log.Error(...) call semantics without doing aliasing.

I agree with the last suggestion
I can create new log package under cmd, to be used from all cmd sub packages

Changed it
Need to change also the other packages to use this new log package (another PR?)

enxebre · 2022-01-10T18:21:47Z

cmd/nodepool/kubevirt/create.go

+	// As long as there is no official container image
+	// The image must be provided by user
+	// Otherwise it must fail
+	if o.ContainerDiskImage == "" {


why do we have this here instead of making the field built-in required in the cli?

enxebre · 2022-01-10T18:36:33Z

cmd/nodepool/core/create.go

+}
+
+// ApplyPlatformSpecifics can be used to create platform specific values as well as enriching the fixure with additional values
+type CreateNodePoolPlatformOptions interface {


naming seems redundant given we are in nodepool/core/create, may be just
type PlatformOptions interface { Update() type() }
wdyt?

Renaming CreateNodePoolPlatformOptions to PlatformOptions and platformType to Type sound good to me, but I'm not sure about renaming UpdatePlatformParams to Update, because it can be interpreted as we this function is updating the PlatformOptions itself
What do you think about changing it to UpdateNodePool?

type PlatformOptions interface { // UpdateNodePool is used to update the platform specific values in the NodePool UpdateNodePool(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, client crclient.Client) error // Type returns the platform type Type() hyperv1.PlatformType }

enxebre · 2022-01-10T18:37:03Z

cmd/nodepool/core/create.go

+		return fmt.Errorf("failed to get HostedCluster %s/%s: %w", o.Namespace, o.Name, err)
+	}
+
+	if platformOpts != nil {


do we need this check? when would this be nil?

You right, it never should be nil, removed the validation

enxebre · 2022-01-10T20:40:20Z

cmd/nodepool/aws/create.go

+func CreateNodePool(ctx context.Context, coreOpts core.CreateNodePoolOptions, platformOpts AWSPlatformCreateOptions) error {
+	return coreOpts.CreateNodePool(ctx, platformOpts)
+}
+


can we add the lines to validate each platform type implements the interface?

Can you please elaborate on this? how do you suggest to add this validation?
Not all platform types implement the interface (Agent and None weren't added to the NodePool CLI)

Any platform implementing PlatformOptions i.e within cmd/nodepool/* needs to validate it satisfy the interface a build time, e.g https://github.com/openshift/hypershift/blob/main/hypershift-operator/controllers/hostedcluster/internal/platform/platform.go#L19-L22

Ok, I will add this
But why do we need it, as already we have the assigment of the implementation on the interface, which fail on build time if the implementation doesn't satisfy the interface
In this case, the call to the function this comment points to would fail in build time

enxebre · 2022-01-10T20:41:59Z

PTAL @rmohr

netlify · 2022-01-12T12:49:50Z

✔️ Deploy Preview for hypershift-docs ready!

🔨 Explore the source changes: ae6859e

🔍 Inspect the deploy log: https://app.netlify.com/sites/hypershift-docs/deploys/61e6a39911c6bf0007489f83

😎 Browse the preview: https://deploy-preview-832--hypershift-docs.netlify.app

ironcladlou · 2022-01-12T12:54:42Z

docs/content/getting-started.md


-hypershift create nodepool --cluster-name $CLUSTER_NAME \
+hypershift create nodepool aws \
+  --cluster-name $CLUSTER_NAME \


@enxebre is it valid for a NodePool's platform to differ from its owning HostedCluster?

Nope, today that's not valid as a design invariant, it's validated here and it'll fail if you try to https://github.com/openshift/hypershift/pull/832/files#diff-d2048aaf4e4de5ddfc058be0d46748ec7e8ab85ad4bc9d62b97dae0a41138020R42-R44

In a way having "platform" --cluster-name $CLUSTER_NAME is redundant though I think being explicit provides a consistent better UX and helper output than implementing some CLUSTER_NAME inferring magic would do. Let me known if you think otherwise.

I had a similar thought about how it seems weird to have to repeat the platform here given the system already knows the platform of --cluster-name... the validation you mentioned helps though. I was thinking repeating the platform here would only leave room for user error, but if it fails fast with a validation error, sounds fine to me.

Plus without subcommands, I'm not sure how we would sanely have per-platform arguments.

Thanks, I think it will be good!

Plus without subcommands, I'm not sure how we would sanely have per-platform arguments.

This is also floating around in my head for some time. In theory we could have a two-stage phase here, where we first determine the cluster type based on the name, and based on the detection register the right cobra commands.

That could for instance work by first doing minimal pflag parsing where only platform independend flags are recognized. Then when we know the platform we could just "register" the corresponding sub-program.

Apart from the fact that it may a little bit over-engineered, the downside with this is that --help could only provide helpful flag output if one would point it against a valid cluster, which sounds wrong.

That may still be overcome, by providing a kubectl-like help message. Where the help message would indicate that to see the corresponsing flags, one has to provide the cluster-type. For example:

$ hypershift nodepool --help Usage: The cluster and the commadnline flags are automatically detected based on the discovered cluster type. To see platform specific commandline flags run hypershift nodepool help aws hypershift nodepool help none [...]

I don't know about over-engineered, but it's a really interesting idea. The nodepool subcommand as currently designed is only intended to be used in the context of an existing hostedcluster, so making the CLI "adaptive" to the target hostedcluster is pretty interesting and clever...

I don't know about over-engineered, but it's a really interesting idea. The nodepool subcommand as currently designed is only intended to be used in the context of an existing hostedcluster, so making the CLI "adaptive" to the target hostedcluster is pretty interesting and clever...

Thinking it through again, it may indeed not even be complex to do. @ironcladlou @nirarg , what do you think?

to keep PR size and changes from existing code to a reasonable size I'd suggest we merge keeping existing approach and have a follow up PR to discuss this concretely?

Works for me.

enxebre · 2022-01-12T20:12:07Z

cmd/nodepool/aws/create.go

+
+func NewCreateCommand(coreOpts core.CreateNodePoolOptions) *cobra.Command {
+	platformOpts := AWSPlatformCreateOptions{
+		InstanceType:   "m4.large",


Let's update as in #850

enxebre · 2022-01-13T13:53:12Z

cmd/nodepool/core/create.go

+		return err
+	}
+
+	if o.Render {


how is this code run? if o.Render { wouldn't this func return in l92?

You right, this is never called
This code was already exists before my change
It seems that the "Render" code (writing bytes to os.Stdout) was written twice and the second call is unnecessary and can be removed
Am I right?

Removed this code

enxebre · 2022-01-13T14:05:47Z

lgtm other than #832 (comment)

enxebre · 2022-01-14T11:48:16Z

/lgtm

enxebre · 2022-01-14T11:51:41Z

Can you please add a PR/commit desc elaborating "why"? e.g Since we introduced multi platform support we want the CLI to be able to provide granular platform specific UX where needed"

nirarg · 2022-01-14T14:30:07Z

Can you please add a PR/commit desc elaborating "why"? e.g Since we introduced multi platform support we want the CLI to be able to provide granular platform specific UX where needed"

Done

Change the NodePool CLI to be platform aware This change is done as part of the multy platform support, to allow the CLI to provide platform specific UX where needed

enxebre · 2022-01-18T11:34:03Z

/approve

openshift-ci · 2022-01-18T11:34:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre, nirarg

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [enxebre]

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

enxebre · 2022-01-18T12:08:44Z

/lgtm

openshift-ci · 2022-01-18T12:13:54Z

@nirarg: all tests passed!

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

Fix three issues added in PR openshift#832: * Fix Global flags to use PersistentFlag insead Flag * Change all objects to be passed as pointers in order to have command values passed * In cmd/log the way passing keysAndValues parameter

cmd/log/log.go was added in PR openshift#832 Change all cmd modules to use this cmd logger This is done following to the discussion here: openshift#832 (comment)

Since changed the NodePool to be platform aware (PR openshift#832) In order to create NodePool for specific platform, the platform must implement specific sub command Therfore add the subcommands for None and Agent platforms, although the NodePool platformSpec is empty for those platforms

Since changed the NodePool to be platform aware (PR openshift#832) In order to create NodePool for specific platform, the platform must implement specific sub command Therfore add the subcommands for Agent platform, although the NodePool platformSpec is empty for those platforms

cmd/log/log.go was added in PR openshift#832 Change all cmd modules to use this cmd logger This is done following to the discussion here: openshift#832 (comment)

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2022

openshift-ci bot requested review from csrwng and sjenning January 10, 2022 07:30

enxebre reviewed Jan 10, 2022

View reviewed changes

nirarg force-pushed the nodepool-cmd-refactor branch from 2117cb0 to f66c396 Compare January 12, 2022 12:49

ironcladlou reviewed Jan 12, 2022

View reviewed changes

enxebre reviewed Jan 12, 2022

View reviewed changes

nirarg force-pushed the nodepool-cmd-refactor branch from f66c396 to 925287d Compare January 13, 2022 06:28

nirarg changed the title ~~[WIP] Change NodePool CLI to have platform sub command~~ Change NodePool CLI to have platform sub command Jan 13, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 13, 2022

enxebre reviewed Jan 13, 2022

View reviewed changes

nirarg force-pushed the nodepool-cmd-refactor branch from 925287d to 7da4d3c Compare January 13, 2022 16:07

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2022

nirarg force-pushed the nodepool-cmd-refactor branch from 7da4d3c to cc25ba3 Compare January 14, 2022 08:44

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2022

openshift-ci bot assigned enxebre Jan 14, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2022

nirarg force-pushed the nodepool-cmd-refactor branch from cc25ba3 to 444ab43 Compare January 14, 2022 14:29

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2022

NodePool CLI: change NodePool CLI to have platform sub command

ae6859e

Change the NodePool CLI to be platform aware This change is done as part of the multy platform support, to allow the CLI to provide platform specific UX where needed

nirarg force-pushed the nodepool-cmd-refactor branch from 444ab43 to ae6859e Compare January 18, 2022 11:25

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2022

openshift-merge-robot merged commit b804e39 into openshift:main Jan 18, 2022

nirarg mentioned this pull request Jan 26, 2022

Fix NodePool CLI #922

Merged

4 tasks

nirarg mentioned this pull request Feb 2, 2022

cmd logger: Change all cmd modules to use the same logger #953

Merged

4 tasks

nirarg mentioned this pull request Feb 2, 2022

Add Agent platform type under NodePool cmd #956

Merged

4 tasks

Change NodePool CLI to have platform sub command #832

Change NodePool CLI to have platform sub command #832

Uh oh!

Conversation

nirarg commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nirarg commented Jan 10, 2022

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!

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!

enxebre commented Jan 10, 2022

Uh oh!

netlify bot commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

rmohr Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

nirarg commented Jan 10, 2022 •

edited

Loading

netlify bot commented Jan 12, 2022 •

edited

Loading

rmohr Jan 13, 2022 •

edited

Loading