scaling deployment replicas #3910

msivasubramaniaan · 2024-02-16T17:54:02Z

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

codecov · 2024-02-16T18:06:44Z

Codecov Report

Attention: Patch coverage is 35.71429% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 45.21%. Comparing base (da60441) to head (b81e032).
Report is 61 commits behind head on main.

❗ Current head b81e032 differs from pull request most recent head 1f0cbb5. Consider uploading reports for the commit 1f0cbb5 to get more accurate results

Files	Patch %	Lines
src/deployment.ts	42.30%	15 Missing ⚠️
src/explorer.ts	20.00%	8 Missing ⚠️
src/oc/ocWrapper.ts	0.00%	3 Missing ⚠️
src/openshift/component.ts	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3910       +/-   ##
===========================================
+ Coverage   32.37%   45.21%   +12.83%     
===========================================
  Files          85       85               
  Lines        6505     6602       +97     
  Branches     1349     1358        +9     
===========================================
+ Hits         2106     2985      +879     
+ Misses       4399     3617      -782

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

datho7561 · 2024-02-16T18:34:24Z

src/explorer.ts

@@ -300,6 +323,10 @@ export class OpenShiftExplorer implements TreeDataProvider<ExplorerItem>, Dispos
                    result = [...helmRepos.sort(Helm.ascRepoName)];
                }
            }
+        } else if ('kind' in element && element.kind === 'Deployment') {
+            const pods = await Oc.Instance.getKubernetesObjects('pods');
+            const filteredPods: DeploymentPodObject[] = pods.filter((pod) => pod.metadata.name.indexOf(element.metadata.name) !== -1 && pod);


I don't think && pod is needed here. && pod will check if pod is not null or undefined, but since pod has a field metadata that you access, we know it's not null.

Removed pod

datho7561

Works pretty well! Here are some things I noticed

datho7561 · 2024-02-16T18:51:46Z

src/deployment.ts

+                if (trimmedValue.length === 0) {
+                    return 'Scale value cannot be empty';
+                }
+                if (parseInt(trimmedValue, 10) < 0) {


I am allowed to enter 0.001 into the wizard. When this is passed to oc scale deployment, it fails.

I think we should do one of:

run parseInt on count before passing it to oc scale deployment

Only allow integer values in the wizard

Scale value now accepts only positive integer

datho7561 · 2024-02-16T18:53:03Z

src/deployment.ts

+        );
+        const response = await Oc.Instance.scalePod(component.metadata.name, count);
+        if (response.indexOf('scaled') !== -1) {
+            OpenShiftExplorer.getInstance().refresh(component);


If you are increasing the number of replicas (eg. 1 --> 3), then the new pods don't show up when you refresh the tree here, since the pods take a while to reach the "Running" state. I think this can be a bit confusing for the user.

However, I can't think of a way to address this other than refreshing the Application Explorer on a timer, which we are trying to avoid, so I think this is good for now.

Refreshing the tree view based on 'Pod is Running' text on terminal view. Now we can get the updated pods

datho7561 · 2024-02-16T18:56:51Z

src/explorer.ts

+                    description: `${contextElement.kind.substring(0, 1).toLocaleUpperCase()}${contextElement.kind.substring(1)}`,
+                    collapsibleState: TreeItemCollapsibleState.None,
+                    iconPath: new ThemeIcon('layers-active'),
+                    tooltip: `${contextElement.status.phase} (1/1)\n${contextElement.status.podIP}`,


I don't think it's worth listing (1/1) if this value is hard coded. For example, what if pod contains a sidecar container, so there should be two containers listed?

Also, you filter out pods that are in the "Running" phase, so the first part will always be "Running"

I also don't get the meaning of that hard coded (1/1) string in pod's tooltip.

Maybe this is a place that later is to be changed to something like (pod #x of y) or something like this, but if not, I don't see any point in hard coding it here.

There were no possibilities of getting the number of pod on running state. So removed the (1/1) hardcoded value

datho7561 · 2024-02-16T18:58:12Z

src/explorer.ts

+        } else if ('kind' in element && element.kind === 'Deployment') {
+            const pods = await Oc.Instance.getKubernetesObjects('pods');
+            const filteredPods: DeploymentPodObject[] = pods.filter((pod) => pod.metadata.name.indexOf(element.metadata.name) !== -1 && pod);
+            return filteredPods.filter((pod) => pod.status.phase === 'Running');


May I ask what's the reason for only listing pods that are running? I think it could be helpful to show pods that are starting or stopping.

May I ask what's the reason for only listing pods that are running? I think it could be helpful to show pods that are starting or stopping.

+1

Now listing all the pods from cluster

vrubezhny

Agree with David's notes. +see some questions inline.

vrubezhny · 2024-02-20T20:32:57Z

src/deployment.ts

+        if (!component) {
+            return;
+        }
+        let count = '1';


Can we find out somehow and use the current scale number as an initial value?

Taking list of running pods as initial value

vrubezhny · 2024-02-20T22:55:09Z

src/deployment.ts

+        if (!component) {
+            return;
+        }
+        let count = '1';


Do we store somewhere or can we find out somehow and use the current scale number as an initial value?

#3910 (comment)

vrubezhny · 2024-02-20T22:56:54Z

src/explorer.ts

+        } else if ('kind' in element && element.kind === 'Deployment') {
+            const pods = await Oc.Instance.getKubernetesObjects('pods');
+            const filteredPods: DeploymentPodObject[] = pods.filter((pod) => pod.metadata.name.indexOf(element.metadata.name) !== -1 && pod);
+            return filteredPods.filter((pod) => pod.status.phase === 'Running');


May I ask what's the reason for only listing pods that are running? I think it could be helpful to show pods that are starting or stopping.

+1

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

msivasubramaniaan · 2024-02-26T12:56:20Z

@datho7561 and @vrubezhny addressed all the comments which you mentioned. Please have a look on the latest code

vrubezhny · 2024-02-26T17:29:01Z

When I'm scaling to 0, I see all the running pods are shutting down (and disappearing from the App explorer tree) with the following lines output to the OS Terminal:

Cleaning resources, please wait
 ✗  Finished executing the application (command: debug) [43s]
 ✗  watch command was unable to push component: fail starting the port forwarding: multiple Pods exist for the selector: component=nodejs-starter-1. Only one must be present

Is this normal? I mean the second error: .watch command was unable to push component:...

vrubezhny · 2024-02-26T17:46:30Z

After I scaled to 2, when clicking on a Deploypent or on one of the running Pods the Deployment YAML is to be opened... So after a while it stops opening Deployment YAML when clicking on a Pod and suddenly stops Dev for a Component...

Not sure if this is related to Scaling or is it any different issue:

Screencast from 2024-02-26 18-37-24.webm
This time the error is Shutting down the API server...:

 ✗  Finished executing the application (command: debug) [43s]
 ✗  watch command was unable to push component: fail starting the port forwarding: multiple Pods exist for the selector: component=nodejs-starter-1. Only one must be present
I0226 18:38:39.986832  259197 starterserver.go:147] Shutting down the API server: context canceled

msivasubramaniaan · 2024-02-26T18:48:50Z

After I scaled to 2, when clicking on a Deploypent or on one of the running Pods the Deployment YAML is to be opened... So after a while it stops opening Deployment YAML when clicking on a Pod and suddenly stops Dev for a Component...

Not sure if this is related to Scaling or is it any different issue:

Screencast from 2024-02-26 18-37-24.webm This time the error is Shutting down the API server...:
 ✗  Finished executing the application (command: debug) [43s]
 ✗  watch command was unable to push component: fail starting the port forwarding: multiple Pods exist for the selector: component=nodejs-starter-1. Only one must be present
I0226 18:38:39.986832  259197 starterserver.go:147] Shutting down the API server: context canceled

We don't have control on getting the terminating state of the pod, that's why I have shown only running pods. If you check it on the cluster the mentioned pod was terminated and there were no config of it. so it fails.

src/deployment.ts

src/openshift/component.ts

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

datho7561

Works like a charm! Thanks, Muthu!

datho7561 · 2024-02-27T13:59:33Z

Do you want to merge this for 1.12.0 or wait until after we release? Personally, I think it should be good to include with 1.12.0.

msivasubramaniaan · 2024-02-27T14:01:17Z

Do you want to merge this for 1.12.0 or wait until after we release? Personally, I think it should be good to include with 1.12.0.

I felt it good to go with 1.12.0 :)

vrubezhny

Looks good to me.
The scaling actually happens - the pods are created and deleted according the Scale value requested, the only the positive integers are accepted.

scaling deployment replicas

e9344c6

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

msivasubramaniaan requested review from vrubezhny and datho7561 February 16, 2024 17:54

msivasubramaniaan self-assigned this Feb 16, 2024

datho7561 reviewed Feb 16, 2024

View reviewed changes

datho7561 requested changes Feb 16, 2024

View reviewed changes

vrubezhny requested changes Feb 20, 2024

View reviewed changes

review comments are addressed

b81e032

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

msivasubramaniaan requested review from vrubezhny and datho7561 February 26, 2024 12:51

datho7561 requested changes Feb 26, 2024

View reviewed changes

src/deployment.ts Outdated Show resolved Hide resolved

src/openshift/component.ts Outdated Show resolved Hide resolved

restricted . value on scale count and refresh when pod terminated

1f0cbb5

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

msivasubramaniaan requested a review from datho7561 February 27, 2024 09:32

datho7561 approved these changes Feb 27, 2024

View reviewed changes

vrubezhny approved these changes Feb 27, 2024

View reviewed changes

msivasubramaniaan merged commit 9c8d8ab into redhat-developer:main Feb 27, 2024
4 checks passed

msivasubramaniaan deleted the 3822-context-menu-item-to-scale-deployments branch February 27, 2024 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scaling deployment replicas #3910

scaling deployment replicas #3910

msivasubramaniaan commented Feb 16, 2024

codecov bot commented Feb 16, 2024 •

edited

datho7561 Feb 16, 2024

msivasubramaniaan Feb 26, 2024

datho7561 left a comment

datho7561 Feb 16, 2024

msivasubramaniaan Feb 26, 2024

datho7561 Feb 16, 2024

msivasubramaniaan Feb 26, 2024

datho7561 Feb 16, 2024

vrubezhny Feb 20, 2024

msivasubramaniaan Feb 26, 2024

datho7561 Feb 16, 2024

vrubezhny Feb 20, 2024

msivasubramaniaan Feb 26, 2024

vrubezhny left a comment

vrubezhny Feb 20, 2024

msivasubramaniaan Feb 26, 2024

vrubezhny Feb 20, 2024

msivasubramaniaan Feb 26, 2024

vrubezhny Feb 20, 2024

msivasubramaniaan commented Feb 26, 2024

vrubezhny commented Feb 26, 2024

vrubezhny commented Feb 26, 2024 •

edited

msivasubramaniaan commented Feb 26, 2024

datho7561 left a comment

datho7561 commented Feb 27, 2024

msivasubramaniaan commented Feb 27, 2024

vrubezhny left a comment

scaling deployment replicas #3910

scaling deployment replicas #3910

Conversation

msivasubramaniaan commented Feb 16, 2024

codecov bot commented Feb 16, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datho7561 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrubezhny 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

msivasubramaniaan commented Feb 26, 2024

vrubezhny commented Feb 26, 2024

vrubezhny commented Feb 26, 2024 • edited

msivasubramaniaan commented Feb 26, 2024

datho7561 left a comment

Choose a reason for hiding this comment

datho7561 commented Feb 27, 2024

msivasubramaniaan commented Feb 27, 2024

vrubezhny left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 16, 2024 •

edited

vrubezhny commented Feb 26, 2024 •

edited