Incorrect schema is used to generate the clients

SimonRichardson commented 2 months ago

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.

james-garner-canonical commented 1 month ago

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

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

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?

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?

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?

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

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?
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 commented 1 month ago

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 commented 1 month ago

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

james-garner-canonical commented 1 month ago

@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 commented 1 month ago

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?

juju / python-libjuju