Skip to content
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

Merged
merged 1 commit into from Jun 28, 2022
Merged

Conversation

matthiashanel
Copy link
Contributor

@matthiashanel matthiashanel commented Jun 25, 2022

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.

@@ -158,6 +162,7 @@ const (
defaultStoreDirName = "_js_"
defaultMetaGroupName = "_meta_"
defaultMetaFSBlkSize = 1024 * 1024
jsPeerExcludeTag = "!jetstream"
Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor

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.

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>
server/jetstream_api.go Show resolved Hide resolved
server/jetstream_api.go Show resolved Hide resolved
server/jetstream_api.go Show resolved Hide resolved
server/jetstream_cluster.go Show resolved Hide resolved
server/jetstream_cluster.go Show resolved Hide resolved
server/jetstream_cluster.go Show resolved Hide resolved
}

func (u *tagsOption) Apply(server *Server) {
server.Noticef("Reloaded: tags")
Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

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...

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor

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...

Copy link
Member

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.

@derekcollison derekcollison self-requested a review June 27, 2022 20:52
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@matthiashanel matthiashanel marked this pull request as ready for review June 27, 2022 20:58
Copy link
Contributor

@tbeets tbeets left a comment

Choose a reason for hiding this comment

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

LGTM

@matthiashanel matthiashanel merged commit 3421c49 into main Jun 28, 2022
@matthiashanel matthiashanel deleted the stream-move branch June 28, 2022 00:36
// 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"
Copy link
Contributor

@ripienaar ripienaar Jul 1, 2022

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

Copy link
Contributor

@ripienaar ripienaar Jul 1, 2022

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"`
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants