-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Add] ability for operator to move streams (WIP) #3217
Conversation
matthiashanel
commented
Jun 25, 2022
•
edited
edited
server/jetstream_cluster.go
Outdated
@@ -158,6 +162,7 @@ const ( | |||
defaultStoreDirName = "_js_" | |||
defaultMetaGroupName = "_meta_" | |||
defaultMetaFSBlkSize = 1024 * 1024 | |||
jsPeerExcludeTag = "!jetstream" |
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.
Suggest jsExcludePlacement
value string, case-insensitive, as clearer meaning.
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.
So we want JS to stay active while we evacuate any assets etc but just have it not applicable for placement correct?
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.
Correct. A marker on a server so it is not considered for new placements.
5f54354
to
555b9c1
Compare
Also added: ability to reload tags special tag (!jetstream) to remove peer from peer placement $JS.API.SERVER.STREAM.MOVE subject to initiate move away from a server This changes a detail about regular stream move as well. Before differing cluster names where used to start/stop a transfer. Now only the peer list and it's size relative to configured replica matter. Once a transfer is considered completed, excess peers will be dropped from the beginning of the list. This allows transfers within the cluster as well. Signed-off-by: Matthias Hanel <mh@synadia.com>
555b9c1
to
0952401
Compare
} | ||
|
||
func (u *tagsOption) Apply(server *Server) { | ||
server.Noticef("Reloaded: tags") |
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.
I like this addition, but please note, AFAIK we have never written updates to config files as a result of a $SYS msg. That was by design. So we should carefully consider this and make sure we are committed here. Also need to loop in Phil imo.
There may be a better way to achieve the same goal of temporarily notifying the metaleader not to consider this server for placement, etc.
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.
This is not triggered by an update on a $SYS message.
This is driven by configuration only.
So if, for or whatever reason, we need placement to stop for a server. just place the new tag and reload the config.
Separating move and stopping of placement into two features was an intentional decision.
Enabling stopping of placement via server tags also piggy backs onto an existing mechanism (propagation already implemented) and is robust against restarts. (persistence is done via config) (just consider, meta leader changing, actual server being restarted for whatever reason...)
Furthermore, reload of tags is a worthwhile feature in its own right.
(With this, tags can also convey the config version)
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, I think black box operations are desirable, but don't think we should be updating the server config. Sounds like we should discuss before more progress is made.
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.
sent out an invite for later today
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.
Thanks, was chatting with Caleb as well this am, should be a short call I think.
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.
It is my understanding that putting this setting into the metaleader would open any number of cans of worms.
Where I'm going to make a big disruptive change recreating a vm, IMHO it's an acceptable compromise to set some persistent server tag as a prep step.
The prime use case is keeping r=1 highly available during a disruptive change. In this case I can spin up a replacement in the same availability zone as a target to keep all data in the same data center.
Arguably a simple tag on the doomed server would be very robust and intentional even across multiple failure modes, including having to upgrade the servers halfway through...
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.
We can always add a dedicated system call with the necessary bells and whistles later.
The config based approach gives us exactly what we want for very little effort.
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.
I wonder if we set the tag on a server that is also the meta leader, its steps down first, delays until new leader ok then sends tag metadata about placement?
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.
I think I lost the script on @derekcollison last comment. How is this feature directly related to a server step-down? That might come later as part of an operator's workflow, but didn't think stream evacuate directly caused any step-downs...
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 tag set is to tell the meta leader not to schedule any JS assets on this server. That tag update is sent to the meta leader who makes these decisions. If the server is the meta leader, might be best to step down first.
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.
LGTM
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.
LGTM
// JSApiServerInfo is the endpoint to abtain which stream a server contains | ||
// Only works from system account. | ||
// Will return JSON response. | ||
//JSApiServerStreamInfo = "$JS.API.SERVER.STREAM.INFO" |
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.
will this come in a follow up PR? Without this is hard to know when its done moving
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.
Are we also adding the ability to get a list of streams+account on a given server?
// Account name the stream to move is in | ||
Account string `json:"account"` | ||
// Name of stream to move | ||
Stream string `json:"string"` |
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.
This should be stream
, not string