ACA-Py Upgrade command implementation

[shaangill025]

Signed-off-by: Shaanjot Gill [email protected]

• upgrade command
  Example

  ./scripts/run_docker upgrade --upgrade-config ./aries_cloudagent/acapy_upgrade_config.yml \
  --from-version v0.7.2 \
  --wallet-type indy \
  --wallet-name issuer \
  --wallet-key mykey \
  --wallet-storage-type postgres_storage \
  --wallet-storage-config '{"url":"host.docker.internal:5432","max_connections":5}' \
  --wallet-storage-creds '{"account":"postgres","password":"pwd","admin_account":"postgres","admin_password":"pwd"}'
  

  • --upgrade-config - path to YAML config file
    Example YAML config

    v0.6.0:
    ...
    v0.7.1:
    ...
    v0.7.2:
      resave_records:
        base_record_path:
          - "aries_cloudagent.connections.models.conn_record.ConnRecord"
        base_exch_record_path:
          - "aries_cloudagent.protocols.issue_credential.v1_0.models.credential_exchange.V10CredentialExchange"
      update_existing_records: false
    

    The above will re-save ConnRecord and V10CredentialExchange. update_existing_records can be used to handle
    changes where existing records need to be updated, for instance, if a new required field has been added to Marshmallow
    schema. It is handled here: link1 and link2 and link3, this will have to be managed every release.
  • --from-version is only required when version information is not there in storage. It will be ignored if version is found in storage
  • Also accepts WalletGroup arguments

Update: now follows this approach.

Thanks @ianco for suggesting the command approach. This is an initial implementation, for now, it can only handle re-save and/or update records but it can be expanded upon. @andrewwhitehead, @swcurran, and @ianco I will appreciate any feedback.

[ianco]

This looks good! I wonder if we should store the agent version in the wallet, that way we know what version we are upgrading from, and we could even check on startup if an "upgrade" was required. (Obviously existing deployments wouldn't have this so would require the "--from-version" to be provided, but if the version number were in the wallet all they would need to do is "aca-py upgrade")

[andrewwhitehead]

I agree, the version should likely be stored to allow for automatic upgrades. This would probably mean auto-populating the version if it is not detected during startup, since we don't store one at the moment.

[ianco]

I wouldn't recommend auto-populating the version number in the database on startup. I suggest we insert the version number when we create the database (on aca-py provision or ... --auto-provision). On startup we query the version from the database and compare to the version of the application, and auto-shutdown if we detect a version incompatibility (which means we need to aca-pu upgrade). Version incompatibility can include "no version in database".

For aca-py upgrade we run all the necessary updates from the version currently in the database (or the version the user specifies) to the version of the software they are trying to run. Once the upgrade is done we insert/update the version # in the database.

I also suggest we have a VERY OBVIOUS MESSAGE somewhere that they backup their database before they run an aca-py upgrade.

[shaangill025]

I wouldn't recommend auto-populating the version number in the database on startup. I suggest we insert the version number when we create the database (on aca-py provision or ... --auto-provision). On startup we query the version from the database and compare to the version of the application, and auto-shutdown if we detect a version incompatibility (which means we need to aca-pu upgrade). Version incompatibility can include "no version in database".

I also suggest we have a VERY OBVIOUS MESSAGE somewhere that they backup their database before they run an aca-py upgrade.

Working on this.

[ianco]

I wouldn't recommend auto-populating the version number in the database on startup. I suggest we insert the version number when we create the database (on aca-py provision or ... --auto-provision). On startup we query the version from the database and compare to the version of the application, and auto-shutdown if we detect a version incompatibility (which means we need to aca-pu upgrade). Version incompatibility can include "no version in database".

I also suggest we have a VERY OBVIOUS MESSAGE somewhere that they backup their database before they run an aca-py upgrade.

Working on this.

OK as long as you agree with it :-D

[shaangill025]

@ianco @andrewwhitehead This is ready.

[ianco]

Looks good to me

@andrewwhitehead do you want to take a look before we merge?

[andrewwhitehead]

@swcurran I think this is good to go, my only concern is that upgraded agents will refuse to start until the upgrade command is run. I believe the user also needs to specify an exact (and supported) version that they are upgrading from, which may not be immediately obvious, so we would need to document that well.

[swcurran]

Can you propose specific changes to be made? @shaangill025 -- can you take a look? I'd really like to get this out today, so collaborating on this would be helpful.

The big issue is refusing to start without the upgrade. Ideas on how best to deal with that?

Next the issue of indicating a version -- what do you propose we document to help them? Can they look at the "version.py" from their previous version to tell them?

[swcurran]

Alternatively:

• slow down on this upgrade and leave it out of the release?
• slow down on the release until we get this right?

Opinions welcome! What would developers want?

[andrewwhitehead]

I'm not sure. I think the upgrade could be made optional, as the only thing that's affected is continuation of existing did-exchange connections (AFAIK).

To get the current version of their aca-py install the easiest way is probably to run aca-py -v

[shaangill025]

@swcurran I think this is good to go, my only concern is that upgraded agents will refuse to start until the upgrade command is run. I believe the user also needs to specify an exact (and supported) version that they are upgrading from, which may not be immediately obvious, so we would need to document that well.

@andrewwhitehead I see there is a flaw here when introducing this into ACA-Py, how about I raise an exception here and ask to execute the upgrade command with --from-version instead of adding the version record when they execute the start --auto-provision or provision command.

--from-version will be required only for the initial release and subsequent upgrades will rely on the stored version record. I think we mention this in the release notes that they record the current version of ACA-Py running and use that to execute the upgrade command.

The YAML config for previous versions will be the same for this release. So even if somebody puts in the wrong --from-version, it shouldn't cause an issue.

v0.6.0:
  resave_records:
    base_record_path:
      - "aries_cloudagent.connections.models.conn_record.ConnRecord"
  update_existing_records: false
v0.7.0:
  resave_records:
    base_record_path:
      - "aries_cloudagent.connections.models.conn_record.ConnRecord"
  update_existing_records: false 
v0.7.1:
  resave_records:
    base_record_path:
      - "aries_cloudagent.connections.models.conn_record.ConnRecord"
  update_existing_records: false
v0.7.2:
  resave_records:
    base_record_path:
      - "aries_cloudagent.connections.models.conn_record.ConnRecord"
  update_existing_records: false

[andrewwhitehead]

Hm, it doesn't make sense to me to repeat the upgrade commands for each version, they should all be done from the --from-version onward in order. v0.6.0-v0.7.1 doesn't require any upgrade for instance. It seems like if the --from-version isn't specified and no version is stored then we should just start from the first defined upgrade. In any case, I think the upgrade command should be optional for this release (only if a user wants to use existing did-exchange connections for instance).

[shaangill025]

Hm, it doesn't make sense to me to repeat the upgrade commands for each version, they should all be done from the --from-version onward in order. v0.6.0-v0.7.1 doesn't require any upgrade for instance. It seems like if the --from-version isn't specified and no version is stored then we should just start from the first defined upgrade. In any case, I think the upgrade command should be optional for this release (only if a user wants to use existing did-exchange connections for instance).

Right now, YAML config for each version is defined separately and not incrementally onwards from the specified version as you have proposed above. I can add this in today. Regarding making upgrade optional, maybe I only raise an exception here if --auto-provision is set otherwise add the acapy_version record and continue? Right now, either --from-version or acapy_version record is required for upgrade.

I see there is a flaw here when introducing this into ACA-Py, how about I raise an exception here and ask to execute the upgrade command with --from-version instead of adding the version record when they execute the start --auto-provision or provision command

I think this change still makes sense.

[andrewwhitehead]

Regarding making upgrade optional, maybe I only raise an exception here if --auto-provision is set otherwise add the acapy_version record and continue?

I don't think we ever want to auto-add the version record unless it's a new wallet. For an existing wallet that should only happen when upgrade is run. However, for 0.7.3 the application could happily run without having a version record stored, and then in 0.8.0 we could make the upgrade step mandatory just to know there's some consistency in the stored connection records. It should still be possible on 0.7.3 to manually run the upgrade command, but not required.

[shaangill025]

I don't think we ever want to auto-add the version record unless it's a new wallet. For an existing wallet that should only happen when upgrade is run. However, for 0.7.3 the application could happily run without having a version record stored, and then in 0.8.0 we could make the upgrade step mandatory just to know there's some consistency in the stored connection records. It should still be possible on 0.7.3 to manually run the upgrade command, but not required.

Sounds good, working on the above changes and I have added the incremental/onward config support. This should be ready in the next 2-3 hours.

@andrewwhitehead With indy provision is creating a new wallet, is it the same with Askar? In that case, it makes sense to add version record here and here

[andrewwhitehead]

The logic there looks a little too complicated to verify right now. Let's just leave the version handling out of the wallet config for now.

[shaangill025]

@andrewwhitehead @swcurran With the new incremental/onwards version approach, I have implemented a sorting logic that assumes major versions like v7.2.0 .. v7.3.0 and cannot handle release candidates, is that a reasonable assumption?
I will update and use from packaging import version version.parse which can handle the above.

The requested changes are in and ready for review.

[andrewwhitehead]

I hadn't realized that the --upgrade-config path was a required parameter in order to perform the upgrade. It should be possible to find the default config file in the aries_cloudagent package, see for example how it's done in the logging configuration: https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/config/logging.py#L14:L36

We can't really expect users to know the path to the configuration file, or even where the aca-py module is installed on their system.

[andrewwhitehead]

Running the upgrade command with no wallet configuration also produces the message aries_cloudagent.commands.upgrade.UpgradeError: ACA-Py version not found in storage and no --from-version specified.. I think this should be failing earlier, because how would it find the version in storage without the wallet config? But also, I think it should be selecting 0.7.2 as the from-version (based on the upgrade configuration) because that is the first one to offer any impact from an upgrade.

[andrewwhitehead]

Testing with the latest, if I run python3 -m aries_cloudagent upgrade --upgrade-config ./aries_cloudagent/config/tests/test-acapy-upgrade-config.yaml I get the following:

No ACA-Py version found in wallet storage and no --from-version specified. Selecting v0.7.2 as --from-version from the config.
Running upgrade process for v0.7.2
All records of <class 'aries_cloudagent.connections.models.conn_record.ConnRecord'> successfully re-saved.
All records of <class 'aries_cloudagent.protocols.issue_credential.v1_0.models.credential_exchange.V10CredentialExchange'> successfully re-saved.

But that's without any wallet configuration, so what records is it re-saving? The same output is produced if run a second time.

[andrewwhitehead]

Oddly if I run without the --upgrade-config parameter it only re-saves the connection records:

No ACA-Py version found in wallet storage and no --from-version specified. Selecting v0.7.2 as --from-version from the config.
Running upgrade process for v0.7.2
All records of <class 'aries_cloudagent.connections.models.conn_record.ConnRecord'> successfully re-saved.

Edit: oops, looks like that's because I was pointing it as the test config not the default one.

[shaangill025]

Testing with the latest, if I run python3 -m aries_cloudagent upgrade --upgrade-config ./aries_cloudagent/config/tests/test-acapy-upgrade-config.yaml I get the following:

No ACA-Py version found in wallet storage and no --from-version specified. Selecting v0.7.2 as --from-version from the config.
Running upgrade process for v0.7.2
All records of <class 'aries_cloudagent.connections.models.conn_record.ConnRecord'> successfully re-saved.
All records of <class 'aries_cloudagent.protocols.issue_credential.v1_0.models.credential_exchange.V10CredentialExchange'> successfully re-saved.

But that's without any wallet configuration, so what records is it re-saving? The same output is produced if run a second time.

With no wallet configuration, we get the InMemoryProfile(backend=in_memory, name=default) profile, this has no ConnRecord or V10CredentialExchange records so they cannot be not re-saved. I have added a conditional around the print statement to address the confusion.