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

refactor: enable Mutagen by default on Mac and Windows, fixes #4637 #4904

Merged
merged 11 commits into from
Jun 30, 2023

Conversation

gilbertsoft
Copy link
Member

@gilbertsoft gilbertsoft commented May 12, 2023

The Issue

How This PR Solves The Issue

This PR enables Mutagen on Mac and Windows by default. A new config performance (project and global) is introduced and the two existing ones for Mutagen and NFS are removed. This makes it much easier for the users to configure their environments because they can not end with invalid config having both Mutagen and NFS enabled. It's also possible now to override the global config project wise which was the other way around before.

Manual Testing Instructions

  • Mutagen is enabled for all projects on Mac and Windows.
  • Mutagen and NFS are disabled for all projects on Linux.
  • Global config can be changed using ddev config global --performance=....
  • Project config can be changed using ddev config --performance=....
  • Command description is understandable of ddev config global -h and ddev config -h.
  • Description of performance is understandable in the .ddev/config.yaml and ~/.ddev/global_config.yaml.
  • Project respects global config.
  • Project can override global config.

Automated Testing Overview

So far, no tests are added. This may be changed if needed. Existing tests are adapted to the new default.

Related Issue Link(s)

Release/Deployment Notes

@gilbertsoft gilbertsoft requested review from a team as code owners May 12, 2023 14:41
@github-actions
Copy link

github-actions bot commented May 12, 2023

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Just took a quick look through.

  • Both mutagen and nfs can be enabled in the project as well as globally.
  • It's true that the new mutagen default globally should be true.
  • But people should be able to change it globally to false
  • People should also be able to override it in a particular project, setting it to false if they want.
  • Mutagen should not automatically override NFS. Instead, if both are enabled, it should be an error. (And this needs to be done at the project level).

I think many of the things that you want to do here will best be checked with app.IsNFSMountEnabled() and app.IsMutagenEnabled(). That way you can check the app situation (error out if both are enabled, for example).

@gilbertsoft
Copy link
Member Author

* Both mutagen and nfs can be enabled in the project as well as globally.

Should I also implement a change of the project if explicitly set to fasle?

* But people should be able to change it globally to `false`

That's still possbile.

* People should also be able to override it in a particular project, setting it to `false` if they want.

Doesn't work currently, will fix it....

* Mutagen should not automatically override NFS. Instead, if both are enabled, it should be an error. (And this needs to be done at the project level).

I hope this is already done at the moment because nothing change here.

@rfay
Copy link
Member

rfay commented May 12, 2023

Should I also implement a change of the project if explicitly set to fasle?

If mutagen_enabled: false on a project, that should override the global setting. (Which is different from the way it works now.)

  • Mutagen should not automatically override NFS. Instead, if both are enabled, it should be an error. (And this needs to be done at the project level).

In current code, mutagen_enabled: true overrides nfs_mount_enabled: true. That's incorrect behavior, and instead it should result in an error.

Some notes:

  • I don't think we should pester people about turning on mutagen, just have the default change. There are good reasons to turn it off on macOS and Windows, and especially with new mount types like VirtioFS, which may actually end up working right someday.
  • On testing Windows, we're currently in trouble because I can't get traditional Windows tests to run successfully, so turned off in Traditional Windows CI turned off due to Docker Desktop breakage #4875, discouraged about that. So until that's fixed, we won't have actual coverage for traditional Windows.
  • I don't think we should pester people every time about nfs_mount_enabled being on. It's worth talking about. Is there any way we could do an occasional warning about that in the new messages feature?

@gilbertsoft
Copy link
Member Author

Should I also implement a change of the project if explicitly set to fasle?

If mutagen_enabled: false on a project, that should override the global setting. (Which is different from the way it works now.)

  • Mutagen should not automatically override NFS. Instead, if both are enabled, it should be an error. (And this needs to be done at the project level).

In current code, mutagen_enabled: true overrides nfs_mount_enabled: true. That's incorrect behavior, and instead it should result in an error.

Some notes:

* I don't think we should pester people about turning on mutagen, just have the default change. There are good reasons to turn it off on macOS and Windows, and especially with new mount types like VirtioFS, which may actually end up working right someday.

* On testing Windows, we're currently in trouble because I can't get traditional Windows tests to run successfully, so turned off in [Traditional Windows CI turned off due to Docker Desktop breakage #4875](https://github.com/ddev/ddev/issues/4875), discouraged about that. So until that's fixed, we won't have actual coverage for traditional Windows.

* I don't think we should pester people every time about nfs_mount_enabled being on. It's worth talking about. Is there any way we could do an occasional warning about that in the new messages feature?

Okay, will simplify the patch...

@gilbertsoft gilbertsoft force-pushed the task/mutagen-enabled-by-default branch from 4b9f408 to 5289776 Compare May 13, 2023 11:19
@gilbertsoft gilbertsoft marked this pull request as draft May 13, 2023 11:19
cmd/ddev/cmd/config.go Outdated Show resolved Hide resolved
docs/content/users/configuration/config.md Outdated Show resolved Hide resolved
docs/content/users/install/performance.md Outdated Show resolved Hide resolved
docs/content/users/install/performance.md Outdated Show resolved Hide resolved
@gilbertsoft gilbertsoft force-pushed the task/mutagen-enabled-by-default branch 6 times, most recently from 6a9f2dc to 20472b2 Compare June 16, 2023 13:11
@rfay
Copy link
Member

rfay commented Jun 16, 2023

For reference, here's what I did with traefik, which is a little simpler than mutagen but the same idea, and would love your review there @gilbertsoft

Traefik is simpler because the existing settings don't have to be preserved, whereas mutagen we should be preserving.

@gilbertsoft gilbertsoft force-pushed the task/mutagen-enabled-by-default branch 3 times, most recently from ed771b0 to 21f5501 Compare June 16, 2023 16:27
@deviantintegral
Copy link
Collaborator

I'm 100% in support of this change. We specifically call out mutagen in our ddev ADR. We didn't include Windows because none of our developers are using Windows enough to test it there.

Since then, I would say probably 1/3rd of new ddev users setting it up have missed this setting. Then, it surfaces as "ddev / docker / the site" is slow or even at times crashing during heavy IO ops. Making this the default would solve all of that for sure. Thanks for working on this @gilbertsoft !

@gilbertsoft gilbertsoft force-pushed the task/mutagen-enabled-by-default branch 7 times, most recently from bfaf090 to d20e736 Compare June 17, 2023 11:29
@gilbertsoft gilbertsoft force-pushed the task/mutagen-enabled-by-default branch from e17a3e0 to 2df0814 Compare June 26, 2023 19:34
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I think this is all going to be fine, would just like to figure out a few things that may make the communication clearer:

  1. Let's do performance_strategy instead of performance. We don't want people on Linux to think they're not going to have "performance" if it's "off" or "none" or whatever. Sorry we didn't figure this out earlier, but definitely will improve how the docs read.
  2. Could we please use the options "none", "mutagen", and "nfs". An empty value on initialization should be changed into the default for the OS, as we do for the other things in globalconfig.New() for example.

Sorry we didn't figure these things out earlier, but it's easier to see them now when they're in black-and-white.

docs/content/users/configuration/config.md Outdated Show resolved Hide resolved
cmd/ddev/cmd/config.go Outdated Show resolved Hide resolved
cmd/ddev/cmd/config.go Outdated Show resolved Hide resolved
docs/content/users/configuration/config.md Outdated Show resolved Hide resolved
docs/content/users/configuration/config.md Outdated Show resolved Hide resolved
docs/content/users/install/performance.md Show resolved Hide resolved
docs/content/users/usage/commands.md Outdated Show resolved Hide resolved
pkg/config/types/performance.go Outdated Show resolved Hide resolved
pkg/ddevapp/templates.go Outdated Show resolved Hide resolved
pkg/nodeps/values.go Outdated Show resolved Hide resolved
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

ddev describe still uses "Mutagen enabled (ok)". I would hate to make that column wider by saying "performance strategy: mutagen" but we should think about what should go there. I don't think we need to change this right now.

@rfay
Copy link
Member

rfay commented Jun 27, 2023

@gilbertsoft currently the project .ddev/config.yaml comments say this:

# performance_mode: "default"
# DDEV offers performance optimization strategies to improve the filesystem
# performance depending on your host system. Should be configured globally.
#
# If set, will override the global config. Possible values are:
#   - "default": will use the value from the global config.
#   - "none":    will disable performance optimization for this project.
#   - "mutagen": will enable Mutagen for this project.
#   - "nfs":     will enable NFS for this project.

Shouldn't this be the same as what it says in the global_config.yaml? Where "default" is default for OS?

# performance_mode: "default"
# DDEV offers performance optimization strategies to improve the filesystem
# performance depending on your host system. Can be overridden with the project
# config.
#
# Possible values are:
#   - "default": will use the recommended value for this operating system.
#   - "none":    will disable performance optimization.
#   - "mutagen": will enable Mutagen.
#   - "nfs":     will enable NFS.

What is the actual behavior?

@gilbertsoft
Copy link
Member Author

@gilbertsoft currently the project .ddev/config.yaml comments say this:

# performance_mode: "default"
# DDEV offers performance optimization strategies to improve the filesystem
# performance depending on your host system. Should be configured globally.
#
# If set, will override the global config. Possible values are:
#   - "default": will use the value from the global config.
#   - "none":    will disable performance optimization for this project.
#   - "mutagen": will enable Mutagen for this project.
#   - "nfs":     will enable NFS for this project.

Shouldn't this be the same as what it says in the global_config.yaml? Where "default" is default for OS?

# performance_mode: "default"
# DDEV offers performance optimization strategies to improve the filesystem
# performance depending on your host system. Can be overridden with the project
# config.
#
# Possible values are:
#   - "default": will use the recommended value for this operating system.
#   - "none":    will disable performance optimization.
#   - "mutagen": will enable Mutagen.
#   - "nfs":     will enable NFS.

What is the actual behavior?

No, that's fine. As long as nothing is set in the project (or set to default) the value from the global config is taken. This way the user can predefine a default globally which will be used for most projects, exactly you can do it with Git e.g.

@gilbertsoft gilbertsoft changed the title Enabled Mutagen by default on Mac and Windows, fixes #4637 refactor: enabled Mutagen by default on Mac and Windows, fixes #4637 Jun 28, 2023
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Ooops...

ddev config global --performance-mode=none
# Now global_config's performance_mode is `none`
ddev config global --performance-mode-reset
# It reports "mutagen" BUT `ddev config global` says "none"
# BUT `performance_mode` is still "none" in ~/.ddev/global_config.yaml

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Minor docs changes required too, for --performance-mode-reset

docs/content/users/configuration/config.md Show resolved Hide resolved
@rfay
Copy link
Member

rfay commented Jun 28, 2023

TestCmdMutagen seems to have been broken by this latest commit.

@gilbertsoft
Copy link
Member Author

gilbertsoft commented Jun 29, 2023

Ooops...

ddev config global --performance-mode=none
# Now global_config's performance_mode is `none`
ddev config global --performance-mode-reset
# It reports "mutagen" BUT `ddev config global` says "none"
# BUT `performance_mode` is still "none" in ~/.ddev/global_config.yaml

Oh, didn't set dirty=true. Only related to Mac and Windows where I didn't test yesterday.

@gilbertsoft gilbertsoft force-pushed the task/mutagen-enabled-by-default branch from e0f8948 to 73ac93f Compare June 29, 2023 08:12
@gilbertsoft
Copy link
Member Author

TestCmdMutagen seems to have been broken by this latest commit.

Should be related to the missing dirty=true too

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Reviewed and tested on macOS and Gitpod/Linux, and it all seems to be working great, thanks!

I have a few suggestions about the docs, think there are some leftover inaccuracies that should be fixed. I'd be fine with committing them with [skip ci] and avoiding another test cycle. We should probably have @mattstein take a quick look after the commit, or comment on these comments.

Yay, ready to go, thanks for the great work on this!

docs/content/users/install/docker-installation.md Outdated Show resolved Hide resolved
docs/content/users/install/performance.md Outdated Show resolved Hide resolved
docs/content/users/install/performance.md Outdated Show resolved Hide resolved
docs/content/users/install/performance.md Outdated Show resolved Hide resolved
docs/content/users/install/performance.md Outdated Show resolved Hide resolved
docs/content/users/install/performance.md Outdated Show resolved Hide resolved
docs/content/users/usage/commands.md Outdated Show resolved Hide resolved
docs/content/users/usage/faq.md Outdated Show resolved Hide resolved
pkg/config/types/config_type.go Show resolved Hide resolved
pkg/config/types/performance_mode.go Show resolved Hide resolved
@gilbertsoft gilbertsoft changed the title refactor: enabled Mutagen by default on Mac and Windows, fixes #4637 refactor: enable Mutagen by default on Mac and Windows, fixes #4637 Jun 29, 2023
@gilbertsoft gilbertsoft merged commit f78e433 into ddev:master Jun 30, 2023
5 checks passed
@gilbertsoft gilbertsoft deleted the task/mutagen-enabled-by-default branch June 30, 2023 08:15
gilbertsoft added a commit to gilbertsoft/ddev that referenced this pull request Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants