magento2 icon indicating copy to clipboard operation
magento2 copied to clipboard

[vcl] Better debug

Open gquintard opened this issue 4 years ago • 91 comments

Detecting if the response is a hit usingthe x-varnish header is flaky and notably doesn't work in a multi-tier setup.

On top of this, HIT/MISS is a false dichotomy (https://info.varnish-software.com/blog/using-obj-hits) and while I'd like something more detailed like https://docs.varnish-software.com/tutorials/hit-miss-logging/ (tech version of the blog), I recognize that it's "a lot" of VCL, so this commit only differentiates between hits, misses and passes, the latter being pretty useful to identify while debugging.

note: all versions have been changed for the sake of consistency but both the 4.x and 5.x series have been EOL'd a (long) while ago and users should be encouraged to upgraded as soon as possible.

edit: I've added a test that you can run with varnishtest dev/tests/varnish/*.vtc since the current php suite doesn't correctly test the committed VCL. It would probably be smart to run that command as part of the test suite, but I'll leave that to more knowledgeable people than myself.

Contribution checklist (*)

  • [ ] Pull request has a meaningful description of its purpose
  • [ ] All commits are accompanied by meaningful commit messages
  • [ ] All new or changed code is covered with unit/integration tests (if applicable)
  • [ ] All automated tests passed successfully (all builds are green)

Resolved issues:

  1. [x] resolves magento/magento2#37912: [vcl] Better debug

gquintard avatar Jun 30 '20 21:06 gquintard

Hi @gquintard. Thank you for your contribution Here is some useful tips how you can test your changes using Magento test environment. Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

:exclamation: Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests

You can find more information about the builds here

:information_source: Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

m2-assistant[bot] avatar Jun 30 '20 21:06 m2-assistant[bot]

@magento run all tests

gquintard avatar Jun 30 '20 21:06 gquintard

@magento run Unit Tests

lbajsarowicz avatar Jun 30 '20 21:06 lbajsarowicz

Hi @lbajsarowicz, thank you for the review. ENGCOM-7766 has been created to process this Pull Request :eight_spoked_asterisk: @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

magento-engcom-team avatar Jun 30 '20 21:06 magento-engcom-team

@magento run Integration Tests, Functional Tests CE

lbajsarowicz avatar Jul 02 '20 10:07 lbajsarowicz

@lbajsarowicz, apologies for my ignorance, is there anything I should do here?

gquintard avatar Jul 15 '20 14:07 gquintard

@gquintard No. Now we wait for the process of the merge to be performed.

lbajsarowicz avatar Jul 15 '20 15:07 lbajsarowicz

Automated Tests are not applicable, as the change is related to infrastructure configuration.

lbajsarowicz avatar Jul 24 '20 12:07 lbajsarowicz

Dev experience is required for testing of this PR. Please note that Manual testing has not been performed.

engcom-Bravo avatar Oct 02 '20 15:10 engcom-Bravo

@engcom-Bravo seems like you missed to assign the PR. I just did it for you

ihor-sviziev avatar Oct 05 '20 12:10 ihor-sviziev

@engcom-Charlie basicwlly varnish 4 already reached end of life https://info.varnish-software.com/blog/options-at-varnish-cache-4-end-of-support . I believe if everything works fine on 5 and 6 - let’s revert changes in varnish4 config

ihor-sviziev avatar Oct 07 '20 10:10 ihor-sviziev

Indeed, obj.uncacheable was introduced later, I missed that, sorry.

@ihor-sviziev , 5.X is also EOL, if you want to just focus on 6.X: https://varnish-cache.org/releases/index.html

gquintard avatar Oct 07 '20 23:10 gquintard

@gquintard could you just revert changes for varnish4 and varnish5 than?

ihor-sviziev avatar Oct 08 '20 08:10 ihor-sviziev

done

gquintard avatar Oct 20 '20 22:10 gquintard

@magento run all tests

ihor-sviziev avatar Oct 21 '20 03:10 ihor-sviziev

Hi @ihor-sviziev, thank you for the review. ENGCOM-7766 has been created to process this Pull Request

magento-engcom-team avatar Dec 16 '20 07:12 magento-engcom-team

@magento run Functional Tests CE, Functional Tests EE

ihor-sviziev avatar Jan 14 '21 15:01 ihor-sviziev

functional tests EE are failing, but I don't see anyway for them to fail the way they do. False negative?

gquintard avatar Feb 06 '21 00:02 gquintard

@magento run Functional Tests EE

ihor-sviziev avatar Feb 09 '21 08:02 ihor-sviziev

@magento run Functional Tests EE

ihor-sviziev avatar Mar 03 '21 14:03 ihor-sviziev

bump?

gquintard avatar May 18 '21 17:05 gquintard

hi, we are almost at the one-year anniversary of that PR, is there something I can do to make it happen?

gquintard avatar Jun 11 '21 22:06 gquintard

@gquintard, unfortunately, this PR has low priority and will be processed after all high priority ones

ihor-sviziev avatar Jun 12 '21 04:06 ihor-sviziev

@gquintard can you resolve conflicts

mrtuvn avatar Aug 22 '23 00:08 mrtuvn

@mrtuvn , done

gquintard avatar Aug 22 '23 04:08 gquintard

@magento run all tests. Rerun checks

mrtuvn avatar Aug 22 '23 13:08 mrtuvn

Failed to run the builds. Please try to re-run them later.

@magento create issue

engcom-Lima avatar Aug 23 '23 07:08 engcom-Lima

@magento run all tests

engcom-Hotel avatar Aug 28 '23 05:08 engcom-Hotel

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.