manila/27c83ba84f9c46d68aeab40a3aaa60383c658f02

{
  "comments": [
    {
      "unresolved": true,
      "key": {
        "uuid": "290dac22_dcf3be80",
        "filename": "/COMMIT_MSG",
        "patchSetId": 1
      },
      "lineNbr": 11,
      "author": {
        "id": 15334
      },
      "writtenOn": "2024-04-05T14:08:56Z",
      "side": 1,
      "message": "To be totally honest, I don\u0027t think this is entirely true. I did initially think this, but I\u0027m now thinking is that something has changed with how SQLAlchemy does ordering across relationships in 2.0, in the absence of a specific `order_by` or similar. The `ShareSnapshot.instance` field is not a field but rather a property that fetches various `ShareSnapshotInstance` instances and returns the zeroth item from this sequence. If you revert this change and apply the below diff you\u0027ll see that the ordering of `ShareSnapshot.instances` has changed which changes what `ShareSnapshot.instance` (and by extension `ShareSnapshot.status`) returns. I just don\u0027t know why. I\u0027m hoping @zzzeek might have some insight here 😅\n\n```\ndiff --git manila/tests/db/sqlalchemy/test_api.py manila/tests/db/sqlalchemy/test_api.py\nindex 9ac81a009..9e4b352d5 100644\n--- manila/tests/db/sqlalchemy/test_api.py\n+++ manila/tests/db/sqlalchemy/test_api.py\n@@ -1740,12 +1740,27 @@ class ShareSnapshotDatabaseAPITestCase(test.TestCase):\n                 progress\u003d\u002787%\u0027,\n                 share_instance_id\u003dself.share_instances[3][\u0027id\u0027]),\n         ]\n+        print(\u0027# create\u0027)\n         self.snapshot_1 \u003d db_utils.create_snapshot(\n             id\u003d\u0027fake_snapshot_id_1\u0027, share_id\u003dself.share_1[\u0027id\u0027],\n             instances\u003dself.snapshot_instances[0:3])\n+        print(f\u0027snapshot 1 instances: {[x.status for x in self.snapshot_1.instances]}\u0027)\n+        print(f\u0027snapshot 1: {self.snapshot_1.status}\u0027)\n         self.snapshot_2 \u003d db_utils.create_snapshot(\n             id\u003d\u0027fake_snapshot_id_2\u0027, share_id\u003dself.share_2[\u0027id\u0027],\n             instances\u003dself.snapshot_instances[3:4], metadata\u003d{\u0027foo\u0027: \u0027bar\u0027})\n+        print(f\u0027snapshot 2 instances: {[x.status for x in self.snapshot_2.instances]}\u0027)\n+        print(f\u0027snapshot 2: {self.snapshot_2.status}\u0027)\n+\n+        print(\u0027# fetch\u0027)\n+        self.snapshot_1 \u003d db_api.share_snapshot_get(\n+            self.ctxt, \u0027fake_snapshot_id_1\u0027)\n+        print(f\u0027snapshot 1 instances: {[x.status for x in self.snapshot_1.instances]}\u0027)\n+        print(f\u0027snapshot 1: {self.snapshot_1.status}\u0027)\n+        self.snapshot_2 \u003d db_api.share_snapshot_get(\n+            self.ctxt, \u0027fake_snapshot_id_2\u0027)\n+        print(f\u0027snapshot 2 instances: {[x.status for x in self.snapshot_2.instances]}\u0027)\n+        print(f\u0027snapshot 2: {self.snapshot_2.status}\u0027)\n \n         self.snapshot_instance_export_locations \u003d [\n             db_utils.create_snapshot_instance_export_locations(\n```",
      "revId": "27c83ba84f9c46d68aeab40a3aaa60383c658f02",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "5f4b0f61_70ec3e0f",
        "filename": "/COMMIT_MSG",
        "patchSetId": 1
      },
      "lineNbr": 11,
      "author": {
        "id": 15334
      },
      "writtenOn": "2024-04-05T14:19:27Z",
      "side": 1,
      "message": "Reproducer pushed [here](https://review.opendev.org/c/openstack/manila/+/915133/). Note that this _still_ works with SQLAlchemy 1.4 despite reassigning `self.snapshot_1` and `self.snapshot_2`, which suggests its purely down to ordering and not, say, different sessions being used.",
      "parentUuid": "290dac22_dcf3be80",
      "revId": "27c83ba84f9c46d68aeab40a3aaa60383c658f02",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "48663c52_e079f8a5",
        "filename": "/COMMIT_MSG",
        "patchSetId": 1
      },
      "lineNbr": 11,
      "author": {
        "id": 15334
      },
      "writtenOn": "2024-04-05T14:29:32Z",
      "side": 1,
      "message": "If my theory is correct, we really should be sorting `self.instances` as part of `ShareSnapshot.instance` property, like we do for `Share.instance`. That\u0027s a follow-up for someone else though...",
      "parentUuid": "5f4b0f61_70ec3e0f",
      "revId": "27c83ba84f9c46d68aeab40a3aaa60383c658f02",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "972bde47_dee3ee88",
        "filename": "/COMMIT_MSG",
        "patchSetId": 1
      },
      "lineNbr": 11,
      "author": {
        "id": 11816
      },
      "writtenOn": "2024-04-05T14:38:46Z",
      "side": 1,
      "message": "there\u0027s nothing different about the ordering of relationships when they are loaded in SQLAlchemy 2.0, if you mean the ORDER BY on the query that loads it.  There\u0027s no  ORDER BY added to the query unless it\u0027s indicated on the relationship itself, and that\u0027s pretty much it.",
      "parentUuid": "48663c52_e079f8a5",
      "revId": "27c83ba84f9c46d68aeab40a3aaa60383c658f02",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "f8d5ae98_38bb129c",
        "filename": "/COMMIT_MSG",
        "patchSetId": 1
      },
      "lineNbr": 11,
      "author": {
        "id": 16643
      },
      "writtenOn": "2024-04-08T22:34:51Z",
      "side": 1,
      "message": "this is strange indeed; but, the unit tests were doing the wrong thing here, and I agree Stephen caught this and fixed it the right way. \n\n\"Snapshot instances\" aren\u0027t ever created in bulk by passing \"instances\" to the share_snapshot_create() query... we were doing it this way for convenience in the unit tests. \n\nThe asserts in the test seem to be testing incorrect expectations.. as I see it:\n\nSnapshot 1 is created with 3 instances \nSnapshot 2 is created with 1 instance \n\nThe tests were written incorrectly to assert that Snapshot 1 had 4 instances and Snapshot 2 had 2 instances (i.e., a snapshot instance was being implicitly created by the DB query when it wasn\u0027t the point of the test)\n\nSo, I agree with this fix that preserves the test\u0027s intended behavior. I\u0027m glad you dug deep and asked the right questions Stephen!",
      "parentUuid": "972bde47_dee3ee88",
      "revId": "27c83ba84f9c46d68aeab40a3aaa60383c658f02",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    }
  ]
}