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?

utils/src/main/java/com/cloud/utils/net/NetUtils.java

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.

...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java

…/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

...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java

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

utils/src/main/java/com/cloud/utils/net/NetUtils.java Outdated Show resolved Hide resolved

refactor cidr evaluation for internal nets

6a18b66

boring-cyborg bot 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

...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java Outdated Show resolved Hide resolved

Apply suggestions from code review

5874e80

DaanHoogland commented Jan 10, 2025

View reviewed changes

...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java Outdated Show resolved Hide resolved

DaanHoogland commented Jan 10, 2025

View reviewed changes

...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java Outdated Show resolved Hide resolved

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

...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java Outdated Show resolved Hide resolved

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.19 Jan 15, 2025
25 of 26 checks passed

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

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!

codecov bot 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jan 9, 2025 •

edited

Loading