Skip to content
This repository has been archived by the owner on Oct 21, 2019. It is now read-only.

Upgrade infrastructure AS script #205

Merged
merged 3 commits into from
Apr 17, 2018

Conversation

juagargi
Copy link
Member

@juagargi juagargi commented Apr 3, 2018

We'll run this script in every AS from our infrastructure, using ansible or similar client that issues commands via ssh.


This change is Reviewable

@juagargi juagargi requested review from hausheer and chaehni and removed request for hausheer and chaehni April 6, 2018 07:03
@jonghoonkwon
Copy link
Contributor

@juagargi, I reviewed the script and took a dry run. I made some comments below.
In overall, :lgtm:


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


upgrade_infrastructure.sh, line 6 at r1 (raw file):

GITREPO="juan"
GITBRANCH="stable_scionproto"

The GITREPO and GITBRANCH should be changed to the actual repo/branch name.
For all the infrastructure ASes, GITREPO=origin and GITBRANCH=scionlab should work.


upgrade_infrastructure.sh, line 23 at r1 (raw file):

# check we can sudo. We have -e, will exit if not
echo sci0nLab | sudo -S -v &>/dev/null && success=1 || success=0

We're not going to write down the sudo pw into the script.
Furthermore, I think we don't need a sudo privilege check to restart Scion.


upgrade_infrastructure.sh, line 74 at r1 (raw file):

    # exit 1
fi
output=$(./supervisor/supervisor.sh reload 2>&1) && success=1 || success=0

Do we actually need to reload supervisor?
Running Scion after shutting down supervisor would do same thing here.

I would like to recommend to run zookeeper clean command instead reloading supervisor.
To do so, you can run the following command.
tools/zkcleanslate --zk 127.0.0.1:2181


Comments from Reviewable

@jonghoonkwon
Copy link
Contributor

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


upgrade_infrastructure.sh, line 6 at r1 (raw file):

Previously, jonghoonkwon wrote…

The GITREPO and GITBRANCH should be changed to the actual repo/branch name.
For all the infrastructure ASes, GITREPO=origin and GITBRANCH=scionlab should work.

And also, it would be nice we can specify the branch name by giving an argument when we run the script, so that we can have more flexibility for the upgrade procedure.


upgrade_infrastructure.sh, line 35 at r1 (raw file):

fi

output=$(git fetch "$GITREPO" "$GITBRANCH" 2>&1) && success=1 || success=0

This line only checks if there is a conflict caused by the changes from user side while rebasing.
Even if the changes do not cause any conflict, it could be a potential error for the future Scion operation.
I think we need to run git status to check whether any of the tracked files is changed,
and should abort the upgrade in that event.


Comments from Reviewable

@jonghoonkwon
Copy link
Contributor

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


upgrade_infrastructure.sh, line 53 at r1 (raw file):

    exit 1
fi
# git pull

As we discussed, we'd better to have a plan B for a case that the upgrade procedure fails.
One suggestion is to checkout a scionlab_local_backup branch before rebasing the scionlab branch,
so that we can simply rollback to current branch if something goes wrong.


Comments from Reviewable

@juagargi
Copy link
Member Author

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


upgrade_infrastructure.sh, line 6 at r1 (raw file):

Previously, jonghoonkwon wrote…

And also, it would be nice we can specify the branch name by giving an argument when we run the script, so that we can have more flexibility for the upgrade procedure.

Done.


upgrade_infrastructure.sh, line 23 at r1 (raw file):

Previously, jonghoonkwon wrote…

We're not going to write down the sudo pw into the script.
Furthermore, I think we don't need a sudo privilege check to restart Scion.

Done.


upgrade_infrastructure.sh, line 35 at r1 (raw file):

Previously, jonghoonkwon wrote…

This line only checks if there is a conflict caused by the changes from user side while rebasing.
Even if the changes do not cause any conflict, it could be a potential error for the future Scion operation.
I think we need to run git status to check whether any of the tracked files is changed,
and should abort the upgrade in that event.

Done.


upgrade_infrastructure.sh, line 53 at r1 (raw file):

Previously, jonghoonkwon wrote…

As we discussed, we'd better to have a plan B for a case that the upgrade procedure fails.
One suggestion is to checkout a scionlab_local_backup branch before rebasing the scionlab branch,
so that we can simply rollback to current branch if something goes wrong.

Done.


upgrade_infrastructure.sh, line 74 at r1 (raw file):

Previously, jonghoonkwon wrote…

Do we actually need to reload supervisor?
Running Scion after shutting down supervisor would do same thing here.

I would like to recommend to run zookeeper clean command instead reloading supervisor.
To do so, you can run the following command.
tools/zkcleanslate --zk 127.0.0.1:2181

Done. We run now the zkcleanslate instead of the supervisor reload


Comments from Reviewable

@jonghoonkwon
Copy link
Contributor

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


upgrade_infrastructure.sh, line 23 at r1 (raw file):

Previously, juagargi (Juan A. García Pardo Giménez de los Galanes) wrote…

Done.

We need to get rid of this sudo check


Comments from Reviewable

@juagargi
Copy link
Member Author

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


upgrade_infrastructure.sh, line 23 at r1 (raw file):

Previously, jonghoonkwon wrote…

We need to get rid of this sudo check

I've fixed to not use -v and it seems to work now.


Comments from Reviewable

Fix repo and branch names. Allow branch as parameter.
Don't use sudo with password.
Check git status.
Backup locally the current branch.
Zookeeper clean slate instead of supervisor reload.
@jonghoonkwon
Copy link
Contributor

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@juagargi juagargi merged commit 8ffff54 into netsec-ethz:master Apr 17, 2018
@juagargi juagargi deleted the upgrade_infrastructure branch April 17, 2018 07:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants