CLOUDSTACK-8940: Wrong value is inserted into nics table netmask fiel… #916
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
So several questions:
|
Member
|
@kansal LGTM. Looking at the JIRA issue, this looks like should apply to 4.6/master but can you update the ticket with the version details, more details on the bug and also note if this also needs to be backported to 4.5/4.4 or older branches. Thanks. |
Contributor
Author
|
@bhaisaab Sure will check about the back-porting of the issue. Apart from that, the previous values stored incorrectly need to be corrected in the database. I think a simple bash script will do. Your views and will it be part of this PR only? |
Member
|
Use db migration from either past 4.5.x to 4.6.0 version, or this PR gets merged post 4.6.0; add the data migration code in 4.6.0 to 4.6.1 |
Contributor
|
Hi @kansal Can you please rebase this with current master? I tried running some tests but face old problems. Also, what about the db migration script to fix old problems? Thanks for the update! |
Contributor
|
LGTM, based on a set of tests that I run on this branch (which I rebased myself first): Result: The 3 errors at the bottom are due to CLOUDSTACK-8991 and unrelated to this PR. Please try running Trying again the other failed test: Result: So, must have been a local issue. Next test run: Result: Before we consider merging, please respond to the comment about the upgrade script. Thanks! |
Contributor
Author
|
@remibergsma Thank you very much. I was busy somewhere. Will update this PR with "update script" and rebase it. Sorry for the delayed response. |
Contributor
Author
|
@remibergsma @bhaisaab The above changes were made as the part of DHCP/DNS offload feature. The related ticket is https://issues.apache.org/jira/browse/CLOUDSTACK-8324 |
…d when creating a VM - Fixed
kansal
force-pushed
the
CLOUDSTACK-8940
branch
from
eb217fa to
ba26efc
Compare
Contributor
|
Hi @kansal Should I deploy a basic zone with shared network in order to see if that fix is fine? I will also run the other set of tests, same as @remibergsma did. Cheers, |
Contributor
|
code lgtm @remibergsma can you retest? A newer commit is in since your lgtm |
Contributor
Author
|
Hi @wilderrodrigues, To check the bug is solved, try deploying the VM in shared network with no service.
As you can see, this function is called only when is isSharedNetworkWithoutService() is true. |
Contributor
|
Ping @remibergsma @karuturi @kansal I will run the hardware required tests as well and try to deployed a VM with a shared network. Partial test results:
Cheers, |
Contributor
|
LGTM, based on a set of tests that I run on this branch (which I rebased myself first): Result: And: Result: These test do not cover your change, all they do is show you didn't break them. Someone else needs to review the code. |
Contributor
|
@wilderrodrigues any update on your review? |
Contributor
|
LGTM 👍 Please proceed with merge. |
asfgit
pushed a commit
that referenced
this pull request
Nov 2, 2015
CLOUDSTACK-8940: Wrong value is inserted into nics table netmask field when creating a VM - Fixed Problem: When creating a VM in shared network with no service, the value of netmask is added in the table in the CIDR format unlike other cases where it is added as normal string in the format xxx.xxx.xxx.xxx. The netmask column in the nics table has a length of 15 chars which gets violated if the CIDR exceeds it(Max CIDR length can be 18). Fix: Before storing the netmask convert from CIDR to native format. * pr/916: CLOUDSTACK-8940: Wrong value is inserted into nics table netmask field when creating a VM - Fixed Signed-off-by: Remi Bergsma <[email protected]>
rohityadavcloud
pushed a commit
that referenced
this pull request
Jan 20, 2021
By switching to this directory we can use relative paths regardless of how people execute the script. cd tools ./docker.sh OR ./tools/docker.sh OR /path/to/git-repo/tools/docker.sh Signed-off-by: Rohit Yadav <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…d when creating a VM - Fixed
Problem: When creating a VM in shared network with no service, the value of netmask is added in the table in the CIDR format unlike other cases where it is added as normal string in the format xxx.xxx.xxx.xxx. The netmask column in the nics table has a length of 15 chars which gets violated if the CIDR exceeds it(Max CIDR length can be 18).
Fix: Before storing the netmask convert from CIDR to native format.