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 3.x Support #608

Merged
merged 26 commits into from
Feb 20, 2024
Merged

Juju 3.x Support #608

merged 26 commits into from
Feb 20, 2024

Conversation

freyes
Copy link
Member

@freyes freyes commented Jul 14, 2023

This pull request adds juju-3.x support.

Summary of changes:

By default when installing 'zaza' won't have a pinned libjuju, hence users will get the latest juju available in pypi, although downstream consumers are encouraged to pin the library to the version compatible with the juju controller they want to connect to.

Running functional tests locally (tox -e func-target -- first) will use libjuju-2.9 by default, if contributors need/want to use a different version they can override the constraint setting the PIP_CONSTRAINTS environment variable, e.g.:

env PIP_CONSTRAINTS=$(pwd)/constraints-juju31.txt tox -e func-target --recreate -- first

Closes #545

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (1b002d2) 89.43% compared to head (aec6e76) 89.50%.
Report is 4 commits behind head on master.

Files Patch % Lines
zaza/utilities/juju.py 86.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #608      +/-   ##
==========================================
+ Coverage   89.43%   89.50%   +0.07%     
==========================================
  Files          44       45       +1     
  Lines        4693     4744      +51     
==========================================
+ Hits         4197     4246      +49     
- Misses        496      498       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@freyes freyes marked this pull request as ready for review July 14, 2023 20:51
@freyes
Copy link
Member Author

freyes commented Jul 14, 2023

⚠️ this is a breaking change, any thoughts on how we could enable a smoother transition are much appreciated.

@freyes
Copy link
Member Author

freyes commented Jul 17, 2023

When we try to use juju<3.0 as the default (empty extra), the installation process fails with the error below when running pip install zaza[juju-32]

ERROR: Cannot install zaza[juju-32]==0.0.2.dev1 because these package versions have conflicting dependencies.

The conflict is caused by:
    zaza[juju-32] 0.0.2.dev1 depends on juju<3.0
    zaza[juju-32] 0.0.2.dev1 depends on juju<3.3.0 and >=3.2.0; extra == "juju-32"

If we do this other change we can pull in the latest version of libjuju by default unless an extra is specified, although this won't prevent us from submitting a batch that changes the test-requirements.txt, and if this was the approach we may not even need to define extras and just a constraints file to lock down libjuju to an specific range.

diff --git a/setup.py b/setup.py
index 8fd19e5..96c5d00 100644
--- a/setup.py
+++ b/setup.py
@@ -48,6 +48,7 @@ tests_require = [
 
 extras_require={
     'testing': tests_require,
+    '': ['juju'],
     'juju-29': ['juju<3.0'],
     'juju-31': ['juju>=3.1.0,<3.2.0'],
     'juju-32': ['juju>=3.2.0,<3.3.0'],

@freyes
Copy link
Member Author

freyes commented Jul 18, 2023

Added a new commit that gives a more sane default 084fced

 Depend on 'juju' when no extra is passed.

By default depend on 'juju' (no pinning), the pinning only comes into
place when an explicit extra is passed.

This makes the extras effectively nothing more than a pinning alias,
basically `pip install juju<3.0 zaza` becomes equivalent to `pip install
zaza[juju-29]`.

@freyes
Copy link
Member Author

freyes commented Jul 18, 2023

Added a new commit that gives a more sane default 084fced

This behavior would still break the charms gate unless we change zosci-config default juju_channel value[0] from 2.9/stable to 3.2/stable in lockstep. It would prevent us from doing a batch and "just" deal with the possible breakages because we would be using juju 3.2 now.

If this is the path we decide to pursue, the change needs to be made "git branch aware", so only master/main use 3.2 while "stable/.*" keep using juju 2.9

[0] https://github.com/openstack-charmers/zosci-config/blob/2eb52a0a72c638a10717b448a3edd6e1a1c639a3/zuul.d/jobs.yaml#L196C5-L196C5

Copy link

@coreycb coreycb left a comment

Choose a reason for hiding this comment

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

Hi Felipe, I added a couple of comments.

zaza/model.py Outdated
results = action.data.get('results')
await action.wait()
results = None
try:
Copy link

Choose a reason for hiding this comment

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

Can we use _normalise_action_object() here?

Copy link
Member Author

Choose a reason for hiding this comment

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

ack, addressed by 10351db

zaza/model.py Outdated
results = action.data.get('results')
await action.wait()
results = None
try:
Copy link

Choose a reason for hiding this comment

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

Can we use _normalise_action_object() here?

Copy link
Member Author

Choose a reason for hiding this comment

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

ack, addressed by 10351db

zaza/model.py Outdated
@@ -1096,6 +1135,7 @@ async def async_run_action(unit_name, action_name, model_name=None,
unit = await async_get_unit_from_name(unit_name, model)
action_obj = await unit.run_action(action_name, **action_params)
await action_obj.wait()
action_obj = _normalise_action_object(action_obj)
if raise_on_failure and action_obj.status != 'completed':
Copy link

Choose a reason for hiding this comment

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

Global comment: should we switch this to action_obj.data['status'] ?

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed by 7a5dba6

@@ -2184,7 +2231,15 @@ async def _check_for_file(model):
for unit in units:
try:
output = await unit.run('test -e "{}"; echo $?'.format(path))
contents = output.data.get('results')['Stdout']
await output.wait()
Copy link

Choose a reason for hiding this comment

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

Can we use _normalise_action_object() here?

Copy link
Member Author

Choose a reason for hiding this comment

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

ack, addressed by 10351db

Copy link
Collaborator

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

I'm generally okay, note @coreycb comments on using _normalize_action_object.

@rgildein
Copy link
Contributor

rgildein commented Oct 4, 2023

Hi folks, can we merge this one along with #617? Thanks

@freyes freyes marked this pull request as draft October 5, 2023 13:31
@freyes
Copy link
Member Author

freyes commented Oct 5, 2023

constraints seem to not be making effect

func-target: install_deps> python -I -m pip install -r /home/runner/work/zaza/zaza/test-requirements.txt -c /home/runner/work/zaza/zaza/constraints-juju-29.txt
...
  Installed /home/runner/work/zaza/zaza/.tox/func-target/lib/python3.10/site-packages/zaza-0.0.2.dev1-py3.10.egg
  Processing dependencies for zaza==0.0.2.dev1
  Searching for juju==3.2.2
  Best match: juju 3.2.2
  Adding juju 3.2.2 to easy-install.pth file
  detected new path './zaza-0.0.2.dev1-py3.10.egg'

@chanchiwai-ray
Copy link

chanchiwai-ray commented Oct 10, 2023

Hi, I am wondering if we should also wrap _normalise_action_results to this line, as in (for example)

def _normalise_action_object(action_obj):
    try:
        # libjuju 3.x
        action_obj.data['status'] = action_obj._status
        action_obj.data['results'] = _normalise_action_results(action_obj.results) # wrap here
    except (AttributeError, KeyError):
        # libjuju 2.x format, no changes needed
        pass

    return action_obj

Because currently running actions with not return an action object with normalised action.results or action.data["results"]

javacruft and others added 13 commits January 16, 2024 16:01
Add separate workflow for testing on Python 3.6 as this
requires an older Ubuntu release.

Fixup unit tests for compatibility with newer versions of mock.

Drop latest/stable functional test targets - this is actually
2.9.x of Juju and is already covered by the 2.9 targets and we
want to avoid suddenly picking up a new Juju version because
this will break with the new approach to version alignment in the
Python module for Juju.

Drop 2.8 functional test target - its broken and we don't really
support this version any longer.

Fixup iptables forwarding issues from LXD containers with a flush
and re-create of rules.

(cherry picked from commit 9277a94)
Juju is pinned to <3.0 earlier. This
patch pins the juju version to <3.2
so that libjuju 3.1 version is used.

Modified run_on_unit to wait for
completion and update results based
on output.
Update channel in github workflows to use Juju 3.1.

Drop --no-gui flag usage as this is the default now.
Update model configuration default-series to focal.

Drop --classic flag for Juju installation.
When juju is strictly confined, random temp directories under /tmp
are not accessible - render any templated bundle files under $HOME
instead as this should be readable.
python-libjuju is released in lockstep with juju, hence if zaza uses a
2.9 controller, it should use libjuju-2.9.x, for a 3.1 controller it
should use libjuju-3.1 and so on.

This change makes libjuju an extra, which means depending on the juju
controller version will be used the right extra should be passed at
install time.

For juju-2.9:

    pip install zaza[juju-29]

For juju-3.1:

    pip install zaza[juju-31]
Summary of changes:

- Add juju-3.2 to the github workflow matrix
- Add 'juju-32' extra to install juju-3.2.x
This forces tox to install the zaza python package and honor the
'extras' defined.
By default depend on 'juju' (no pinning), the pinning only comes into
place when an explicit extra is passed.

This makes the extras effectively nothing more than a pinning alias,
basically `pip install juju<3.0 zaza` becomes equivalent to `pip install
zaza[juju-29]`.
By default libjuju 2.9 will be used, this can overriden by passing the
PIP_CONSTRAINTS environment variable

Examples:

    PIP_CONSTRAINTS=./constraints-juju31.txt tox -e pep8 --recreate

This allows running functional tests with different versions of juju
@freyes freyes marked this pull request as ready for review January 17, 2024 20:46
@freyes freyes requested a review from ajkavanagh January 17, 2024 20:47
@freyes freyes removed the request for review from gnuoy January 19, 2024 13:13
The testing runners use Python 3.8 (Focal), hence no need to keep py36
alive.
@freyes
Copy link
Member Author

freyes commented Feb 7, 2024

Switching to draft until this issue gets sorted out.

2024-02-06 18:07:38.229111 | focal-medium |   "msg": "Ooops! Snap installation failed while executing 'sh -c \"/usr/bin/snap install --classic --channel 3.1/stable charmcraft\"', please examine logs and error output for more details.",

https://openstack-ci-reports.ubuntu.com/artifacts/f0b/908183/1/check/charm-build/f0b6c6e/job-output.txt
https://review.opendev.org/c/openstack/charm-keystone/+/908183

@freyes freyes marked this pull request as draft February 7, 2024 13:04
@freyes
Copy link
Member Author

freyes commented Feb 7, 2024

Related PR: openstack-charmers/zosci-config#309

fnordahl and others added 2 commits February 13, 2024 12:02
Juju 3.x replaced the `series` status key with a `base` key that
consists of Distribution type and version number.

To avoid maintenance burden we add a Launchpad module that
implements functions to look up available Ubuntu series data.

Update the `get_machine_series` helper function to determine
Ubuntu series from `base` when no `series` key is available.

Signed-off-by: Frode Nordahl <[email protected]>
This file aims to hold the default version of juju that it's expected
charms to be tested with. It helps to serve as a sane default for
tox.ini
@freyes
Copy link
Member Author

freyes commented Feb 20, 2024

Issue found here in the CI runs - juju/python-libjuju#1028

The juju snap can't run programs that are outside the snap due to the
confinement restrictions
@freyes
Copy link
Member Author

freyes commented Feb 20, 2024

Here I'm attaching the logs of running env TEST_ZAZA_BUG_LP1987332=on PIP_CONSTRAINTS=$(pwd)/constraints-juju31.txt tox -e func-target -- third 2>&1 | tee func-target.thirds.juju31.log successfully.

2024-02-20 11:26:53 [INFO] Events:
  Configure zaza.charm_tests.noop.setup.basic_setup:
    Start: 1708439209.878124
    Finish: 1708439209.8789518
    Elapsed Time: 0.000827789306640625
    PCT Of Run Time: 1
  Deploy Bundle:
    Start: 1708438869.2582512
    Finish: 1708438878.273779
    Elapsed Time: 9.015527725219727
    PCT Of Run Time: 3
  Prepare Environment:
    Start: 1708438867.0046818
    Finish: 1708438869.258144
    Elapsed Time: 2.253462076187134
    PCT Of Run Time: 1
  Test zaza.charm_tests.libjuju.tests.RegressionTest:
    Start: 1708439209.8796163
    Finish: 1708439213.3465443
    Elapsed Time: 3.466928005218506
    PCT Of Run Time: 2
  Test zaza.charm_tests.noop.tests.NoopTest:
    Start: 1708439209.8790593
    Finish: 1708439209.8796015
    Elapsed Time: 0.0005421638488769531
    PCT Of Run Time: 1
  Wait for Deployment:
    Start: 1708438878.2737825
    Finish: 1708439209.8773825
    Elapsed Time: 331.603600025177
    PCT Of Run Time: 96
Metadata: {}

Full logs: func-target.thirds.juju31.log

@ajkavanagh ajkavanagh merged commit c44d359 into openstack-charmers:master Feb 20, 2024
8 of 11 checks passed
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.

Regression in libjuju 3 ; API changes in (at least) running commands on a unit
8 participants