openstack
/
octavia-tempest-plugin
Code Issues Proposed changes
octavia-tempest-plugin/4664bd127ff0051cc22feb7ba54...

369 lines
13 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": false,
"key": {
"uuid": "5f9edd9c_0ed5810d",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 6
},
"lineNbr": 0,
"author": {
"id": 31664
},
"writtenOn": "2023-10-19T15:53:45Z",
"side": 1,
"message": "Nice first o-t-p patch Lukas! I reviewed up to line 312\nI will review the rest of the patch soon",
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "26bec41b_e3367d7b",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 6
},
"lineNbr": 0,
"author": {
"id": 30674
},
"writtenOn": "2023-10-20T08:20:23Z",
"side": 1,
"message": "Thank you for the review so far!",
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "1620a7fe_f624d3f4",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 1,
"author": {
"id": 31664
},
"writtenOn": "2023-10-19T15:53:45Z",
"side": 1,
"message": "That should probably be 2023 Red Hat",
"range": {
"startLine": 1,
"startChar": 12,
"endLine": 1,
"endChar": 24
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "06030aaa_eb16d11b",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 1,
"author": {
"id": 30674
},
"writtenOn": "2023-10-20T08:20:23Z",
"side": 1,
"message": "Done",
"parentUuid": "1620a7fe_f624d3f4",
"range": {
"startLine": 1,
"startChar": 12,
"endLine": 1,
"endChar": 24
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "156cc2eb_4994a3e6",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 49,
"author": {
"id": 31664
},
"writtenOn": "2023-10-19T15:53:45Z",
"side": 1,
"message": "A good question might be - would (should) backup member work with IPv6?\n\n(to other reviewers) Should it be tested with IPv6? Would it worth the overhead?",
"range": {
"startLine": 49,
"startChar": 8,
"endLine": 49,
"endChar": 22
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "0724a043_da9fcb28",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 49,
"author": {
"id": 28609
},
"writtenOn": "2023-10-25T16:16:08Z",
"side": 1,
"message": "I think that the backup members feature should not be affected by IP version used (it\u0027s transparent) and it should work exactly the same for both.\nSupporting both: IPV4 and IPV6 setups, makes sense to me and if it doesn\u0027t require drastic code changes I would recommend adding this option.\nBTW - there are some lines in the code like L122 or L129 that logs and exports some IPV6 values, but the IPV6 is not in the game as of now.",
"parentUuid": "156cc2eb_4994a3e6",
"range": {
"startLine": 49,
"startChar": 8,
"endLine": 49,
"endChar": 22
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "e961719d_33fb21a1",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 49,
"author": {
"id": 30674
},
"writtenOn": "2023-11-06T12:01:13Z",
"side": 1,
"message": "I updated the tests to work with IPv6. Let me know what you think:)",
"parentUuid": "0724a043_da9fcb28",
"range": {
"startLine": 49,
"startChar": 8,
"endLine": 49,
"endChar": 22
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "d39aab47_646162b5",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 49,
"author": {
"id": 28609
},
"writtenOn": "2023-11-21T11:52:49Z",
"side": 1,
"message": "Hi Lukas!\n\nWen I\u0027m looking on IPV6 gate [1] and its log [2] I see that the \"backup members\" tests are running twice (BackupMembersTestIPv6,BackupMembersTestIPv4), for example:\n2023-11-09 09:04:48.168484 | controller | {0} octavia_tempest_plugin.tests.scenario.v2.test_backup_members.BackupMembersTestIPv6.test_backup_to_default [6.350633s] ... ok\n2023-11-09 09:04:48.354259 | controller | {1} octavia_tempest_plugin.tests.scenario.v2.test_backup_members.BackupMembersTestIPv4.test_backup_to_default [6.225020s] ... ok\n\n\nIt\u0027s weird, I think that the Devnest used for this gate based on [3] is IPV6 only, so it\u0027s not so clear to me what have been tested for IPV4 😕, anyway we just need to run these tests once, deepening on setup IP version. \n\n\n[1] - https://zuul.opendev.org/t/openstack/build/6ee42b4fab9c4ef5b3e7c4fc2d90b240 \n[2] - https://storage.bhs.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_6ee/897564/17/check/octavia-v2-dsvm-scenario-ipv6-only/6ee42b4/job-output.txt\n[3] - https://zuul.opendev.org/t/openstack/build/6ee42b4fab9c4ef5b3e7c4fc2d90b240/log/controller/logs/local_conf.txt#72",
"parentUuid": "e961719d_33fb21a1",
"range": {
"startLine": 49,
"startChar": 8,
"endLine": 49,
"endChar": 22
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "8b143cd9_f8c3cd2e",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 49,
"author": {
"id": 30674
},
"writtenOn": "2023-11-21T13:17:27Z",
"side": 1,
"message": "For example, the traffic ops tests are also running the IPv6 and IPv4 tests in the job [1]. I got inspired by those tests so I concluded that it should be ok.\n\nAm I missing something or should we fix this also for the traffic ops tests?\n\nI\u0027m not sure about this but I think that in this case, IP_VERSION is more important than SERVICE_IP_VERSION (?). By default, IP_VERSION is set to 6+4 which means dual-stack openstack [2].\n\nThank you Arkady for looking at this again:). \n\n[1] https://opendev.org/openstack/octavia-tempest-plugin/src/commit/5b2eca40a886ec3fc7c50605925a727247993612/octavia_tempest_plugin/tests/scenario/v2/test_traffic_ops.py#L61\n[2] https://opendev.org/openstack/devstack/src/commit/bb0c273697bf54dd569ad38e459cd161b62f96cb/lib/neutron_plugins/services/l3#L3",
"parentUuid": "d39aab47_646162b5",
"range": {
"startLine": 49,
"startChar": 8,
"endLine": 49,
"endChar": 22
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "d4bbff9b_0083c07a",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 56,
"author": {
"id": 31664
},
"writtenOn": "2023-10-19T15:53:45Z",
"side": 1,
"message": "You can use the cascade option e.g. \nhttps://opendev.org/openstack/octavia-tempest-plugin/src/branch/master/octavia_tempest_plugin/tests/scenario/v2/test_healthmonitor.py#L48\n\nWhenever a LB has related resources (i.e. children), you cannot delete the LB directly, you will have to delete its related resources first and only then you can delete it. The cascade option does that for you.\n\nSo you won\u0027t need the other (again - LB related resources) cleanups. But you will need the FIP cleanup",
"range": {
"startLine": 54,
"startChar": 8,
"endLine": 56,
"endChar": 22
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "67c606f0_9f9288f9",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 56,
"author": {
"id": 30674
},
"writtenOn": "2023-10-20T08:20:23Z",
"side": 1,
"message": "+1 this is useful",
"parentUuid": "d4bbff9b_0083c07a",
"range": {
"startLine": 54,
"startChar": 8,
"endLine": 56,
"endChar": 22
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "d2daaded_dd1371fa",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 270,
"author": {
"id": 31664
},
"writtenOn": "2023-10-19T15:53:45Z",
"side": 1,
"message": "webserver4 is created with cls.lb_member_1_net on line #155-156",
"range": {
"startLine": 269,
"startChar": 8,
"endLine": 270,
"endChar": 78
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "a0c78363_083e777d",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 270,
"author": {
"id": 30674
},
"writtenOn": "2023-10-20T08:20:23Z",
"side": 1,
"message": "Done, thanks!",
"parentUuid": "d2daaded_dd1371fa",
"range": {
"startLine": 269,
"startChar": 8,
"endLine": 270,
"endChar": 78
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "39879540_e2498f23",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 303,
"author": {
"id": 31664
},
"writtenOn": "2023-10-19T15:53:45Z",
"side": 1,
"message": "I wonder how that copy paste err didn\u0027t make any problems :)\nAnyway, you can delete both the cls.addClassResourceCleanup on lines 302 \u0026 310 if you use cascade\u003dTrue where I indicated before",
"range": {
"startLine": 303,
"startChar": 12,
"endLine": 303,
"endChar": 57
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "55a2652b_5f808e18",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 303,
"author": {
"id": 30674
},
"writtenOn": "2023-10-20T08:20:23Z",
"side": 1,
"message": "I added the cascade option and removed the cleanups, it is better this way!\n\nWhy do you think it should cause problems? I can\u0027t see it straight away now so I\u0027m just curious.",
"parentUuid": "39879540_e2498f23",
"range": {
"startLine": 303,
"startChar": 12,
"endLine": 303,
"endChar": 57
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "7ecc3be1_b2a9cb16",
"filename": "octavia_tempest_plugin/tests/scenario/v2/test_backup_members.py",
"patchSetId": 6
},
"lineNbr": 303,
"author": {
"id": 31664
},
"writtenOn": "2023-10-20T10:47:02Z",
"side": 1,
"message": "I forgot that you can provide any method as a cleanup method for the test changes.\nas I am used to see usually only the following line provided to cleanups: \ncls.mem_x_client.cleanup_x\n\nso I thought it was a copy paste typo lol...",
"parentUuid": "55a2652b_5f808e18",
"range": {
"startLine": 303,
"startChar": 12,
"endLine": 303,
"endChar": 57
},
"revId": "4664bd127ff0051cc22feb7ba54380e47a700da5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
}
]
}
View Git Blame Copy Permalink