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

Test and use functional controller from host list. #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

csoylu
Copy link

@csoylu csoylu commented May 31, 2022

Hello, we are a small company called Geco-iT located in south France.

We have started using this plugin in our docker environment. Being able to switch controllers in case of a controller change was essential to our use case.

So I have included the functionality to test all the given hosts, find and use the controller between them. Most of the code is from the last version of linstor.

Tests
We have tested it in our environment with three servers, one of them being controller.
Tested with:

  • IPs that doesn't exist
  • IPs that exist but is a random machine
  • IPs that are server but not controller
  • and a mix off all that

For each test, the plugin was able to find the controller between all the given list of hosts.

@rck
Copy link
Collaborator

rck commented Jun 3, 2022

Hi, I will certainly have beornf have the final word, just some thoughts from my side:

If you already trust DRBD and LINSTOR, you might want to make your controller's storage HA (https://linbit.com/drbd-user-guide/linstor-guide-1_0-en/#s-linstor_ha) and put a OCF cluster IP on top of it (https://github.com/LINBIT/drbd-reactor/blob/master/doc/promoter.md), then it has the same IP wherever it runs.

If that sounds too complicated or does not fit your particular use case, then I'd still suggest that we expose that "try to find a controller that talks to me" logic from golinstor and then call it from here. I did not check, but from what I remember it would not require too many changes in golinstor if at all?

@csoylu
Copy link
Author

csoylu commented Jun 3, 2022

Hi, I will certainly have beornf have the final word, just some thoughts from my side:

If you already trust DRBD and LINSTOR, you might want to make your controller's storage HA (https://linbit.com/drbd-user-guide/linstor-guide-1_0-en/#s-linstor_ha) and put a OCF cluster IP on top of it (https://github.com/LINBIT/drbd-reactor/blob/master/doc/promoter.md), then it has the same IP wherever it runs.

If that sounds too complicated or does not fit your particular use case, then I'd still suggest that we expose that "try to find a controller that talks to me" logic from golinstor and then call it from here. I did not check, but from what I remember it would not require too many changes in golinstor if at all?

Hi, unfortunately, OCF cluster IP does not really fit into our use case.

I've considered using golinstor client's functions, but the plugin actually uses golinstor v0.16.3, and these functions I added don't exist in that version.

If there's a way to import the last version of golinstor, I can already see how to do it, and it would require a few lines to be changed in the plugin.

Don't know if this is the logic "try to find a controller that talks to me" you were talking about, or if there's some other function that does this in the actual version.

@rck
Copy link
Collaborator

rck commented Jun 3, 2022

I've considered using golinstor client's functions, but the plugin actually uses golinstor v0.16.3, and these functions I added don't exist in that version.

It is just a regular go module, I don't see a reason why bumping that to the latest version would make any troubles. Looking at the versions, there must have been lots of changes, but we don't break golinstor just for fun. I would expect it just works or needs minimal changes. IMO that would be the way forward, but let's wait for @beornf if would even accept such a PR in general.

@beornf
Copy link
Owner

beornf commented Jun 3, 2022

@csoylu I think it would be best to move to the latest golinstor version in this PR. Especially if that means less duplication of logic. I'm taking a look at golinstor and will see what's changed since v0.16.3.

Copy link
Owner

@beornf beornf left a comment

Choose a reason for hiding this comment

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

The client constructor below should be updated.

return client.NewClient(
client.BaseURL(baseURL),

By passing in the controllers as seen directly below the client will internally use findRespondingController on each HTTP request.

controllers := strings.Split(config.Controllers, ",")
return client.NewClient(client.Controllers(controllers))

@@ -115,7 +217,14 @@ func (l *LinstorDriver) newClient() (*client.Client, error) {
return nil, err
}

baseURL, err := l.newBaseURL(config.Controllers)
Copy link
Owner

Choose a reason for hiding this comment

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

This can be deleted along with the corresponding newBaseURL method.

@@ -77,28 +80,127 @@ func NewLinstorDriver(config, node, root string) *LinstorDriver {
}
}

func (l *LinstorDriver) newBaseURL(hosts string) (*url.URL, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

This method can be deleted as it no longer needs to be referenced.

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.

3 participants