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

[JUJU-1200] Wait for CRM to be active before migrating #14388

Merged

Conversation

jack-w-shaw
Copy link
Member

@jack-w-shaw jack-w-shaw commented Jul 28, 2022

Model migration tests using cross-model relations were often failing because the migration would be started before the CRM was active. Resolve this by waiting until the CRM is active in effected tests

Also fix some shfmt lint failures

Checklist

  • Code style: imports ordered, good names, simple structure, etc
  • [ ] Comments saying why design decisions were made
  • [ ] Go unit tests, with comments saying what you're testing
  • Integration tests, with comments saying what you're testing
  • [ ] doc.go added or updated in changed packages

QA steps

./main.sh -v -s 'test_model_config,test_model_multi,test_model_metrics' model

Also fix some shfmt failures
Copy link
Member

@wallyworld wallyworld left a comment

Choose a reason for hiding this comment

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

We need to fix the root cause. Papering over the cracks by adding a wait to the test doesn't help users who try and run a migration "too soon". The root cause is that export model fails if the connection is not yet ready. So there's 2 things to fix in juju: 1. juju dump-model (which calls export model) needs to handle this case gracefully; 2. migration needs to fail with a user friendly error or else handle the case gracefully if we thing that it is feasible that the migrated model will still pick up where things were left off

tests/suites/model/migration.sh Show resolved Hide resolved
Copy link
Member

@wallyworld wallyworld left a comment

Choose a reason for hiding this comment

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

This PR #14414 fixes the underlying issue highlighted by the test failure - a 5 year old bug. We still need the wait, but the error being surfaced is now correctly determined as part of the migration prechecks and not via an unexpected internal error.

@jack-w-shaw
Copy link
Member Author

/merge

@jujubot jujubot merged commit 027f450 into juju:develop Aug 3, 2022
@jack-w-shaw jack-w-shaw deleted the JUJU-1200_fix_failing_migration_tests branch August 3, 2022 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants