-
Notifications
You must be signed in to change notification settings - Fork 1.2k
server: trim autoscale Windows VM hostname #11327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes apache#9505 Signed-off-by: Abhishek Kumar <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11327 +/- ##
============================================
+ Coverage 17.50% 17.54% +0.03%
- Complexity 15427 15476 +49
============================================
Files 5894 5897 +3
Lines 526847 527426 +579
Branches 64335 64416 +81
============================================
+ Hits 92234 92538 +304
- Misses 424236 424489 +253
- Partials 10377 10399 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Abhishek Kumar <[email protected]>
Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan package |
@shwstppr 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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14463 |
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
@shwstppr |
@weizhouapache yes, though I wanted to confirm if it is a good idea to use the last 15 characters of the long generated name. Not using the first 15 as they can turn out to be the same. |
@shwstppr is it possible the first letter is unaccepted (for example |
Signed-off-by: Abhishek Kumar <[email protected]>
if (Character.isLetterOrDigit(hostName.charAt(0))) { | ||
return new Pair<>(hostName, name); | ||
} | ||
String temp = name.substring(0, Math.max(0, name.length() - 15)).replaceAll("[^a-zA-Z0-9]", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edge case: all of the last 15 chars are special chars or whitespace. I think we better do a replace first and then take the last 15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaanHoogland I don't think this case can happen.
The only user-defined part in the VM name is the name of the autoscale group, and we've checks for that to only contain letters, numbers and hyphen. checkAutoScaleVmGroupName` does that
Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan package |
@shwstppr 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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14730 |
server/src/main/java/com/cloud/network/as/AutoScaleManagerImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan package |
@shwstppr 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. |
Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan package |
@shwstppr 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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 15239 |
@abh1sar please try one again with new packages. Added a change. I didn't realize we store guest OSes with null name. |
@blueorangutan test |
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shwstppr please see if we need to add this to documentation? |
Thanks @abh1sar. (I've also added minor refactoring to not consider OS as Windows if both name and display_name turn out to be NULL) |
Related apache/cloudstack#11327 Signed-off-by: Abhishek Kumar <[email protected]>
Added doc PR: apache/cloudstack-documentation#575 @blueorangutan package |
@shwstppr 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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15248 |
[SF] Trillian test result (tid-14488)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments.
server/src/main/java/com/cloud/network/as/AutoScaleManagerImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan package |
@shwstppr 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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15341 |
@weizhouapache @shwstppr is this not ready yet? |
@DaanHoogland based on the latest comments I think we would need changes in naming implementation. I would close this for now for a different solution |
Description
Fixes #9505
Trim the generated hostname for Windows VM to 15 characters.
Doc PR: apache/cloudstack-documentation#575
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?