#20 Add needed stuff and fixes for bubble restore process

Merged
jonathan merged 116 commits from kris/add_support_for_restore_ui into master 4 years ago
kris commented 4 years ago
There is no content yet.
kris changed title from WIP: (testing) Add UI needed for restore bubble process to WIP: (testing) Add needed stuff and fixes for bubble restore process 4 years ago
jonathan commented 4 years ago
Owner

Overall looks OK. I’d guess there might be changes required to the ansible roles too? Now that the packer stuff is committed, it’d be good to try some live runs.

Overall looks OK. I'd guess there might be changes required to the ansible roles too? Now that the packer stuff is committed, it'd be good to try some live runs.
jonathan requested changes 4 years ago
jonathan left a comment

Looks good, just a few questions.

@@ -292,6 +293,7 @@ public class BubbleConfiguration extends PgRestServerConfiguration
{TAG_LOCALES, getAllLocales()},
{TAG_CLOUD_CONFIGS, accountDAO.activated() ? null : activationService.getCloudDefaults()},
{TAG_LOCKED, accountDAO.locked()},
{TAG_RESTORE_MODE, thisNode.wasRestored()},
jonathan commented 4 years ago

If TAG_RESTORE_MODE is isInRestoringStatus then shouldn’t the method name be restoring() instead of wasRestored() ?

If `TAG_RESTORE_MODE` is `isInRestoringStatus` then shouldn't the method name be `restoring()` instead of `wasRestored()` ?
kris commented 4 years ago

Yes, wasRestored was wrongly named at the first place. Or maybe it is not.

isInRestoringStatus refers to the bubble/network object.

wasRestored is called over node object which was actually restored and is currently in running state waiting on a used to call restore API call.

Confusing, but it might be correct.

Yes, `wasRestored` was wrongly named at the first place. Or maybe it is not. `isInRestoringStatus` refers to the bubble/network object. `wasRestored` is called over node object which was actually restored and is currently in running state waiting on a used to call restore API call. Confusing, but it might be correct.
@@ -649,0 +651,4 @@
// also, should this go into other method here that are locking network?
try { unlockNetwork(network.getUuid(), lock); } catch (Exception e1) { }
log.error("startNetwork: original SimpleViolationException: ", e);
throw e;
jonathan commented 4 years ago

What would throw a SimpleViolationException in the above block?

What would throw a `SimpleViolationException` in the above block?
kris commented 4 years ago

Lines 615 and 618 here in this class (just slightly changed within this PR) are resulting with this exception type.

unlocking the network here so this exact method will not be blocking when next time it is called after some of the conditions above those mentioned lines are met.

I’ve produced the issue with this when trying to test this locally as after you stop bubble locally, it will still have that same node object inside. This node object is removed after stopping bubble on btest5, but not in local env. So I was removing it directly from database, and then rely on this part of code to work so I can re-test restore on that unlocked network.

Lines 615 and 618 here in this class (just slightly changed within this PR) are resulting with this exception type. unlocking the network here so this exact method will not be blocking when next time it is called after some of the conditions above those mentioned lines are met. I've produced the issue with this when trying to test this locally as after you stop bubble locally, it will still have that same node object inside. This node object is removed after stopping bubble on btest5, but not in local env. So I was removing it directly from database, and then rely on this part of code to work so I can re-test restore on that unlocked network.
kris changed title from WIP: (testing) Add needed stuff and fixes for bubble restore process to Add needed stuff and fixes for bubble restore process 4 years ago
kris changed title from Add needed stuff and fixes for bubble restore process to WIP: (merge web, and then update link in this one) Add needed stuff and fixes for bubble restore process 4 years ago
jonathan commented 4 years ago
Owner

@kris Please fix merge conflicts, hopefully not as bad as the conflicts in bubble-web

@kris Please fix merge conflicts, hopefully not as bad as the conflicts in `bubble-web`
jonathan requested changes 4 years ago
jonathan left a comment

Looks great. Just a few comments/questions.

bin/bpatchfull
@@ -46,3 +46,3 @@
find "./src/main" -type f -newer "$(find "./target" -type f -name "bubble*.jar" | head -1)"
fi
mvn -DskipTests=true -Dcheckstyle.skip=true clean package || die "Error packaging jar"
INSTALL_WEB=web mvn -DskipTests=true -Dcheckstyle.skip=true clean package || die "Error packaging jar"
jonathan commented 4 years ago

Let’s set BUBBLE_PRODUCTION=1 instead of INSTALL_WEB=web.
BUBBLE_PRODUCTION=1 is builds and includes the web app, but uses webpack in production mode, results in a much smaller main.js file.

Let's set `BUBBLE_PRODUCTION=1` instead of `INSTALL_WEB=web`. `BUBBLE_PRODUCTION=1` is builds and includes the web app, but uses `webpack` in production mode, results in a much smaller `main.js` file.
@@ -59,0 +60,4 @@
if [[ $(jar tf ./target/bubble*.jar | grep "^site/$") ]] ; then
echo "Deploying new web..."
ssh ${HOST} "cd ~bubble && jar xf /tmp/bubble.jar site"
fi
jonathan commented 4 years ago

We should also chown -R bubble ~bubble/site after unrolling the site files. Otherwise they will be owned by root, which is undesirable.

We should also `chown -R bubble ~bubble/site` after unrolling the site files. Otherwise they will be owned by root, which is undesirable.
bin/vultr/vcurl
@@ -4,3 +4,3 @@
#
if [[ -z "${VULTR_API_KEY}" ]] ; then
echo "VULTR_API_KEY not defined in environment"
>&2 echo "VULTR_API_KEY not defined in environment"
jonathan commented 4 years ago

I prefer redirection to be after the command, and always use the stream numbers, like:

echo 1>&2 "VULTR_API_KEY not defined in environment"
I prefer redirection to be after the command, and always use the stream numbers, like: echo 1>&2 "VULTR_API_KEY not defined in environment"
@@ -77,1 +77,4 @@
public boolean canDelete() { return status.isDeletable() || getCtimeAge() > BR_STATE_LOCK_TIMEOUT; }

public long getCreationTime() { return getCtime(); }
public void setCreationTime(long c) { /* noop */ }
jonathan commented 4 years ago

If you want to expose the ctime, it is cleaner to do:

@JsonProperty public long getCtime () { return super.getCtime(); }

Otherwise, absent a @Transient annotation, this looks like you are adding another column to the database that will always be null.

If you want to expose the `ctime`, it is cleaner to do: @JsonProperty public long getCtime () { return super.getCtime(); } Otherwise, absent a `@Transient` annotation, this looks like you are adding another column to the database that will always be null.
@@ -112,6 +112,7 @@ public class AuthResource {
@GET @Path(EP_READY)
public Response getNodeIsReady(@Context ContainerRequest ctx) {
try {
if (configuration.getThisNetwork().getState().equals(BubbleNetworkState.restoring)) return ok();
jonathan commented 4 years ago

Please use == instead of .equals when comparing enums

Please use `==` instead of `.equals` when comparing enums
@@ -90,6 +92,7 @@ public class BubbleConfiguration extends PgRestServerConfiguration
public static final String TAG_REQUIRE_SEND_METRICS = "requireSendMetrics";
public static final String TAG_SUPPORT = "support";
public static final String TAG_SECURITY_LEVELS = "securityLevels";
public static final String TAG_RESTORE_MODE = "isWaitingRestoring";
jonathan commented 4 years ago

A bit awkwardly worded. I’d prefer this String to be awaitingRestore

A bit awkwardly worded. I'd prefer this String to be `awaitingRestore`
@@ -26,0 +32,4 @@
// backward compatibility - the following getter may be removed and default one may be used after some time, while
// ctime can be changed to be of simple `long` type
public long getCtime() { return ctime == null ? 0 : ctime; }

jonathan commented 4 years ago

Why do we need a ctime here?

Why do we need a `ctime` here?
kris commented 4 years ago

Whenever I start second instance for the restored bubble node, I’m getting 2 objects for that same network with ticks. First one is from the original bubble and is stuck on 100%. And there I needed the second (newer) one with ticks from restored bubble instances that is building up. So I added ctime into that tick object.

For some time, old tick objects in redis will be without this ctime property. So this part right here is just for backward compatibility, and awoing altering existing objects in redis (i.e. to add them ctime when this code is deployed).

Whenever I start second instance for the restored bubble node, I'm getting 2 objects for that same network with ticks. First one is from the original bubble and is stuck on 100%. And there I needed the second (newer) one with ticks from restored bubble instances that is building up. So I added ctime into that tick object. For some time, old tick objects in redis will be without this ctime property. So this part right here is just for backward compatibility, and awoing altering existing objects in redis (i.e. to add them ctime when this code is deployed).
jonathan commented 4 years ago

OK. I think this should be fixed in the latest code, but I like the idea of using the newest tick, just in case there are more than one for the same bubble. Let’s leave this as-is.

OK. I think this should be fixed in the latest code, but I like the idea of using the newest tick, just in case there are more than one for the same bubble. Let's leave this as-is.
@@ -0,0 +96,4 @@

# Remove old keys
log "Removing node keys"
echo "DELETE FROM bubble_node_key" | bsql.sh
jonathan commented 4 years ago

This seems to be a static file -- can we put it in back in packer/... instead of ansible/...?

This seems to be a static file -- can we put it in back in `packer/...` instead of `ansible/...`?
kris commented 4 years ago

Yes, it is static. But it is installed and used only on instances which are meant to be restored bubbles. There is a condition when: restore_key is defined when restore.yml is included. And that restore.yml contains tasks for installing and starting this monitor.

So, it could go both ways, just please confirm that this file should be moved to packer and so installed for regular bubbles and not only for those that will be restored.

Yes, it is static. But it is installed and used only on instances which are meant to be restored bubbles. There is a condition `when: restore_key is defined` when `restore.yml` is included. And that restore.yml contains tasks for installing and starting this monitor. So, it could go both ways, just please confirm that this file should be moved to packer and so installed for regular bubbles and not only for those that will be restored.
jonathan commented 4 years ago

Yes, let’s move to packer. Even if it’s never used when not in restore mode.

Yes, let's move to packer. Even if it's never used when not in restore mode.
jonathan commented 4 years ago

Let’s move it to packer in another PR. This one is ready to merge!

Let's move it to packer in another PR. This one is ready to merge!
@@ -8063,4 +8058,1 @@
porno
porns
porny
porta
jonathan commented 4 years ago

nice catches. thanks.

nice catches. thanks.
jonathan changed title from WIP: (merge web, and then update link in this one) Add needed stuff and fixes for bubble restore process to Add needed stuff and fixes for bubble restore process 4 years ago
jonathan referenced this issue from a commit 4 years ago
Add needed stuff and fixes for bubble restore process (#20) Merge branch 'master' of git.bubblev.org:bubblev/bubble into kris/add_support_for_restore_ui Rename isWaitingRestoring Use == instead of equals on enums Use ctime instead of creationTime in backup objects Fix bin scripts after CR Merge branch 'master' into kris/add_support_for_restore_ui # Conflicts: # bubble-server/src/main/resources/message_templates/en_US/server/post_auth/ResourceMessages.properties # bubble-web Add missing label Update web Merge branch 'master' into kris/add_support_for_restore_ui # Conflicts: # bubble-server/src/main/java/bubble/service/cloud/StandardNetworkService.java # bubble-server/src/main/resources/ansible/roles/algo/tasks/main.yml # bubble-web Update web Use better check if restore is started Merge branch 'master' into kris/add_support_for_restore_ui # Conflicts: # bubble-server/src/main/java/bubble/service/cloud/StandardNetworkService.java Init sage node on init self node if needed Update log message on post copy entities Update web Remove another word from host prefixes Mark restoring node as ready Add ctime to ticks-stats returned to FE Merge branch 'master' into kris/add_support_for_restore_ui # Conflicts: # bubble-web Try resetting progress meter on each new node Add auth/ready ep in skip auth for restore node Add more logs of api exception Update web Remove some bad host prefixes Update web Deploy web if it is included in jar Merge branch 'master' into kris/add_support_for_restore_ui Add support for showing latest backup on FE Merge branch 'master' into kris/add_support_for_restore_ui Update web Use proper flag for waiting restoring bubble Use separate bash to avoid continuing within venv Start restore monitor on instance when needed Save iptables in packer instance Merge branch 'master' into kris/add_support_for_restore_ui Save iptables before corresponding service restart Merge branch 'master' into kris/add_support_for_restore_ui # Conflicts: # bubble-server/src/main/java/bubble/server/BubbleConfiguration.java # bubble-web Fix iptables entries again Echo error to stderr Fix iptable rules creation Add back needed tags in algo related ansible tasks Update web Merge branch 'master' into kris/add_support_for_restore_ui # Conflicts: # bubble-web Add new labels and update web Use network state as restore mode tag Create full jar with web on full patching Update first_time_marker file with correct value Run first time listener for restoring instances also Merge branch 'master' into kris/add_support_for_restore_ui # Conflicts: # bubble-web Add new lib to utils pom Co-authored-by: Jonathan Cobb <jonathan@kyuss.org> Co-authored-by: Kristijan Mitrovic <kmitrovic@itekako.com> Reviewed-on: https://git.bubblev.org/bubblev/bubble/pulls/20
jonathan closed this pull request 4 years ago
jonathan deleted branch kris/add_support_for_restore_ui 4 years ago

Reviewers

jonathan requested changes 4 years ago
The pull request has been merged as f805953f6d.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.