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

(#MODULES-1058) root_password.pp cannot create /root/.my.cnf due to depe... #651

Merged
merged 1 commit into from
Jan 30, 2015
Merged

(#MODULES-1058) root_password.pp cannot create /root/.my.cnf due to depe... #651

merged 1 commit into from
Jan 30, 2015

Conversation

lodgenbd
Copy link
Contributor

...ndency

The dependency of creating the root .my.cnf file is a command which requires
the .my.cnf file. This patch removes that dependency. Without removing the
dependency, if a user already has a mysql server installed with a root password
and no root .my.cnf file, the module application will fail.

…ependency

The dependency of creating the root .my.cnf file is a command which requires
the .my.cnf file. This patch removes that dependency. Without removing the
dependency, if a user already has a mysql server installed with a root password
and no root .my.cnf file, the module application will fail.
@matthewfischer
Copy link

I tried similar but it seemed to cause other issues in my environment. I ran out of time to dive in more, but this does seem like the obvious fix. I will try it again hopefully this week.

@igalic
Copy link
Contributor

igalic commented Jan 29, 2015

@lodgenbd help me understand the use-case here:

i have a mysql-server installed, with a known root password, and i want to use puppetlabs-mysql to manage it?

how will this change affect the usecase of, i want to install a new mysql server?

@lodgenbd
Copy link
Contributor Author

@igalic The use case you mentioned of already having a mysql server installed is one where the mysql server is already set up and in production setting, the user is trying to have puppet manage services for ease of configuration, and wants to apply mysql module to manage mysql server. In this case, the module will fail. With my code change, the module tested successful.

I tested my code change in the use case of "i want to install a new mysql server" with no issues, as well.

igalic added a commit that referenced this pull request Jan 30, 2015
(#MODULES-1058) root_password.pp cannot create /root/.my.cnf due to depe...
@igalic igalic merged commit a839489 into puppetlabs:master Jan 30, 2015
@igalic
Copy link
Contributor

igalic commented Jan 30, 2015

okay, thank you for the clarification, @lodgenbd, and for the patch!

@lodgenbd lodgenbd deleted the bug/master/root_account_fix branch January 30, 2015 18:32
@hogepodge
Copy link

igalic, fwiw, this patch broke my mysql use case.

@lodgenbd
Copy link
Contributor Author

lodgenbd commented Feb 2, 2015

@hogepodge what's your use case?

@hogepodge
Copy link

I'm running the puppet-openstack module set. When I run the code from those modules to set up a variety of users and accounts the .my.cnf file is not handled properly, and I get locked out of mysql administration.

This is on both Ubuntu 14.04 (mysql) and CentOS 7 (mariadb).

@lodgenbd
Copy link
Contributor Author

lodgenbd commented Feb 2, 2015

@hogepodge when you say the file isn't handled properly, what exactly is happening with the file?

@lodgenbd
Copy link
Contributor Author

lodgenbd commented Feb 2, 2015

@cyberious can you share what you're seeing?

@hogepodge
Copy link

In a while I can post the error I'm seeing. I actually depend on this module for day to day work, so when it broke in my environment I pinned back to a stable version so I could stay productive. What I do know is that previously I could create new users and databases, and my ability to do that with the module broke. I'm not sure exactly what's going on, but if I recall correctly it's telling me that I can't log in as the root user.

@lodgenbd
Copy link
Contributor Author

lodgenbd commented Feb 2, 2015

@cyberious can you confirm if the .my.cnf file was created before that error occurred?

@lodgenbd
Copy link
Contributor Author

lodgenbd commented Feb 2, 2015

It sounds like we need an order relationship so that the .my.cnf file is created before anything that would run a mysql command requiring it. I'm going to assume that's what's happening, unless someone reports that their .my.cnf file is being incorrectly built, which would be another issue altogether.

@lodgenbd
Copy link
Contributor Author

lodgenbd commented Feb 2, 2015

@igalic, Let me know what you think, but I see a couple of issues:

The root user and the .my.cnf file are in the same class, so when the below is called, it tries to take actions before making the .my.cnf file, and they fail. This is in the server.pp:

} else {
Anchor['mysql::server::start'] ->
Class['mysql::server::install'] ->
Class['mysql::server::config'] ->
Class['mysql::server::service'] ->
Class['mysql::server::root_password'] ->
Class['mysql::server::providers'] ->
Anchor['mysql::server::end']

If you move the root_password class higher in the order above the install class, it would also fail because the class would try to do two things:

1.) create the root user within mysql
2.) create the .my.cnf file

...before mysql is installed. In that case, step 1 above would fail because mysql isn't installed, but step 2 would succeed, because it's just a file and doesn't rely on anything.

I believe that step 2 should be pulled out, added to a class of its own, and placed in order before the "install" class, then made a requirement of any resource that would use a "mysql" command.

Having said all of that... after looking through almost all of the code for this module, I believe the module was written moreso with the idea that this is for installation of a mysql server and less in the mind of managing one that's already installed and running. If that's true, then the best plan may be just to remove and revert this code back before this merge, then add a note in the README that this module isn't meant for that use case, noting the above as a reason why.

Please update and let me know what you think.

@cyberious
Copy link
Contributor

@lodgenbd @igalic for now we are reverting the change until a new one that address both use cases can be found.

@lodgenbd
Copy link
Contributor Author

lodgenbd commented Feb 3, 2015

Agreed, thank you

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

Successfully merging this pull request may close these issues.

6 participants