consider a valid ipv4 address as a validish ipv4 /32 cidr by DaanHoogland · Pull Request #10174 · apache/cloudstack

DaanHoogland · 2025-01-09T12:21:54Z

Description

This PR...

Fixes: #10172

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

Screenshots (if appropriate):

How Has This Been Tested?

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

codecov · 2025-01-09T12:29:52Z

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 15.12%. Comparing base (afc95f1) to head (03c2598).
Report is 4 commits behind head on 4.19.

Files with missing lines	Patch %	Lines
.../secondarystorage/SecondaryStorageManagerImpl.java	0.00%	5 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## 4.19 #10174 +/- ##
============================================
- Coverage 15.12% 15.12% -0.01% 
Complexity 11268 11268 ============================================
Files 5408 5408 Lines 473954 473961 +7 Branches 57810 57812 +2 ============================================
- Hits 71709 71706 -3 - Misses 394231 394240 +9 - Partials 8014 8015 +1

Flag	Coverage Δ
uitests	`4.30% <ø> (ø)`
unittests	`15.84% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java

DaanHoogland · 2025-01-10T12:20:44Z

@Pearl1594 , could you verify if this fits your purpose?

DaanHoogland · 2025-01-13T12:36:51Z

@blueorangutan package

blueorangutan · 2025-01-13T12:38:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

wido

Code LGTM

blueorangutan · 2025-01-13T15:30:12Z

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

DaanHoogland · 2025-01-13T16:02:31Z

@blueorangutan test

blueorangutan · 2025-01-13T16:04:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

Pearl1594

tested. LTGM. However, I am not sure how 0.0.0.0 is to be handled

blueorangutan · 2025-01-14T05:38:00Z

[SF] Trillian test result (tid-12097)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 46380 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10174-t12097-kvm-ol8.zip
Smoke tests completed. 133 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

…/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java

DaanHoogland · 2025-01-14T09:30:51Z

@blueorangutan package

blueorangutan · 2025-01-14T09:32:04Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2025-01-14T10:36:32Z

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

DaanHoogland · 2025-01-14T11:06:35Z

@blueorangutan test

blueorangutan · 2025-01-14T11:08:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2025-01-15T00:23:48Z

[SF] Trillian test result (tid-12107)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 45168 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10174-t12107-kvm-ol8.zip
Smoke tests completed. 133 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

weizhouapache

code lgtm

* 4.20: Rollback of changes with errors during the VM assign (#7061) [VMware] Consider CD/DVD drive when calculating next free unit number for volume attachment over IDE controller (#9644) consider a valid ipv4 address as a validish ipv4 /32 cidr (#10174)

* consider a valid ipv4 address as a validish ipv4 /32 cidr * refactor cidr evaluation for internal nets * Apply suggestions from code review * Update services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java * Update services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java

consider a valid ipv4 address as a validish ipv4 /32 cidr
59dfcfd

DaanHoogland requested a review from Pearl1594 January 9, 2025 12:21

DaanHoogland linked an issue Jan 9, 2025 that may be closed by this pull request
SSVM goes into alert state if secstorage.allowed.internal.sites is set to an IP (not CIDR) #10172
Closed

Pearl1594 reviewed Jan 9, 2025
View reviewed changes

Comment threadutils/src/main/java/com/cloud/utils/net/NetUtils.java Outdated

refactor cidr evaluation for internal nets
6a18b66

boring-cyborgBot added component:secondary-storage component:ssvm labels Jan 9, 2025

weizhouapache reviewed Jan 9, 2025
View reviewed changes

DaanHoogland added this to the 4.19.2 milestone Jan 10, 2025

DaanHoogland commented Jan 10, 2025
View reviewed changes

Comment thread...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java Outdated

Apply suggestions from code review
5874e80

DaanHoogland commented Jan 10, 2025
View reviewed changes

Comment thread...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java Outdated

DaanHoogland commented Jan 10, 2025
View reviewed changes

Comment thread...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java Outdated

DaanHoogland requested a review from Pearl1594 January 10, 2025 10:03

Update services/secondary-storage/controller/src/main/java/org/apache…
deb353e
…/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java

apache deleted a comment from blueorangutan Jan 10, 2025

DaanHoogland marked this pull request as ready for review January 10, 2025 12:19

apache deleted a comment from blueorangutan Jan 13, 2025

wido self-requested a review January 13, 2025 13:35

wido approved these changes Jan 13, 2025
View reviewed changes

apache deleted a comment from blueorangutan Jan 13, 2025

DaanHoogland added the status:needs-testing label Jan 13, 2025

weizhouapache reviewed Jan 13, 2025
View reviewed changes

Comment thread...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java Outdated

Pearl1594 approved these changes Jan 14, 2025
View reviewed changes

Update services/secondary-storage/controller/src/main/java/org/apache…
03c2598
…/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java

DaanHoogland removed the status:needs-testing label Jan 14, 2025

weizhouapache approved these changes Jan 15, 2025
View reviewed changes

weizhouapache merged commit cf5bd80 into apache:4.19Jan 15, 2025

weizhouapache mentioned this pull request Jan 15, 2025
SSVM goes into alert state if secstorage.allowed.internal.sites is set to an IP (not CIDR) #10172
Closed

DaanHoogland deleted the ghi10172-isValidCidr branch January 15, 2025 14:15

Uh oh!

Conversation

DaanHoogland commented Jan 9, 2025

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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

Uh oh!

Uh oh!

codecovBot commented Jan 9, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented Jan 10, 2025

Uh oh!

DaanHoogland commented Jan 13, 2025

Uh oh!

blueorangutan commented Jan 13, 2025

Uh oh!

wido left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jan 13, 2025

Uh oh!

DaanHoogland commented Jan 13, 2025

Uh oh!

blueorangutan commented Jan 13, 2025

Uh oh!

Uh oh!

Pearl1594 left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jan 14, 2025

Uh oh!

DaanHoogland commented Jan 14, 2025

Uh oh!

blueorangutan commented Jan 14, 2025

Uh oh!

blueorangutan commented Jan 14, 2025

Uh oh!

DaanHoogland commented Jan 14, 2025

Uh oh!

blueorangutan commented Jan 14, 2025

Uh oh!

blueorangutan commented Jan 15, 2025

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecovBot commented Jan 9, 2025•
edited
Loading