Uh oh!
There was an error while loading. Please reload this page.
[4.10] CLOUDSTACK-7985: assignVM in Advanced zone with Security Groups - #844
Conversation
asfbot
commented
Sep 16, 2015
cloudstack-pull-rats #636 SUCCESS |
asfbot
commented
Sep 16, 2015
cloudstack-pull-analysis #575 UNSTABLE |
miguelaferreira
commented
Sep 16, 2015
Apache Jenkins build reports 5 failing tests (https://builds.apache.org/job/cloudstack-pull-analysis/575/testReport/) I'm seeing the same tests failing in other PRs, and given that recently master has been broken quite often, it might be that the failing tests have nothing to do with this PR. |
DaanHoogland
commented
Sep 17, 2015
@ustcweizhou@miguelaferreira@karuturi@remibergsma so no merges till this is fixed? |
wilderrodrigues
commented
Sep 17, 2015
Which tests did you execute? I would say: no merge until it's fixed, tested by @ustcweizhou and he let us know how we should be able to test. Cheers, |
DaanHoogland
commented
Sep 17, 2015
@wilderrodrigues that is not fair, it is not @ustcweizhou 's bug that needs fixing! |
miguelaferreira
commented
Sep 17, 2015
@DaanHoogland@wilderrodrigues@ustcweizhou I agree with Daan, the problem is most likely elsewhere. But I also agree with Wilder, first we need to figure out where the problem is, fix it, rebase all PRs to branch off after the fix and then continue the normal process. We can't be merging things hoping all will be well. @ustcweizhou could you please try rebasing on master to see if the build for this PR still fails? it would be something like this: Where upstream is the https://github.com/apache/cloudstack remote |
4896e47 to
2cf3c7cCompareasfbot
commented
Sep 17, 2015
cloudstack-pull-rats #644 SUCCESS |
miguelaferreira
commented
Sep 17, 2015
Thanks for rebasing @ustcweizhou The pull-analysis is now running. |
ustcweizhou
commented
Sep 17, 2015
@DaanHoogland@miguelaferreira@wilderrodrigues |
miguelaferreira
commented
Sep 17, 2015
The build job already passed the point it had failed before (fingers-crossed) |
asfbot
commented
Sep 17, 2015
cloudstack-pull-analysis #583 SUCCESS |
wilderrodrigues
commented
Sep 17, 2015
Hi @ustcweizhou , I will 👎 this PR until we get to know which integration tests you executed, against which environment and the steps so we can do the same. Cheers, |
ustcweizhou
commented
Sep 23, 2015
test cases |
ustcweizhou
commented
Oct 5, 2015
guys, anyone has update ? |
wilderrodrigues
commented
Oct 7, 2015
Hi @ustcweizhou Thanks for the details. Are there some integration tests that we could use to cover the cases you described? I can runt the following tests to make sure it doesn't break existing features: But I'm not sure if those tests will cover all of your changes. Cheers, |
wilderrodrigues
commented
Oct 7, 2015
Hi @ustcweizhou@DaanHoogland@remibergsma@karuturi@miguelaferreira I executed the tests on my side. Despite some exceptions with network cleanup and the VPC tests that will pass only after PR #908 gets merged, everything went fine. This PR L1/2GTM (Look 1/2 Good to Me) taking into account that it doesn't break existing features. I need more time to tests the security groups bit - or @ustcweizhou can test it manually, or perhaps someone else can do help on that. Results: |
ustcweizhou
commented
Oct 9, 2015
@wilderrodrigues thanks for you testing. I believe this PR will not impact the current network/vr/storage. |
miguelaferreira
commented
Oct 9, 2015
@ustcweizhou may I ask you if you believe that the change in this PR will not impact the areas you mention because you've tested it (even if manually)? Or because you assume that this change is isolated from hose areas of the system? We have had production issues in the recent past due to "isolated" changes to plugins for hypervisors we don't even use. Therefore, the urge for testing! |
ustcweizhou
commented
Oct 12, 2015
@miguelaferreira I have tested the test cases listed above. I think it is isolated from other areas. I agree more and careful testing are better. Can you show more details in your issues so maybe we can get some lessons from them? |
miguelaferreira
commented
Oct 12, 2015
@ustcweizhou It is not clear to me if you tested the same things @wilderrodrigues did, or if you tested "the security groups bit". Would you be so kind as to make that explicit. Regarding the remark I made about seemingly isolated code: the PR that fixed the issue we had in production is #761. If you are interested, please have a look at the problem description and solution (arguably no the best!) in the PR comments. |
ustcweizhou
commented
Oct 12, 2015
|
miguelaferreira
commented
Oct 12, 2015
Thanks for clarifying @ustcweizhou |
ustcweizhou
commented
Oct 12, 2015
no problem for me. |
DaanHoogland
commented
Oct 19, 2015
@miguelaferreira@remibergsma are you satisfied with this PR, now? |
miguelaferreira
commented
Oct 19, 2015
@DaanHoogland I think the status is still the same as of 7 days ago. Or am I missing something? |
DaanHoogland
commented
Oct 19, 2015
So that means the SG tests are missing still. Ok, I will add it to my testing backlog. |
2cf3c7c to
c497fddCompareDaanHoogland
commented
Oct 31, 2015
did the regular regression suite in my sbp provided bubble and all passed. network: and vpc: the skip is because for migration at least 2 hosts are needed LGTM (limitation of warranty: security groups are not covered by the standard regression tests) I will run these test in an advanced zone with sg enabled as well, time permitting. next specific tests are to be coded for ensuring sg. For now Wei's personal testing should do. |
e523ffd to
1fd66d6Compare1fd66d6 to
59db8a9Compare5e1a2b2 to
c6558caCompareyadvr
commented
Jan 27, 2016
@ustcweizhou Can you please meld the commits into a single commit, and incorporate any remaining changes asked in the review? Ping, can we have a re-review |
c6558ca to
e89bb2dComparee89bb2d to
b13148bCompareustcweizhou
commented
Mar 9, 2016
rebased, and fixed an issue in assignvm to project. |
| message.alert.state.detected=Alert state detected | ||
| message.allow.vpn.access=Please enter a username and password of the user that you want to allow VPN access. | ||
| message.apply.snapshot.policy=You have successfully updated your current snapshot policy. | ||
| message.assign.instance.another=Please specify the account type, domain, account name and network (optional) of the new account. <br> If the default nic of the vm is on a shared network, CloudStack will check if the network can be used by the new account if you do not specify one network. <br> If the default nic of the vm is on a isolated network, and the new account has more one isolated networks, you should specify one. |
There was a problem hiding this comment.
Hi @ustcweizhou, just pointing a small typo: "on a isolated network" => "on an isolated network". Thanks.
yadvr
commented
May 2, 2016
@ustcweizhou please rebase against latest master and push -f, update on status of your PR |
bvbharatk
commented
Jun 3, 2016
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no):https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests: Skipped tests: Passed test suits: |
b13148b to
ab0d04dCompareustcweizhou
commented
Dec 27, 2016
rebased with latest master |
DaanHoogland
commented
Feb 8, 2017
@blueorangutan package |
blueorangutan
commented
Feb 8, 2017
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Feb 8, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-475 |
DaanHoogland
commented
Feb 8, 2017
@blueorangutan test |
blueorangutan
commented
Feb 8, 2017
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Feb 8, 2017
Trillian test result (tid-807)
|
karuturi
commented
Feb 9, 2017
merging |
[4.10] CLOUDSTACK-7985: assignVM in Advanced zone with Security GroupsThis commit contains the following changes: (1) implementation of assignVM in Advanced zone with Security Groups (2) keep the default nic on shared network when assignVM (3) allow migrate vm from/to project; (4) UI change for selecting account/project/network * pr/844: CLOUDSTACK-7985: assignVM in Advanced zone with Security Groups CLOUDSTACK-7985: keep the default nic on shared network when assignVM CLOUDSTACK-7985: (1) allow migrate vm from/to project; (2) UI change for selecting account/project/network Signed-off-by: Rajani Karuturi <rajani.karuturi@accelerite.com>

This commit contains the following changes:
(1) implementation of assignVM in Advanced zone with Security Groups
(2) keep the default nic on shared network when assignVM
(3) allow migrate vm from/to project;
(4) UI change for selecting account/project/network