Add machine pool logic #1795

dimakis · 2023-01-18T15:21:56Z

Addition of some machine pool logic, which validates machine pool (replica) node count --but does not currently validate name, taint or labels
constructs the payload and executes

Closes #

Verification Steps

Do x
Do y

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation change
Other (please specify)

…ter endpoint in KFM

pkg/cmd/dedicated/register/registercluster.go

machi1990 · 2023-01-23T14:31:06Z

pkg/cmd/dedicated/register/registercluster.go

+func validateMachinePoolNodes(opts *options) error {
+	if len(opts.clusterMachinePoolList.Slice()) > 0 {
+		for _, machinePool := range opts.clusterMachinePoolList.Slice() {
+			if validateMachinePoolCount(machinePool.Replicas()) &&


The machinePool.Replicas() is only set for non autoscaled clusters.
For autoscaling clusters the value will we need to use here is max replicas.

I propose a functional change like

Suggested change

if validateMachinePoolCount(machinePool.Replicas()) &&

nodeCount := 0

replicas, ok := machinePool.GetReplicas()

if ok { // only retrieve the replica if not autoscaled

nodeCount = replicas

} else { // machine pool is autoscaled

autoscaledReplicas, ok := machinePool.GetAutoscaling()

if ok {

nodeCount, _ = autoscaledReplicas.GetMaxReplicas()

}

}

machi1990 · 2023-01-23T14:32:09Z

pkg/cmd/dedicated/register/registercluster.go

+		ClusterId:                     opts.selectedCluster.ID(),
+		ClusterExternalId:             opts.selectedCluster.ExternalID(),
+		ClusterIngressDnsName:         clusterIngressDNSName,
+		KafkaMachinePoolNodeCount:     int32(opts.selectedClusterMachinePool.Replicas()),


same comment as https://github.com/redhat-developer/app-services-cli/pull/1795/files#r1084128248 if ever the machine pool is autoscaled

pkg/cmd/dedicated/register/registercluster.go

machi1990 · 2023-01-23T14:41:07Z

pkg/cmd/dedicated/register/registercluster.go

+	response, r, err := resource.Execute()
+	if err != nil {
+		return err
+		return nil


Suggested change

return nil

pkg/cmd/dedicated/register/registercluster.go

machi1990 · 2023-01-23T14:46:31Z

pkg/shared/connection/api/defaultapi/default_client.go

+	connection, err := ocmSdkClient.NewConnectionBuilder().
+		Tokens(a.AccessToken).
+		Build()


This will always use the api.openshift.com as base url.
That's fine for Stage/Prod

but for testing we might need the .URL() customizable somehow so that we could use api.stage.openshift.com

machi1990 · 2023-01-23T14:48:23Z

pkg/shared/connection/api/defaultapi/default_client.go

+
+func (a *defaultAPI) CreateOCMConnection() (*ocmSdkClient.Connection, error) {
+	connection, err := ocmSdkClient.NewConnectionBuilder().
+		Tokens(a.AccessToken).


This is fine for Prod/Stage but for testing it might be great to have the token customizable since someone using kas-installer (https://github.com/bf2fc6cc711aee1a0c2a/kas-installer) might as well have KFM deployed using a different Auth server than the one used for OCM Stage/Prod (which is always sso.redhat.com)

machi1990 · 2023-01-23T14:48:47Z

pkg/shared/connection/api/defaultapi/default_client.go

+}
+
+// create an OCM clustermgmt client
+func (a *defaultAPI) OCMClustermgmt() (*ocmclustersmgmtv1.Client, error) {


I do not see this being used anywhere

machi1990 · 2023-01-23T14:51:17Z

pkg/cmd/dedicated/register/registercluster.go

+
+	//TODO add Localizer
+	cmd := &cobra.Command{
+		Use: "register-cluster",


This gives a command like

rhoas dedicated register-cluster

I am wondering if in the future we might consider

rhoas dedicated kafka register

pkg/cmd/dedicated/register/registercluster.go

machi1990 · 2023-01-23T16:07:07Z

pkg/cmd/dedicated/register/registercluster.go

+					return err
+				}
+			}
+		}


Suggested change

}

Related to https://github.com/redhat-developer/app-services-cli/pull/1795/files#r1084249926

pkg/cmd/dedicated/register/registercluster.go

machi1990 · 2023-01-23T16:12:40Z

pkg/cmd/dedicated/register/registercluster.go

+		return nil
+	}
+	opts.f.Logger.Info("response: ", response)
+	opts.f.Logger.Info("r: ", r)


To create the addon:

These method:

https://github.com/bf2fc6cc711aee1a0c2a/kas-fleet-manager/blob/d0c418df2eeea93d22aeb5cd476d28e8eadec71a/pkg/client/ocm/client.go#L231

https://github.com/bf2fc6cc711aee1a0c2a/kas-fleet-manager/blob/d0c418df2eeea93d22aeb5cd476d28e8eadec71a/pkg/client/ocm/client.go#L412

are useful.

The strimzi addon will have the id: managed-kafka and the parameters will be an empty list.
The fleetshard addon will have the id: kas-fleetshard-operator and the parameters will be from response.FleetshardParameters

Note: the addOns id changes depending on the environment. For Stage, they'll have a suffix of -qe e.g kas-fleetshard-operator-qe

pkg/cmd/dedicated/register/registercluster.go

machi1990

I've seen the demo of this working end to end. Thanks for the great work @dimakis

I'll let other experienced in the codebase have a look at the code.

dimakis · 2023-01-26T13:20:52Z

I've seen the demo of this working end to end. Thanks for the great work @dimakis

I'll let other experienced in the codebase have a look at the code.

Thanks a million for your great and thorough help on this. Always a pleasure working with you 😁

rkpattnaik780

Looks good! Dropped a few comments. It will be better to add more inline descriptions to the functions defined here.

pkg/cmd/dedicated/dedicated.go

pkg/shared/connection/api/defaultapi/default_client.go

rkpattnaik780 · 2023-01-27T06:15:15Z

pkg/shared/connection/api/defaultapi/default_client.go

@@ -40,7 +44,7 @@ type defaultAPI struct {
 }

 // New creates a new default API client wrapper
-func New(cfg *api.Config) api.API {
+func New(cfg *api.Config) *defaultAPI {


Can you briefly explain the purpose of this change?

pkg/shared/connection/api/defaultapi/default_client.go

pkg/cmd/dedicated/register/registercluster.go

rkpattnaik780 · 2023-01-27T07:01:41Z

pkg/cmd/dedicated/register/registercluster.go

@@ -0,0 +1,433 @@
+package register


Can we look if this file can be split into multiple?

this file should def be split up, generic cluster mgmt api logic etc needs to move into its own dir.

I was planning on doing this after delivering the MVP

rkpattnaik780

Needs some more localizations.

pkg/core/localize/locales/en/cmd/dedicated.en.toml

The base branch was changed.

* feat: addition of the new dedicated command to select ocm cluster * feat: addition of getting machine pools and hitting the register cluster endpoint in KFM * refactor: addition of close ocm connection * fix: addition of autoscaling check logic * refactor: addresses commments made in PR * refactor: addresses comments in review * feat: addition of the addon installation flow * feat: addition of cluster-id flag * refactor: addresses some further comments in the review * feat: addition of localization * refactor: addition of error when user has no valid clusters

* Enterprise crete kafka interactive (#1819) * refactor: moving list enterprise clusters to util package * feat: start of interactive enterprise create * refactor: refactor of create kafka ams checks for enterprise quota (#1822) * feat: when using dedicated with kafka create, we now check the capacity of the cluster * refactor: refactor of the interactive create kafka cmd * refactor: refactoring dedicated list to use refactored search string func * refactor: refactor and addition of name + id of cluster in prompt * refactor: refactor to functional create enterprise flow with some updates based on reviews * Add machine pool logic (#1795) * feat: addition of the new dedicated command to select ocm cluster * feat: addition of getting machine pools and hitting the register cluster endpoint in KFM * refactor: addition of close ocm connection * fix: addition of autoscaling check logic * refactor: addresses commments made in PR * refactor: addresses comments in review * feat: addition of the addon installation flow * feat: addition of cluster-id flag * refactor: addresses some further comments in the review * feat: addition of localization * refactor: addition of error when user has no valid clusters * chore(vendor): update vendoring (#1798) * Add cluster id to create kafka (#1804) * feat: addition of descriptions and examples to dedicated * feat: Allows the CLI to install stage env addons when terraforming customer-cloud dataplane clusters * refactor: the default api url is now set when creating a connection instead of in the registercluster cmd file * refactor: update some func and var names * refactor: add cluster-id flag to create kafka cmd to allow kafka creation on CC cluster * refactor: doc and small refactor * refactor: Refactor ocm methods out of register cmd file (#1807) * feat: addition of descriptions and examples to dedicated * feat: Allows the CLI to install stage env addons when terraforming customer-cloud dataplane clusters * refactor: extraction of clustermgmt funcs from register to own dir * refactor: addresses cluster pagination and refactor of ocm functions * feat: added deregister command * refactor: imrpoved quality of deregister command * chore: added localisation to deregister command * feat: deregister now waits for all kafkas to be deleted * fix: better logging when deleting kafka * refactor: getting cluster list now ask kfm then ocm * chore: remove unused fields * fix: edge case handling in deregister * docs: update the docs (#1826) * refactor: print the httpresponse (#1827) * fix: fixing broken lints * chore: rebase changes --------- Co-authored-by: Dimitri Saridakis <[email protected]> Co-authored-by: Ramakrishna Pattnaik <[email protected]>

dimakis added 2 commits January 18, 2023 10:59

feat: addition of the new dedicated command to select ocm cluster

1baae06

feat: addition of getting machine pools and hitting the register clus…

85958fc

…ter endpoint in KFM

dimakis force-pushed the add-machine-pool-logic branch from 190102e to 85958fc Compare January 23, 2023 14:07

machi1990 reviewed Jan 23, 2023

View reviewed changes