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.

Sign up for GitHub

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

Jump to bottom

Optional Mutagen #749

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Optional Mutagen #749

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7948a82
optional Mutagen in warden env config
Feb 15, 2024
3d18dc8
added mutagen enable by default
Feb 16, 2024
c5f8761
refactoring appendEnvPartialIfExists
Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion commands/env.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ trap '' ERR
## define source repository
if [[ -f "${WARDEN_HOME_DIR}/.env" ]]; then
eval "$(sed 's/\r$//g' < "${WARDEN_HOME_DIR}/.env" | grep "^WARDEN_")"

## configure mutagen enable by default
WARDEN_MUTAGEN_ENABLE=${WARDEN_MUTAGEN_ENABLE:-1}
Copy link
Member

Choose a reason for hiding this comment

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

We should only automatically enable mutagen for Darwin OS Types. While the current code won't have an issue with this variable being enabled, in a theoretical future where we expand Mutagen support beyond Mac OS (why would we ever?) this would be enable Mutagen for people who were not expecting it.

Copy link
Author

Choose a reason for hiding this comment

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

There is no problem because WARDEN_MUTAGEN_ENABLE is not consulted by itself, to use Mutagen it is verified that it is active and that you use Darwin OS Types

fi
export WARDEN_IMAGE_REPOSITORY="${WARDEN_IMAGE_REPOSITORY:-"docker.io/wardenenv"}"

Expand Down Expand Up @@ -174,7 +177,10 @@ TRAEFIK_ADDRESS="$(docker container inspect traefik \
)"
export TRAEFIK_ADDRESS;

if [[ $OSTYPE =~ ^darwin ]]; then
if [[ $OSTYPE =~ ^darwin ]] && [[ ${WARDEN_MUTAGEN_ENABLE} -eq 1 ]]; then
echo -e "\033[31mMutagen is being deprecated!!\033[0m"
Copy link
Contributor

Choose a reason for hiding this comment

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

Did anyone compare performance on different Docker installations, not only for OrbStack?
Unless we confirm that it works better w/o mutagen all the time, we can't make it deprecated. It will add confusion

Copy link
Author

Choose a reason for hiding this comment

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

Hello @ihor-sviziev, in the latest versions of Docker Desktop, performance is higher, RAM consumption is reduced and we will not have file synchronization problems.
It is important to use VirtioFS

Copy link
Member

Choose a reason for hiding this comment

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

@joseluisi4 I think what @ihor-sviziev is getting at is: do we have numbers to back up the fact that Docker bind mounts are now more performant than Mutagen? Docker can spout off that it fixed memory or resource usage, but it doesn't always turn into real-world gains in every scenario.

So the question remains is do we have evidence to back up the claim that docker has higher performance, less RAM consumption and no file sync problems? Do we have it just for a single system type, or is it across all system types (Windows, Intel Mac, ARM Mac, x86_64 Linux, ARM Linux)? Without evidence to that, I think he's suggesting we maybe not mark it as deprecated yet, and wait until we get more concrete evidence that it is in fact more performant before calling it deprecated.

Copy link
Member

Choose a reason for hiding this comment

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

Particularly I remember asking someone to run these figures before and they came back with VirtioFS was not more performant than Mutagen, but unfortunately I've been having trouble finding them here or in the Den repository.

At the same time, I don't want to with-hold progress - but it would be good to know how performant VirtioFS is compared to Mutagen within large projects.

Copy link
Contributor

@ihor-sviziev ihor-sviziev Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@bap14, yeah, you're right. My suggestion was not to mark it as deprecated but rather add the ability to disable it in a backward-compatible manner.

We might add a notice like "We noticed that you're on MacOS and using Mutagen sync. We recommend configuring Docker Desktop to use VirtioFS and disable mutagen sync by adding WARDEN_MUTAGEN_DISABLED=1 to your project .env file.".

In the next six months, we can collect feedback if people see any issues by disabling mutagen on macOS. There might be cases when mutagen-enabled flow works significantly faster. If no such issues - we can finally mark it as deprecated.

Also, I have a question: how will it work for those now using mutagen but then switching to non-mutagen installation? We should probably recommend terminating some volume to reduce disk usage. This also should be documented.

Copy link

Choose a reason for hiding this comment

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

I'm agree with @ihor-sviziev
I have checked performance on mac M1 Pro and my subjective assessment - there is no performance difference.
Make sense to wait another developers responses and to have this option.

Copy link

Choose a reason for hiding this comment

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

UPD
I have played little bit more with virtioFS sync and found performance degrade for cli command execution.

di:co command execution with mutagen:

Compilation was started.
App action list generation... 8/8 [============================] 100% 11 secs 542.6 MiB
Generated code and dependency injection configuration successfully.

di:co command execution with virtioFS:

Compilation was started.
App action list generation... 8/8 [============================] 100% 56 secs 544.6 MiB
Generated code and dependency injection configuration successfully.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. @joseluisi4 please remove the deprecation notice. I fully support letting everyone make this decision for themselves, but the Warden project still fully endorses Mutagen as the best way for fast, iterative development.

echo -e "\033[31mTo disable it, add WARDEN_MUTAGEN_ENABLE=0 in ~/.warden/.env file\033[0m"

export MUTAGEN_SYNC_FILE="${WARDEN_DIR}/environments/${WARDEN_ENV_TYPE}/${WARDEN_ENV_TYPE}.mutagen.yml"

if [[ -f "${WARDEN_HOME_DIR}/environments/${WARDEN_ENV_TYPE}/${WARDEN_ENV_TYPE}.mutagen.yml" ]]; then
Expand Down
2 changes: 2 additions & 0 deletions commands/install.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,5 +93,7 @@ if [[ ! -f "${WARDEN_HOME_DIR}/.env" ]]; then
WARDEN_PORTAINER_ENABLE=0
# SEt to "0" to disable DNSMasq
WARDEN_DNSMASQ_ENABLE=1
# Set to "1" to enable Mutagen
WARDEN_MUTAGEN_ENABLE=1
Copy link
Member

Choose a reason for hiding this comment

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

I need to look into why this section is using tabs instead of spaces, but for now please use tabs for alignment here.

Suggested change
# Set to "1" to enable Mutagen
WARDEN_MUTAGEN_ENABLE=1
# Set to "1" to enable Mutagen
WARDEN_MUTAGEN_ENABLE=1

EOT
fi
22 changes: 18 additions & 4 deletions utils/env.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ function loadEnvConfig () {
;;
esac

if [[ -f "${WARDEN_HOME_DIR}/.env" ]]; then
eval "$(sed 's/\r$//g' < "${WARDEN_HOME_DIR}/.env" | grep "^WARDEN_")"
fi

assertValidEnvType
}

Expand Down Expand Up @@ -122,18 +126,28 @@ function appendEnvPartialIfExists () {
"${WARDEN_DIR}/environments/includes/${PARTIAL_NAME}.base.yml" \
"${WARDEN_DIR}/environments/includes/${PARTIAL_NAME}.${WARDEN_ENV_SUBT}.yml" \
"${WARDEN_DIR}/environments/${WARDEN_ENV_TYPE}/${PARTIAL_NAME}.base.yml" \
"${WARDEN_DIR}/environments/${WARDEN_ENV_TYPE}/${PARTIAL_NAME}.${WARDEN_ENV_SUBT}.yml" \
"${WARDEN_HOME_DIR}/environments/includes/${PARTIAL_NAME}.base.yml" \
"${WARDEN_HOME_DIR}/environments/includes/${PARTIAL_NAME}.${WARDEN_ENV_SUBT}.yml" \
"${WARDEN_HOME_DIR}/environments/${WARDEN_ENV_TYPE}/${PARTIAL_NAME}.base.yml" \
"${WARDEN_HOME_DIR}/environments/${WARDEN_ENV_TYPE}/${PARTIAL_NAME}.${WARDEN_ENV_SUBT}.yml" \
"${WARDEN_ENV_PATH}/.warden/environments/includes/${PARTIAL_NAME}.base.yml" \
"${WARDEN_ENV_PATH}/.warden/environments/includes/${PARTIAL_NAME}.${WARDEN_ENV_SUBT}.yml" \
"${WARDEN_ENV_PATH}/.warden/environments/${WARDEN_ENV_TYPE}/${PARTIAL_NAME}.base.yml" \
"${WARDEN_ENV_PATH}/.warden/environments/${WARDEN_ENV_TYPE}/${PARTIAL_NAME}.${WARDEN_ENV_SUBT}.yml"
"${WARDEN_ENV_PATH}/.warden/environments/${WARDEN_ENV_TYPE}/${PARTIAL_NAME}.base.yml"
do
if [[ -f "${PARTIAL_PATH}" ]]; then
DOCKER_COMPOSE_ARGS+=("-f" "${PARTIAL_PATH}")
fi
done

Copy link
Member

Choose a reason for hiding this comment

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

@joseluisi4 This doesn't seem like it would work to me. The value of PARTIAL_NAME will be "mutagen", but there's no check against the partial name.

I think a better solution would be to check the PARTIAL_NAME prior to the for loop, and if it's "mutagen" and mutagen is disabled, then just return from the function; otherwise, process the for-loop as originally developed.

Copy link
Author

Choose a reason for hiding this comment

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

PARTIAL_NAME will never be "mutagen", it is the name of the environment

The problem I am trying to solve is that the xxx.darwin.yml files are not used because these volumes are designed for Mutange

Another solution would be to rename those files to xxx.mutagen.yml, this way you could filter by name

Copy link
Member

Choose a reason for hiding this comment

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

Partial name is not the name of the environment. The appendEnvPartialIfExists function takes the name of the service partial to include (e.g. nginx, or php), not the environment. See

warden/commands/env.cmd

Line 80 in 39d4729

&& appendEnvPartialIfExists "nginx"

My reservation about removing the OS-specific partials is that if you have an os-specific partial for a service in your environment but aren't using Mutagen, it will never be included.

To me this would be an opportunity to properly refactor the mutagen inclusion to use the appendEnvPartialIfExists function to add Mutagen syncs, and only including them if the OS is darwin and Mutagen is enabled. This would then also mean that the checks for starting / pausing Mutagen would need to be adjusted to use the WARDEN_MUTAGEN_ENABLE variable.

if [[ "${WARDEN_ENV_SUBT}" == "darwin" ]] && [[ "${WARDEN_MUTAGEN_ENABLE}" == "1" ]]; then
for PARTIAL_PATH in \
"${WARDEN_DIR}/environments/${WARDEN_ENV_TYPE}/${PARTIAL_NAME}.${WARDEN_ENV_SUBT}.yml" \
"${WARDEN_HOME_DIR}/environments/${WARDEN_ENV_TYPE}/${PARTIAL_NAME}.${WARDEN_ENV_SUBT}.yml" \
"${WARDEN_ENV_PATH}/.warden/environments/${WARDEN_ENV_TYPE}/${PARTIAL_NAME}.${WARDEN_ENV_SUBT}.yml"
do
if [[ -f "${PARTIAL_PATH}" ]]; then
DOCKER_COMPOSE_ARGS+=("-f" "${PARTIAL_PATH}")
fi
done
fi;

}