You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "madhukar93 (via GitHub)" <gi...@apache.org> on 2023/10/26 12:27:34 UTC
[PR] Fixes:7863: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
madhukar93 opened a new pull request, #8151:
URL: https://github.com/apache/cloudstack/pull/8151
### Description
This PR...
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ********************************************************************************* -->
### 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)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] build/CI
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [x] Minor
### Screenshots (if appropriate):
### How Has This Been Tested?
TODO: integration test using marvin
Manually, on the simulator
```
(localcloud) π± > list physicalnetworks id=a0fc1b59-ae44-42a3-a1f5-9bbb6d83a731
{
"count": 1,
"physicalnetwork": [
{
"broadcastdomainrange": "ZONE",
"id": "a0fc1b59-ae44-42a3-a1f5-9bbb6d83a731",
"isolationmethods": "VLAN",
"name": "Sandbox-pnet",
"state": "Enabled",
"vlan": "100-200",
"zoneid": "279434a2-4b78-4858-b714-69ee93df0db2",
"zonename": "Sandbox-simulator"
}
]
}
```
#### How did you try to break this feature and the system with this change?
TODO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1375238124
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListTrafficTypesCmd.java:
##########
@@ -27,14 +27,13 @@
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.api.response.PhysicalNetworkResponse;
-import org.apache.cloudstack.api.response.ProviderResponse;
import org.apache.cloudstack.api.response.TrafficTypeResponse;
import com.cloud.network.PhysicalNetworkTrafficType;
import com.cloud.user.Account;
import com.cloud.utils.Pair;
-@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = ProviderResponse.class, since = "3.0.0",
+@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = TrafficTypeResponse.class, since = "3.0.0",
Review Comment:
@rohityadavcloud noticed that the python client generation for `listTrafficTypes` (tools/marvin/marvin/cloudstackAPI/listTrafficTypes.py) did not match the fields in the `TrafficTypeResponse`, while exploring marvin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1374211092
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3082,6 +3082,7 @@ public TrafficTypeResponse createTrafficTypeResponse(PhysicalNetworkTrafficType
PhysicalNetwork pnet = ApiDBUtils.findPhysicalNetworkById(result.getPhysicalNetworkId());
if (pnet != null) {
response.setPhysicalNetworkId(pnet.getUuid());
+ response.setIsolationMethods(pnet.getIsolationMethods());
Review Comment:
`cmk` is able to display the array, I've put the output in the PR description if you want to take a look. I will check the serialization via a marvin test as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1799126757
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7665
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1373204412
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4997,6 +4997,7 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 && isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation methods, why are multiple isolation methods allowed per pnet? They seem to be filterable by provider/preifx (PhysicalNetwork.java). Documentation says only two are allowed but code and tests contradict this.
Review Comment:
Physical network response turns isolation methods into a string csv value, should we handle it similarly? -
```
list physicalnetworks id=a0fc1b59-ae44-42a3-a1f5-9bbb6d83a731
{
"count": 1,
"physicalnetwork": [
{
"broadcastdomainrange": "ZONE",
"id": "a0fc1b59-ae44-42a3-a1f5-9bbb6d83a731",
"isolationmethods": "VLAN",
"name": "Sandbox-pnet",
"state": "Enabled",
"vlan": "100-200",
"zoneid": "279434a2-4b78-4858-b714-69ee93df0db2",
"zonename": "Sandbox-simulator"
}
]
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1378379019
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListTrafficTypesCmd.java:
##########
@@ -27,14 +27,13 @@
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.api.response.PhysicalNetworkResponse;
-import org.apache.cloudstack.api.response.ProviderResponse;
import org.apache.cloudstack.api.response.TrafficTypeResponse;
import com.cloud.network.PhysicalNetworkTrafficType;
import com.cloud.user.Account;
import com.cloud.utils.Pair;
-@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = ProviderResponse.class, since = "3.0.0",
+@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = TrafficTypeResponse.class, since = "3.0.0",
Review Comment:
@rohityadavcloud I can revert this, but won't the clients not be able to depend on the fields from `ProviderResponse` anyway, since they aren't set.
@kishankavala I'm not sure how this attribute is used. Cloudmonkey doesn't seem to use it. Is this used for programmatic client generation and docs generation generally, but not for cmk? The one test that uses this API doesn't seem to care about the attributes in the response
This is causing confusion for the reporter in the related issue as well - https://github.com/apache/cloudstack/issues/7837#issuecomment-1672122978
I can revert this and maybe this should exist as a separate issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1801851387
> [SF] Trillian test result (tid-8263) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 50196 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8151-t8263-kvm-centos7.zip Smoke tests completed. 117 look OK, 1 have errors, 0 did not run Only failed and skipped tests results shown below:
> Test Result Time (s) Test File
> test_01_create_iso_with_checksum_sha1 `Error` 66.46 test_iso.py
> test_02_create_iso_with_checksum_sha256 `Error` 66.45 test_iso.py
> test_03_create_iso_with_checksum_md5 `Error` 66.45 test_iso.py
> test_04_create_iso_with_no_checksum `Error` 66.45 test_iso.py
> test_01_create_iso `Failure` 1514.32 test_iso.py
> ContextSuite context=TestISO>:setup `Error` 3027.76 test_iso.py
all errors have to do with iso failed to download. this is an environmental problem, not with this code. (test_templates has no errors)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1833792480
@DaanHoogland 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1378652762
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListTrafficTypesCmd.java:
##########
@@ -27,14 +27,13 @@
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.api.response.PhysicalNetworkResponse;
-import org.apache.cloudstack.api.response.ProviderResponse;
import org.apache.cloudstack.api.response.TrafficTypeResponse;
import com.cloud.network.PhysicalNetworkTrafficType;
import com.cloud.user.Account;
import com.cloud.utils.Pair;
-@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = ProviderResponse.class, since = "3.0.0",
+@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = TrafficTypeResponse.class, since = "3.0.0",
Review Comment:
@rohityadavcloud @kishankavala
# tl;dr;
We will need to make this change. Because cloudstack-go is currently based on output of `listApis` response and having a wrong response here will generate an incorrect client and terraform provider won't be able to use the keys we add in this PR.
Only keys which are present in both `ProviderResponse` & `TrafficTypeResponse` are accessible as of now using the cloudstack-go client. So, I don't think backward compatibility will be an issue here.
## Details
In cloudstack-go client's TrafficType struct, it has some keys which are not part of `listTrafficType` response and missing keys which should be in the response.
Ref: https://github.com/apache/cloudstack-go/blob/c9c595770d7dcc2c87090227ef15c94ceee27b2b/cloudstack/UsageService.go#L1092-L1107
This is the difference in output of cmk & cloudstack-go:
cmk
```
{
"count": 2,
"traffictype": [
{
"id": "04q1bbe1-0e1e-4a1a-94e3-btcd257z44b9",
"kvmnetworklabel": "cloudbr0",
"physicalnetworkid": "47191b54-510c-475e-va6f-d935e22a9744",
"traffictype": "Management",
"vmwarenetworklabel": "vSwitch0,,vmwaresvs",
"xennetworklabel": "Network0"
}
]
}
```
cloudstack-go
```
cloudstack.TrafficType{
Canenableindividualservice: false,
Destinationphysicalnetworkid: "",
Id: "04q1bbe1-0e1e-4a1a-94e3-btcd257z44b9",
JobID: "",
Jobstatus: 0,
Name: "",
Physicalnetworkid: "47191b54-510c-475e-va6f-d935e22a9744",
Servicelist: []string(nil),
State: ""
}
```
P.S. - This is one of my current env and not built on this PR.
As you can see there are some keys (`traffictype`, `kvmnetworklabel`, etc.) which are present in response but we can't access them using cloudstack-go client because it's not part of the struct.
code to listTrafficType using cloudstack-go
```go
func main() {
cs := cloudstack.NewAsyncClient(
"http://localhost:8080/client/api",
"access-key",
"secret-key",
false,
)
p := cs.Usage.NewListTrafficTypesParams("47191b54-510c-475e-va6f-d935e22a9744");
r, err := cs.Usage.ListTrafficTypes(p)
if err != nil {
log.Fatalf("%+v", err)
}
fmt.Printf("%#v\n", *r.TrafficTypes[0])
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1784482488
@harikrishna-patnala 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1375784587
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListTrafficTypesCmd.java:
##########
@@ -27,14 +27,13 @@
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.api.response.PhysicalNetworkResponse;
-import org.apache.cloudstack.api.response.ProviderResponse;
import org.apache.cloudstack.api.response.TrafficTypeResponse;
import com.cloud.network.PhysicalNetworkTrafficType;
import com.cloud.user.Account;
import com.cloud.utils.Pair;
-@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = ProviderResponse.class, since = "3.0.0",
+@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = TrafficTypeResponse.class, since = "3.0.0",
Review Comment:
All that was needed was to add some new keys in the response; if we're changing response object we must ensure the resultant response has keys that are superset of original response. I would run a list API call for before and after the fix to compare.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1385046562
##########
test/integration/smoke/test_network_traffic_type_api.py:
##########
@@ -0,0 +1,73 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Import Local Modules
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.base import (
+ TrafficType,
+ PhysicalNetwork,
+ Zone,
+)
+from nose.plugins.attrib import attr
+
+
+class TestTrafficTypeApi(cloudstackTestCase):
+ @classmethod
+ def setUpClass(cls):
+ cls.test_client = super(TestTrafficTypeApi, cls).getClsTestClient()
+ cls.api_client = cls.testClient.getApiClient()
+ cls.services = cls.testClient.getParsedTestDataConfig()
+ cls._cleanup = []
+ cls.services["advanced_sg"]["zone"]["name"] = "TestTrafficTypeApi-zone"
+ cls.zone = Zone.create(cls.api_client, cls.services["advanced_sg"]["zone"])
+ cls._cleanup.append(cls.zone)
+
+ cls.physical_network = PhysicalNetwork.create(
+ cls.api_client,
+ cls.services["l2-network"],
+ isolationmethods="VLAN",
+ zoneid=cls.zone.id,
+ )
+ cls._cleanup.append(cls.physical_network)
+
+ @classmethod
+ def tearDownClass(cls):
+ super(TestTrafficTypeApi, cls).tearDownClass()
Review Comment:
```suggestion
@classmethod
def tearDownClass(cls):
super(TestTrafficTypeApi, cls).tearDownClass()
```
guess this was a bad suggestion ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-3460338747
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-3460106690
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1799528203
@vishesh92 a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1804516316
@vishesh92 a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1804515026
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1816825023
@shwstppr a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1844820095
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1375785037
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4997,6 +4997,7 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 && isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation methods, why are multiple isolation methods allowed per pnet? They seem to be filterable by provider/preifx (PhysicalNetwork.java). Documentation says only two are allowed but code and tests contradict this.
Review Comment:
comma-separated string (vlans) is completed fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1378652762
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListTrafficTypesCmd.java:
##########
@@ -27,14 +27,13 @@
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.api.response.PhysicalNetworkResponse;
-import org.apache.cloudstack.api.response.ProviderResponse;
import org.apache.cloudstack.api.response.TrafficTypeResponse;
import com.cloud.network.PhysicalNetworkTrafficType;
import com.cloud.user.Account;
import com.cloud.utils.Pair;
-@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = ProviderResponse.class, since = "3.0.0",
+@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = TrafficTypeResponse.class, since = "3.0.0",
Review Comment:
@rohityadavcloud @kishankavala
# tl;dr;
We will need to make this change. Because cloudstack-go is currently based on output of `listApis` response and having a wrong response here will generate an incorrect client and terraform provider won't be able to use the keys we add in this PR.
Only keys which are present in both `ProviderResponse` & `TrafficTypeResponse` are accessible as of now using the cloudstack-go client. So, I don't think backward compatibility will be an issue here.
## Details
In cloudstack-go client's TrafficType struct, it has some keys which are not part of `listTrafficType` response and missing keys which should be in the response.
Ref: https://github.com/apache/cloudstack-go/blob/c9c595770d7dcc2c87090227ef15c94ceee27b2b/cloudstack/UsageService.go#L1092-L1107
This is the difference in output of cmk & cloudstack-go:
cmk
```json
{
"count": 2,
"traffictype": [
{
"id": "04q1bbe1-0e1e-4a1a-94e3-btcd257z44b9",
"kvmnetworklabel": "cloudbr0",
"physicalnetworkid": "47191b54-510c-475e-va6f-d935e22a9744",
"traffictype": "Management",
"vmwarenetworklabel": "vSwitch0,,vmwaresvs",
"xennetworklabel": "Network0"
}
]
}
```
cloudstack-go
```go
cloudstack.TrafficType{
Canenableindividualservice: false,
Destinationphysicalnetworkid: "",
Id: "04q1bbe1-0e1e-4a1a-94e3-btcd257z44b9",
JobID: "",
Jobstatus: 0,
Name: "",
Physicalnetworkid: "47191b54-510c-475e-va6f-d935e22a9744",
Servicelist: []string(nil),
State: ""
}
```
P.S. - This is one of my current env and not built on this PR.
As you can see there are some keys (`traffictype`, `kvmnetworklabel`, etc.) which are present in response but we can't access them using cloudstack-go client because it's not part of the struct.
code to listTrafficType using cloudstack-go
```go
func main() {
cs := cloudstack.NewAsyncClient(
"http://localhost:8080/client/api",
"access-key",
"secret-key",
false,
)
p := cs.Usage.NewListTrafficTypesParams("47191b54-510c-475e-va6f-d935e22a9744");
r, err := cs.Usage.ListTrafficTypes(p)
if err != nil {
log.Fatalf("%+v", err)
}
fmt.Printf("%#v\n", *r.TrafficTypes[0])
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-3522331706
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1833995287
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1784646009
@madhukar93 the integration test scenario would to be call the API and check the response against expected values of the API response. When changing the response type, you may need to make changes to Marvin library if a utility is used to call the api and process response; otherwise you would use the API client to make the API call and from its response object you'd get keys/entities to compare. When you rebuilt cloudstack (entirely), apidocs and marvin API stubs will be regenerated (which covers the case of say adding new api param or responses, or changing response type class etc).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1835624105
<b>[SF] Trillian test result (tid-8436)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 57136 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8151-t8436-kvm-centos7.zip
Smoke tests completed. 116 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_02_trigger_shutdown | `Failure` | 336.99 | test_safe_shutdown.py
test_01_invalid_upgrade_kubernetes_cluster | `Failure` | 3606.30 | test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster | `Failure` | 3606.46 | test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster | `Failure` | 0.03 | test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster | `Failure` | 0.03 | test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster | `Failure` | 51.95 | test_kubernetes_clusters.py
test_11_test_unmanaged_cluster_lifecycle | `Error` | 0.24 | test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 56.69 | test_kubernetes_clusters.py
test_hostha_enable_ha_when_host_in_maintenance | `Error` | 307.97 | test_hostha_kvm.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1377498054
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3082,6 +3082,10 @@ public TrafficTypeResponse createTrafficTypeResponse(PhysicalNetworkTrafficType
PhysicalNetwork pnet = ApiDBUtils.findPhysicalNetworkById(result.getPhysicalNetworkId());
if (pnet != null) {
response.setPhysicalNetworkId(pnet.getUuid());
+ // using PhysicalNetworkResponse only to convert isolation methods to string
+ PhysicalNetworkResponse pnetResponse = new PhysicalNetworkResponse();
+ pnetResponse.setIsolationMethods(pnet.getIsolationMethods());
+ response.setIsolationMethods(pnetResponse.getIsolationMethods());
Review Comment:
@DaanHoogland do you think moving this logic to `PhysicalNetwork`, by overloading the getIsolationMethods method works? What exists right now is `List<String> getIsolationMethods()`, I'm talking about adding a `String getIsolationMethods()` as well, that returns csv.
I'm not sure why `String.join` isn't used either, I don't think a separate utility is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1373146984
##########
api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java:
##########
@@ -56,6 +58,17 @@ public class TrafficTypeResponse extends BaseResponse {
@Param(description = "The network name label of the physical device dedicated to this traffic on a HyperV host")
private String hypervNetworkLabel;
+ // why not VLAN_ID
Review Comment:
clarify
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:7863: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1781023517
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:
- In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
- Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
- Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
- Please follow [ASF Code of Conduct](https://github.com/apache/.github/blob/main/.github/CODE_OF_CONDUCT.md) for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
- Be sure to read the [CloudStack Coding Conventions](https://cwiki.apache.org/confluence/display/CLOUDSTACK/Coding+conventions).
Apache CloudStack is a community-driven project and together we are making it better π.
In case of doubts contact the developers at:
Mailing List: dev@cloudstack.apache.org (https://cloudstack.apache.org/mailing-lists.html)
Slack: https://apachecloudstack.slack.com/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:7863: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1373104999
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4997,6 +4997,7 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 && isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation methods, why are multiple isolation methods allowed per pnet? They seem to be filterable by provider/preifx (PhysicalNetwork.java). Documentation says only two are allowed but code and tests contradict this.
Review Comment:
I see [checks](https://github.com/apache/cloudstack/blob/main/server/src/main/java/com/cloud/network/NetworkServiceImpl.java#L3824-L3824) to ensure only one isolation method can exist at this time. @rohityadavcloud @DaanHoogland I would like to confirm if the API field should be plural for futureproofing, or should we maintain parity with with [addTrafficType](https://cloudstack.apache.org/api/apidocs-4.17/apis/addTrafficType.html), and use singular?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-2943574820
hi @madhukar93 please resolve the conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1816720443
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1816812610
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7789
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland closed pull request #8151: Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse
URL: https://github.com/apache/cloudstack/pull/8151
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1378379019
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListTrafficTypesCmd.java:
##########
@@ -27,14 +27,13 @@
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.api.response.PhysicalNetworkResponse;
-import org.apache.cloudstack.api.response.ProviderResponse;
import org.apache.cloudstack.api.response.TrafficTypeResponse;
import com.cloud.network.PhysicalNetworkTrafficType;
import com.cloud.user.Account;
import com.cloud.utils.Pair;
-@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = ProviderResponse.class, since = "3.0.0",
+@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = TrafficTypeResponse.class, since = "3.0.0",
Review Comment:
@rohityadavcloud I can revert this, but won't the clients not be able to depend on the fields from `ProviderResponse` anyway, since they aren't set.
@kishankavala I'm not sure how this attribute is used. Cloudmonkey doesn't seem to use it. Is this used for programmatic client generation and docs generation generally, but not for cmk? The one marvin test that uses this API doesn't seem to care about the attributes in the response
This is causing confusion for the reporter in the related issue as well - https://github.com/apache/cloudstack/issues/7837#issuecomment-1672122978
I can revert this and maybe this should exist as a separate issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1784529344
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7562
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1374167540
##########
api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java:
##########
@@ -56,6 +58,17 @@ public class TrafficTypeResponse extends BaseResponse {
@Param(description = "The network name label of the physical device dedicated to this traffic on a HyperV host")
private String hypervNetworkLabel;
+ // why not VLAN_ID
+ @SerializedName(ApiConstants.VLAN)
+ @Param(description = "The VLAN id to be used for Management traffic by VMware host")
+ private String vlan;
+
+
+
Review Comment:
nit - unnecessary new lines
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4997,6 +4997,7 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 && isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation methods, why are multiple isolation methods allowed per pnet? They seem to be filterable by provider/preifx (PhysicalNetwork.java). Documentation says only two are allowed but code and tests contradict this.
Review Comment:
Yes, that would be okay.
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3082,6 +3082,7 @@ public TrafficTypeResponse createTrafficTypeResponse(PhysicalNetworkTrafficType
PhysicalNetwork pnet = ApiDBUtils.findPhysicalNetworkById(result.getPhysicalNetworkId());
if (pnet != null) {
response.setPhysicalNetworkId(pnet.getUuid());
+ response.setIsolationMethods(pnet.getIsolationMethods());
Review Comment:
Worth testing if the serialised API response (json/xml), converts the string list as expected.
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -5148,6 +5148,7 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 && isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation methods, why are multiple isolation methods allowed per pnet? They seem to be filterable by provider/preifx (PhysicalNetwork.java). Documentation says only two are allowed but code and tests contradict this.
Review Comment:
In theory a physical network represents a network device on the phy. host; Something say a eth0 or a bond0 (if it's a team/bond) or a bridge0 (if it's bridge on the hypervisor host), but then a phy network could carry tagged VLAN traffic such as eth0.10, or bridge0-10 carrying VLAN 10 etc. For the same phy. network, a shared network with a different VLAN can be created.
I don't have the historical context to answer why it's the way it is, generally we don't expect more than a single isolation method of a phy network - but some network plugins can decide to do things differently for example support both VLAN, GRE etc. so it's possible the isolate methods is a list. For the sake of the issue you're trying to address, the scope is limited to returning data that's already in the database.
##########
api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java:
##########
@@ -128,4 +141,19 @@ public String getOvm3Label() {
public void setOvm3Label(String ovm3Label) {
this.ovm3NetworkLabel = ovm3Label;
}
+
+ public List<String> getIsolationMethods() {
+ return isolationMethods;
+ }
+
+ public void setIsolationMethods(List<String> isolationMethods) {
+ this.isolationMethods = isolationMethods;
+ }
Review Comment:
nit - missing new line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1833886592
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7874
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-3460110891
@DaanHoogland 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-3466427818
```
(localcloud) π± > list traffictypes physicalnetworkid=30346ff3-00f0-4155-98dd-7ebddfe48d85
{
"count": 2,
"traffictype": [
{
"id": "54c84eac-abb9-41b9-8662-89c5081859a2",
"isolationmethods": "VLAN",
"kvmnetworklabel": "cloudbr1",
"physicalnetworkid": "30346ff3-00f0-4155-98dd-7ebddfe48d85",
"traffictype": "Guest",
"vmwarenetworklabel": "vSwitch1,,vmwaresvs",
"xennetworklabel": "Network1"
},
{
"id": "ec058b01-3536-4cf7-94d1-c237eb8a7f6d",
"isolationmethods": "VLAN",
"kvmnetworklabel": "cloudbr1",
"physicalnetworkid": "30346ff3-00f0-4155-98dd-7ebddfe48d85",
"traffictype": "Public",
"vmwarenetworklabel": "vSwitch1,,vmwaresvs",
"xennetworklabel": "Network1"
}
]
}
(localcloud) π± > list traffictypes physicalnetworkid=f39759cd-3fc0-433b-acb8-c24613a9168e
{
"count": 2,
"traffictype": [
{
"id": "733e8d2e-bec7-4069-9867-3d5e0000d736",
"isolationmethods": "VLAN",
"kvmnetworklabel": "cloudbr0",
"physicalnetworkid": "f39759cd-3fc0-433b-acb8-c24613a9168e",
"traffictype": "Management",
"vmwarenetworklabel": "vSwitch0,,vmwaresvs",
"xennetworklabel": "Network0"
},
{
"id": "090cf3c5-e7aa-46d4-b13d-c12ed5a59edb",
"isolationmethods": "VLAN",
"kvmnetworklabel": "cloudbr0",
"physicalnetworkid": "f39759cd-3fc0-433b-acb8-c24613a9168e",
"traffictype": "Storage",
"vmwarenetworklabel": "vSwitch0,,vmwaresvs",
"xennetworklabel": "Network0"
}
]
}
```
@Pearl1594 , it looks like vlan tags are not returned (vlan://51 for public and vlan://untagged for mgmt). Is this good enough like this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-3484597108
ping @Pearl1594 , how do you suggest we proceed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-3465943537
<b>[SF] Trillian test result (tid-14737)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 63318 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8151-t14737-kvm-ol8.zip
Smoke tests completed. 147 look OK, 1 have errors, 2 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_02_enableHumanReadableLogs | `Error` | 0.36 | test_human_readable_logs.py
all_test_image_store_object_migration | `Skipped` | --- | test_image_store_object_migration.py
all_test_import_unmanage_volumes | `Skipped` | --- | test_import_unmanage_volumes.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-3460335709
@blueorangutan test keepEnv
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1833790182
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1804085407
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7697
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1782472540
> @madhukar93 you can also consider to add a unit test and/or a functional test to cover the API response.
will do after clarifying the schema
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1374211092
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3082,6 +3082,7 @@ public TrafficTypeResponse createTrafficTypeResponse(PhysicalNetworkTrafficType
PhysicalNetwork pnet = ApiDBUtils.findPhysicalNetworkById(result.getPhysicalNetworkId());
if (pnet != null) {
response.setPhysicalNetworkId(pnet.getUuid());
+ response.setIsolationMethods(pnet.getIsolationMethods());
Review Comment:
`cmk` is able to display the array, I've put the output in the PR description if you want to take a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1374223984
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4997,6 +4997,7 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 && isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation methods, why are multiple isolation methods allowed per pnet? They seem to be filterable by provider/preifx (PhysicalNetwork.java). Documentation says only two are allowed but code and tests contradict this.
Review Comment:
> Yes, that would be okay.
to summarize - even though string array is getting serialized correctly (cmk output in description), I will stick to how it is handled currently in the [PhysicalNetworkResponse](https://github.com/apache/cloudstack/blob/a06f8a8763dc0d30b14e41d579ec73d40613e1d9/api/src/main/java/org/apache/cloudstack/api/response/PhysicalNetworkResponse.java#L129) where the methods array is converted into a string csv value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1380417411
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListTrafficTypesCmd.java:
##########
@@ -27,14 +27,13 @@
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.api.response.PhysicalNetworkResponse;
-import org.apache.cloudstack.api.response.ProviderResponse;
import org.apache.cloudstack.api.response.TrafficTypeResponse;
import com.cloud.network.PhysicalNetworkTrafficType;
import com.cloud.user.Account;
import com.cloud.utils.Pair;
-@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = ProviderResponse.class, since = "3.0.0",
+@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = TrafficTypeResponse.class, since = "3.0.0",
Review Comment:
besides the functional test, what kind of tests are required to ensure backward compatibility? @rohityadavcloud you mentioned making the response a superset of new and existing fields, confirming if that is required
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1377498054
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3082,6 +3082,10 @@ public TrafficTypeResponse createTrafficTypeResponse(PhysicalNetworkTrafficType
PhysicalNetwork pnet = ApiDBUtils.findPhysicalNetworkById(result.getPhysicalNetworkId());
if (pnet != null) {
response.setPhysicalNetworkId(pnet.getUuid());
+ // using PhysicalNetworkResponse only to convert isolation methods to string
+ PhysicalNetworkResponse pnetResponse = new PhysicalNetworkResponse();
+ pnetResponse.setIsolationMethods(pnet.getIsolationMethods());
+ response.setIsolationMethods(pnetResponse.getIsolationMethods());
Review Comment:
@DaanHoogland do you think moving this logic to `PhysicalNetwork`, by overloading the getIsolationMethods method. What exists right now is `List<String> getIsolationMethods()`, I'm talking about adding a `String getIsolationMethods()` as well, that returns csv.
I'm not sure why `String.join` isn't used either, I don't think a separate utility is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1374223984
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4997,6 +4997,7 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 && isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation methods, why are multiple isolation methods allowed per pnet? They seem to be filterable by provider/preifx (PhysicalNetwork.java). Documentation says only two are allowed but code and tests contradict this.
Review Comment:
> Yes, that would be okay.
to summarize - even though string array is getting serialized correctly (cmk output in description), I will stick to how it is handled currently in the [PhysicalNetworkResponse](https://github.com/apache/cloudstack/blob/a06f8a8763dc0d30b14e41d579ec73d40613e1d9/api/src/main/java/org/apache/cloudstack/api/response/PhysicalNetworkResponse.java#L129) where the isolation methods array is converted into a string csv value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1375863936
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3082,6 +3082,10 @@ public TrafficTypeResponse createTrafficTypeResponse(PhysicalNetworkTrafficType
PhysicalNetwork pnet = ApiDBUtils.findPhysicalNetworkById(result.getPhysicalNetworkId());
if (pnet != null) {
response.setPhysicalNetworkId(pnet.getUuid());
+ // using PhysicalNetworkResponse only to convert isolation methods to string
+ PhysicalNetworkResponse pnetResponse = new PhysicalNetworkResponse();
+ pnetResponse.setIsolationMethods(pnet.getIsolationMethods());
+ response.setIsolationMethods(pnetResponse.getIsolationMethods());
Review Comment:
this seems to me to indicate that the functionality should be extracted from `PhysicalNetworkResponse` and be put in a utility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1382906864
##########
test/integration/smoke/test_network_traffic_type_api.py:
##########
@@ -0,0 +1,50 @@
+from marvin.cloudstackTestCase import cloudstackTestCase
Review Comment:
this will need a license
```suggestion
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
# Import Local Modules
from marvin.cloudstackTestCase import cloudstackTestCase
```
##########
test/integration/smoke/test_network_traffic_type_api.py:
##########
@@ -0,0 +1,50 @@
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.base import (
+ TrafficType,
+ PhysicalNetwork,
+ Zone,
+)
+from nose.plugins.attrib import attr
+
+
+class TestTrafficTypeApi(cloudstackTestCase):
+ @classmethod
+ def setUpClass(cls):
+ cls.test_client = super(TestTrafficTypeApi, cls).getClsTestClient()
+ cls.api_client = cls.testClient.getApiClient()
+ cls.services = cls.testClient.getParsedTestDataConfig()
+ cls._cleanup = []
+ cls.services["advanced_sg"]["zone"]["name"] = "TestTrafficTypeApi-zone"
+ cls.zone = Zone.create(cls.api_client, cls.services["advanced_sg"]["zone"])
+ cls._cleanup.append(cls.zone)
+
+ cls.physical_network = PhysicalNetwork.create(
+ cls.api_client,
+ cls.services["l2-network"],
+ isolationmethods="VLAN",
+ zoneid=cls.zone.id,
+ )
+ cls._cleanup.append(cls.physical_network)
Review Comment:
this will need a
```suggestion
cls._cleanup.append(cls.physical_network)
@classmethod
def tearDownClass(cls):
super(TestTrafficTypeApi, cls).tearDownClass()
```
to work
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1805521087
<b>[SF] Trillian test result (tid-8295)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 53983 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8151-t8295-kvm-centos7.zip
Smoke tests completed. 116 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_invalid_upgrade_kubernetes_cluster | `Failure` | 3612.96 | test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster | `Failure` | 3605.83 | test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.03 | test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster | `Failure` | 49.72 | test_kubernetes_clusters.py
test_11_test_unmanaged_cluster_lifecycle | `Error` | 1.21 | test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 65.65 | test_kubernetes_clusters.py
test_05_vmschedule_test_e2e | `Failure` | 361.65 | test_vm_schedule.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1798875082
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1817423810
<b>[SF] Trillian test result (tid-8363)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48520 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8151-t8363-kvm-centos7.zip
Smoke tests completed. 118 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_07_deploy_kubernetes_ha_cluster | `Failure` | 3609.78 | test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 86.89 | test_kubernetes_clusters.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1782432567
@madhukar93 you can also consider to add a unit test and/or a functional test to cover the API response.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1374217109
##########
api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java:
##########
@@ -56,6 +58,17 @@ public class TrafficTypeResponse extends BaseResponse {
@Param(description = "The network name label of the physical device dedicated to this traffic on a HyperV host")
private String hypervNetworkLabel;
+ // why not VLAN_ID
Review Comment:
left it as a note to myself, but thank you for the clarification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-3460303707
Packaging result [SF]: βοΈ el8 βοΈ el9 βοΈ el10 βοΈ debian βοΈ suse15. SL-JID 15581
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1842674623
@vishesh92 @harikrishna-patnala @kishankavala are your concerns addressed. Can you please re-review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1374223984
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4997,6 +4997,7 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 && isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation methods, why are multiple isolation methods allowed per pnet? They seem to be filterable by provider/preifx (PhysicalNetwork.java). Documentation says only two are allowed but code and tests contradict this.
Review Comment:
> Yes, that would be okay.
to summarize - even though string array is getting serialized correctly, I will stick to how it is handled currently in the [PhysicalNetworkResponse](https://github.com/apache/cloudstack/blob/a06f8a8763dc0d30b14e41d579ec73d40613e1d9/api/src/main/java/org/apache/cloudstack/api/response/PhysicalNetworkResponse.java#L129) where the methods array is converted into a string csv value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "kishankavala (via GitHub)" <gi...@apache.org>.
kishankavala commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1375680616
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListTrafficTypesCmd.java:
##########
@@ -27,14 +27,13 @@
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.api.response.PhysicalNetworkResponse;
-import org.apache.cloudstack.api.response.ProviderResponse;
import org.apache.cloudstack.api.response.TrafficTypeResponse;
import com.cloud.network.PhysicalNetworkTrafficType;
import com.cloud.user.Account;
import com.cloud.utils.Pair;
-@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = ProviderResponse.class, since = "3.0.0",
+@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = TrafficTypeResponse.class, since = "3.0.0",
Review Comment:
@madhukar93 listTrafficTypes returning TrafficTypeResponse is the right thing to do. But, how do you ensure backward compatibility?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1373104999
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4997,6 +4997,7 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 && isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation methods, why are multiple isolation methods allowed per pnet? They seem to be filterable by provider/preifx (PhysicalNetwork.java). Documentation says only two are allowed but code and tests contradict this.
Review Comment:
I see [checks](https://github.com/apache/cloudstack/blob/a06f8a8763dc0d30b14e41d579ec73d40613e1d9/server/src/main/java/com/cloud/network/NetworkServiceImpl.java#L3947C13-L3947C22) to ensure only one isolation method can exist at this time. @rohityadavcloud @DaanHoogland I would like to confirm if the API field should be plural for futureproofing, or should we maintain parity with with [addTrafficType](https://cloudstack.apache.org/api/apidocs-4.17/apis/addTrafficType.html), and use singular?
There seem to be [legitimate cases](https://github.com/apache/cloudstack/blob/a06f8a8763dc0d30b14e41d579ec73d40613e1d9/plugins/network-elements/nicira-nvp/src/test/java/com/cloud/network/guru/NiciraNvpGuestNetworkGuruTest.java#L117C1-L117C1) where we can have multiple values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1374167213
##########
api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java:
##########
@@ -56,6 +58,17 @@ public class TrafficTypeResponse extends BaseResponse {
@Param(description = "The network name label of the physical device dedicated to this traffic on a HyperV host")
private String hypervNetworkLabel;
+ // why not VLAN_ID
Review Comment:
@madhukar93 sorry didn't get your comment; if it's by use of the term - vlan and vlan-id are interchangeable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1378585674
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3082,6 +3082,10 @@ public TrafficTypeResponse createTrafficTypeResponse(PhysicalNetworkTrafficType
PhysicalNetwork pnet = ApiDBUtils.findPhysicalNetworkById(result.getPhysicalNetworkId());
if (pnet != null) {
response.setPhysicalNetworkId(pnet.getUuid());
+ // using PhysicalNetworkResponse only to convert isolation methods to string
+ PhysicalNetworkResponse pnetResponse = new PhysicalNetworkResponse();
+ pnetResponse.setIsolationMethods(pnet.getIsolationMethods());
+ response.setIsolationMethods(pnetResponse.getIsolationMethods());
Review Comment:
@madhukar93, I am not sure about the dependency tree for responses but importing a response for the sole purpose of getting a field well formatted for another response seems a bit dodgy to me.
Overloading with only a different return value is not going to work.
I think a dependency for both `PhysicalNetworkResponse` and `TrafficTypeResponse` should be found or created and the logic moved there.
Just calling a `String.join()` would work as well, but a common method to use in all places that need this formatting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1816718309
@harikrishna-patnala @kishankavala @rohityadavcloud @vishesh92 are your comments addressed?
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1816822594
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1798876905
@DaanHoogland 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1799526747
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1803946636
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1833996870
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1378652762
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListTrafficTypesCmd.java:
##########
@@ -27,14 +27,13 @@
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.api.response.PhysicalNetworkResponse;
-import org.apache.cloudstack.api.response.ProviderResponse;
import org.apache.cloudstack.api.response.TrafficTypeResponse;
import com.cloud.network.PhysicalNetworkTrafficType;
import com.cloud.user.Account;
import com.cloud.utils.Pair;
-@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = ProviderResponse.class, since = "3.0.0",
+@APICommand(name = "listTrafficTypes", description = "Lists traffic types of a given physical network.", responseObject = TrafficTypeResponse.class, since = "3.0.0",
Review Comment:
@rohityadavcloud @kishankavala
# tl;dr;
We will need to make this change. Because cloudstack-go is currently based on output of `listApis` response and having a wrong response here will generate an incorrect client and terraform provider won't be able to use the keys we add in this PR.
Only keys which are present in both `ProviderResponse` & `TrafficTypeResponse` are accessible as of now using the cloudstack-go client. So, I don't think backward compatibility will be an issue here.
## Details
In cloudstack-go client's TrafficType struct, it has some keys which are not part of `listTrafficType` response and missing keys which should be in the response.
Ref: https://github.com/apache/cloudstack-go/blob/c9c595770d7dcc2c87090227ef15c94ceee27b2b/cloudstack/UsageService.go#L1092-L1107
This is the difference in output of cmk & cloudstack-go:
cmk
```
{
"count": 2,
"traffictype": [
{
"id": "04q1bbe1-0e1e-4a1a-94e3-btcd257z44b9",
"kvmnetworklabel": "cloudbr0",
"physicalnetworkid": "47191b54-510c-475e-va6f-d935e22a9744",
"traffictype": "Management",
"vmwarenetworklabel": "vSwitch0,,vmwaresvs",
"xennetworklabel": "Network0"
}
]
}
```
cloudstack-go
```
cloudstack.TrafficType{
Canenableindividualservice: false,
Destinationphysicalnetworkid: "",
Id: "04q1bbe1-0e1e-4a1a-94e3-btcd257z44b9",
JobID: "",
Jobstatus: 0,
Name: "",
Physicalnetworkid: "47191b54-510c-475e-va6f-d935e22a9744",
Servicelist: []string(nil),
State: ""
}
```
As you can see there are some keys (`traffictype`, `kvmnetworklabel`, etc.) which are present in response but we can't access them using cloudstack-go client because it's not part of the struct.
code to listTrafficType using cloudstack-go
```go
func main() {
cs := cloudstack.NewAsyncClient(
"http://localhost:8080/client/api",
"access-key",
"secret-key",
false,
)
p := cs.Usage.NewListTrafficTypesParams("47191b54-510c-475e-va6f-d935e22a9744");
r, err := cs.Usage.ListTrafficTypes(p)
if err != nil {
log.Fatalf("%+v", err)
}
fmt.Printf("%#v\n", *r.TrafficTypes[0])
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1374211092
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3082,6 +3082,7 @@ public TrafficTypeResponse createTrafficTypeResponse(PhysicalNetworkTrafficType
PhysicalNetwork pnet = ApiDBUtils.findPhysicalNetworkById(result.getPhysicalNetworkId());
if (pnet != null) {
response.setPhysicalNetworkId(pnet.getUuid());
+ response.setIsolationMethods(pnet.getIsolationMethods());
Review Comment:
`cmk` is able to display the array, I've put the output in the PR description if you want to take a look. I will check the serialization via a marvin test as well.
edit: the serialization was working, but I have now converted the response to csv string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "harikrishna-patnala (via GitHub)" <gi...@apache.org>.
harikrishna-patnala commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1784481668
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1803976057
@vishesh92 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1385073127
##########
test/integration/smoke/test_network_traffic_type_api.py:
##########
@@ -0,0 +1,73 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Import Local Modules
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.base import (
+ TrafficType,
+ PhysicalNetwork,
+ Zone,
+)
+from nose.plugins.attrib import attr
+
+
+class TestTrafficTypeApi(cloudstackTestCase):
+ @classmethod
+ def setUpClass(cls):
+ cls.test_client = super(TestTrafficTypeApi, cls).getClsTestClient()
+ cls.api_client = cls.testClient.getApiClient()
+ cls.services = cls.testClient.getParsedTestDataConfig()
+ cls._cleanup = []
+ cls.services["advanced_sg"]["zone"]["name"] = "TestTrafficTypeApi-zone"
+ cls.zone = Zone.create(cls.api_client, cls.services["advanced_sg"]["zone"])
+ cls._cleanup.append(cls.zone)
+
+ cls.physical_network = PhysicalNetwork.create(
+ cls.api_client,
+ cls.services["l2-network"],
+ isolationmethods="VLAN",
+ zoneid=cls.zone.id,
+ )
+ cls._cleanup.append(cls.physical_network)
+
+ @classmethod
+ def tearDownClass(cls):
+ super(TestTrafficTypeApi, cls).tearDownClass()
Review Comment:
@DaanHoogland I was afk, should've vetted better π
##########
test/integration/smoke/test_network_traffic_type_api.py:
##########
@@ -0,0 +1,73 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Import Local Modules
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.base import (
+ TrafficType,
+ PhysicalNetwork,
+ Zone,
+)
+from nose.plugins.attrib import attr
+
+
+class TestTrafficTypeApi(cloudstackTestCase):
+ @classmethod
+ def setUpClass(cls):
+ cls.test_client = super(TestTrafficTypeApi, cls).getClsTestClient()
+ cls.api_client = cls.testClient.getApiClient()
+ cls.services = cls.testClient.getParsedTestDataConfig()
+ cls._cleanup = []
+ cls.services["advanced_sg"]["zone"]["name"] = "TestTrafficTypeApi-zone"
+ cls.zone = Zone.create(cls.api_client, cls.services["advanced_sg"]["zone"])
+ cls._cleanup.append(cls.zone)
+
+ cls.physical_network = PhysicalNetwork.create(
+ cls.api_client,
+ cls.services["l2-network"],
+ isolationmethods="VLAN",
+ zoneid=cls.zone.id,
+ )
+ cls._cleanup.append(cls.physical_network)
+
+ @classmethod
+ def tearDownClass(cls):
+ super(TestTrafficTypeApi, cls).tearDownClass()
Review Comment:
@DaanHoogland I was afk, should've vetted better π
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1801364414
<b>[SF] Trillian test result (tid-8263)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 50196 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8151-t8263-kvm-centos7.zip
Smoke tests completed. 117 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_create_iso_with_checksum_sha1 | `Error` | 66.46 | test_iso.py
test_02_create_iso_with_checksum_sha256 | `Error` | 66.45 | test_iso.py
test_03_create_iso_with_checksum_md5 | `Error` | 66.45 | test_iso.py
test_04_create_iso_with_no_checksum | `Error` | 66.45 | test_iso.py
test_01_create_iso | `Failure` | 1514.32 | test_iso.py
ContextSuite context=TestISO>:setup | `Error` | 3027.76 | test_iso.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1373145753
##########
api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java:
##########
@@ -56,6 +58,17 @@ public class TrafficTypeResponse extends BaseResponse {
@Param(description = "The network name label of the physical device dedicated to this traffic on a HyperV host")
private String hypervNetworkLabel;
+ // why not VLAN_ID
+ @SerializedName(ApiConstants.VLAN)
+ @Param(description = "The VLAN id to be used for Management traffic by VMware host")
+ private String vlan;
+
Review Comment:
remove whitespace
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1373104999
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4997,6 +4997,7 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 && isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation methods, why are multiple isolation methods allowed per pnet? They seem to be filterable by provider/preifx (PhysicalNetwork.java). Documentation says only two are allowed but code and tests contradict this.
Review Comment:
I see [checks](https://github.com/apache/cloudstack/blob/main/server/src/main/java/com/cloud/network/NetworkServiceImpl.java#L3824-L3824) to ensure only one isolation method can exist at this time. @rohityadavcloud @DaanHoogland I would like to confirm if the API field should be plural for futureproofing, or should we maintain parity with with [addTrafficType](https://cloudstack.apache.org/api/apidocs-4.17/apis/addTrafficType.html), and use singular?
There seem to be [legitimate cases](https://github.com/apache/cloudstack/blob/a06f8a8763dc0d30b14e41d579ec73d40613e1d9/plugins/network-elements/nicira-nvp/src/test/java/com/cloud/network/guru/NiciraNvpGuestNetworkGuruTest.java#L117C1-L117C1) where we can have multiple values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "madhukar93 (via GitHub)" <gi...@apache.org>.
madhukar93 commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1373204412
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4997,6 +4997,7 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 && isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation methods, why are multiple isolation methods allowed per pnet? They seem to be filterable by provider/preifx (PhysicalNetwork.java). Documentation says only two are allowed but code and tests contradict this.
Review Comment:
Physical network response turns isolation methods into a string csv value, should we handle it similarly here? -
```
list physicalnetworks id=a0fc1b59-ae44-42a3-a1f5-9bbb6d83a731
{
"count": 1,
"physicalnetwork": [
{
"broadcastdomainrange": "ZONE",
"id": "a0fc1b59-ae44-42a3-a1f5-9bbb6d83a731",
"isolationmethods": "VLAN",
"name": "Sandbox-pnet",
"state": "Enabled",
"vlan": "100-200",
"zoneid": "279434a2-4b78-4858-b714-69ee93df0db2",
"zonename": "Sandbox-simulator"
}
]
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#issuecomment-1782446343
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/8151?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#8151](https://app.codecov.io/gh/apache/cloudstack/pull/8151?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (0b1950b) into [main](https://app.codecov.io/gh/apache/cloudstack/commit/a06f8a8763dc0d30b14e41d579ec73d40613e1d9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a06f8a8) will **decrease** coverage by `24.48%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #8151 +/- ##
============================================
- Coverage 29.26% 4.78% -24.48%
============================================
Files 5127 346 -4781
Lines 361479 26150 -335329
Branches 52802 4497 -48305
============================================
- Hits 105771 1252 -104519
+ Misses 241194 24763 -216431
+ Partials 14514 135 -14379
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/8151/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [simulator-marvin-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.78% <ΓΈ> (ΓΈ)` | |
| [unit-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
[see 4781 files with indirect coverage changes](https://app.codecov.io/gh/apache/cloudstack/pull/8151/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: Weβre building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland merged PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org