server: fix enable/disable static nat if userdata is not supported

[weizhouapache]

Description

This PR fixes #5824

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?

[sureshanaparti]

changes LGTM

[sureshanaparti]

@blueorangutan package

[blueorangutan]

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

[DaanHoogland]

clgtm, do we have a reason for this regression?

[shwstppr]

code LGTM

[weizhouapache]

clgtm, do we have a reason for this regression?

@DaanHoogland

the exception is thrown at
https://github.com/apache/cloudstack/blob/main/engine/schema/src/main/java/com/cloud/network/dao/NetworkServiceMapDaoImpl.java#L125-L127

        if (ntwkSvc == null) {
            throw new UnsupportedServiceException("Service " + service.getName() + " is not supported in the network id=" + networkId);
        }

removing the lines might also fix the issue. but it has large impact.
this PR has much smaller impact.

[DaanHoogland]

clgtm, do we have a reason for this regression?

@DaanHoogland

the exception is thrown at https://github.com/apache/cloudstack/blob/main/engine/schema/src/main/java/com/cloud/network/dao/NetworkServiceMapDaoImpl.java#L125-L127

        if (ntwkSvc == null) {
            throw new UnsupportedServiceException("Service " + service.getName() + " is not supported in the network id=" + networkId);
        }

removing the lines might also fix the issue. but it has large impact. this PR has much smaller impact.

I would agree @weizhouapache, I approve of this change and intuitively it is the best solution but still have some questions;

• why is applyUserData called at all if it is not supported? and,
• is this a regression that might have other unforeseen consequences?

It looks like it was introduced by 9c6b02f.

[blueorangutan]

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

[weizhouapache]

clgtm, do we have a reason for this regression?

@DaanHoogland
the exception is thrown at https://github.com/apache/cloudstack/blob/main/engine/schema/src/main/java/com/cloud/network/dao/NetworkServiceMapDaoImpl.java#L125-L127

        if (ntwkSvc == null) {
            throw new UnsupportedServiceException("Service " + service.getName() + " is not supported in the network id=" + networkId);
        }

removing the lines might also fix the issue. but it has large impact. this PR has much smaller impact.

I would agree @weizhouapache, I approve of this change and intuitively it is the best solution but still have some questions;

• why is applyUserData called at all if it is not supported? and,
• is this a regression that might have other unforeseen consequences?

It looks like it was introduced by 9c6b02f.

@DaanHoogland
when enable/disable static nat, the public IP of the vm needs to also updated in userdata server (=VR).
by default, VM public IP is source nat IP. if static nat is enabled, VM public IP is the static nat IP.
that's why we apply userdata of the vm/nic.

you might have also noticed that there are already check below in code

        if (element == null) {
            s_logger.error("Can't find network element for " + Service.UserData.getName() + " provider needed for UserData update");

if element cannot be found, then do not apply userdata. but the exception is thrown in Dao (I think it is not a good idea)

I have no idea if there are other issues caused by the code in Dao I pasted above.
for enable/disable static nat, both should work no matter userdata is supported or not.

[DaanHoogland]

clgtm, do we have a reason for this regression?

@DaanHoogland
the exception is thrown at https://github.com/apache/cloudstack/blob/main/engine/schema/src/main/java/com/cloud/network/dao/NetworkServiceMapDaoImpl.java#L125-L127

        if (ntwkSvc == null) {
            throw new UnsupportedServiceException("Service " + service.getName() + " is not supported in the network id=" + networkId);
        }

removing the lines might also fix the issue. but it has large impact. this PR has much smaller impact.

I would agree @weizhouapache, I approve of this change and intuitively it is the best solution but still have some questions;

• why is applyUserData called at all if it is not supported? and,
• is this a regression that might have other unforeseen consequences?

It looks like it was introduced by 9c6b02f.

@DaanHoogland when enable/disable static nat, the public IP of the vm needs to also updated in userdata server (=VR). by default, VM public IP is source nat IP. if static nat is enabled, VM public IP is the static nat IP. that's why we apply userdata of the vm/nic.

you might have also noticed that there are already check below in code

        if (element == null) {
            s_logger.error("Can't find network element for " + Service.UserData.getName() + " provider needed for UserData update");

if element cannot be found, then do not apply userdata. but the exception is thrown in Dao (I think it is not a good idea)

I have no idea if there are other issues caused by the code in Dao I pasted above. for enable/disable static nat, both should work no matter userdata is supported or not.

I get it @weizhouapache , as discussed of line applyUserData should be renamed to applyUserDataIfNeeded. I thought it should not have been invoked in the first place, if it is harmfull. Your solution is good, thanks.

[weizhouapache]

I get it @weizhouapache , as discussed of line applyUserData should be renamed to applyUserDataIfNeeded. I thought it should not have been invoked in the first place, if it is harmfull. Your solution is good, thanks.

@DaanHoogland thanks.

Renamed the method as per your comment.

[weizhouapache]

@blueorangutan package

[blueorangutan]

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

[blueorangutan]

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

[sureshanaparti]

@blueorangutan test

[blueorangutan]

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

[GutoVeronezi]

CLGTM

[blueorangutan]

Trillian test result (tid-2823)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46814 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5839-t2823-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_loadbalance.py
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Intermittent failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermittent failure detected: /marvin/tests/smoke/test_password_server.py
Intermittent failure detected: /marvin/tests/smoke/test_deploy_vm_iso.py
Intermittent failure detected: /marvin/tests/smoke/test_deploy_vm_with_userdata.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermittent failure detected: /marvin/tests/smoke/test_deploy_vms_with_varied_deploymentplanners.py
Intermittent failure detected: /marvin/tests/smoke/test_templates.py
Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
Intermittent failure detected: /marvin/tests/smoke/test_reset_vm_on_reboot.py
Intermittent failure detected: /marvin/tests/smoke/test_resource_accounting.py

[rohityadavcloud]

@blueorangutan package

[blueorangutan]

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

[blueorangutan]

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

[sureshanaparti]

@blueorangutan package

[weizhouapache]

@blueorangutan package

[blueorangutan]

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

[blueorangutan]

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

[sureshanaparti]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

Trillian test result (tid-2844)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30953 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5839-t2844-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File