Skip to content

Conversation

@DaanHoogland
Copy link
Contributor

Description

This PR intends to allow users to choose an IP address for the VR of their network from the pool of available addresses.

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)

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?

@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #6442 (6dbcee2) into main (941cc83) will increase coverage by 0.32%.
The diff coverage is 32.46%.

@@             Coverage Diff              @@
##               main    #6442      +/-   ##
============================================
+ Coverage     12.69%   13.01%   +0.32%     
- Complexity     8674     9172     +498     
============================================
  Files          2729     2728       -1     
  Lines        256579   262916    +6337     
  Branches      39987    42677    +2690     
============================================
+ Hits          32575    34222    +1647     
- Misses       219859   224397    +4538     
- Partials       4145     4297     +152     
Impacted Files Coverage Δ
.../main/java/com/cloud/network/IpAddressManager.java 100.00% <ø> (ø)
...n/java/com/cloud/network/dao/IPAddressDaoImpl.java 31.22% <0.00%> (ø)
...src/main/java/com/cloud/api/ApiResponseHelper.java 4.96% <0.00%> (+1.13%) ⬆️
...com/cloud/network/NetworkMigrationManagerImpl.java 0.28% <ø> (ø)
.../cloud/configuration/ConfigurationManagerImpl.java 16.10% <5.55%> (+0.85%) ⬆️
...ain/java/com/cloud/network/vpc/VpcManagerImpl.java 9.37% <25.00%> (+0.70%) ⬆️
...ain/java/com/cloud/network/NetworkServiceImpl.java 12.91% <38.21%> (+1.50%) ⬆️
...n/java/com/cloud/network/IpAddressManagerImpl.java 2.90% <100.00%> (+0.77%) ⬆️

... and 144 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kiranchavala
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@kiranchavala a 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
Copy link

Packaging result: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6017

@blueorangutan
Copy link

Packaging result: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6042

@blueorangutan
Copy link

Trillian Build Failed (tid-6520)

@DaanHoogland DaanHoogland reopened this May 9, 2023
Copy link
Contributor

@kiranchavala kiranchavala left a comment

Choose a reason for hiding this comment

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

LGTM, tested the feature and all the scenarios

Configurable SNAT IP for VR (test cases).xlsx

@kiranchavala
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@kiranchavala a [SF] 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
Copy link

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

@DaanHoogland DaanHoogland marked this pull request as ready for review June 6, 2023 10:59
@DaanHoogland
Copy link
Contributor Author

@weizhouapache @GutoVeronezi @alexandremattioli , I am assuming your comments have all been addressed?

@DaanHoogland DaanHoogland reopened this Jun 6, 2023
@weizhouapache
Copy link
Member

@weizhouapache @GutoVeronezi @alexandremattioli , I am assuming your comments have all been addressed?

@DaanHoogland
lgtm

@DaanHoogland DaanHoogland reopened this Jun 9, 2023
@DaanHoogland
Copy link
Contributor Author

@blueorangutan test

@blueorangutan
Copy link

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

@DaanHoogland DaanHoogland merged commit ae10263 into apache:main Jun 9, 2023
@weizhouapache
Copy link
Member

@DaanHoogland
I tested this feature, overall it looks nice !

two issues

  • update network with a public IP did nothing, if the IP is not acquired before. Would be good to display an error message ?
  • the network router is destroyed and then recreated, which caused 43 seconds downtime (static nat). this can be improved I think.

@DaanHoogland
Copy link
Contributor Author

  • update network with a public IP did nothing, if the IP is not acquired before. Would be good to display an error message ?

that seems a bug. It should throw an exception in this case.

@blueorangutan
Copy link

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

Test Result Time (s) Test File

Comment on lines +302 to +323
<a-form-item
ref="networkdomain"
name="networkdomain"
v-if="!isObjectEmpty(selectedNetworkOffering) && !selectedNetworkOffering.forvpc">
<template #label>
<tooltip-label :title="$t('label.networkdomain')" :tooltip="apiParams.networkdomain.description"/>
</template>
<a-input
v-model:value="form.networkdomain"
:placeholder="apiParams.networkdomain.description"/>
</a-form-item>
<a-form-item
ref="account"
name="account"
v-if="accountVisible">
<template #label>
<tooltip-label :title="$t('label.account')" :tooltip="apiParams.account.description"/>
</template>
<a-input
v-model:value="form.account"
:placeholder="apiParams.account.description"/>
</a-form-item>
Copy link
Contributor

Choose a reason for hiding this comment

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

@DaanHoogland, this code seems duplicated, considering that the network domain and account fields are already present in the form. Is this on purpose?

While working on PR #8919, there is no option to create a network for a project, so I was thinking of adding the Ownership component to this view as well and removing this duplicate code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great @BryanMLima , I think this was not intentional. I just wonder about this dialog containing a "network domain" twice, while it should be a "cloudstack domain" and a real network domain. So a bit of semantic hacking needed ;) Thanks.

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.

10 participants