Skip to content
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

Fixed the management server setup line #6822

Merged
merged 6 commits into from
Dec 7, 2022

Conversation

rahulbcn27
Copy link
Contributor

@rahulbcn27 rahulbcn27 commented Oct 11, 2022

Description

This PR is for changing the line printed as : "Please ensure the following ports are open for the management server to function properly : 8080 8250 8443 9090" to more understandable lines. The above line was misleading , so it was changed to new lines :

          print("Please ensure ports 8080, 8250, 8443, and 9090 are opened and not fire walled for the management server and not in use by other processes on this host.")

This happens after the management server is setup. Behaviour functionally has not changed at all. Only,meaning of the lines printed has changed.

The behavior Functionally has not changed at all.

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
  • [yes] Trivial

Screenshots (if appropriate):

How Has This Been Tested?

This has been tested on local environment.

Trivial Normal tests were done , that is usually done when source code is compiled and management server is started.

@boring-cyborg
Copy link

boring-cyborg bot commented Oct 11, 2022

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

print("Please ensure the following ports are open for the management server to function properly : 8080 8250 8443 9090")
print("CloudStack Management Server setup is Done!")
print("You can now log into CloudStack UI")
print("Management Server uses following ports to function properly: 8080 8250 8443 9090")
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a WARNING here

Copy link
Member

Choose a reason for hiding this comment

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

I think this is not logging but printing on the console when cloudstack-setup-management is run.

@rahulbcn27 I think the line can be improved to still suggest that the (a) ports must be open and not firewalled, and (b) not in use by other processes on the host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Rohit for the suggestion. I have made the changes proposed. Please check it.

@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #6822 (e46ed96) into main (fa39e61) will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #6822      +/-   ##
============================================
+ Coverage     10.84%   10.87%   +0.02%     
- Complexity     7104     7118      +14     
============================================
  Files          2485     2485              
  Lines        245417   245507      +90     
  Branches      38326    38334       +8     
============================================
+ Hits          26627    26710      +83     
- Misses       215521   215526       +5     
- Partials       3269     3271       +2     
Impacted Files Coverage Δ
...rg/apache/cloudstack/quota/QuotaStatementImpl.java 36.28% <0.00%> (-3.99%) ⬇️
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java 6.71% <0.00%> (-0.02%) ⬇️
...om/cloud/hypervisor/kvm/resource/LibvirtVMDef.java 67.65% <0.00%> (+0.61%) ⬆️
...ervisor/kvm/resource/LibvirtComputingResource.java 17.03% <0.00%> (+1.34%) ⬆️
...ypervisor/kvm/resource/LibvirtDomainXMLParser.java 50.61% <0.00%> (+2.11%) ⬆️
...apache/cloudstack/syslog/AlertsSyslogAppender.java 58.75% <0.00%> (+2.25%) ⬆️
...org/apache/cloudstack/utils/linux/KVMHostInfo.java 68.08% <0.00%> (+11.70%) ⬆️

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

Copy link
Member

@rohityadavcloud rohityadavcloud left a comment

Choose a reason for hiding this comment

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

Additional changes requested

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland 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 ✔️ debian ✔️ suse15. LL-JID 196

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

Trillian test result (tid-40)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43962 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6822-t40-kvm-centos7.zip
Smoke tests completed. 102 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers Failure 410.07 test_vpc_redundant.py
test_01_redundant_vpc_site2site_vpn Failure 746.72 test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_options Error 1277.08 test_vpc_vpn.py
test_01_vpc_site2site_vpn Error 546.30 test_vpc_vpn.py

Co-authored-by: Rohit Yadav <rohityadav89@gmail.com>
@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with SystemVM template(s). I'll keep you posted as I make progress.

@blueorangutan
Copy link

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

@rohityadavcloud
Copy link
Member

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

Trillian Build Failed (tid-5256)

client/bindir/cloud-setup-management.in Outdated Show resolved Hide resolved
@DaanHoogland
Copy link
Contributor

@rahulbcn27 can you add a test description, please?

@DaanHoogland
Copy link
Contributor

@rahulbcn27 can you add a test description, please?

sorry never mind, this is not really a functional change... (guess I was tired/it was late)

Copy link
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

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

@rahulbcn27 , please consider @stephankruggg 's suggestion which looks good

Co-authored-by: Stephan Krug <stekrug@icloud.com>
@rohityadavcloud rohityadavcloud merged commit 90413cb into apache:main Dec 7, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Dec 7, 2022

Awesome work, congrats on your first merged pull request!

@rohityadavcloud rohityadavcloud added this to the 4.18.0.0 milestone Dec 7, 2022
@rahulbcn27 rahulbcn27 deleted the feature_x branch January 19, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants