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

Incorrect schema is used to generate the clients #1099

Closed
SimonRichardson opened this issue Sep 16, 2024 · 8 comments · Fixed by #1179
Closed

Incorrect schema is used to generate the clients #1099

SimonRichardson opened this issue Sep 16, 2024 · 8 comments · Fixed by #1179
Assignees
Labels
area/schema-changes Updates the Juju API schema hint/3.x going on main branch kind/maintenance priority/normal normal priority

Comments

@SimonRichardson
Copy link
Member

SimonRichardson commented Sep 16, 2024

Description

Whilst reviewing the schema outputted from Juju, it's become aware that we also output all controller and agent schema, not just the client schema. This was never the intended case. It should have just been the client schema.

Although the methods are there, it shouldn't be possible to interact with the methods using a user. Instead, you would require the right tag and permissions to do so.

With that in mind, it should be possible to drop the amount of Python code generated for all the facades. This might be something that @dimaqq might want to look into doing.

You would have to generate the schema with the additional flag -facade-group=client.

https://github.com/juju/juju/blob/1eadc941cf64c3a0652583c8e65d870e7cccf111/Makefile#L494-L504

Urgency

Casually reporting

Python-libjuju version

current

Juju version

3.6.0

Reproduce / Test

Generate the facade with `-facade-group=client` which should reduce the number of generated Python facades, considerably.
@Aflynn50 Aflynn50 added area/schema-changes Updates the Juju API schema kind/maintenance hint/3.x going on main branch labels Sep 20, 2024
@benhoyt benhoyt added the priority/normal normal priority label Sep 22, 2024
@james-garner-canonical
Copy link
Contributor

I've been looking into this, and I have some questions.


What changes might need to be made in the juju repo?

  1. Are there consumers of the full (not just client) schema? If so, then we don't want to mess with generating the full schema, which happens on every build, writing to juju/apiserver/facades/schema.json.

Maybe we add a rebuild-client-schema directive and add that to the build step, writing to client-schema.json and using -facade-group=client? Or add the directive, but make it the python-libjuju devs' responsibility to call it after a Juju release, instead of just copying the generated schema?

On the other hand, if no one should be using non-client schema, then do we want to just modify rebuild-client and bring the new version to the various release branches?

  1. If we're going to generate new schemas that only have the client bits (dropping the typical schema size from 1.9mb to ~650kb), I guess we should do that for every current schema in main and 2.9 branches of python-libjuju?

Generating schemas for these older releases is made a little bit more painful by the fact that the schema generation code lives in the Juju repo. Would it be good to commit new schema generation code and new schemas to the relevant Juju branches? Alternatively, we could just generate the client only schemas without committing any changes to Juju.


Is python-libjuju missing schema updates for recent releases?

  1. It doesn't seem like schemas have been updated in a while. Do they need to be? Should there be a schema file in python-libjuju for every Juju release? Or maybe only when there's a relevant change?

The latest schema in the main branch of python-libjuju, which tracks the Juju 3.X releases, seems to be 3.3.0, which was added in September 2023. python-libjuju also has the 2.9 branch, which follows the Juju 2.9.X releases. The latest schema is 2.9.43, added in July 2023. There have been a number of Juju releases since then. The latest ones listed on the github releases are 3.5.3, 3.4.5, 3.3.6, 3.1.9 and 2.9.50 in late July 2024.

Is it plausible that there haven't been any changes involving facades since 3.3.0 and 2.9.43 were added last year?

  1. It seems that tracking the Juju facade versions isn't just a matter of generating a schema file for every release.

python-libjuju/juju/client/connection.py contains what appears to be a manually maintained list of class names and versions, which saw changes when the 2.9.43 and 3.3.0 schemas were added in 2023.

# Please keep in alphabetical order
client_facades = {
    'Action': {'versions': [2, 6, 7]},
    'ActionPruner': {'versions': [1]},
    'Agent': {'versions': [2, 3]},
    ...
    'VolumeAttachmentsWatcher': {'versions': [2]},
    'VolumeAttachmentPlansWatcher': {'versions': [1]},
}

If we need to add schemas for missing releases, how do we figure out the client_facade versioning? I guess the Juju devs bump the version number whenever they make a breaking change? Maybe after checking out each Juju release we can check a diff or release notes to figure out any changes needed for client_facades.


It might be nice if we could pre-emptively address this purely in python-libjuju.

If there was a good way to just filter the schemas, then we could skip any changes for old releases of Juju, and filter the schemas in python-libjuju/juju/client/facade.py. This would let us work with old schema without generating additional code regardless of when/if changes to schema generation hit the Juju repo

Unfortunately I couldn't see an equivalent to the -facade-group=client identifier used during schema generation in the actual generated schemas. I'm not sure that there is a reliable way to filter the generated schemas without extra knowledge about the api.

@benhoyt
Copy link
Collaborator

benhoyt commented Sep 24, 2024

I strongly suspect Juju uses that full schema JSON for other purposes, so we probably need to add a rebuild-client-schema Makefile target for this as you suggest.

And yes, looks like we should get the schemas up to date. I'm not sure about the manually-maintained list of class names and versions in connection.py -- that doesn't look great.

I'll ask for the Juju team's take on this on Matrix, and we can set up a meeting with a few key people to discuss if needed.

@jameinel
Copy link
Member

For (1), the other groups exist to support the Juju Controller work, and for Juju Agent work, and I don't know of anyone trying to build their own Juju Agent via python-libjuju (and it wouldn't be supported if they did). They would also need a different set of credentials, otherwise they would get permission denied even if they tried accessing them as a normal User.

For (2), I don't think we particularly care to ship a new version of python-libjuju for 2.9 just to drop extra facades that aren't used. If we were already doing something there, maybe. But espec. 2.9 should not really have much focus given to it. Consider what is there to be stable, and focus more on the newer releases.

The supported API changes (from the Juju CLI, but it keeps up to date with the controller) would be available from:

cd github.com/juju/juju
git diff 3.3..3.6 api/facadeversions.go

This is what I see:

-var facadeVersions = map[string][]int{
+var facadeVersions = facades.FacadeVersions{
        "Action":                       {7},
        "ActionPruner":                 {1},
        "Agent":                        {3},
@@ -24,8 +29,8 @@ var facadeVersions = map[string][]int{
        "AllModelWatcher":              {4},
        "AllWatcher":                   {3},
        "Annotations":                  {2},
-       "Application":                  {15, 16, 17, 18, 19},
-       "ApplicationOffers":            {4},
+       "Application":                  {15, 16, 17, 18, 19, 20},
+       "ApplicationOffers":            {4, 5},
        "ApplicationScaler":            {1},
        "Backups":                      {3},
        "Block":                        {2},
@@ -46,13 +51,13 @@ var facadeVersions = map[string][]int{
        "CharmRevisionUpdater":         {2},
        "Charms":                       {5, 6, 7},
        "Cleaner":                      {2},
-       "Client":                       {6, 7},
+       "Client":                       {6, 7, 8},
        "Cloud":                        {7},
-       "Controller":                   {11},
+       "Controller":                   {11, 12},
        "CredentialManager":            {1},
        "CredentialValidator":          {2},
        "CrossController":              {1},
-       "CrossModelRelations":          {2},
+       "CrossModelRelations":          {2, 3},
        "CrossModelSecrets":            {1},
        "Deployer":                     {1},
        "DiskManager":                  {2},
@@ -86,10 +91,10 @@ var facadeVersions = map[string][]int{
        "MigrationMaster":              {3},
        "MigrationMinion":              {1},
        "MigrationStatusWatcher":       {1},
-       "MigrationTarget":              {1, 2},
+       "MigrationTarget":              {1, 2, 3},
        "ModelConfig":                  {3},
        "ModelGeneration":              {4},
-       "ModelManager":                 {9},
+       "ModelManager":                 {9, 10},
        "ModelSummaryWatcher":          {1},
        "ModelUpgrader":                {1},
        "NotifyWatcher":                {1},
@@ -129,20 +134,9 @@ var facadeVersions = map[string][]int{
        "UnitAssigner":                 {1},
        "Uniter":                       {18, 19},
        "Upgrader":                     {1},
-       "UpgradeSeries":                {3},
+       "UpgradeSeries":                {3, 4},
        "UpgradeSteps":                 {2},
        "UserManager":                  {3},
        "VolumeAttachmentsWatcher":     {2},
        "VolumeAttachmentPlansWatcher": {1},
 }

In general, anything that a 3.3 client was doing (vs a 3.3 controller) should be supported and possible to do versus any 3.4/5/6 controller. It is better to negotiate a discussion on the version that you know works than to jump to the version in 3.5 which might now require an additional field, etc.
Essentially the reason python-libjuju has a list of Facade versions is because that is the place you record that you've done the work to know how to interact with that version. (The Juju team commits to supporting all the versions that have existed within a major release number.)

@james-garner-canonical
Copy link
Contributor

james-garner-canonical commented Sep 25, 2024

Thank you @benhoyt and @jameinel for your responses. If I'm understanding correctly, juju does use the full schemas internally, but python-libjuju definitely only needs the client schema, as @SimonRichardson pointed out originally.

In terms of contributing to juju then, I think it makes sense for us to add rebuild-client-schema to 3.6 so that we'll get the client only schema on the next release. I guess we'll want it to run on every build like rebuild-schema does.

For python-libjuju, this means updating the release documentation to use the generated client-schema.json instead of schema.json.

If checking juju's api/facades.go for changes and updating client_facades in python-libjujus juju/client/connection.py isn't already described in the release documentation, I guess it should be. I'm not 100% here on whether this something that should only be done when you've done manual work to ensure python-libjuju can interact with a given version, or whether it's all taken care of by the code generated from the schemas.

This leaves me with a couple of additional questions.

  1. Should we add schemas to python-libjuju for the missed releases? Looking at api/facades.go, there were changes from 3.3.1 (latest schema in python-libjuju) to 3.3.6 (latest 3.3 patch), and there further changes going to 3.5. I'm guessing it makes sense to add these, but it's not clear to me what versions we should add.
    a. Every minor and patch release after 3.3.1, since the schemas should have been added each time?
    b. Or just 3.3.latest, 3.4.latest, and 3.5.latest since Juju only supports the latest patch of each (supported) minor release?

  2. Should we update all the old schemas (and 'new' pre-3.6 schemas we might add) to be client only in python-libjuju main? I think this makes sense in terms of simplifying the python-libjuju codebase going forward, and reducing the amount of code generated for the facades. But we'd skip doing this for python-libjuju 2.9 since it should be considered stable, and this job will involve a bunch of manual steps, checking out the corresponding Juju release tag, adding the rebuild-client-schema directive and running it.

@benhoyt
Copy link
Collaborator

benhoyt commented Sep 25, 2024

Thanks James. From a quick look I'd say yes to both (1b and 2) -- even though it's more work, if we get this right now, it'll clean things up considerably for future work.

@dimaqq
Copy link
Contributor

dimaqq commented Sep 26, 2024

+1 on this plan, (1b and 2).

@james-garner-canonical
Copy link
Contributor

james-garner-canonical commented Sep 26, 2024

@dimaqq also noted that we do only need the latest patch version schema for each minor version, so we can remove the 3.3.0 and 3.3.1 schemas when adding 3.3.6

Can we also remove the 3.1.X and 3.2.X schemas? It doesn't seem worthwhile for future releases of python-libjuju to put effort into continuing to support EOL versions of juju. And @dimaqq tells me that users of python-libjuju for older versions of juju use the python-libjuju release corresponding to their juju version anyway.

So we will end up with just client schemas for 3.3.6, 3.4.5, and 3.5.3.

@james-garner-canonical
Copy link
Contributor

Actually since 3.1.X is still receiving security updates I guess we should have the 3.1.9 client schema as well.

3.2 is EOL. Is there any value in keeping a schema for 3.2.4?

jujubot added a commit that referenced this issue Sep 27, 2024
…remove-eol-schema

#1113

Remove `3.2.X` schemas. Delete `_client*.py` and run `make client`.

#### Description

Moving towards the goal of having schemas and generated code in python-libjuju only for the latest supported Juju versions (`3.1.9`, `3.3.6`, `3.5.4`, `3.5.3`), and using client only schemas (see #1099), this PR removes schemas for EOL Juju 3.2, and reruns code generations, removing `_client*.py` files and then running `make client`.


#### QA Steps

CI steps should all continue to pass, except for integration testing which should continue to fail with the usual suspects (see #1108 for a non-exhaustive table of tests that sometimes fail on `main`). 


#### Notes

To hopefully simplify the diffs, this is the first PR in a planned sequence of PRs that will depend on each other. Subsequent PRs will be:
1. replace current schemas with latest release client only schemas (`3.1.9`, `3.3.6`) and regenerate code
2. add client only schema for `3.4.5` and regenerate code
3. add client only schema for `3.5.3` and regenerate code
james-garner-canonical added a commit to james-garner-canonical/juju that referenced this issue Oct 16, 2024
The schemas output for python-libjuju should not have contained any
non-client schemas (see this python-libjuju issue
juju/python-libjuju#1099). Switching to only
the client schemas reduces the amount of generated code in
python-libjuju substantially.
jujubot added a commit that referenced this issue Oct 16, 2024
…lace-existing-w-client-only

#1160

#### Description

Moving towards the goal of having schemas and generated code in `python-libjuju` only for the latest supported Juju versions (`3.1.9`, `3.3.6`, `3.5.4`, `3.5.3`), and using client only schemas (see #1099), this PR replaces the existing `3.1.X` and `3.3.0` schemas with their client-only counterparts.

#### QA Steps

Tests pass, but is there anything else we should check here?
james-garner-canonical added a commit to james-garner-canonical/juju that referenced this issue Oct 24, 2024
The schemas output for python-libjuju should not have contained any
non-client schemas (see this python-libjuju issue
juju/python-libjuju#1099). Switching to only
the client schemas reduces the amount of generated code in
python-libjuju substantially.
@jujubot jujubot closed this as completed in a70983d Nov 8, 2024
james-garner-canonical added a commit to james-garner-canonical/juju that referenced this issue Nov 11, 2024
The schemas output for python-libjuju should not have contained any
non-client schemas (see this python-libjuju issue
juju/python-libjuju#1099). Switching to only
the client schemas reduces the amount of generated code in
python-libjuju substantially.
james-garner-canonical added a commit to james-garner-canonical/juju that referenced this issue Nov 17, 2024
The schemas output for python-libjuju should not have contained any
non-client schemas (see this python-libjuju issue
juju/python-libjuju#1099). Switching to only
the client schemas reduces the amount of generated code in
python-libjuju substantially.
jujubot added a commit to juju/juju that referenced this issue Nov 17, 2024
…build-schema-client-only

#18256

The schemas output for python-libjuju should not have contained any non-client schemas -- see python-libjuju issue
[#1099](juju/python-libjuju#1099). Switching to only the client schemas reduces the amount of generated code in python-libjuju substantially.

python-libjuju now uses client-only schema for the schema files currently checked in (see PR [#1160](juju/python-libjuju#1160)), so it would be nice to land this change for 3.6 to streamline things going forward.


## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] ~Code style: imports ordered, good names, simple structure, etc~
- [x] ~Comments saying why design decisions were made~
- [x] ~Go unit tests, with comments saying what you're testing~
- [x] ~[Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- [x] ~[doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps

<!-- Describe steps to verify that the change works. -->

* Move the existing `schema.json` elsewhere.
* Run `make rebuild-schema` and verify that a new `schema.json` file has been generated.
* Run `make build` and verify that the same contents are generated for the `schema.json`.
* Verify that the new `schema.json` contains a subset of the contents of the original (moved) `schema.json`.

## Documentation changes

<!-- How it affects user workflow (CLI or API). -->

python-libjuju release instructions will be updated to reflect this change. No changes should be needed for juju documentation.

## Links

<!-- Link to all relevant specification, documentation, bug, issue or JIRA card. -->

python-libjuju issue: juju/python-libjuju#1099

draft PR closed in favour of this PR: #18148
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/schema-changes Updates the Juju API schema hint/3.x going on main branch kind/maintenance priority/normal normal priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants