Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spike] Use crc config to use custom location for .crc directory #4088

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vyasgun
Copy link
Contributor

@vyasgun vyasgun commented Apr 1, 2024

Fixes: Issue #3966

Solution/Idea

The location for .crc directory is made configurable for the user through crc config.
Currently, this PR will use the value set in the config as the new base directory.

Proposed changes

List main as well as consequential changes you introduced or had to introduce.

  • Add a new config key called crc-dir
  • Initialise Crc base directory variables based on the saved config
  • New config will take effect after rerunning crc setup

Testing

What is the bottom-line functionality that needs testing? Describe in pseudo-code or in English. Use verifiable statements that tie your changes to existing functionality.

gvyas-mac:crc gvyas$ crc config view
- consent-telemetry                     : no
- preset                                : microshift

gvyas-mac:crc gvyas$ crc config set crc-dir ~/test1
Changes to configuration property 'crc-dir' are only applied during 'crc setup'.
Please run 'crc setup' for this configuration to take effect.

gvyas-mac:crc gvyas$ crc config view
- consent-telemetry                     : no
- crc-dir                               : /Users/gvyas/test1
- preset                                : microshift

gvyas-mac:crc gvyas$ crc setup
INFO Using bundle path /Users/gvyas/test1/.crc/cache/crc_microshift_vfkit_4.15.3_amd64.crcbundle 
INFO Checking if running macOS version >= 13.x    
INFO Checking if running as non-root              
INFO Checking if crc-admin-helper executable is cached 
INFO Caching crc-admin-helper executable          
INFO Using root access: Changing ownership of /Users/gvyas/test1/.crc/bin/crc-admin-helper-darwin 
Password:
INFO Using root access: Setting suid for /Users/gvyas/test1/.crc/bin/crc-admin-helper-darwin 
INFO Checking if running on a supported CPU architecture 
INFO Checking if crc executable symlink exists    
INFO Creating symlink for crc executable          
INFO Checking minimum RAM requirements            
INFO Check if Podman binary exists in: /Users/gvyas/test1/.crc/bin/oc 
INFO Checking if running emulated on Apple silicon 
INFO Checking if vfkit is installed               
INFO Setting up virtualization with vfkit         
INFO Checking if CRC bundle is extracted in "/Users/gvyas/test1/.crc/cache/crc_microshift_vfkit_4.15.3_amd64.crcbundle" 
INFO Checking if /Users/gvyas/test1/.crc/cache/crc_microshift_vfkit_4.15.3_amd64.crcbundle exists 
INFO Checking if old launchd config for tray and/or daemon exists 
INFO Checking if crc daemon plist file is present and loaded 
INFO Adding crc daemon plist file and loading it  
INFO Checking SSH port availability               
Your system is correctly setup for using CRC. Use 'crc start' to start the instance
gvyas-mac:crc gvyas$ crc start
INFO Using bundle path /Users/gvyas/test1/.crc/cache/crc_microshift_vfkit_4.15.3_amd64.crcbundle 
INFO Checking if running macOS version >= 13.x    
INFO Checking if running as non-root              
INFO Checking if crc-admin-helper executable is cached 
INFO Checking if running on a supported CPU architecture 
INFO Checking if crc executable symlink exists    
INFO Checking minimum RAM requirements            
INFO Check if Podman binary exists in: /Users/gvyas/test1/.crc/bin/oc 
INFO Checking if running emulated on Apple silicon 
INFO Checking if vfkit is installed               
INFO Checking if old launchd config for tray and/or daemon exists 
INFO Checking if crc daemon plist file is present and loaded 
INFO Checking SSH port availability               
INFO Loading bundle: crc_microshift_vfkit_4.15.3_amd64... 
INFO Creating CRC VM for MicroShift 4.15.3...     
INFO Generating new SSH key pair...               
INFO Starting CRC VM for microshift 4.15.3...     
INFO CRC instance is running with IP 127.0.0.1    
INFO CRC VM is running                            
INFO Updating authorized keys...                  
INFO Configuring shared directories
INFO Check internal and public DNS query...       
INFO Check DNS query from host...                 
INFO Starting Microshift service... [takes around 1min] 
INFO Check internal and public DNS query...       
INFO Check DNS query from host...                 
INFO Starting Microshift service... [takes around 1min] 
INFO Waiting for kube-apiserver availability... [takes around 2min] 
INFO Adding microshift context to kubeconfig...   
Started the MicroShift cluster.

Use the 'oc' command line interface:
  $ eval $(crc oc-env)
  $ oc COMMAND

Copy link

openshift-ci bot commented Apr 1, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link

openshift-ci bot commented Apr 1, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign gbraad for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@vyasgun vyasgun changed the title Use CRC_HOME_DIR environment variable while setting base directory [WIP] Use CRC_HOME_DIR environment variable while setting base directory Apr 1, 2024
@cfergeau
Copy link
Contributor

cfergeau commented Apr 3, 2024

This PR is related to #3966

@gbraad
Copy link
Contributor

gbraad commented Apr 10, 2024

This PR is related to #3966

That should be in the first message, as currently it is just the template.

- Add a new config key called crc-dir
- Initialise Crc base directory variables based on the saved config
- New config will take effect after rerunning `crc setup`
@vyasgun vyasgun changed the title [WIP] Use CRC_HOME_DIR environment variable while setting base directory Use crc config to use custom location for .crc directory Apr 25, 2024
@vyasgun
Copy link
Contributor Author

vyasgun commented Apr 25, 2024

@gbraad @cfergeau so sorry about the confusion - It was a WIP so I didn't add details. 😓 Updated the PR with the details and the updated code.

@vyasgun vyasgun marked this pull request as ready for review April 25, 2024 09:01
@gbraad
Copy link
Contributor

gbraad commented Apr 25, 2024

Reading the issue/PR here I wonder how this could work. as the config itself lives inside the .crc folder.

  1. we can store a path inside a ~/.crcpath
  2. or we use a shell/env variable

Doing a crc config set makes sense for a partial relocation, like only cache, and the machines folder.

Using an environment variable also makes this a more deliberate choice, as there are side effects for sure.

@gbraad
Copy link
Contributor

gbraad commented Apr 25, 2024

so sorry about the confusion - It was a WIP so I didn't add details.

Thinking out loud is always helpful, especially in the case when, like in this issue, the situation is not clearly described. It becomes more of a trial and error. Almost like a spike issue. Writing down some things before working on this might help with the actual implementation. I, for example, immediately think of a chicken-egg problem when using this as a configurable from the config itself, but also, what about the long-running processes? They need to be restarted? Also, for a start this can be enforced from a validation with a pre-flight. But for commands like status and stop this only can operate when the VM folder exists, though there is no current mechanism to deal with this.

Copy link

openshift-ci bot commented Apr 25, 2024

@vyasgun: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/security ae9585f link false /test security
ci/prow/integration-crc ae9585f link true /test integration-crc
ci/prow/e2e-crc ae9585f link true /test e2e-crc

Full PR test history. Your PR dashboard.

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.

@gbraad gbraad self-requested a review May 9, 2024 03:24
@gbraad
Copy link
Contributor

gbraad commented May 9, 2024

I haven't looked at the implementation yet, but what would happen when the location is changed after the VM is already created? One of the things we do not perform before the start is to check if all the VM files are available. This might be something that has to exist regardless of this change, but due to the possibility to change the crc-dir it is likely this would get triggered.

@@ -37,6 +37,7 @@ const (
EmergencyLogin = "enable-emergency-login"
PersistentVolumeSize = "persistent-volume-size"
EnableBundleQuayFallback = "enable-bundle-quay-fallback"
CrcDir = "crc-dir"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the config directory or crc directory?

@@ -167,6 +173,10 @@ func GetPreset(config Storage) preset.Preset {
return preset.ParsePreset(config.Get(Preset).AsString())
}

func GetConfigDir(config Storage) string {
return config.Get(CrcDir).AsString()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the question: "Is this the config directory or crc directory?"

@@ -175,6 +176,26 @@ func EnsureBaseDirectoriesExist() error {
return nil
}

func initialiseAllDirectories(crcDir string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not just about directories, as there are also sockets, and configuration files.

@gbraad
Copy link
Contributor

gbraad commented May 9, 2024

Overall the changes make sense, especially to comnfirm the permissions with the validate. However, as mentioned above, more can be at fault. Edge cases exist when a VM is already created; in that case a start could result in an error of an already existing VM by name, but not in the 'new' machines folder.

Please try some of these scenarios to see what I mean.

A possible solution would be to prevent changing this folder when a VM is currently registered.

@gbraad gbraad changed the title Use crc config to use custom location for .crc directory [Spike] Use crc config to use custom location for .crc directory May 16, 2024
@@ -168,6 +168,16 @@ func ValidatePath(path string) error {
return nil
}

// ValidateDirectory checks if provided path exists and has sufficient permissions
func ValidateDirectory(path string) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great for re-use

@vyasgun
Copy link
Contributor Author

vyasgun commented May 17, 2024

The objective of this spike was to test whether the crc config can be used to configure a custom .crc location. The conclusion is that it is not something that will solve the problem and could potentially cause unexpected issues in certain edge cases:

  • If VM has already been started, modifying .crc location could result in inconsistent behaviour.
  • The base case (for the default directory to open from) has to be defined outside of .crc folder as it will result in a chicken and egg problem.
  • The major request to use a custom directory mainly comes from having a custom location to store the larger files such as cache and machine configs. Using more straightforward solution such as using environment variable related configuration may be more helpful to accomplish this.
  • The original request for using XDG base directory env variables in certain cases may be solved using this commit:

Next steps:

  • Redefine the problem and come up with a solution that enables the user to manage their storage more effectively.

@cfergeau
Copy link
Contributor

  • If VM has already been started, modifying .crc location could result in inconsistent behaviour.
  • Using more straightforward solution such as using environment variable related configuration may be more helpful to accomplish this.

Setting the location using crc config set allows us to control when it's allowed/not allowed. Basically, if a VM exists or is running, then changing the location is forbidden, otherwise, why not.
An environment variable would complicate things as crc could only check if it's allowed after it has changed, and without knowing what was .crc location before the location change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants