Skip to content

Conversation

sinclert-canonical
Copy link
Contributor

This PR refactors the upgrade set of tests to Jubilant (JuJu 3 exclusive):

  • Upgrade vanila tests, porting existing ones from this file.
  • Upgrade from stable tests, porting existing ones from this file.
  • Upgrade incompatible tests, porting existing ones from this file.
  • Upgrade check-skipping tests, porting existing ones from this file.

The _juju_3 suffix has been added to the file names containing the ported integration tests to differentiate the Juju 3 exclusive tests, from their juju version agnostic siblings. Can discuss an alternative terminology.

@sinclert-canonical sinclert-canonical added the not bug or enhancement PR is not 'bug' or 'enhancement'. For release notes label Sep 22, 2025
@sinclert-canonical sinclert-canonical force-pushed the sinclert/8314/jubilant-upgrade-tests branch 8 times, most recently from a14d8ef to 5e18658 Compare September 25, 2025 07:42
@sinclert-canonical sinclert-canonical force-pushed the sinclert/8314/jubilant-upgrade-tests branch from 5e18658 to 51cfc11 Compare September 25, 2025 10:46
@sinclert-canonical
Copy link
Contributor Author

👋🏻 Hey folks! A couple of things worth discussing when reviewing this PR:

  1. Given that upgrade tests are currently run both for Juju 2 & 3, the old ops_test modules and the new jubilant rewritten modules will need to co-exists. Are we happy disabling Juju 3 execution in the old ops_test based tests? Are we comfortable with the _juju_3 prefix added to the jubilant based tests?

  2. I think there is something fishy with the skip-pre-upgrade-check upgrade tests. Both tests listed within such file implement a very custom waiting strategy (see code), which, unless I am missing something, does not work as intended. I checked the logs for the all 5 nightly runs, and the log message stated when a unit reaches maintance status is never printed (see, 21/09 job, 22/09 job, 23/09 job, 24/09 job, 25/09 job).

    Therefore, and it seems we never wait for the MySQL application to become active: we do a simple 120s wait, where we check stale unit information over and over. Opinions how to proceed?

@sinclert-canonical sinclert-canonical marked this pull request as ready for review September 25, 2025 12:05
@carlcsaposs-canonical
Copy link
Contributor

  1. Given that upgrade tests are currently run both for Juju 2 & 3, the old ops_test modules and the new jubilant rewritten modules will need to co-exists

I believe jubilant has support for juju 2 through https://github.com/tonyandrewmeyer/jubilant-backports, could we use that? Also, I believe Tony is currently working to upstream that into official jubilant

2. I think there is something fishy with the skip-pre-upgrade-check upgrade tests.

@paulomach does refresh v1 without pre-upgrade-check work at all? I thought that wasn't supported; surprised we have a test for that

@paulomach
Copy link
Contributor

  1. Given that upgrade tests are currently run both for Juju 2 & 3, the old ops_test modules and the new jubilant rewritten modules will need to co-exists

I believe jubilant has support for juju 2 through https://github.com/tonyandrewmeyer/jubilant-backports, could we use that? Also, I believe Tony is currently working to upstream that into official jubilant

Yes, that would be ideal.

  1. I think there is something fishy with the skip-pre-upgrade-check upgrade tests.

@paulomach does refresh v1 without pre-upgrade-check work at all? I thought that wasn't supported; surprised we have a test for that

It's strongly not recommend and can cause some availability disruption, but we did include some code here and there on the charm to mitigate a catastrophe ;)

@sinclert-canonical
Copy link
Contributor Author

I believe jubilant has support for juju 2 through https://github.com/tonyandrewmeyer/jubilant-backports, could we use that?

I have my concerns about using a package hanging from a GitHub user (despite being a Canonical employee), and not from an organization. Apparently, only a single repo in the whole organization is using it (see GitHub search).

Nevertheless, it seems you both are for it, so I will investigate and report my findings.


What should we do about the skip_pre_upgrade_check then?

  • A) Rewrite it as is? (i.e. removing the useless wait in the Jubilant version)
  • B) Get rid of it altogether?

@sinclert-canonical sinclert-canonical marked this pull request as draft September 26, 2025 08:46
@sinclert-canonical sinclert-canonical force-pushed the sinclert/8314/jubilant-upgrade-tests branch 2 times, most recently from c9d1863 to 60ec76a Compare September 26, 2025 09:50
@sinclert-canonical sinclert-canonical force-pushed the sinclert/8314/jubilant-upgrade-tests branch from 60ec76a to 76e6183 Compare September 26, 2025 11:50
@sinclert-canonical sinclert-canonical force-pushed the sinclert/8314/jubilant-upgrade-tests branch from 76e6183 to 6d70aa7 Compare September 26, 2025 11:54
@sinclert-canonical
Copy link
Contributor Author

Nevertheless, it seems you both are for it, so I will investigate and report my findings.

Just chatted with Tony Meyers.

He guaranteed that the jubilant_backports API will not change, and that regardless of where the code lives, they will keep publishing it to the same PyPi package name. He also clarified that the only design difference with respect to the jubilant package lies in the juju bootstrap command, which is something we are not leveraging in our tests. I tried it out, and seems to work just as well.

Both this and K8s PR are ready for review, aside from:

  • The legacy _juju_3 prefix set on all the new test modules (their tests will replace the old ones before merge).
  • The upgrade rollback-incompat test error that I still need to debug locally.

cc @paulomach @carlcsaposs-canonical

@sinclert-canonical sinclert-canonical marked this pull request as ready for review September 26, 2025 12:50
@sinclert-canonical sinclert-canonical force-pushed the sinclert/8314/jubilant-upgrade-tests branch from 6d70aa7 to 5f6b91b Compare September 26, 2025 13:57
Copy link
Contributor

@paulomach paulomach left a comment

Choose a reason for hiding this comment

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

Looks good

@sinclert-canonical
Copy link
Contributor Author

Rewritten upgrade tests moved to the existing Python files.

@sinclert-canonical sinclert-canonical force-pushed the sinclert/8314/jubilant-upgrade-tests branch from e095733 to 1eb238e Compare September 30, 2025 09:41
@sinclert-canonical sinclert-canonical force-pushed the sinclert/8314/jubilant-upgrade-tests branch from 1eb238e to c76bc0e Compare September 30, 2025 12:21
@sinclert-canonical sinclert-canonical merged commit 3063901 into main Oct 1, 2025
735 of 749 checks passed
@sinclert-canonical sinclert-canonical deleted the sinclert/8314/jubilant-upgrade-tests branch October 1, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Libraries: Out of sync not bug or enhancement PR is not 'bug' or 'enhancement'. For release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants