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

fix: prevent various Mutagen conflicts between daemons when changing versions or global directory, fixes #6234 #6239

Merged
merged 34 commits into from
Jun 5, 2024

Conversation

rfay
Copy link
Member

@rfay rfay commented May 23, 2024

The Issue

With our new ability to change the global configuration directory from ~/.ddev to other places come new risks for mutagen.

(Removed): Mutagen now keeps its directory in <globaldir>/.mdd instead of <home>/.ddev_mutagen_data_directory.

There's a risk in changing that mutagen could continue using an obsolete mutagen session or accidentally use a docker volume that did not match.

How This PR Solves The Issue

  • When computing the mutagen hash for uniqueness, include not only the mutagen.yml contents but also the location of the mutagen.yml and the global configuration location.
  • Revert to using ~/.ddev_mutagen_data_directory to resolve risks of upgrades and environment variable changes (XDG_CONFIG_HOME)

Manual Testing Instructions

Still working on explicit instructions that can demonstrate the underlying problem.

Automated Testing Overview

I think the existing tests probably already do enough.

Related Issue Link(s)

Release/Deployment Notes

@rfay rfay requested a review from a team as a code owner May 23, 2024 22:52
@rfay rfay marked this pull request as draft May 23, 2024 22:52
Copy link

github-actions bot commented May 23, 2024

@rfay rfay force-pushed the 20240523_rfay_fix_globaldir_creation branch from 4b5cfd5 to e527960 Compare May 24, 2024 15:07
pkg/globalconfig/global_config.go Outdated Show resolved Hide resolved
pkg/globalconfig/global_config.go Outdated Show resolved Hide resolved
Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

I can't make a suggestion for this line, but we can check for directory with:

-if _, err := os.Stat(linuxDir); err == nil {
+if stat, err := os.Stat(linuxDir); err == nil && stat.IsDir() {

@stasadev stasadev changed the title fix: Mutagen must be unique based on global config and project mutagen location, fifor #6234 fix: Mutagen must be unique based on global config and project mutagen location, for #6234 May 24, 2024
@rfay rfay marked this pull request as ready for review May 24, 2024 18:24
@rfay
Copy link
Member Author

rfay commented May 24, 2024

I think this will be the general fix. I am still working on the catastrophic failure case where mutagen can delete the whole project.

@rfay rfay changed the title fix: Mutagen must be unique based on global config and project mutagen location, for #6234 fix: Mutagen must be unique based on global config and project mutagen location, fixes #6234 May 26, 2024
@rfay rfay force-pushed the 20240523_rfay_fix_globaldir_creation branch from 0ff07a4 to c5ba6d9 Compare May 29, 2024 02:10
@rfay rfay requested a review from stasadev May 29, 2024 13:20
@rfay
Copy link
Member Author

rfay commented May 29, 2024

@stasadev please take a very skeptical look at this. It over-does a couple of things, and it probably actually changes your intent for default global config location on Linux. Now it does XDG_CONFIG_HOME if set, then ~/.ddev if exists, then ~/.config/ddev if exists, and that should be the same on all platforms. AFAICT the test failures are upstream ones at this point, I'm still working on those, but I think this can go in when you and I have looked at it again.

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

I checked for active Mutagen processes and confirmed that they stop after running the new version of DDEV.

There is also an edge case where people decide to use XDG_CONFIG_HOME, run ddev start, and then set it to a different location or unset it, and leave the old Mutagen running.
But I don't think there is anything we can do about that (since we have no way of knowing the old value for XDG_CONFIG_HOME).

Thank you, I think this PR will smooth things over for the Mutagen.

@stasadev
Copy link
Member

Now it does XDG_CONFIG_HOME if set, then ~/.ddev if exists, then ~/.config/ddev if exists, and that should be the same on all platforms.

I think this is a smart change considering DDEV is designed for all platforms 👍

@rfay
Copy link
Member Author

rfay commented May 30, 2024

This one was intended to go in before the other ones and somehow I lost track of it while working on those :)

@rfay
Copy link
Member Author

rfay commented May 30, 2024

I refactored this AGAIN to detect when the MUTAGEN_DATA_DIRECTORY has changed and to use mutagen daemon stop per @xenoscopic's suggestion in #6234 (comment) - instead of pausing sessions when it has. Added global config for last_mutagen_data_directory.

@rfay rfay changed the title fix: Mutagen must be unique based on global config and project mutagen location, fixes #6234 fix: prevent various Mutagen conflicts between daemons when changing versions or global directory, fixes #6234 May 30, 2024
Comment on lines 105 to 117
allKnownMutagenDataDirectories := []string{
filepath.Join(userHome, ".ddev_mutagen_data_directory"), // through v1.23.1
filepath.Join(userHome, ".ddev", ".mdd"), // default current
filepath.Join(userHome, ".config", "ddev", ".mdd"),
}
Copy link
Member

Choose a reason for hiding this comment

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

If we allow MUTAGEN_DATA_DIRECTORY to be changed from the env variable, and that value is saved in the DDEV global configuration, perhaps we should add it to this list:

if globalconfig.DdevGlobalConfig.LastMutagenDataDirectory != "" && !slices.Contains(allKnownMutagenDataDirectories, globalconfig.DdevGlobalConfig.LastMutagenDataDirectory) {
	allKnownMutagenDataDirectories = append(allKnownMutagenDataDirectories, globalconfig.DdevGlobalConfig.LastMutagenDataDirectory)
}

This will cover the case when someone adds a custom MUTAGEN_DATA_DIRECTORY, but then changes it, or unsets it:

export DDEV_DEBUG=true
export MUTAGEN_DATA_DIRECTORY=~/.ddev_mutagen2
ddev start
# if we don't check for LastMutagenDataDirectory, mutagen daemon in ~/.ddev_mutagen2 won't be stopped
unset MUTAGEN_DATA_DIRECTORY
ddev start

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we ever allow MUTAGEN_DATA_DIRECTORY to be set in the user's environment do we? We force it to the configuration determined by the code.

Copy link
Member

Choose a reason for hiding this comment

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

I think this was an unintentional change from me in #5813, but GetMutagenDataDirectory() now reads MUTAGEN_DATA_DIRECTORY env before setting the directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

But we set MUTAGEN_DATA_DIRECTORY in the init() in a.go.

_ = os.Setenv("MUTAGEN_DATA_DIRECTORY", globalconfig.GetMutagenDataDirectory())

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. I'm going to have to take another look at that. Thanks for pointing that out.

Copy link
Member

@stasadev stasadev May 30, 2024

Choose a reason for hiding this comment

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

Oh, this was added from the start:

func GetMutagenDataDirectory() string {
currentMutagenDataDirectory := os.Getenv("MUTAGEN_DATA_DIRECTORY")
if currentMutagenDataDirectory != "" {
return currentMutagenDataDirectory
}
// If it's not already set, return ~/.ddev_mutagen_data_directory
// This may be affected by tests that change $HOME
return GetGlobalDdevDir() + "_" + "mutagen_data_directory"
}

Some people may rely on using a custom MUTAGEN_DATA_DIRECTORY even if this is not documented in our docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's likely that people are setting this, but I sure appreciate you pointing this out.

It's quite awkward for mutagen not to have a command-line arg instead of env var. I'll open an issue about that.

Copy link
Member Author

Choose a reason for hiding this comment

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

pkg/testcommon/testcommon.go Outdated Show resolved Hide resolved
@rfay rfay force-pushed the 20240523_rfay_fix_globaldir_creation branch 2 times, most recently from 0d92953 to 5944ace Compare May 31, 2024 21:24
pkg/globalconfig/global_config.go Outdated Show resolved Hide resolved
@rfay rfay force-pushed the 20240523_rfay_fix_globaldir_creation branch 3 times, most recently from 358b86f to d6db153 Compare June 2, 2024 23:04
@rfay
Copy link
Member Author

rfay commented Jun 4, 2024

It was such a good idea! But my brain was off-kilter. The mutagen daemon sync session syncs between filesystem on the host and the container's /var/www/html. So the more of these we have going, the more trouble we have.

Here's the situation on my computer right now:

MUTAGEN_DATA_DIRECTORY=/Users/rfay/tmp/dd1/ddev/.mdd
$ export MUTAGEN_DATA_DIRECTORY=/Users/rfay/tmp/dd1/ddev/.mdd; ~/.ddev/bin/mutagen sync list
--------------------------------------------------------------------------------
Name: d10
Identifier: sync_X4NOdoqOMTlIgEMo6lpDn9TK06jtsRBMBPhgqxRCRtW
Alpha:
	URL: /Users/rfay/workspace/d10
	Connected: Yes
	Synchronizable contents:
		7040 directories
		27066 files (136 MB)
		4 symbolic links
Beta:
	URL: docker://ddev-d10-web/var/www/html
		DOCKER_HOST=unix:///Users/rfay/.orbstack/run/docker.sock
	Connected: Yes
	Synchronizable contents:
		7040 directories
		27066 files (136 MB)
		4 symbolic links
Status: Watching for changes
--------------------------------------------------------------------------------

export MUTAGEN_DATA_DIRECTORY=/Users/rfay/.ddev/.mdd; ~/.ddev/bin/mutagen sync list
$ export MUTAGEN_DATA_DIRECTORY=/Users/rfay/.ddev/.mdd; ~/.ddev/bin/mutagen sync list
--------------------------------------------------------------------------------
Name: d10
Identifier: sync_xT9dQybWgGvLks1FxjkrXtUhyQDyaK1AlgW6mJyQ4Bf
Alpha:
	URL: /Users/rfay/workspace/d10
	Connected: Yes
	Synchronizable contents:
		7040 directories
		27066 files (136 MB)
		4 symbolic links
Beta:
	URL: docker://ddev-d10-web/var/www/html
		DOCKER_HOST=unix:///Users/rfay/.orbstack/run/docker.sock
	Connected: Yes
	Synchronizable contents:
		7040 directories
		27066 files (136 MB)
		4 symbolic links
Status: Watching for changes
--------------------------------------------------------------------------------

Now both of these are synchonizing from the same alpha (host) to the same beta (here the docker container) at docker://ddev-d10-web/var/www/html - both fighting away!

With this situation I was able to get the host-side code deleted nearly every time :) I happened to have GoLand set up with a debug config that had XDG_CONFIG_HOME=~/tmp/dd1 and an iterm terminal that had XDG_CONFIG_HOME unset (so using ~/.ddev)

Every time (almost?) I do the start in the debugger it deletes everything. And if I kill the debug session... it leaves yet another mutagen session running, synchronizing to the same place. I had 8 of them :)

@rfay rfay force-pushed the 20240523_rfay_fix_globaldir_creation branch from 7bb3cd6 to 3d52e89 Compare June 4, 2024 16:35
@rfay
Copy link
Member Author

rfay commented Jun 4, 2024

Oh so sad. Latest commits

  • Revert (previously unpushed) GetMutagenVolumeName() "improvements", which were pretty fancy but offered no value in the end. And they caused a lot of calls to docker context
  • Revert use of MUTAGEN_DATA_DIRECTORY being in the global ddev dir + .mdd. It seemed impossible to sort out the risks with this.

@stasadev
Copy link
Member

stasadev commented Jun 4, 2024

I prepared a revert patch for the docs:

curl https://github.com/ddev/ddev/commit/d3b8ee8ca59875ac13102337b261e1f4d4b13079.patch -o mutagen.patch
git apply mutagen.patch

@rfay
Copy link
Member Author

rfay commented Jun 4, 2024

Thanks so much for that. I'll apply that after we see how the tests do.

(It would probably have been easier and easier to review to do a PR against this PR)

@rfay
Copy link
Member Author

rfay commented Jun 4, 2024

I think this is ready to go. Or I'll die. I'm pretty sure that we've removed most of the risk by going back to the traditional MUTAGEN_DATA_DIRECTORY.

Anyway, it's important to get it in and get experience with it. I think it will do fine.

@rfay rfay merged commit 213f469 into ddev:master Jun 5, 2024
19 checks passed
@rfay rfay deleted the 20240523_rfay_fix_globaldir_creation branch June 5, 2024 00:52
jonesrussell pushed a commit to jonesrussell/ddev that referenced this pull request Jun 9, 2024
…versions or global directory, fixes ddev#6234 (ddev#6239)

Co-authored-by: Stanislav Zhuk <stasadev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants