GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Vpcr Marvin test and some fixes for vprc by isoutham · Pull Request #558 · apache/cloudstack

Conversation

@isoutham

@isoutham isoutham commented Jul 2, 2015

Copy link
Copy Markdown
Contributor

A combined commit because otherwise it would not make sense.

If I commit the fixes alone there is not test to verify them
If I commit the tests alone it will not work because it found bugs

So both together

The test test_vpc_redundant.py is added (requires hardware) together with the following fixes.

  1. Fix incorrect SNAT configuration (was adding SNAT to interfaces not marked as nat in the json config).
  2. Fix interface used for keepalived multicast traffic to be lowest guest i/f. This stops some unnecessary keepalived flip-flops
  3. If all nat rules are removed and re-added public interfaces would stay down (because master status does not change and CS deletes the interface)
  4. Small change to Marvin to allow a test to change the retries count (is downwardly compatible).
  5. A couple of pep8 things.
  6. Improved locking during master/backup transitions.

The test still very occasionally fails with a (correct) double master detection. When I have worked out why, I will produce another PR. It appears to be a timing issue and could be tricky to find.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Ian Southam added 3 commits July 2, 2015 08:15
Ian Southam
New test and fixes for vpcr code in 4.6
b2cf51c
Ian Southam
Fixes to enable test to succeed. Small change to Marvin to be able to…
6989a17 
… override retries
Ian Southam
Forgot to activate teardown
73e654e
@wilderrodrigues

wilderrodrigues commented Jul 3, 2015

Copy link
Copy Markdown
Contributor

Nice one, @isoutham !

I will test it this week and also try to get someone else to have a look so we get the 2 LGTM needed to merge.

Thanks!

Cheers,
Wilder
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@wilderrodrigues

wilderrodrigues commented Jul 3, 2015

Copy link
Copy Markdown
Contributor

Travis failed due timeout... so, that's not relevant and won't be taken into account when reviewing the PR:

No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

The build has been terminated

Cheers,
Wilder
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@remibergsma

remibergsma commented Jul 3, 2015

Copy link
Copy Markdown
Contributor

@isoutham @wilderrodrigues is there a way to trigger the build again?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@wilderrodrigues

wilderrodrigues commented Jul 3, 2015

Copy link
Copy Markdown
Contributor

Only if something get pushed to the branch which generated the PR.

Only 1 job out of 9 timed out... I wouldn't bother.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sanju1010
sanju1010 reviewed Jul 6, 2015
View reviewed changes
Comment thread test/integration/component/test_vpc_redundant.py

@sanju1010 sanju1010 Jul 6, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you remove this Services class and leverage the test data available @ tools/marvin/marvin/config/test_data.py file? You can refer to any tests available under test/integration/smoke/*.py on how to read test data from test_data.py config file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@wilderrodrigues wilderrodrigues Jul 6, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Hi @sanju1010

@isoutham is currently on holidays and I would like to test and get this PR through before he is back.

I agree with the improvement you suggested, however it's not a blocking thing for the PR because it's only related to the marvin test.

Once his PR get's through, I will proceed with removing the Services class and use test_data.py instead. A new PR will be created afterwards.

Cheers,
Wilder

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@wilderrodrigues

wilderrodrigues commented Jul 6, 2015

Copy link
Copy Markdown
Contributor

Hi @isoutham

Unfortunately the test is failing. Details about my environment and results below:

Management Server running on CentOS 7.1
Multi-host zone: 2 XenServer 6.2 hosts

It all starts fine: rVPC, tiers and VMs are created. After this...

self.delete_nat_rules()
self.check_master_status(1)
self.do_vpc_test(True)

All the tests work. However, the call to start the router doesnt

self.start_router()
self.add_nat_rules()
time.sleep(60)
self.check_master_status(2)
self.do_vpc_test(False)

The router never starts and the rules are not applied. I tried to give it a chance and restarted it manually (yes, deliberate action). After the router was running again, we got timeouts.

I checked the IPs and found out they had no PF rules to port 22, although the ACLs were properly configured. I added the PF rules and after a while I got:

===SSH to Host 192.168.23.8 port : 22 SUCCESSFUL===

I also connected to the VM manually"

Last login: Mon Jul 6 12:13:11 on ttys004
sbpltk1zffh04:asf_cloudstack wrodrigues$ ping 192.168.23.7
PING 192.168.23.7 (192.168.23.7): 56 data bytes
64 bytes from 192.168.23.7: icmp_seq=0 ttl=63 time=4.225 ms
64 bytes from 192.168.23.7: icmp_seq=1 ttl=63 time=4.085 ms
^C
--- 192.168.23.7 ping statistics ---
2 packets transmitted, 2 packets received, 0.0% packet loss
round-trip min/avg/max/stddev = 4.085/4.155/4.225/0.070 ms
sbpltk1zffh04:asf_cloudstack wrodrigues$ ssh [email protected]
The authenticity of host '192.168.23.7 (192.168.23.7)' can't be established.
RSA key fingerprint is c5:d3:aa:6e:09:b2:c7:6d:41:dd:ea:6c:4c:22:72:80.
Are you sure you want to continue connecting (yes/no)? yes
Warning: Permanently added '192.168.23.7' (RSA) to the list of known hosts.
[email protected]'s password:

ls /

bin boot dev etc home lib lib64 linuxrc lost+found media mnt opt proc root run sbin sys tmp usr var

date

Mon Jul 6 13:48:23 UTC 2015

Due to the previous failures, the test failed anyway.

I will have a closer look at the changes and try to get it to work - will probably be part of our next Sprint.

Cheers,
Wilder
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@wilderrodrigues

wilderrodrigues commented Jul 8, 2015

Copy link
Copy Markdown
Contributor

@DaanHoogland @remibergsma @karuturi

It LGTM.

There is still 1 issue, that was also mentioned by Ian, but I'm fixing that.

The details can be found here: https://issues.apache.org/jira/browse/CLOUDSTACK-8616

Since Ian is on holidays, I would like to get this PR merged so I can apply the fixes based on his work and create a separate PR.

Would one of you help me with that? :)

Thanks in advance.

Cheers,
Wilder
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DaanHoogland
DaanHoogland reviewed Jul 8, 2015
View reviewed changes
Comment thread systemvm/patches/debian/config/opt/cloud/bin/cs/CsAddress.py

@DaanHoogland DaanHoogland Jul 8, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

can we remove these lines and not leave code in comment?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@wilderrodrigues wilderrodrigues Jul 8, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

+1

Can I do all that in the new PR along with the fixes?

I mean... I will have 1 commit removing all the commented out lines. Is it okay?

:)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@DaanHoogland DaanHoogland Jul 8, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You can but you can also make a new pr inlcuding @isoutham code (with attribution intact) and add ammending commits
My worry is: will this break any existing tests. prod code is safe so I won't -1, just looking for loopholes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@wilderrodrigues wilderrodrigues Jul 8, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The idea is to get his PR merged, then I will apply the fixes, remove the commented lines, test and create a new PR.

No, the changes above won't break any existing tests. The redundant VPC has just 1 test till now, which tests the offering and creation of rVPC - it was added by @afornie. @isoutham is adding the second test from scratch, so that won't break anything.

Cheers,
Wilder

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@DaanHoogland DaanHoogland Jul 8, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

ok, clear. you got my LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@DaanHoogland

DaanHoogland commented Jul 8, 2015

Copy link
Copy Markdown
Contributor

L reasonably GTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@asfgit asfgit closed this in 820a406 Jul 8, 2015
@wilderrodrigues

wilderrodrigues commented Jul 8, 2015

Copy link
Copy Markdown
Contributor

Thanks, @DaanHoogland

Will proceed from here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
yadvr pushed a commit that referenced this pull request Jan 20, 2021
Hoang Nguyen @yadvr
compute: Fix bug missing Security Group in Advanced Zone with SG enab…
79078ac 
…led (#558)

* Fix bug missing Security Group in Advanced Zone
1. Add a new step security group in a zone with SG enabled.
2. Fix error of displaying sshKeypair data incorrectly

* remove `this.hypervisor!=='KVM'` condition.

Signed-off-by: Rohit Yadav <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@isoutham @wilderrodrigues @remibergsma @DaanHoogland @sanju1010
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.