-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
fix a bug related to user update failed in case of proxysql cluster #84
base: master
Are you sure you want to change the base?
fix a bug related to user update failed in case of proxysql cluster #84
Conversation
a8305bf
to
6e21fc1
Compare
@@ -80,13 +80,11 @@ def initialize(*args) | |||
|
|||
newproperty(:backend) do | |||
desc 'Backend or not.' | |||
defaultto 1 |
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.
Agreed. Defaults in resource properties
in the type are almost always wrong.
max_connections = @resource.value(:max_connections) || 10_000 | ||
|
||
query = 'INSERT INTO mysql_users (`username`, `password`, `active`, `use_ssl`, `default_hostgroup`, `default_schema`, ' | ||
query << ' `schema_locked`, `transaction_persistent`, `fast_forward`, `backend`, `frontend`, `max_connections`) ' | ||
query << ' `schema_locked`, `transaction_persistent`, `fast_forward`, ' | ||
query << ' `backend`,' if defined?(backend).nil? |
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'm not following this. Won't the variable always be defined?
and return local-variable
(a String which won't be nil
)?
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.
Only if you will define it in your manifest. Else as it don't have default values this would be skipped
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.
each user defined in puppet should just create 2 entries in the proxysql config, one backend and one frontend user... These settings currently do not mean anything in the upstream product but they are designed this way to add some functionality to them in the future...
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.
Yeah, that's OK. But the problem is with constraint violation, which i've described in first post. That's really make some problems with cluster deployments
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.
then I would assume you need a GROUP BY username
statement here and change this line to SELECT password, active, use_ssl, default_hostgroup, default_schema, schema_locked, transaction_persistent, fast_forward, MAX(backend) as backend, MAX(frontend) as frontend, max_connections FROM mysql_users WHERE username = '#{name}' GROUP BY username
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 insert and update statements need to update both entries when making changes:
with a WHERE username = '<blah>' AND backend = 0
and a WHERE username = '<blah>' AND backend = 1
to keep both records up-to-date...
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 agree with adding GROUP BY and MAX statements to query. But i don't understand the need to have inserting or updating both entries. Right now, as this fields are not used, no one will specify them in user hash, so basically a user with default values will be inserted (frontend = 1, backend = 1). Then as update also do not specify if this user is frontend or backend - both will be updated. Isn't it correct behavior for current situation?
let's suppose that you have a proxysql cluster which had 2 nodes configured like this:
on the first node you will get
but on the second node users get duplicated - you will have separate users for frontend and backend (see sysown/proxysql#1580)
if you will now change user configuration on the second node (where you have duplicated users), for example add
you will get following error:
this is becase in Proxysql primary key is not username, but username+backend. And as we have default value for this column defined in proxysql_user resource
, puppet tries to upgrade each of users to change to defaults values and got constraint violation.
This pull request fixes this issue. It removes default values from frontend and backend columns (because they already have defaults defined in proxysql database), so this value won't be updated every puppet run if it is not explicitly configured.