Skip to content

Fixed null pointer and deployment issue on Xenserver with L2 Guest network with configDrive#4004

Merged
yadvr merged 7 commits into
apache:4.13from
shapeblue:null-pointer-on-xen-instance
Jun 23, 2020
Merged

Fixed null pointer and deployment issue on Xenserver with L2 Guest network with configDrive#4004
yadvr merged 7 commits into
apache:4.13from
shapeblue:null-pointer-on-xen-instance

Conversation

@Spaceman1984

@Spaceman1984 Spaceman1984 commented Mar 31, 2020

Copy link
Copy Markdown
Contributor

Description

This PR fixes an issue where an instance fails to deploy due to a null pointer when using an L2 Guest Network with DefaultL2NetworkOfferingConfigDrive on Xenserver. It also fixes migrating an instance to another host.

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)

Screenshots (if appropriate):

How Has This Been Tested?

This has been tested by:

  • Creating an L2 Guest network, using DefaultL2NetworkOfferingConfigDrive as the network offering.
  • Deploying an instance using the L2 Guest network created.
  • Migrating the instance away from the host and back

@Spaceman1984

Copy link
Copy Markdown
Contributor Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@Spaceman1984 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos7 ✔debian. JID-1109

@nvazquez

nvazquez commented Apr 9, 2020

Copy link
Copy Markdown
Contributor

@blueorangutan test centos7 xenserver-65sp1

@blueorangutan

Copy link
Copy Markdown

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + xenserver-65sp1) has been kicked to run smoke tests

@blueorangutan

Copy link
Copy Markdown

Trillian test result (tid-1402)
Environment: xenserver-65sp1 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35354 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4004-t1402-xenserver-65sp1.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Smoke tests completed. 76 look OK, 1 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_02_vpc_privategw_static_routes Failure 272.70 test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup Failure 256.84 test_privategw_acl.py
test_04_rvpc_privategw_static_routes Failure 402.47 test_privategw_acl.py

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

code looks good, but CitrixStartCommandWrapper is a one method class in which the method is 180 lines. can you partition that instead of adding to it, please.

@Spaceman1984 Spaceman1984 marked this pull request as draft April 15, 2020 14:27
@Spaceman1984 Spaceman1984 marked this pull request as ready for review April 17, 2020 09:37
@Spaceman1984 Spaceman1984 marked this pull request as draft April 17, 2020 10:02
@Spaceman1984 Spaceman1984 marked this pull request as ready for review April 20, 2020 11:33
@Spaceman1984 Spaceman1984 marked this pull request as draft April 20, 2020 12:36
@Spaceman1984

Copy link
Copy Markdown
Contributor Author

@blueorangutan package

@Spaceman1984 Spaceman1984 marked this pull request as ready for review May 4, 2020 08:43
@Spaceman1984 Spaceman1984 changed the title Fixed null pointer and deployment issue on Xenserver and L2 Guest network with configDrive Fixed null pointer and deployment issue on Xenserver with L2 Guest network with configDrive May 4, 2020
@blueorangutan

Copy link
Copy Markdown

@Spaceman1984 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@Spaceman1984

Copy link
Copy Markdown
Contributor Author

@blueorangutan test centos7 xenserver-65sp1

@blueorangutan

Copy link
Copy Markdown

@Spaceman1984 a Trillian-Jenkins test job (centos7 mgmt + xenserver-65sp1) has been kicked to run smoke tests

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos7 ✔debian. JID-1208

@borisstoyanov borisstoyanov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, was able to configure and use CD in Xen 7.0
Screenshot 2020-05-19 at 11 10 26

@borisstoyanov

Copy link
Copy Markdown
Contributor

@blueorangutan test centos7 xenserver70

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one more question, @Spaceman1984

Comment thread server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java Outdated
@DaanHoogland DaanHoogland added this to the 4.13.2.0 milestone May 26, 2020

@Pearl1594 Pearl1594 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NPE has been fixed for both creation and migration of VM on xen.
LGTM

@nvazquez

Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos7 ✔debian. JID-1251

@nvazquez

Copy link
Copy Markdown
Contributor

@blueorangutan test centos7 xenserver-71

@blueorangutan

Copy link
Copy Markdown

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + xenserver-71) has been kicked to run smoke tests

@blueorangutan

Copy link
Copy Markdown

Trillian test result (tid-1575)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38207 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4004-t1575-xenserver-71.zip
Intermittent failure detected: /marvin/tests/smoke/test_iso.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_scale_vm.py
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 75 look OK, 2 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_02_vpc_privategw_static_routes Failure 262.94 test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup Failure 223.79 test_privategw_acl.py
test_04_rvpc_privategw_static_routes Failure 314.26 test_privategw_acl.py
test_01_scale_vm Failure 20.63 test_scale_vm.py

@yadvr

yadvr commented Jun 4, 2020

Copy link
Copy Markdown
Member

@DaanHoogland can you review this? Thanks (I'm not following this)

@DaanHoogland

Copy link
Copy Markdown
Contributor

@nvazquez (cc @Spaceman1984 ) I'm leaving this with you for now? it was reviewed and tested. Can you give it a last glance and merge?

@yadvr

yadvr commented Jun 12, 2020

Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos7 ✔debian. JID-1355

@yadvr

yadvr commented Jun 13, 2020

Copy link
Copy Markdown
Member

@blueorangutan test centos7 xenserver-71

@blueorangutan

Copy link
Copy Markdown

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + xenserver-71) has been kicked to run smoke tests

@apache apache deleted a comment from blueorangutan Jun 13, 2020
@yadvr

yadvr commented Jun 13, 2020

Copy link
Copy Markdown
Member

@blueorangutan test centos7 xenserver-71

@blueorangutan

Copy link
Copy Markdown

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + xenserver-71) has been kicked to run smoke tests

@blueorangutan

Copy link
Copy Markdown

Trillian test result (tid-1719)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29887 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4004-t1719-xenserver-71.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_scale_vm.py
Smoke tests completed. 75 look OK, 2 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_02_vpc_privategw_static_routes Failure 225.13 test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup Failure 233.12 test_privategw_acl.py
test_04_rvpc_privategw_static_routes Failure 342.23 test_privategw_acl.py
test_01_scale_vm Failure 20.49 test_scale_vm.py

@yadvr yadvr requested review from DaanHoogland and nvazquez June 15, 2020 03:52

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks ok, only case i wonder about is a VM with multiple nics and configdrive. did you test this ?

@yadvr

yadvr commented Jun 15, 2020

Copy link
Copy Markdown
Member

@Spaceman1984 cc @nvazquez please advise, if you've tested then we can merge this

@Spaceman1984

Copy link
Copy Markdown
Contributor Author

I didn't test multiple network interfaces.

@yadvr

yadvr commented Jun 16, 2020

Copy link
Copy Markdown
Member

@borisstoyanov per your testing is this ready to be merged, or needs further testing, the case of multiple nics per Daan's comment?

@yadvr

yadvr commented Jun 23, 2020

Copy link
Copy Markdown
Member

Discussed with @Pearl1594, the operations for config drive are only consumed on the default nics; in case of multiple nics there won't be any regressions.

@yadvr yadvr merged commit 97f21c1 into apache:4.13 Jun 23, 2020
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.

8 participants