Improve the cmd: get clusters #449

cmoulliard · 2024-11-14T10:05:57Z

Improve the cmd: get clusters to show the cluster, nodes and their internal IP.
Created a help function to access globally to the kube client.
Add a new parameter to set the kubeconfig path

./idpbuilder -h
Manage reference IDPs

Usage:
  idpbuilder [command]

Available Commands:
  completion  Generate the autocompletion script for the specified shell
  create      (Re)Create an IDP cluster
  delete      Delete an IDP cluster
  get         get information from the cluster
  help        Help about any command
  version     Print idpbuilder version and environment info

Flags:
      --color              Enable colored log messages.
  -h, --help               help for idpbuilder
      --kubePath string    kube config file Path.

Command currently shows:

Cluster: my-konflux
URL of the kube API server: https://127.0.0.1:63473
TLS Verify: false
  Node: my-konflux-control-plane
  Internal IP: 10.89.0.2
----------

Add list clusters command #445

… internal IP. Created a help function to create globally a kube client. cnoe-io#445 Signed-off-by: cmoulliard <[email protected]>

Signed-off-by: cmoulliard <[email protected]>

cmoulliard · 2024-11-14T11:58:09Z

If several clusters/contexts/nodes have been created, then the command will output the following information:

Cluster: dabou
URL of the kube API server: https://127.0.0.1:53369
TLS Verify: false
External Port: 9443

Node: dabou-control-plane
Internal IP: 10.89.0.4
Capacity of the node: 
  cpu                  8
  memory               12.66 GB
  pods                 110
Allocated resources on node:
  CPU Requests: 1050m
  Memory Requests: 380Mi
----------------------------------------
Cluster: my-konflux
URL of the kube API server: https://127.0.0.1:63473
TLS Verify: false
External Port: 8443

Node: my-konflux-control-plane
Internal IP: 10.89.0.2
Capacity of the node: 
  cpu                  8
  memory               12.66 GB
  pods                 110
Allocated resources on node:
  CPU Requests: 2820m
  Memory Requests: 6898Mi
----------------------------------------

Signed-off-by: cmoulliard <[email protected]>

cmoulliard · 2024-11-15T11:13:51Z

I refactored the code to show the clusters using a Table

❯ ./idpbuilder get clusters
Nov 15 12:12:21 INFO enabling experimental podman provider 
NAME         EXTERNAL-PORT   KUBE-API                  TLS     KUBE-PORT
my-konflux   8443            https://127.0.0.1:65073   false   6443
test         9443            https://127.0.0.1:53454   false   6443

@nabuskey

Signed-off-by: cmoulliard <[email protected]>

nabuskey · 2024-11-21T16:36:22Z

pkg/cmd/helpers/k8s.go

Once the static password PR merges, we could merge these into the util package. No action needed for this PR imo

pkg/cmd/root.go

nabuskey · 2024-11-21T16:49:18Z

pkg/cmd/get/clusters.go

+		c, found := findClusterByName(config, "kind-"+cluster)
+		if !found {
+			//logger.Error(nil, fmt.Sprintf("Cluster not found: %s within kube config file\n", cluster))
+			return nil, err


Log and continue here instead? Even if client doesn't exist for some reason, we can find other valid clusters.

Yes. This is better to continue and to log a message. See: 031e81b

pkg/cmd/get/clusters.go

nabuskey · 2024-11-21T17:26:08Z

pkg/cmd/get/clusters.go

+					//logger.Error(err, "failed to get the allocated resources.")
+					return nil, err
+				}
+				aNode.Allocated = allocated


Are we doing anything with aNode? Looks like it's discarded each iteration?

Good question. Do we want part of the command "get clusters" to also show the allocated or available resources OR this is something that we will only show when we will display the detail of a cluster ?

Hmm it would be nice to have but I'd say it's not critical since you can get that information with kubectl describe nodes command.

The big plus of our command is that we could show everything with one command (= developer like that) in one place vs running several kubectl commands ;-)

I don't see aNode value used for anything here still.

Added a new column to list the nodes

NAME EXTERNAL-PORT KUBE-API TLS KUBE-PORT NODES my-konflux 8443 https://127.0.0.1:57063 false 6443 my-konflux-control-plane

See: 9fc54e2

Signed-off-by: cmoulliard <[email protected]>

…ubeconfig file BUT we log a message Signed-off-by: cmoulliard <[email protected]>

Signed-off-by: cmoulliard <[email protected]>

cmoulliard · 2024-11-27T07:39:56Z

Can we merge it ? @nabuskey

nabuskey · 2024-11-27T18:39:43Z

pkg/cmd/get/clusters.go

+
+	detectOpt, err := util.DetectKindNodeProvider()
+	if err != nil {
+		//logger.Error(err, "failed to detect the provider.")


remove commented out logger message like this

Removed the comments.

See: 9fc54e2

nabuskey · 2024-11-27T18:43:25Z

pkg/cmd/root.go

@@ -21,6 +21,7 @@ var rootCmd = &cobra.Command{
 func init() {
 	rootCmd.PersistentFlags().StringVarP(&helpers.LogLevel, "log-level", "l", "info", helpers.LogLevelMsg)
 	rootCmd.PersistentFlags().BoolVar(&helpers.ColoredOutput, "color", false, helpers.ColoredOutputMsg)
+	rootCmd.PersistentFlags().StringVarP(&helpers.KubeConfigPath, "kubeconfig", "", "", "kube config file Path.")


This is not good. This means kubeconfig flag is available to ALL commands. While this is desirable in the future, this is not supported by all commands yet. This is going to cause a lot of confusions.

Either:

Move this to the get cluster command only. OR

Remove this flag entirely and do this in a separate PR.

I moved the flag to the get command: See: dffda70

nabuskey · 2024-11-27T19:04:25Z

pkg/cmd/get/clusters.go

+	service := corev1.Service{}
+	namespacedName := types.NamespacedName{
+		Name:      "kubernetes",
+		Namespace: "default",
+	}
+	err := cli.Get(context.TODO(), namespacedName, &service)
+	if err != nil {
+		return 0, fmt.Errorf("failed to get the kubernetes default service on the cluster. %w", err)
+	}


I think it's better to look at the localbuild object instead.
I see you are doing port.Name != "" && strings.HasPrefix(port.Name, "https"). But this is not guaranteed to be available. For example, if you run the create command with --protocol http, the https port returned is incorrect.

So I'd rather look at the spec.buildCustomization field of the localbuild object.

idpbuilder/api/v1alpha1/localbuild_types.go

Lines 52 to 59 in 03559e6

type BuildCustomizationSpec struct {

Protocol string `json:"protocol,omitempty"`

Host string `json:"host,omitempty"`

IngressHost string `json:"ingressHost,omitempty"`

Port string `json:"port,omitempty"`

UsePathRouting bool `json:"usePathRouting,omitempty"`

SelfSignedCert string `json:"selfSignedCert,omitempty"`

}

I pushed a commit f1d03fa & 8f4810c to grab the Localbuild of the cluster

nabuskey · 2024-11-27T19:09:27Z

pkg/cmd/get/clusters.go

+	// Display the total allocated resources
+	//fmt.Printf("  CPU Requests: %s\n", totalCPU.String())
+	//fmt.Printf("  Memory Requests: %s\n", totalMemory.String())
+


Remove these

nabuskey · 2024-11-27T19:12:26Z

pkg/cmd/get/clusters.go

+					//logger.Error(err, "failed to get the allocated resources.")
+					return nil, err
+				}
+				aNode.Allocated = allocated


I don't see aNode value used for anything here still.

nabuskey · 2024-11-27T19:16:04Z

pkg/cmd/get/clusters.go

+	}
+
+	// Populate a list of Kube client for each cluster/context matching a idpbuilder cluster
+	manager, _ := CreateKubeClientForEachIDPCluster(config, clusters)


Need to handle error here. If an error is returned and manager is nil, we will panic.

…of nodes Signed-off-by: cmoulliard <[email protected]>

Signed-off-by: cmoulliard <[email protected]>

nabuskey · 2024-12-02T15:47:28Z

/e2e

nabuskey

LGTM

cmoulliard added 3 commits November 14, 2024 11:03

Improve the cmd: get clusters to show the cluster and nodes and their…

7eee957

… internal IP. Created a help function to create globally a kube client. cnoe-io#445 Signed-off-by: cmoulliard <[email protected]>

Show the kube API server URL, if TLS is checked. cnoe-io#445

6e05aec

Signed-off-by: cmoulliard <[email protected]>

Printing the node capacity and allocated resources

aca1830

Signed-off-by: cmoulliard <[email protected]>

cmoulliard added 5 commits November 14, 2024 13:10

Fix fmt.Printf error due to wrong var used

0dad331

Signed-off-by: cmoulliard <[email protected]>

Print the external port of the ingress controller of the current ctx

7cab4bf

Signed-off-by: cmoulliard <[email protected]>

Populate a list of kuybe clients; one per cluster/context

0207f16

Signed-off-by: cmoulliard <[email protected]>

Added a function to get the internal port of the kube API

d7c1247

Signed-off-by: cmoulliard <[email protected]>

Refactor the code to display the data using go cli-runtime table

3a41ab4

Signed-off-by: cmoulliard <[email protected]>

cmoulliard added 3 commits November 15, 2024 13:57

Review log messages

717388b

Signed-off-by: cmoulliard <[email protected]>

Review log messages

05cb831

Signed-off-by: cmoulliard <[email protected]>

Return the error instead of logging it.

489bd45

Signed-off-by: cmoulliard <[email protected]>

cmoulliard marked this pull request as ready for review November 21, 2024 13:48

cmoulliard requested a review from a team as a code owner November 21, 2024 13:48

nabuskey reviewed Nov 21, 2024

View reviewed changes

cmoulliard added 3 commits November 22, 2024 12:28

Rename flag from kubePath to kubeconfig

1f99c48

Signed-off-by: cmoulliard <[email protected]>

We continue even if the idpbuilder cluster is not found part of the k…

031e81b

…ubeconfig file BUT we log a message Signed-off-by: cmoulliard <[email protected]>

Remove populate function as non needed and look it up in map directly

68bc257

Signed-off-by: cmoulliard <[email protected]>

nabuskey reviewed Nov 27, 2024

View reviewed changes

nabuskey requested changes Nov 27, 2024

View reviewed changes

cmoulliard added 4 commits November 28, 2024 16:56

Remove comments. Handle the error. Add a new column to show the list …

9fc54e2

…of nodes Signed-off-by: cmoulliard <[email protected]>

Moving the kubeconfig flag from root to get command

dffda70

Signed-off-by: cmoulliard <[email protected]>

Grab the LocalBuild resource

f1d03fa

Signed-off-by: cmoulliard <[email protected]>

Adding the missing scheme to the clients created

8f4810c

Signed-off-by: cmoulliard <[email protected]>

nabuskey approved these changes Dec 2, 2024

View reviewed changes

cmoulliard merged commit 9925fb3 into cnoe-io:main Dec 2, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the cmd: get clusters #449

Improve the cmd: get clusters #449

cmoulliard commented Nov 14, 2024 •

edited

Loading

cmoulliard commented Nov 14, 2024 •

edited

Loading

cmoulliard commented Nov 15, 2024

nabuskey Nov 21, 2024

nabuskey Nov 21, 2024

cmoulliard Nov 22, 2024

nabuskey Nov 21, 2024

cmoulliard Nov 22, 2024

nabuskey Nov 22, 2024

cmoulliard Nov 22, 2024

nabuskey Nov 27, 2024

cmoulliard Nov 28, 2024 •

edited

Loading

cmoulliard Nov 28, 2024

cmoulliard commented Nov 27, 2024

nabuskey Nov 27, 2024 •

edited

Loading

cmoulliard Nov 28, 2024 •

edited

Loading

cmoulliard Nov 28, 2024

nabuskey Nov 27, 2024

cmoulliard Nov 28, 2024

nabuskey Nov 27, 2024 •

edited

Loading

cmoulliard Nov 28, 2024 •

edited

Loading

nabuskey Nov 27, 2024

cmoulliard Nov 28, 2024

nabuskey Nov 27, 2024

nabuskey Nov 27, 2024

cmoulliard Nov 28, 2024

nabuskey commented Dec 2, 2024

nabuskey left a comment

	type BuildCustomizationSpec struct {
	Protocol string `json:"protocol,omitempty"`
	Host string `json:"host,omitempty"`
	IngressHost string `json:"ingressHost,omitempty"`
	Port string `json:"port,omitempty"`
	UsePathRouting bool `json:"usePathRouting,omitempty"`
	SelfSignedCert string `json:"selfSignedCert,omitempty"`
	}

Improve the cmd: get clusters #449

Improve the cmd: get clusters #449

Conversation

cmoulliard commented Nov 14, 2024 • edited Loading

cmoulliard commented Nov 14, 2024 • edited Loading

cmoulliard commented Nov 15, 2024

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

cmoulliard Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmoulliard commented Nov 27, 2024

nabuskey Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

cmoulliard Nov 28, 2024 • edited Loading

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

nabuskey Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

cmoulliard Nov 28, 2024 • edited Loading

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

nabuskey commented Dec 2, 2024

nabuskey left a comment

Choose a reason for hiding this comment

cmoulliard commented Nov 14, 2024 •

edited

Loading

cmoulliard commented Nov 14, 2024 •

edited

Loading

cmoulliard Nov 28, 2024 •

edited

Loading

nabuskey Nov 27, 2024 •

edited

Loading

cmoulliard Nov 28, 2024 •

edited

Loading

nabuskey Nov 27, 2024 •

edited

Loading

cmoulliard Nov 28, 2024 •

edited

Loading