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

Clear Unknown Indices #1065

Merged
merged 8 commits into from
Dec 31, 2022
Merged

Conversation

bodo-hugo-barwich
Copy link
Contributor

@bodo-hugo-barwich bodo-hugo-barwich commented Aug 20, 2022

This implements the --delete --all functionality discussed at:
Fix Corrupted Indices
which is needed to close definitely the issue of Index Corruption
Unexpected Index Corruption
by

  1. Preventing the Index Corruption
  2. Recovering from Index Corruption with "Clear All Indices" option

Additionally it implements Exit with Error Code for unconfirmed Operations
as mentioned at:
ElasticSearch Availabilty Check
This is also part of the logic to prevent accidental Indices Corruption

To test the functionality it introduces several Mapping Script Tests.

These tests are required to check the "Verify Mappings" functionality developed at:
Verify Indices Mappings

@bodo-hugo-barwich
Copy link
Contributor Author

bodo-hugo-barwich commented Aug 21, 2022

The tests of the --delete --all logic show that this functionality is save and should not be able to run in production if my assumptions about the environment recognition are as documented in the related issue:
recognize development environment
flag development environment with environment variables
Still there is also a manual protection as requested at:
manual confirmation required
which must throw an exception and exit the script as documented at:
unconfirmed operation throws exception and exits the script

Of course the ultimate test will happen in the actual production environment where the script must fail on --delete --all and the prompt ALL Indices will be deleted !!! must not happen.

@mickeyn
Copy link
Contributor

mickeyn commented Aug 23, 2022

@bodo-hugo-barwich thanks for all this effort, but I'm a bit lost here,
Can you please explain what is the purpose of all these big changes? they are all touching our very sensitive production indexing system but we don't have an idea of why this is all happening.

@bodo-hugo-barwich
Copy link
Contributor Author

bodo-hugo-barwich commented Sep 3, 2022

@mickeyn thank you for taking a look at this development.

It was already discussed that this functionality cannot be run in a production environment and therefore it cannot touch the sensitive production indexing system

I would always have preferred that you had taken part in your requirement discussion at:
fix corrupted development environment
because at this stage the development is already completed.

@bodo-hugo-barwich
Copy link
Contributor Author

what is the state of this development ?

@bodo-hugo-barwich bodo-hugo-barwich force-pushed the no-47_clear-indices branch 3 times, most recently from 514ea79 to 8410fab Compare December 23, 2022 05:55
mickeyn
mickeyn previously approved these changes Dec 23, 2022
@oalders
Copy link
Member

oalders commented Dec 23, 2022

@bodo-hugo-barwich
Copy link
Contributor Author

Hi @oalders , yes, I saw that the tests are failing.
The create_index operation requires that the index does not exist but it was created to test the --delete --all operation which did fail because the test environment could not be recognized as such:

2022/12/23 06:05:21 E mapping: Operation not permitted!
[2022/12/23 06:05:21] [catalyst] [ERROR] Operation not permitted!
2022/12/23 06:05:21 F mapping: Operation not permitted in environment: production
[2022/12/23 06:05:21] [catalyst] [FATAL] Operation not permitted in environment: production

        #   Failed test 'delete all succeeds'
        #   at t/lib/MetaCPAN/TestServer.pm line 434.

        #   Failed test 'Exit Code '0' - No Error'
        #   at t/lib/MetaCPAN/TestServer.pm line 435.
        #          got: '1'
        #     expected: '0'
        # Looks like you failed 2 tests of 2.

    #   Failed test 'delete all succeeds'
    #   at t/lib/MetaCPAN/TestServer.pm line 437.

@oalders Is there a way that the automated test environment can be marked as such? Perhaps setting the MOJO_MODE to an appropriate value?

@oalders
Copy link
Member

oalders commented Dec 26, 2022

@bodo-hugo-barwich I think that would be appropriate. Maybe we just need to set an env var in .circleci/config.yml? If it were part of this PR, that would be a helpful way to test it. https://circleci.com/docs/set-environment-variable/

@bodo-hugo-barwich
Copy link
Contributor Author

The change in the Automated Testing Environment is part of the metacpan-docker.
Set MOJO_MODE in Automated Testing Environment
So, this will become an requirement for this merge to succeed.

@bodo-hugo-barwich
Copy link
Contributor Author

With the MOJO_MODE setting in the MetaCPAN API test the test suite should now succeed.
But the Automated Tests are not rerun since Friday.
https://app.circleci.com/pipelines/github/metacpan/metacpan-api/229/workflows/b736e36e-3283-4d2b-b5e6-ea2e0092e2e6/jobs/237
@oalders is it possible to trigger a rerun?

@oalders
Copy link
Member

oalders commented Dec 29, 2022

@bodo-hugo-barwich I have just triggered a re-run at CircleCI.

@bodo-hugo-barwich
Copy link
Contributor Author

bodo-hugo-barwich commented Dec 30, 2022

Inspecting the runtime environment during the test it turns out that the expected Environment Variables are never set:
https://app.circleci.com/pipelines/github/metacpan/metacpan-api/232/workflows/24008785-20e1-4fa8-9cb1-855daa386658/jobs/241

Use of uninitialized value $ENV{"PLACK_ENV"} in concatenation (.) or string at t/lib/MetaCPAN/TestServer.pm line 377.
test_delete_fails - PLACK_ENV: ''
Use of uninitialized value $ENV{"MOJO_MODE"} in concatenation (.) or string at t/lib/MetaCPAN/TestServer.pm line 378.
test_delete_fails - MOJO_MODE: ''
2022/12/30 13:28:50 E mapping: Operation not permitted!
[2022/12/30 13:28:50] [catalyst] [ERROR] Operation not permitted!
2022/12/30 13:28:50 F mapping: Operation not permitted in environment: production
[2022/12/30 13:28:50] [catalyst] [FATAL] Operation not permitted in environment: production

This can be understood from the fact that the test are run in the sandboxed Docker environment. And this environment is defined in docker-compose.yml:
api_test definition in docker-compose.yml

    env_file:
      - localapi_test.env

Understanding this I found that I made two changes to the localapi*.env files:

$ git diff origin/no-47_clear-indices localapi_test.env
diff --git a/localapi_test.env b/localapi_test.env
index 4ea8bc2..1f49954 100644
--- a/localapi_test.env
+++ b/localapi_test.env
@@ -8,3 +8,4 @@ METACPAN_SERVER_CONFIG_LOCAL_SUFFIX=testing
 MINICPAN=/CPAN
 PERL_MM_USE_DEFAULT=1
 PERL_CARTON_PATH=/carton
+MOJO_MODE=testing
$ git diff origin/no-47_clear-indices localapi.env
diff --git a/localapi.env b/localapi.env
index f0a4aa4..97514a8 100644
--- a/localapi.env
+++ b/localapi.env
@@ -6,3 +6,4 @@ ES_TEST=elasticsearch_test:9200
 MINICPAN=/CPAN
 PERL_MM_USE_DEFAULT=1
 PERL_CARTON_PATH=/carton
+MOJO_MODE=development

So, I will need to revert the changes in the Automated Testing Environment and add those changes to the metacpan-docker project.

@bodo-hugo-barwich
Copy link
Contributor Author

The changes in
MOJO_MODE for Docker executions
will introduce these settings in the Docker environment.

@oalders oalders merged commit d6d656d into metacpan:master Dec 31, 2022
@oalders
Copy link
Member

oalders commented Dec 31, 2022

This is only failing on a tidy test now, so we can merge and tidy in a different PR. Thanks for your patience @bodo-hugo-barwich and thank you for the reviews @mickeyn.

Happy New Year!

@mickeyn
Copy link
Contributor

mickeyn commented Dec 31, 2022

Cool. Thanks the work and follow up @bodo-hugo-barwich and @oalders .

Happy New Year! 😉

@bodo-hugo-barwich
Copy link
Contributor Author

Thank you @oalders and @mickeyn for your trust! 😄

Happy New Year!
And welcoming a new year of meaningful quality work! 😄 🎉

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.

3 participants