Skip to content

Conversation

@rg9975
Copy link

@rg9975 rg9975 commented Dec 21, 2024

Description

This PR...
This PR fixes several small issues with Fiberchannel implementation and locally discovered issues:

  1. volume disconnect timing issues
  2. rescan cleanup script not working correctly
  3. template copy issues
  4. masked/hidden errors and exceptions

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Tested locally with real world workloads

How did you try to break this feature and the system with this change?

Tested locally with real world workloads

Glover, Rene (rg9975) and others added 30 commits March 31, 2024 01:33
Copy link
Contributor

@slavkap slavkap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland, unfortunately, I don't have time now to test this but the code LGTM

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12232

Copy link
Contributor

@rp- rp- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll run this patch against a Linstor cluster to see if it breaks anything.

* Code cleanup tabs/newlines
@DaanHoogland
Copy link
Contributor

is there consensus growing on this or shall I move this to 4.19.3 and up?

@rg9975
Copy link
Author

rg9975 commented Jan 30, 2025

@DaanHoogland I'd like to review @rp- comments today, just could not get to it yesterday. Will provide comments/updates later today.

@DaanHoogland DaanHoogland requested a review from rp- February 1, 2025 15:19
@DaanHoogland
Copy link
Contributor

@rp- can you review the last changes.

@rp-
Copy link
Contributor

rp- commented Feb 3, 2025

@rp- can you review the last changes.

@DaanHoogland @rg9975

So I dug a bit deeper and found the actual problem to be the cloud-install-sys-tmplt script.
I use it in my automated CloudStack installations and this scripts sets size and physical_size 0 in the template_store_ref table, which than Linstor can't use to create a template.

If I don't use the script at all, and let CloudStack fetch the template on its own, the entries are correct and the Linstor driver already creates the volume on management server and everything is fine.

So my question is, is this script still recommended for installation? And if so can we fix it?

@DaanHoogland
Copy link
Contributor

@rp- can you review the last changes.

@DaanHoogland @rg9975

So I dug a bit deeper and found the actual problem to be the cloud-install-sys-tmplt script. I use it in my automated CloudStack installations and this scripts sets size and physical_size 0 in the template_store_ref table, which than Linstor can't use to create a template.

If I don't use the script at all, and let CloudStack fetch the template on its own, the entries are correct and the Linstor driver already creates the volume on management server and everything is fine.

So my question is, is this script still recommended for installation? And if so can we fix it?

because ACS packages no (can) include the template the script has somewhat gotten out of use. It should still work, but I am not sure how to fix it.

@rp-
Copy link
Contributor

rp- commented Feb 3, 2025

@rp- can you review the last changes.

@DaanHoogland @rg9975
So I dug a bit deeper and found the actual problem to be the cloud-install-sys-tmplt script. I use it in my automated CloudStack installations and this scripts sets size and physical_size 0 in the template_store_ref table, which than Linstor can't use to create a template.
If I don't use the script at all, and let CloudStack fetch the template on its own, the entries are correct and the Linstor driver already creates the volume on management server and everything is fine.
So my question is, is this script still recommended for installation? And if so can we fix it?

because ACS packages no (can) include the template the script has somewhat gotten out of use. It should still work, but I am not sure how to fix it.

AFAIS the script already gets the sizes and writes them correctly into the template.properties file, so all it would need to do is also update the database entries too?

@DaanHoogland
Copy link
Contributor

AFAIS the script already gets the sizes and writes them correctly into the template.properties file, so all it would need to do is also update the database entries too?

sounds good, does that mean we can merge this, @rp- ?

@rp-
Copy link
Contributor

rp- commented Feb 3, 2025

AFAIS the script already gets the sizes and writes them correctly into the template.properties file, so all it would need to do is also update the database entries too?

sounds good, does that mean we can merge this, @rp- ?

well, we can revert/drop then that last commit: f066100

@rg9975
Copy link
Author

rg9975 commented Feb 3, 2025

I've reverted commit #f066100

AFAIS the script already gets the sizes and writes them correctly into the template.properties file, so all it would need to do is also update the database entries too?

sounds good, does that mean we can merge this, @rp- ?

well, we can revert/drop then that last commit: [f066100]

Reverted.

@DaanHoogland DaanHoogland requested a review from rp- February 3, 2025 14:19
@DaanHoogland
Copy link
Contributor

@rp- , can you approve??

Copy link
Contributor

@rp- rp- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to have it noted here, I'll probably create an issue out of it.
It isn't the cloud-install-sys-tmplt script (maybe its usage), but the actual problem is in:

For pre-seeded templates, the template_store_ref entries get pre filled with 0 and never updated, which is bad I guess for most primary storages.
Using preseeded templates and this commit will break using those templates, until the DB entries are fixed.

@DaanHoogland
Copy link
Contributor

Just to have it noted here, I'll probably create an issue out of it. It isn't the cloud-install-sys-tmplt script (maybe its usage), but the actual problem is in:

For pre-seeded templates, the template_store_ref entries get pre filled with 0 and never updated, which is bad I guess for most primary storages. Using preseeded templates and this commit will break using those templates, until the DB entries are fixed.

ok, that to me sounds like we shouldn't merge this untill we fix that as well.

@rp-
Copy link
Contributor

rp- commented Feb 3, 2025

Just to have it noted here, I'll probably create an issue out of it. It isn't the cloud-install-sys-tmplt script (maybe its usage), but the actual problem is in:

For pre-seeded templates, the template_store_ref entries get pre filled with 0 and never updated, which is bad I guess for most primary storages. Using preseeded templates and this commit will break using those templates, until the DB entries are fixed.

ok, that to me sounds like we shouldn't merge this untill we fix that as well.

I have worked out a possible fix #10317

@DaanHoogland
Copy link
Contributor

I have worked out a possible fix #10317

will regression test #10317 and merge it together with this.

@DaanHoogland DaanHoogland merged commit 3337f42 into apache:4.19 Feb 7, 2025
25 of 26 checks passed
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jun 19, 2025
Co-authored-by: GLOVER RENE <[email protected]>
Co-authored-by: Suresh Kumar Anaparti <[email protected]>
@rg9975 rg9975 deleted the primera-pure-patches branch June 23, 2025 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants