Merge ~hloeung/charm-telegraf:master into charm-telegraf:master

[Haw Loeung]

Fix support for Xenial

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

b73a4e4... by Haw Loeung on 2023-07-21

Restore support for Xenial

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:ec5daf02475b96c4097ec902d500b08408e90c3f
https://jenkins.canonical.com/bootstack/job/lp-charm-telegraf-ci/15/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1325/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/387/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-telegraf-ci/15//rebuild

review: Approve (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:b73a4e45cdbbe9a339dbdfa37997ecba2c97697a
https://jenkins.canonical.com/bootstack/job/lp-charm-telegraf-ci/16/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1326/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/388/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-telegraf-ci/16//rebuild

review: Approve (continuous-integration)

[Tianqi Xiao (txiao)]

Could you please explain the reason for restoring xenial support? This support was dropped because xenial has long passed its end of security support. And since telegraf, along w/ other LMA charms, is deprecated in favor of COS, we need to be mindful about what changes can and should be made to the project.

review: Needs Information

[Haw Loeung (hloeung)]

Xenial is still supported via Expanded Security Maintenance (ESM) per the link below:

| https://ubuntu.com/security/esm

It says "Until 2026" and sadly, we still have critical servers deployed using Juju with this charm running Xenial.

I feel that we have dropped support too early here but sure, feel free to reject this MP. I'll publish the charm under a different namespace.

[Haw Loeung (hloeung)]

FWIW, I published this as https://charmhub.io/hloeung-telegraf and confirms it works fine on Jammy, Focal, and Xenial. Had no Bionic machines in this environment that I needed it for.

[Robert Gildein (rgildein)]

Hi @hloung, you are right that Xenial is supported via Expanded Security Maintenance (ESM),
but I do not think that it's good idea to restore it to main branch. Why i think so is
because, in charmhub.io it will be visible as supported, what is not really true, because
only security patches should be supported and that should be done via separate branch and
channel. Like `xenial` and `xenial/stable`.

Is this patch fixing some critical issues for Xenial? If not what is reason to do it, since
you can still do `juju deploy telegraf --series xenial` and you will get revision 49. If yes,
I would suggest to create Xenial branch as I mention before.

review: Needs Information

[Tianqi Xiao (txiao)]

As Robert mentioned, if the changes are really necessary, a more appropriate approach would be push the changes to a separate "xenial" branch and release the built charm to the "xenial" track. The branch and the track has already existed for telegraf and are available to use. But we first need to evaluate the necessity of these changes.

[Haw Loeung (hloeung)]

Would it help being accepted if I drop commit b73a4e4 adding Xenial support to charmcraft.yaml? That way it won't appear to be supported on Charm hub.

But we retain the Python package pinning in wheelhouse.txt so the charm would continue to work on Xenial. The wheelhouse.txt changes bumps the versions for requests and urllib3 but pins and drops versions for MarkupSafe, Jinja2, and certifi. That's currently:

MarkupSafe-2.0.1.tar.gz -> MarkupSafe-1.1.1.tar.gz
Jinja2-3.0.3.tar.gz -> Jinja2-2.11.0.tar.gz
certifi-2022.12.7.tar.gz -> certifi-2021.10.8.tar.gz

The reason for doing this is because we currently still have systems using Xenial. Using mojo with bundles automatically upgrades the charms or pushes the latest charms[1]. We're using a single Juju subordinate application that's related to applications of different series[2] so would be nice to not have a separate "xenial" branch/channel.

[1]https://pastebin.canonical.com/p/gd95cKk4yQ/
[2]https://pastebin.canonical.com/p/J6vjbzMwhv/

[Robert Gildein (rgildein)]

I do not think that it's the beast idea in general to limit python package, because:
- those older version could have security issues
- it can cause some other package to fail, because it will required newer version
- we can hit an issue when updating them again

However the charm-telegraf has lot of old dependencies (it's "nice" to see lines like
`# Support for python3.4 (Trusty)`).

What we can do if you really need to have it in main branch is to move these changes to
separate branch, but instead of releasing to `xenial/...` we will release it to `latest`
and immediately build +release version from master branch. Like this the newest version
can be hidden behind `latest`. This will bring more work with release (releasing, promoting
always two version), but if I'm correct there will no much contribution to these charm due
migration to cos.

I'll open discussion in our team about this and I'll let you know.

[Tianqi Xiao (txiao)]

@hloeung Your proposal would not work as what you expect. Juju uses the `runs-on` definitions in charmcraft.yaml to decided whether a revision can be deployed on a Ubuntu series. So without adding xenial support in charmcraft.yaml, the charm built on top of your changes will not be recognized as a revision that xenial telegraf deployment can be upgraded to.

I agree with Robert, an internal discussion is needed to determine whether supporting a more-complicated release workflow is possible/desirable.

[Haw Loeung (hloeung)]

Yes, please open an internal discussion with your team.

@txiao, it isn't Juju that uses the `runs-on` but rather `charmcraft`, the tooling used to build said charm. The way it is set up in `charm-telegraf` though, with `build-on` in addition to the pinned versions in `wheelhouse.txt` doesn't really make this limited to the series defined in `runs-on`. Not to mention that with `charm/2.x/stable`, it uses the bundled python 3.6 with the `charm` snap.

As for limiting python packages, sadly, it's needed as newer releases of python packages will (eventually) drop support for certain python versions. layer-basic, used by reactive charms, has bunch of limitations for this reason, see:

| https://github.com/juju-solutions/layer-basic/blob/master/wheelhouse.txt

[Tianqi Xiao (txiao)]

I think you misunderstood my point. Yes, charmcraft is the tool building the charm, but my point is the way Juju determines whether a revision of the charm can support a specific series is to call a charmhub API which contains the series information defined under `runs-on` in charm's `charmcraft.yaml`. So your proposal, pinging python packages to lower versions without explicitly adding xenial support in charmcraft.yaml, won't work because when juju queries charmhub, it sees that this new revision based on your changes is not xenial-compatible, even if the code itself and the python packages packed inside it are.

[Haw Loeung (hloeung)]

`juju deploy --force` or `juju upgrade-charm --force`

[Haw Loeung (hloeung)]

deploy --force:

| Allow a charm/bundle to be deployed which bypasses checks such as supported series or LXD profile allow list

upgrade-charm --force-series:

| Refresh even if series of deployed applications are not supported by the new charm

[Tianqi Xiao (txiao)]

Hmm, now I understand your intention, thanks for letting me know. "--force" option is something I usually saves as the last resort, so I was not taking it into account dealing with this issue. We will keep you posted when the internal discussion reaches a conclusion.

[Robert Gildein (rgildein)]

Hi @hloeung, we dissuaded this and we unanimously agreed that the best solution here would be fix your
tool to support separate channel and provide separate channel for backporting Xenial.
What that be possible? Can we report such bug in tool you are using?

[Haw Loeung (hloeung)]

Thanks for the update and time in discussing this internally with the team.

Any chance we could publish this to a separate channel then? Can you do that or should I?

[Robert Gildein (rgildein)]

Happy to help.

Are there any in previous commits (since [1] or rev 49, which I successfully deployed), which we want to address and they are fixing
some security issues? If no, I don't see reason for creating such channel, since Expanded
Security Maintenance (ESM) is meant only for security and I do not want to create the obligation
that we will support Xenial.

I believe that the best solution will be to release your own version of charm-telegraph under any
personal account.

---
[1]: https://git.launchpad.net/charm-telegraf/commit/charmcraft.yaml?id=0a7832aa6aa5a665ec80bb386db9b0915979d9c6

[Haw Loeung (hloeung)]

Okay, appreciate the time taken to review this MP. Thanks!

Unmerged commits

b73a4e4... by Haw Loeung on 2023-07-21

Restore support for Xenial

ec5daf0... by Haw Loeung on 2023-07-21

Fix support for Xenial

diff --git a/charmcraft.yaml b/charmcraft.yaml
index 559f214..29f7ec2 100644
--- a/charmcraft.yaml
+++ b/charmcraft.yaml
@@ -34,3 +34,7 @@ bases:
         channel: "18.04"
         architectures:
           - amd64
+      - name: ubuntu
+        channel: "16.04"
+        architectures:
+          - amd64
diff --git a/src/wheelhouse.txt b/src/wheelhouse.txt
index 2943205..4efd906 100644
--- a/src/wheelhouse.txt
+++ b/src/wheelhouse.txt
@@ -1,10 +1,6 @@
 # Support for python3.4 (Trusty)
 defusedxml<0.6.0
 redis<3.4.0
-requests<2.22.0
-urllib3<1.25.8
-# https://code.launchpad.net/~hloeung/layer-snap/+git/layer-snap/+merge/403105
-tenacity<5.0.4
 
 # insights-core -> lockfile -> pbr
 # lockfile is retired, and it won't install pbr, specify explicitly here
@@ -12,4 +8,10 @@ pbr
 insights-core
 
 # Support for python 3.5 (Xenial)
+MarkupSafe<2.0.0
+Jinja2==2.11
 cachecontrol<=0.12.6
+certifi<=2021.10.8
+charmhelpers<1.2.1
+requests<2.26.0
+urllib3<1.26.10