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.

Sign up for GitHub

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

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

fix a bug related to user update failed in case of proxysql cluster #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions lib/puppet/provider/proxy_mysql_user/proxysql.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,20 @@ def create
schema_locked = @resource.value(:schema_locked) || 0
transaction_persistent = @resource.value(:transaction_persistent) || 1
fast_forward = @resource.value(:fast_forward) || 0
backend = @resource.value(:backend) || 1
frontend = @resource.value(:frontend) || 1
backend = @resource.value(:backend)
frontend = @resource.value(:frontend)
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?
Copy link
Member

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)?

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Member

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

Copy link
Contributor Author

@MaxFedotov MaxFedotov Jul 10, 2019 •
edited
Loading

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?

query << ' `frontend`,' if defined?(frontend).nil?
query << ' `max_connections`)'
query << " VALUES ('#{name}', '#{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?
query << " #{frontend}," if defined?(frontend).nil?
query << " #{max_connections})"
mysql([defaults_file, '-e', query].compact)
@property_hash[:ensure] = :present

Expand Down
2 changes: 0 additions & 2 deletions lib/puppet/type/proxy_mysql_user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,11 @@ def initialize(*args)

newproperty(:backend) do
desc 'Backend or not.'
defaultto 1
Copy link
Member

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.

newvalue(%r{[01]})
end

newproperty(:frontend) do
desc 'Frontend or not.'
defaultto 1
newvalue(%r{[01]})
end

Expand Down