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

Magento 2: Cache key id prefix #3453

Merged
merged 7 commits into from
Mar 30, 2023
Merged

Conversation

valguss
Copy link
Contributor

@valguss valguss commented Jan 18, 2023

  • Bug fix #…?

  • New feature?

  • BC breaks?

  • Tests added?

  • Docs added?

    Please, regenerate docs by running next command:
    $ php bin/docgen
    

@valguss
Copy link
Contributor Author

valguss commented Jan 18, 2023

@antonmedv This is an updated version of this #3388

It now does all the operations on the server and should be a bit easier to see what's going on

The general gist is that during deployment, we copy the configuration and update the cache keys for the new deployment so that it uses it own totally separate cache to the current live site. It then moves the updated configuration in place to be the live configuration ready for next deployment

@antonmedv
Copy link
Member

Looks good to me. 👍🏻

@antonmedv
Copy link
Member

@peterjaap please review

@valguss
Copy link
Contributor Author

valguss commented Feb 13, 2023

Just rebasing the docs again

@antonmedv
Copy link
Member

@peterjaap merging?

@valguss valguss changed the title Cache key id prefix Magento 2: Cache key id prefix Feb 28, 2023
@valguss
Copy link
Contributor Author

valguss commented Mar 6, 2023

Rebased docs again

@peterjaap
Copy link
Contributor

@valguss I'm still unsure what problem this exactly solves. I understand what it does, but I don't know which issues this fixes. So I'm hesitant to include new code for something that doesn't effect everyone.

@valguss
Copy link
Contributor Author

valguss commented Mar 6, 2023

Ahh, The issue is based from here davidalger/capistrano-magento2#151
Essentially, it's a way to run a deployed instance of magento on a totally fresh cache.
The main issue we see is that on high traffic sites where this is not implemented, the code generation part of magento setup:di:compile uses the currently deployed cache rather than a fresh cache which can cause it to incorrectly compile the site. When this happens, another redeploy is required to compile correctly

Hope that explains it more

Cheers

Tom

@peterjaap
Copy link
Contributor

@valguss so this is in the scenario where you run setup:di:compile on the target production machine instead of in a build pipeline?

@valguss
Copy link
Contributor Author

valguss commented Mar 6, 2023

So it runs in the pipeline but because the cache in redis would be shared between the current version and the deploying version, the deploying version wouldn't necessarily generate all the classes required to run. We see this happening where we add new modules and the dependency injection is not created because the production cache dosen't know about the new module.

@peterjaap
Copy link
Contributor

@valguss ok so it does run in a pipeline, but it runs on the production machine and not on an isolated build runner?

@valguss
Copy link
Contributor Author

valguss commented Mar 6, 2023

That's correct. Most of the Magento commands run in the deployment folder on the deployment server

@peterjaap
Copy link
Contributor

In that case I'm not in favor of merging this into the default recipe since running those commands on the production server instead of a build pipeline isn't standard (or advised) practice. What do you think @schmengler / @Schrank ?

@valguss
Copy link
Contributor Author

valguss commented Mar 6, 2023

Not sure I follow. All the magent2 commands in the recipe run on a target server before being pushed live? Maybe I've confused the situation

recipe/magento2.php Outdated Show resolved Hide resolved
recipe/magento2.php Outdated Show resolved Hide resolved
recipe/magento2.php Outdated Show resolved Hide resolved
recipe/magento2.php Outdated Show resolved Hide resolved
recipe/magento2.php Outdated Show resolved Hide resolved
recipe/magento2.php Outdated Show resolved Hide resolved
recipe/magento2.php Outdated Show resolved Hide resolved
recipe/magento2.php Outdated Show resolved Hide resolved
recipe/magento2.php Outdated Show resolved Hide resolved
recipe/magento2.php Outdated Show resolved Hide resolved
@peterjaap
Copy link
Contributor

peterjaap commented Mar 6, 2023

@valguss ok I misunderstood, this recipe does indeed run every command straight on the production server (I work on another one that does a push-only approach from a runner instance, hence the confusion).

I've added some comments.

@valguss
Copy link
Contributor Author

valguss commented Mar 6, 2023

Thanks @peterjaap I'll work through those and get another push later today. Appreciate the input :)

@schmengler
Copy link
Contributor

schmengler commented Mar 6, 2023

@valguss what is the reason of manipulating env.php like this instead of using bin/magento setup:config:set? Looks overly complicated to me but I'm probably missing something

recipe/magento2.php Outdated Show resolved Hide resolved
@valguss
Copy link
Contributor Author

valguss commented Mar 6, 2023

It's like this because the env.php is shared across deployments. If we set the config update during deployment then the currently deployed site can also use the updated value and pollute the cache

@schmengler
Copy link
Contributor

it could be set after the symlink has been switched

@schmengler
Copy link
Contributor

Oh, I should have read the whole conversation

the code generation part of magento setup:di:compile uses the currently deployed cache rather than a fresh cache which can cause it to incorrectly compile the site. When this happens, another redeploy is required to compile correctly

I'm only using artifact deployment where setup:di:compile is not executed on the target server, so as long as this is not affected by default, I'm fine with whatever helps your use case

@valguss
Copy link
Contributor Author

valguss commented Mar 6, 2023

Unfortunately, it needs to be set before then as the fresh cache is required during the Magento compile section. Without this change, the currently live and deploying codebases share the config and cache during magento:compile. It's specifically this section where having a fresh non-polluted cache is essential for the compilation to work correctly

@valguss
Copy link
Contributor Author

valguss commented Mar 29, 2023

Now updated to use a config flag to enable

@antonmedv
Copy link
Member

@peterjaap @schmengler let's merge it and release?

@peterjaap
Copy link
Contributor

peterjaap commented Mar 30, 2023

@valguss could you move the if check if (get('use_redis_cache_id')) { to around where the hook is defined?

That way, the task itself doesn't start if use_redis_cache_id is not true. Now, it does start and finishes immediatly, showing up in the task log.

So:

if (get('use_redis_cache_id')) {
    after('deploy:shared', 'magento:set_cache_prefix');
}

and

if (get('use_redis_cache_id')) {
    after('deploy:magento', 'magento:cleanup_cache_prefix');
}

@valguss
Copy link
Contributor Author

valguss commented Mar 30, 2023

@peterjaap That's now added. Cheers

@peterjaap
Copy link
Contributor

Ok, LGTM!

@antonmedv antonmedv merged commit 7fa09a2 into deployphp:master Mar 30, 2023
@valguss valguss deleted the cacheKeyIdPrefix branch March 30, 2023 15:53
@guvra
Copy link
Contributor

guvra commented Apr 5, 2023

FYI, I don't mind this feature because it's optional, but it will break your env.php file if you follow good practices, such as using preload keys

cf. https://experienceleague.adobe.com/docs/commerce-operations/configuration-guide/cache/redis/redis-pg-cache.html#redis-preload-feature

'cache' => [
    'frontend' => [
        'default' => [
            'id_prefix' => '061_',
            'backend' => 'Magento\\Framework\\Cache\\Backend\\Redis',
            'backend_options' => [
                'preload_keys' => [
                    '061_EAV_ENTITY_TYPES',
                    '061_GLOBAL_PLUGIN_LIST',
                    '061_DB_IS_UP_TO_DATE',
                    '061_SYSTEM_DEFAULT',
                ],
        ],
        'page_cache' => [
            'id_prefix' => '061_'
        ]
    ]
]

The prefix of the preload keys must match the cache id prefix so it's a very bad idea to change it during each deployment.

@valguss
Copy link
Contributor Author

valguss commented Apr 5, 2023

Thanks @guvra i'll work on updating the repload keys as well for a future update

valguss added a commit to valguss/deployer that referenced this pull request Apr 6, 2023
- Also updated to remove the config set and instead manually add the hocks to your deployer file
- And move var-exporter into the main composer require
- deployphp#3453 (comment)
valguss added a commit to valguss/deployer that referenced this pull request May 22, 2023
- Also updated to remove the config set and instead manually add the hocks to your deployer file
- And move var-exporter into the main composer require
- deployphp#3453 (comment)
@schmengler
Copy link
Contributor

By the way, this is also helpful in edge cases if you don't compile on the target server, for example when you deploy an update where a cache type was removed:

[staging] run /usr/bin/php7.4 /.../releases/.../src/bin/magento cache:flush
[staging] err In ClassReader.php line 34:
[staging] err Class Vertex\Tax\Model\Cache\Type does not exist

valguss added a commit to valguss/deployer that referenced this pull request Jul 3, 2023
- Also updated to remove the config set and instead manually add the hocks to your deployer file
- And move var-exporter into the main composer require
- deployphp#3453 (comment)
antonmedv pushed a commit that referenced this pull request Jul 4, 2023
* DD#0000: feat: Magento2: Add support for preload keys

- Also updated to remove the config set and instead manually add the hocks to your deployer file
- And move var-exporter into the main composer require
- #3453 (comment)

* DD#0000: feat: Reverted composer updates

* DD#0000: feat: Updated docs

* DD#0000: feat: Updated replace to use preg to make sure it only checks beginning of string
Updated readme use better grammer

* DD#0000: feat: Updated md docs
okolya added a commit to integer-net/deployer that referenced this pull request Sep 29, 2023
okolya added a commit to integer-net/deployer that referenced this pull request Sep 29, 2023
* DD#0000: feat: Magento2: Add support for preload keys

- Also updated to remove the config set and instead manually add the hocks to your deployer file
- And move var-exporter into the main composer require
- deployphp#3453 (comment)

* DD#0000: feat: Reverted composer updates

* DD#0000: feat: Updated docs

* DD#0000: feat: Updated replace to use preg to make sure it only checks beginning of string
Updated readme use better grammer

* DD#0000: feat: Updated md docs

(cherry picked from commit 48193c6)
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.

5 participants