fix: allow to customize config values on create new page #228

lstocchi · 2024-06-14T15:06:10Z

This PR updates the extension to show the properties that can be customized when starting the VM in the create new page (memory, cpus, disksize and pull-secret-file).

They have been removed from the settings page to be in line with the other podman desktop extensions.

So, when clicking on create, it updates the config values of cpus/memory/disksize/pull-secret and if the start now option is true (by default) the VM is also started. Otherwise it just updates the config and register the provider to start/stop/delete the VM into Desktop.

Right now there is no way to customize/update dynamically the minimum value of a config so i had to create 2 entries for both the memory and cpus (one for openshift and one for minishift). They are displayed accordingly to the preset value.

This requires podman-desktop/podman-desktop#7659

package.json

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

Signed-off-by: Denis Golovin <[email protected]>

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

lstocchi · 2024-07-01T16:20:49Z

This should be ready for a review. You need to use the main branch of desktop as this PR requires podman-desktop/podman-desktop#7659 to work.

There is just one missing part which is we should hide the create new ... button when the kubernetes connection is registered but it's not easy as i thought so i'm going to tackle it in a different PR.

dgolovin · 2024-07-02T04:11:30Z

src/extension.ts

        await presetChanged(provider, extensionContext, telemetryLogger);
-        await createCrcVm(provider, extensionContext, telemetryLogger, logger);
+        await saveConfig(params);


I would suggest to preserve previous behavior when this form creates and starts VM.

When opened it should show default values set in CRC Extension settings and let user to changes them and use as part of 'crc start' command without saving them in settings as default.

There could be a switch 'Save as default settings' if we want and saveConfig method should be called in create function before calling createCrcVm().

I agree about showing the default values. The initial plan i had was this one but this is not currently supported by Desktop as far as i recall and I don't think a user is going to delete/create the VM several time so we should be good with what we have now as an initial release. This can be done in a follow-up PR. Even because it can be useful for other extensions and it is an improvement we should do on the Desktop side.

Regarding saving the values as default vs using them when starting the VM - i think there is a bit of confusion about how crc fit inside the Podman Desktop mechanisms.

In the create new page we usually create the instance of something (for podman - a podman machine, for kind - a kind cluster) but we are not actually starting them (unless you set the option start now). In CRC i tried to do the same but crc does not have the concept of a new instance. Everything is set up during the setup phase and then you have to start it.
So in the "creation" process what we do is just registering the kubernetes connection to Desktop with its custom lifecycle (start/stop/delete).

Now if you want to start the VM you can click on the start button from the resources page, but in that case you don't have any way to set the memory/cpu/disk, and i don't think that showing a quickpick is really user friendly. So it is easier to set them globally during the "create" process so that when the user start the VM, the properties are already saved.

Yes +1, I don't think the settings for the "OpenShift Local" environments should be handle in a different way than what we are doing for podman machine. (For podman machine, we have "default values", but the user has no way to configure those in the preferences.)

Sounds good to me then with nit. There should be markup paragraph before pull-secret file location configuration field explaining that if you have file with it you can configure it here, but it is optional. If you do not fill up file location for pull-secret file you are going to be requested to login using Red Hat SSO to pull pull-secret and store it in platform specific secure storage.

@dgolovin done. Description updated. You can see it in the image on the first post above

slemeur · 2024-07-10T08:59:00Z

FYI, the font for the pullsecret is not displayed (testing with podman-desktop/podman-desktop#7995)

dgolovin

LGTM, with nit (see my comment).

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

lstocchi · 2024-07-15T08:56:02Z

FYI, the font for the pullsecret is not displayed (testing with containers/podman-desktop#7995)

Nice catch 👍 It is a problem on podman desktop tough, i'm gonna fix it there

jeffmaury

Remarks:

memory and disk are described in GiB but displayed in GB: that's incoherent
can't we detect the pull secret has already been set and hide the parameter
why can't we set the preset ?

Also I was in the state where CRC was installed but not setup (missing the bundle) and it is very hard from the resource page to leave that state but that may be not related to this PR

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

lstocchi · 2024-07-17T08:16:55Z

Remarks:

memory and disk are described in GiB but displayed in GB: that's incoherent

updated

can't we detect the pull secret has already been set and hide the parameter

if it's set, the input field is filled. I think it is more correct, if you notice that you're using a wrong file you can still update it.

why can't we set the preset ?

The preset is currently chosen during the setup as it downloads the correct bundle. Not sure if setting the preset during the create new ... phase is more user friendly bc when you click on start it would download everything requiring minutes.

Also I was in the state where CRC was installed but not setup (missing the bundle) and it is very hard from the resource page to leave that state but that may be not related to this PR

I'll look into this in following PR

jeffmaury · 2024-07-23T13:44:41Z

2 remarks:

some values for memory (4,5, 4.6 leads to errors but we cannot read the error message). Strange to understand but some values 4,5 and 4,6 or 5 and 5.1 leads to the same setting
when the machine is created, an emty screen is displayed:

lstocchi · 2024-07-24T16:06:06Z

2 remarks:

some values for memory (4,5, 4.6 leads to errors but we cannot read the error message). Strange to understand but some values 4,5 and 4,6 or 5 and 5.1 leads to the same setting

When did you face the error with the memory value? When setting the slider or when hitting create?

jeffmaury · 2024-07-25T07:40:01Z

2 remarks:

some values for memory (4,5, 4.6 leads to errors but we cannot read the error message). Strange to understand but some values 4,5 and 4,6 or 5 and 5.1 leads to the same setting

When did you face the error with the memory value? When setting the slider or when hitting create?

No create as the value set for memory is rejected by HyperV

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

lstocchi · 2024-07-25T13:49:55Z

some values for memory (4,5, 4.6 leads to errors but we cannot read the error message). Strange to understand but some values 4,5 and 4,6 or 5 and 5.1 leads to the same setting

I was not able to replicate it. Tried to set the memory to 4.5 -5 and the vm was started without any problem.
I had an issue that occurred repeatedly when the VM was started and the cli was not able to reach it (ssh: handshake failed). Eventually i clean up everything and re-done the setup + start and worked fine. Maybe you had some similar thing? Can you try by deleting + cleanup + setup + start again?

when the machine is created, an emty screen is displayed:

After some investigation it seems that the problem is not that easy to solve. Basically the create new button that you see in the resources page, it is displayed if the KubernetesProviderConnectionFactory is initialized. And because it does not make any sense to have it once one VM is created, the factory was disposed to hide that button.
As a quick solution I just removed the dispose action. I think we should update podman desktop to provide an enablement clause to allow extensions to decide when the create new button should be visible or not. WDYT?

jeffmaury · 2024-07-25T14:14:35Z

some values for memory (4,5, 4.6 leads to errors but we cannot read the error message). Strange to understand but some values 4,5 and 4,6 or 5 and 5.1 leads to the same setting

I was not able to replicate it. Tried to set the memory to 4.5 -5 and the vm was started without any problem. I had an issue that occurred repeatedly when the VM was started and the cli was not able to reach it (ssh: handshake failed). Eventually i clean up everything and re-done the setup + start and worked fine. Maybe you had some similar thing? Can you try by deleting + cleanup + setup + start again?

when the machine is created, an emty screen is displayed:

After some investigation it seems that the problem is not that easy to solve. Basically the create new button that you see in the resources page, it is displayed if the KubernetesProviderConnectionFactory is initialized. And because it does not make any sense to have it once one VM is created, the factory was disposed to hide that button. As a quick solution I just removed the dispose action. I think we should update podman desktop to provide an enablement clause to allow extensions to decide when the create new button should be visible or not. WDYT?

I don't understand fully because when I create a Kind cluster (I suppose this is also KubernetesProviderConnectionFactory), I have this screen:

So what is the difference ?

lstocchi · 2024-07-25T14:20:42Z

I don't understand fully because when I create a Kind cluster (I suppose this is also KubernetesProviderConnectionFactory), I have this screen:

So what is the difference ?

With the latest commit (remove of the disposable) you should see the same when the creation + start process of crc is completed.
The difference should be about this button

For kind it makes sense to have it always there because you can create X cluster. For crc, you can just create one VM, so it would be great if the create button is hidden when a VM is there.

By disposing the factory we were able to achieve this behavior. But if we dispose the factory, we would see the empty screen as you noted above when the start process is completed

jeffmaury · 2024-07-25T14:25:39Z

I don't understand fully because when I create a Kind cluster (I suppose this is also KubernetesProviderConnectionFactory), I have this screen:

So what is the difference ?

With the latest commit (remove of the disposable) you should see the same when the creation + start process of crc is completed. The difference should be about this button

For kind it makes sense to have it always there because you can create X cluster. For crc, you can just create one VM, so it would be great if the create button is hidden when a VM is there.

By disposing the factory we were able to achieve this behavior. But if we dispose the factory, we would see the empty screen as you noted above when the start process is completed

I think we are not in sync: I'm talking about the screen that is displayed when the VM has been created: for kind I have to button to return to the Resources page. For CRC I have

I don't understand fully because when I create a Kind cluster (I suppose this is also KubernetesProviderConnectionFactory), I have this screen:

So what is the difference ?

With the latest commit (remove of the disposable) you should see the same when the creation + start process of crc is completed. The difference should be about this button

For kind it makes sense to have it always there because you can create X cluster. For crc, you can just create one VM, so it would be great if the create button is hidden when a VM is there.

By disposing the factory we were able to achieve this behavior. But if we dispose the factory, we would see the empty screen as you noted above when the start process is completed

I think we are not in sync: for kind I have a screen with a button that returns to the Resources page. For CRC I have an empty screen

lstocchi · 2024-07-25T14:26:29Z

I think we are not in sync: for kind I have a screen with a button that returns to the Resources page. For CRC I have an empty screen

Have you tried with the latest commit?

jeffmaury · 2024-07-25T14:31:02Z

I think we are not in sync: for kind I have a screen with a button that returns to the Resources page. For CRC I have an empty screen

Have you tried with the latest commit?

Not yet :)

lstocchi · 2024-07-25T14:35:24Z

Not yet :)

Ah ok 👍
This is what you should see (i cut the gif bc it was 3 mins long lol)

jeffmaury · 2024-07-25T15:10:37Z

Seems it's working better than expected:

So if I set 4.5GB I have these settings:

- consent-telemetry                     : no
- disk-size                             : 32
- memory                                : 4291
- preset                                : microshift

And starting crc --log-level debug give me this:```

DEBU CRC version: 2.37.1+36d451
DEBU OpenShift version: 4.15.14
DEBU Running 'crc start'
DEBU Total memory of system is 34105286656 bytes
WARN A new version (2.39.0) has been published on https://developers.redhat.com/content-gateway/file/pub/openshift-v4/clients/crc/2.39.0/crc-windows-installer.zip
DEBU Checking file: C:\Users\Jeff.crc\machines\crc.crc-exist
INFO Using bundle path C:\Users\Jeff.crc\cache\crc_microshift_hyperv_4.15.14_amd64.crcbundle
INFO Checking minimum RAM requirements
DEBU Total memory of system is 34105286656 bytes
INFO Check if Podman binary exists in: C:\Users\Jeff.crc\bin\oc
INFO Checking if running in a shell with administrator rights
DEBU Running '$currentPrincipal = New-Object Security.Principal.WindowsPrincipal([Security.Principal.WindowsIdentity]::GetCurrent());$currentPrincipal.IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)'
INFO Checking Windows release
DEBU Running '(Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion" -Name ReleaseId).ReleaseId'
INFO Checking Windows edition
DEBU Running '(Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion").EditionID'
DEBU Running on Windows Professional edition
INFO Checking if Hyper-V is installed and operational
DEBU Running '@(Get-Wmiobject Win32_ComputerSystem).HypervisorPresent'
DEBU Running '@(Get-Service vmms).Status'
INFO Checking if Hyper-V service is enabled
DEBU Running '@(Get-Service vmms).Status'
INFO Checking if crc-users group exists
DEBU Running 'Get-LocalGroup -Name crc-users'
INFO Checking if current user is in crc-users and Hyper-V admins group
DEBU Running '(Get-LocalGroupMember -Group 'crc-users').Name'
DEBU Checking current user is in the 'crc-user' group
DEBU group members: DESKTOP-JEFF\Jeff
DEBU Running '(Get-LocalGroupMember -SID 'S-1-5-32-578').Name'
DEBU Checking current user is in the 'Hyper-v Administrators' group
DEBU group members: DESKTOP-JEFF\Jeff
INFO Checking if vsock is correctly configured
DEBU Running 'Get-Item -Path "HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Virtualization\GuestCommunicationServices\00000400-FACB-11E6-BD58-64006A7986D3"'
INFO Checking if the win32 background launcher is installed
DEBU Running '(Get-Item 'C:\Program Files\Red Hat OpenShift Local\crc-background-launcher.exe').VersionInfo.FileVersion'

DEBU Found crc-background-launcher.exe version 0.0.0.1
INFO Checking if the daemon task is installed
DEBU Running 'Get-ScheduledTask -TaskName crcDaemon'
DEBU Running '(Get-ScheduledTask -TaskName "crcDaemon").Version'
INFO Checking if the daemon task is running
DEBU Running '(Get-ScheduledTask -TaskName "crcDaemon").State'
INFO Checking admin helper service is running
DEBU Running '(Get-Service crcAdminHelper).Status'
INFO Checking SSH port availability
DEBU Checking file: C:\Users\Jeff.crc\machines\crc.crc-exist
DEBU Copying 'C:\Users\Jeff.crc\cache\crc_microshift_hyperv_4.15.14_amd64\oc.exe' to 'C:\Users\Jeff.crc\bin\oc\oc.exe'

DEBU Copying 'C:\Users\Jeff.crc\cache\crc_microshift_hyperv_4.15.14_amd64\podman.exe' to 'C:\Users\Jeff.crc\bin\podman\podman.exe'
INFO Loading bundle: crc_microshift_hyperv_4.15.14_amd64...
DEBU Cannot load secret from configuration: empty path
DEBU Using secret from keyring
INFO Creating CRC VM for MicroShift 4.15.14...
DEBU Running pre-create checks...
DEBU Running '@(Get-Module -ListAvailable hyper-v).Name | Get-Unique'
DEBU Running '@([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole(([System.Security.Principal.SecurityIdentifier]::new("S-1-5-32-578")))'
DEBU Creating machine...
DEBU Creating VM...
DEBU Running 'Hyper-V\New-VM crc -Path 'C:\Users\Jeff.crc\machines\crc' -MemoryStartupBytes 4291MB'
DEBU Command failed: exit status 1
DEBU stdout:
DEBU stderr: Hyper-V\New-VM : �chec de la modification du p�riph�rique 'Memory'.
Valeur de m�moire non valide attribu�e � ��crc��. Les valeurs de m�moire doivent �tre correctement align�es.
'crc' n'a pas r�ussi � modifier le p�riph�rique 'Memory'. (ID d'ordinateur virtuel
BBB1A6F8-4048-4461-B5B4-FBBE8D6E6DC4)
Valeur de m�moire non valide attribu�e � �crc�. La valeur de m�moire attribu�e (�4291�Mo) n'est pas correctement
align�e. R�essayez avec une valeur de m�moire correctement align�e. (ID d'ordinateur virtuel
BBB1A6F8-4048-4461-B5B4-FBBE8D6E6DC4)
Au caract�re Ligne:1 : 43

... yContinue'; Hyper-V\New-VM crc -Path 'C:\Users\Jeff.crc\machines\crc ...
```
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
- CategoryInfo : InvalidArgument : (:) [New-VM], VirtualizationException
- FullyQualifiedErrorId : InvalidParameter,Microsoft.HyperV.PowerShell.Commands.NewVM

Error creating machine: Error in driver during machine creation: exit status 1

package.json

jeffmaury

I still have an issue with VM start failure not reported but it is not related to this PR so I will open a follow up issue

lstocchi · 2024-07-26T08:38:56Z

I still have an issue with VM start failure not reported but it is not related to this PR so I will open a follow up issue

Yeah, I'll investigate it as well. I'm worried that when using the minimum amount of memory allowed, the conversion is not optimal and the resources given to micro shift are not enough. I'll give this a look when I have some free time

dgolovin reviewed Jun 15, 2024

View reviewed changes

package.json Show resolved Hide resolved

lstocchi marked this pull request as draft June 15, 2024 06:53

lstocchi and others added 4 commits July 1, 2024 18:10

fix: allow to customize config values on create new page

7e10c2a

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

fix: format and eslint errors

1c0e5ce

Signed-off-by: Denis Golovin <[email protected]>

fix: add test

ca9d37f

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

fix: rebase

1dcfc94

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

lstocchi force-pushed the i203 branch from 4935b55 to 1dcfc94 Compare July 1, 2024 16:11

fix: lint

b5b7491

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

lstocchi force-pushed the i203 branch from 282161b to b5b7491 Compare July 1, 2024 16:17

lstocchi marked this pull request as ready for review July 1, 2024 16:18

lstocchi requested a review from jeffmaury July 1, 2024 16:18

lstocchi requested a review from dgolovin July 1, 2024 16:20

dgolovin reviewed Jul 2, 2024

View reviewed changes

dgolovin self-requested a review July 13, 2024 00:43

dgolovin approved these changes Jul 13, 2024

View reviewed changes

fix: update description of pull secret field

ed95663

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

lstocchi mentioned this pull request Jul 15, 2024

Markdown text missing in "Create new" page podman-desktop/podman-desktop#8066

Closed

jeffmaury reviewed Jul 15, 2024

View reviewed changes

fix: update disk/memory field description

b07bba2

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

lstocchi closed this Jul 17, 2024

lstocchi reopened this Jul 17, 2024

lstocchi requested a review from jeffmaury July 17, 2024 08:20

fix: do not dispose factory when finish starting the VM

23e9a07

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

jeffmaury reviewed Jul 25, 2024

View reviewed changes

package.json Show resolved Hide resolved

jeffmaury approved these changes Jul 26, 2024

View reviewed changes

lstocchi merged commit 2f809c9 into crc-org:main Jul 29, 2024
3 checks passed

lstocchi deleted the i203 branch July 29, 2024 10:35

This was referenced Jul 29, 2024

Improve the UX create an environment #203

Closed

Allow to configure cpus, memory, disk size and pull-secret file location when creating new cluster #215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow to customize config values on create new page #228

fix: allow to customize config values on create new page #228

lstocchi commented Jun 14, 2024 •

edited

Loading

lstocchi commented Jul 1, 2024

dgolovin Jul 2, 2024

lstocchi Jul 2, 2024 •

edited

Loading

slemeur Jul 10, 2024

dgolovin Jul 13, 2024

lstocchi Jul 15, 2024

slemeur commented Jul 10, 2024

dgolovin left a comment

lstocchi commented Jul 15, 2024

jeffmaury left a comment

lstocchi commented Jul 17, 2024 •

edited

Loading

jeffmaury commented Jul 23, 2024

lstocchi commented Jul 24, 2024

jeffmaury commented Jul 25, 2024

lstocchi commented Jul 25, 2024

jeffmaury commented Jul 25, 2024

lstocchi commented Jul 25, 2024 •

edited

Loading

jeffmaury commented Jul 25, 2024

lstocchi commented Jul 25, 2024

jeffmaury commented Jul 25, 2024

lstocchi commented Jul 25, 2024

jeffmaury commented Jul 25, 2024

jeffmaury left a comment

lstocchi commented Jul 26, 2024

fix: allow to customize config values on create new page #228

fix: allow to customize config values on create new page #228

Conversation

lstocchi commented Jun 14, 2024 • edited Loading

lstocchi commented Jul 1, 2024

dgolovin Jul 2, 2024

Choose a reason for hiding this comment

lstocchi Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

slemeur Jul 10, 2024

Choose a reason for hiding this comment

dgolovin Jul 13, 2024

Choose a reason for hiding this comment

lstocchi Jul 15, 2024

Choose a reason for hiding this comment

slemeur commented Jul 10, 2024

dgolovin left a comment

Choose a reason for hiding this comment

lstocchi commented Jul 15, 2024

jeffmaury left a comment

Choose a reason for hiding this comment

lstocchi commented Jul 17, 2024 • edited Loading

jeffmaury commented Jul 23, 2024

lstocchi commented Jul 24, 2024

jeffmaury commented Jul 25, 2024

lstocchi commented Jul 25, 2024

jeffmaury commented Jul 25, 2024

lstocchi commented Jul 25, 2024 • edited Loading

jeffmaury commented Jul 25, 2024

lstocchi commented Jul 25, 2024

jeffmaury commented Jul 25, 2024

lstocchi commented Jul 25, 2024

jeffmaury commented Jul 25, 2024

jeffmaury left a comment

Choose a reason for hiding this comment

lstocchi commented Jul 26, 2024

lstocchi commented Jun 14, 2024 •

edited

Loading

lstocchi Jul 2, 2024 •

edited

Loading

lstocchi commented Jul 17, 2024 •

edited

Loading

lstocchi commented Jul 25, 2024 •

edited

Loading