kris
changed title from WIP: (testing) Add UI needed for restore bubble process to WIP: (testing) Add needed stuff and fixes for bubble restore process4 years ago
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.
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.
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 process4 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 process4 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.
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.
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).
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.
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
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 process4 years ago
WIP: (testing) Add UI needed for restore bubble processto WIP: (testing) Add needed stuff and fixes for bubble restore process 4 years agoOverall 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.
Looks good, just a few questions.
If
TAG_RESTORE_MODE
isisInRestoringStatus
then shouldn’t the method name berestoring()
instead ofwasRestored()
?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.
What would throw a
SimpleViolationException
in the above block?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.
WIP: (testing) Add needed stuff and fixes for bubble restore processto Add needed stuff and fixes for bubble restore process 4 years agoAdd needed stuff and fixes for bubble restore processto WIP: (merge web, and then update link in this one) Add needed stuff and fixes for bubble restore process 4 years ago@kris Please fix merge conflicts, hopefully not as bad as the conflicts in
bubble-web
Looks great. Just a few comments/questions.
Let’s set
BUBBLE_PRODUCTION=1
instead ofINSTALL_WEB=web
.BUBBLE_PRODUCTION=1
is builds and includes the web app, but useswebpack
in production mode, results in a much smallermain.js
file.We should also
chown -R bubble ~bubble/site
after unrolling the site files. Otherwise they will be owned by root, which is undesirable.I prefer redirection to be after the command, and always use the stream numbers, like:
If you want to expose the
ctime
, it is cleaner to do:Otherwise, absent a
@Transient
annotation, this looks like you are adding another column to the database that will always be null.Please use
==
instead of.equals
when comparing enumsA bit awkwardly worded. I’d prefer this String to be
awaitingRestore
Why do we need a
ctime
here?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).
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.
This seems to be a static file -- can we put it in back in
packer/...
instead ofansible/...
?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
whenrestore.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, let’s move to packer. Even if it’s never used when not in restore mode.
Let’s move it to packer in another PR. This one is ready to merge!
nice catches. thanks.
WIP: (merge web, and then update link in this one) Add needed stuff and fixes for bubble restore processto Add needed stuff and fixes for bubble restore process 4 years agoReviewers
f805953f6d
.