)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":10459,"name":"Luigi Toscano","email":"ltoscano@redhat.com","username":"ltoscano"},"change_message_id":"aa835cefec3f97611f3d1f899cc78b5aea4720e9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"5f8bb230_69d40e4b","updated":"2023-03-16 15:35:25.000000000","message":"It seems to improve the overall status and make the deployment of NVMe-oF easier","commit_id":"65b7cd46b751d4a6881b9aaff971f78cd82b73d5"}],"deployment/cinder/cinder-backend-nvmeof-puppet.yaml":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"7e6bde26898cd890b4a4c09c899f50cf07099417","unresolved":true,"context_lines":[{"line_number":23,"context_line":"    type: string"},{"line_number":24,"context_line":"    default: \u0027nvmet_rdma\u0027"},{"line_number":25,"context_line":"    description: \u003e"},{"line_number":26,"context_line":"      The target protocol, supported values are nvmet_rdma and nvmet_tcp."},{"line_number":27,"context_line":"  CinderNVMeOFTargetPrefix:"},{"line_number":28,"context_line":"    type: string"},{"line_number":29,"context_line":"    default: \u0027nvme-subsystem\u0027"}],"source_content_type":"text/x-yaml","patch_set":2,"id":"b6e62501_12c807e4","line":26,"updated":"2023-03-15 17:14:57.000000000","message":"This should have an allowed_values constraint, similar to one like this [1].\n\n[1] https://github.com/openstack/tripleo-heat-templates/blob/master/deployment/cinder/cinder-backend-pure-puppet.yaml#L63..L67","commit_id":"a74c9a003724aabcf9ebe73df15ad9a1538f7cb3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"5838b37d76ea06b54402ed789bdb7fa893c7a707","unresolved":false,"context_lines":[{"line_number":23,"context_line":"    type: string"},{"line_number":24,"context_line":"    default: \u0027nvmet_rdma\u0027"},{"line_number":25,"context_line":"    description: \u003e"},{"line_number":26,"context_line":"      The target protocol, supported values are nvmet_rdma and nvmet_tcp."},{"line_number":27,"context_line":"  CinderNVMeOFTargetPrefix:"},{"line_number":28,"context_line":"    type: string"},{"line_number":29,"context_line":"    default: \u0027nvme-subsystem\u0027"}],"source_content_type":"text/x-yaml","patch_set":2,"id":"d6b7e394_fb16984e","line":26,"in_reply_to":"b6e62501_12c807e4","updated":"2023-03-15 18:09:13.000000000","message":"Done","commit_id":"a74c9a003724aabcf9ebe73df15ad9a1538f7cb3"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"4275960cdb858c9f39f1e590d2f33013050a5b3f","unresolved":false,"context_lines":[{"line_number":68,"context_line":"    value:"},{"line_number":69,"context_line":"      service_name: cinder_backend_nvmeof"},{"line_number":70,"context_line":"      firewall_rules:"},{"line_number":71,"context_line":"        \u0027120 LVM nvmet target\u0027:"},{"line_number":72,"context_line":"          dport: {get_param: CinderNVMeOFTargetPort}"},{"line_number":73,"context_line":"      config_settings:"},{"line_number":74,"context_line":"        map_merge:"}],"source_content_type":"text/x-yaml","patch_set":7,"id":"abfb7b31_935da5fa","line":71,"range":{"start_line":71,"start_character":8,"end_line":71,"end_character":31},"updated":"2023-03-22 16:08:46.000000000","message":"this can be optional, and can enabled according to CinderEnableNVMeOFBackend . Probably we can follow up this (and firewall rule for iscsi target) later.","commit_id":"b5dc00f8da4338d49de7a6c205aeb5e157eaf09a"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"7b475ab65ac3108cd616cabe01239c60c139c9f4","unresolved":false,"context_lines":[{"line_number":68,"context_line":"    value:"},{"line_number":69,"context_line":"      service_name: cinder_backend_nvmeof"},{"line_number":70,"context_line":"      firewall_rules:"},{"line_number":71,"context_line":"        \u0027120 LVM nvmet target\u0027:"},{"line_number":72,"context_line":"          dport: {get_param: CinderNVMeOFTargetPort}"},{"line_number":73,"context_line":"      config_settings:"},{"line_number":74,"context_line":"        map_merge:"}],"source_content_type":"text/x-yaml","patch_set":7,"id":"cd7182ba_dffbb42d","line":71,"range":{"start_line":71,"start_character":8,"end_line":71,"end_character":31},"in_reply_to":"abfb7b31_935da5fa","updated":"2023-03-22 16:22:10.000000000","message":"This rule won\u0027t be added unless this template is active (by default corresponding tripleo service is OS::Heat::None). I suppose someone could activate the template and still set CinderEnableNVMeOFBackend to False, but that\u0027s not an interesting use case. The iscsi rule has been there since forever, and I wouldn\u0027t worry about changing things now. It\u0027s not been an issue, and I\u0027d be concerned about unintentional breakage, etc.","commit_id":"b5dc00f8da4338d49de7a6c205aeb5e157eaf09a"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"b3ddb8cbbb1096bf76e233104231b6e039220ba3","unresolved":false,"context_lines":[{"line_number":68,"context_line":"    value:"},{"line_number":69,"context_line":"      service_name: cinder_backend_nvmeof"},{"line_number":70,"context_line":"      firewall_rules:"},{"line_number":71,"context_line":"        \u0027120 LVM nvmet target\u0027:"},{"line_number":72,"context_line":"          dport: {get_param: CinderNVMeOFTargetPort}"},{"line_number":73,"context_line":"      config_settings:"},{"line_number":74,"context_line":"        map_merge:"}],"source_content_type":"text/x-yaml","patch_set":7,"id":"2a621013_db7ba21d","line":71,"range":{"start_line":71,"start_character":8,"end_line":71,"end_character":31},"in_reply_to":"cd7182ba_dffbb42d","updated":"2023-03-23 01:11:55.000000000","message":"Thanks. I somehow again misunderstood the implementation and I thought the resource is enabled by default (mainly because we have that Enabled parameter) if the resource is optional then I agree with you and adding the if condition sounds like overkilling.","commit_id":"b5dc00f8da4338d49de7a6c205aeb5e157eaf09a"}],"deployment/cinder/cinder-volume-container-puppet.yaml":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"bb3c9db459dab868eaff0201cb0dbb03502e8a75","unresolved":true,"context_lines":[{"line_number":122,"context_line":"    description: Role data for the Cinder Volume role."},{"line_number":123,"context_line":"    value:"},{"line_number":124,"context_line":"      service_name: cinder_volume"},{"line_number":125,"context_line":"      firewall_rules:"},{"line_number":126,"context_line":"        \u0027120 iscsi initiator\u0027:"},{"line_number":127,"context_line":"          dport: 3260"},{"line_number":128,"context_line":"        \u0027120 Cinder LVM with nvmet target (NVMe-oF)\u0027:"},{"line_number":129,"context_line":"          dport: 4420"},{"line_number":130,"context_line":"      monitoring_subscription: {get_param: MonitoringSubscriptionCinderVolume}"},{"line_number":131,"context_line":"      config_settings:"},{"line_number":132,"context_line":"        map_merge:"}],"source_content_type":"text/x-yaml","patch_set":5,"id":"b099dcd0_3ad322bd","line":129,"range":{"start_line":125,"start_character":0,"end_line":129,"end_character":21},"updated":"2023-03-16 18:20:27.000000000","message":"The \u0027120 iscsi initiator\u0027 rule is an old tripleo legacy that is present just to support cinder\u0027s LVM backend for test purposes. I know the same LVM backend can now be leveraged to support testing the nvmet protocol, but I don\u0027t think we want to always expose two ports on controller nodes, especially when the LVM backend isn\u0027t enabled.\n\nI\u0027d like to consider a few enhancements. Define a new THT parameter, something like this:\n\n  CinderLVMTargetPort:\n    type: number\n    default: 3260\n\nThen reference the port in the firewall_rules, but also make it conditional on the backend being enabled, like this:\n\n      firewall_rules:\n        - if {get_param: CinderEnableIscsiBackend}\n        - \u0027120 iscsi/nvmet target\u0027:\n            dport: {get_param: CinderLVMTargetPort}\n        - {}","commit_id":"65b7cd46b751d4a6881b9aaff971f78cd82b73d5"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"ce48cad11eef304a7b2132d18e85b1443bf088f3","unresolved":true,"context_lines":[{"line_number":122,"context_line":"    description: Role data for the Cinder Volume role."},{"line_number":123,"context_line":"    value:"},{"line_number":124,"context_line":"      service_name: cinder_volume"},{"line_number":125,"context_line":"      firewall_rules:"},{"line_number":126,"context_line":"        \u0027120 iscsi initiator\u0027:"},{"line_number":127,"context_line":"          dport: 3260"},{"line_number":128,"context_line":"        \u0027120 Cinder LVM with nvmet target (NVMe-oF)\u0027:"},{"line_number":129,"context_line":"          dport: 4420"},{"line_number":130,"context_line":"      monitoring_subscription: {get_param: MonitoringSubscriptionCinderVolume}"},{"line_number":131,"context_line":"      config_settings:"},{"line_number":132,"context_line":"        map_merge:"}],"source_content_type":"text/x-yaml","patch_set":5,"id":"45f1dcf2_858bc34a","line":129,"range":{"start_line":125,"start_character":0,"end_line":129,"end_character":21},"in_reply_to":"89fc11cf_d12c5148","updated":"2023-03-20 14:33:57.000000000","message":"Initially there were reasons why downstream RHOSP testing was based on the iscsi/LVM backend, and not the tripleo nvmeof backend. We\u0027re in the process of updating the test strategy, and will likely incorporate some further updates to enable using nvmeof backend. In other words, I anticipate the next patchset will move the firewall rule as you recommended.","commit_id":"65b7cd46b751d4a6881b9aaff971f78cd82b73d5"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"b68db186b8a701506b890cf99b6ea4d5d97f9e5a","unresolved":true,"context_lines":[{"line_number":122,"context_line":"    description: Role data for the Cinder Volume role."},{"line_number":123,"context_line":"    value:"},{"line_number":124,"context_line":"      service_name: cinder_volume"},{"line_number":125,"context_line":"      firewall_rules:"},{"line_number":126,"context_line":"        \u0027120 iscsi initiator\u0027:"},{"line_number":127,"context_line":"          dport: 3260"},{"line_number":128,"context_line":"        \u0027120 Cinder LVM with nvmet target (NVMe-oF)\u0027:"},{"line_number":129,"context_line":"          dport: 4420"},{"line_number":130,"context_line":"      monitoring_subscription: {get_param: MonitoringSubscriptionCinderVolume}"},{"line_number":131,"context_line":"      config_settings:"},{"line_number":132,"context_line":"        map_merge:"}],"source_content_type":"text/x-yaml","patch_set":5,"id":"89fc11cf_d12c5148","line":129,"range":{"start_line":125,"start_character":0,"end_line":129,"end_character":21},"in_reply_to":"b099dcd0_3ad322bd","updated":"2023-03-17 05:36:16.000000000","message":"I think we can add the firewall rule for NVME target in coinder-backend-nvmeof-puppet.yaml. This is more simple because it allows you to enable that rule only when nvme backend is used.\n\nYou can modify cinder-volume-container-puppet.yaml to enable the filewall rule for iscsi target according to CinderEnableIscsiBackend\n\n```\n      firewall_rules:\n        - if {get_param: CinderEnableIscsiBackend}\n        - \u0027120 iscsi initiator\u0027\n            dport: 3260\n\n```\n\nand add the nvme target firewall rule to cinder-backend-nvmeof-puppet.yaml\n\n```\n      firewall_rules:\n        - \u0027120 nvmet target\u0027\n             dport: {get_param: CinderNVMeOFTargetPort}\n```","commit_id":"65b7cd46b751d4a6881b9aaff971f78cd82b73d5"}]}