Skip to content

Persist vpn connection state before restarting#4635

Merged
nvazquez merged 1 commit intoapache:mainfrom
ravening:persist-vpn-state
Sep 16, 2021
Merged

Persist vpn connection state before restarting#4635
nvazquez merged 1 commit intoapache:mainfrom
ravening:persist-vpn-state

Conversation

@ravening
Copy link
Copy Markdown
Member

@ravening ravening commented Feb 1, 2021

Description

If the vpn connection is in pending state then we cant restart the
vpn connection. So manually set the state to disconnected and then
try to restart the vpn connection

This pr is needed along with https://github.com/apache/cloudstack/pull/4429/files

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

@DaanHoogland
Copy link
Copy Markdown
Contributor

@ravening did you try this while state == pending or while state == disconnected? it seems a bit brute force to me.
Also #4429 is on 4.14, would you rather point this to 4.14 as well?

@ravening
Copy link
Copy Markdown
Member Author

ravening commented Feb 1, 2021

@ravening did you try this while state == pending or while state == disconnected? it seems a bit brute force to me.

Also #4429 is on 4.14, would you rather point this to 4.14 as well?

@DaanHoogland this was when state is pending. We need to persist state in DB so that the connection is restarted. If we don't persist then check fails and connection is not restarted

The other pr is already in 4.16. so this can go-to 4.16 as well

@DaanHoogland
Copy link
Copy Markdown
Contributor

@ravening did you try this while state == pending or while state == disconnected? it seems a bit brute force to me.
Also #4429 is on 4.14, would you rather point this to 4.14 as well?

@DaanHoogland this was when state is pending. We need to persist state in DB so that the connection is restarted. If we don't persist then check fails and connection is not restarted

The other pr is already in 4.16. so this can go-to 4.16 as well

the other pr is also in 4.14, so if this only goes into 4.16 do we then have half a fix in the LTS versions?

@ravening
Copy link
Copy Markdown
Member Author

ravening commented Feb 1, 2021

@ravening did you try this while state == pending or while state == disconnected? it seems a bit brute force to me.
Also #4429 is on 4.14, would you rather point this to 4.14 as well?

@DaanHoogland this was when state is pending. We need to persist state in DB so that the connection is restarted. If we don't persist then check fails and connection is not restarted
The other pr is already in 4.16. so this can go-to 4.16 as well

the other pr is also in 4.14, so if this only goes into 4.16 do we then have half a fix in the LTS versions?

good point. i will raise for 4.14 as well

@shwstppr shwstppr added this to the 4.16.0.0 milestone Feb 8, 2021
@yadvr yadvr changed the base branch from master to 4.15 March 6, 2021 09:25
@yadvr yadvr changed the base branch from 4.15 to master March 6, 2021 09:25
_vpnConnectionDao.persist(conn);

// Stop and start the connection again
stopVpnConnection(id);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would without checking for the current conn state cause any issue @ravening ? For example, try to stop vpn connection when VR has errors is powered off etc or in other transitional state?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@rhtyd probably no as this involves updating the state in db

Copy link
Copy Markdown
Contributor

@sureshanaparti sureshanaparti left a comment

Choose a reason for hiding this comment

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

CLGTM

}
// Set vpn state to disconnected
conn.setState(State.Disconnected);
_vpnConnectionDao.persist(conn);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why not just persist DB only when state is pending ?

        if (conn.getState() == State.Pending) {
            conn.setState(State.Disconnected);
            _vpnConnectionDao.persist(conn);
        }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@weizhouapache since we are restting the connection, it should transit to disconnect state irrecpective of the previous state. since we are stopping and starting the vpn connection, checking for pending state or any other state doesnt matter

@nvazquez
Copy link
Copy Markdown
Contributor

nvazquez commented Sep 3, 2021

Ping @ravening can you address the open comments and fix conflicts? Thanks

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Sep 8, 2021

Ping @ravening

If the vpn connection is in pending state then we cant restart the
vpn connection. So manually set the state to disconnected and then
try to restart the vpn connection
@ravening
Copy link
Copy Markdown
Member Author

ravening commented Sep 8, 2021

Ping @ravening

@rhtyd done

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Sep 14, 2021

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

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

@blueorangutan
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

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

code lgtm

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Sep 15, 2021

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

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

@blueorangutan
Copy link
Copy Markdown

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

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Sep 15, 2021

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

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

@blueorangutan
Copy link
Copy Markdown

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

Test Result Time (s) Test File
ContextSuite context=TestKubernetesCluster>:teardown Error 78.82 test_kubernetes_clusters.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants