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

Jump to bottom

Fix MySQL vagrant test vm build #569

Merged
merged 3 commits into from
Apr 25, 2017
Merged

Fix MySQL vagrant test vm build #569

merged 3 commits into from
Apr 25, 2017

Conversation

MMatten
Copy link
Contributor

@MMatten MMatten commented Apr 23, 2017

Allow MySQL service to run on vagrant CentOS box that comes with SELinux running in enforcing mode.

Resolves #568

javornikolov
javornikolov reviewed Apr 23, 2017
View reviewed changes
test_vm/vendor/cookbooks/dbfit_test/metadata.rb Outdated
@@ -7,7 +7,7 @@
version '0.3.0'
depends 'mysql2_chef_gem'
depends 'yum-mysql-community'
depends 'mysql', '~> 8.2'
depends 'mysql'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need that change already? With ~> 8.2 we should be getting the latest 8.x >= 8.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, after reading the docs I thought this meant any 8.2.x but not 8.3.0 or later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still not sure what's going on here. The docs

https://docs.chef.io/cookbook_versions.html

suggest that it wouldn't use 8.3.1 but it does end up in Berksfile.lock if I include the '~> 8.2' constraint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So why would we now want to keep this constraint?

Copy link
Contributor

Choose a reason for hiding this comment

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

~> 8.2 is pessimistic locking and means >= 8.2 and < 9.0. So 8.3.1 meets the constraint.

So why would we now want to keep this constraint?

Well, I don't know should we keep it or not. The old setting has the constraint, I guess we did it with idea that:

  • It reminds us that versions < 8.2 are problematic and prevents us from using them.
  • Also it would not automatically jump to 9.0 which might have breaking changes (mysql cookbook has proven to be a bit brittle).

Essentially the question is do we prefer ~> 8.2 or >= 8.2. The latter will give us signal that there is version >= 9.0. Maybe that's not a bad thing - we're anyway locking the versions with the Berksfile.lock and testing if things still work OK upon changing it.

Up to your judgement whether to remove the dependency constraint. It might be better to have it as separate PR since I believe it doesn't contribute to fixing the broken provisioning this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm leaning towards >= 8.2 and falling back to ~> 8.2 if the mysql cookbook develops new issues in 9.x.

Agreed; a separate PR would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm leaning towards >= 8.2 and falling back to ~> 8.2 if the mysql cookbook develops new issues in 9.x.

ok

javornikolov
javornikolov reviewed Apr 23, 2017
View reviewed changes
test_vm/vendor/cookbooks/dbfit_test/recipes/selinux.rb Outdated
action :permissive
end

selinux_state 'SELinux Permissive' do
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we give that other name? There used it be some caveats for resources with same name (I think Chef treats is as a single entity and runs it only once).

@javornikolov
Copy link
Contributor

Loosen SELinux enforcement to allow MySQL to run

The commit message is not quite reflecting the change (we had it loosened before but after reboot only).

@javornikolov
Copy link
Contributor

Updates for latest mysql2_chef_gem version

It might be good to refine a bit that commit message too since we update some other cookbooks too.

@javornikolov
Copy link
Contributor

Besides the other remarks, the approach implemented in selinux.rb looks good to me.

@MMatten MMatten closed this Apr 25, 2017
@MMatten MMatten force-pushed the fix-mysql-vagrant-test-vm-build branch from 0c2394d to 83bd9ca Compare April 25, 2017 20:57
@MMatten
Copy link
Contributor Author

MMatten commented Apr 25, 2017

Ooops. Accidentally closed. I'm not sure how I did that.

@javornikolov javornikolov reopened this Apr 25, 2017
@javornikolov
Copy link
Contributor

Ooops. Accidentally closed. I'm not sure how I did that.

No problem, there is reopen button which I just used :-)

@MMatten
Copy link
Contributor Author

MMatten commented Apr 25, 2017

Ooops. Accidentally closed. I'm not sure how I did that.

No problem, there is reopen button which I just used :-)

Thanks! I've made a few tweaks. Let me know what you think.

javornikolov
javornikolov reviewed Apr 25, 2017
View reviewed changes
test_vm/vendor/cookbooks/dbfit_test/recipes/selinux.rb Outdated
action :permissive
end

selinux_state 'SELinux Permissive Perm' do
Copy link
Contributor

Choose a reason for hiding this comment

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

Perm could mean permissive, permanent, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking that I should leave out the temporary false for the permanent mode change as false is the default but leaving in makes it more obvious. What's your view?

Copy link
Contributor

Choose a reason for hiding this comment

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

Explicit temporary flag looks OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we come up with a good idea - we may try to get s support into selinux cookbook itself (e.g. scope=temporary|configuration-file|both.

@MMatten MMatten force-pushed the fix-mysql-vagrant-test-vm-build branch from 158eac6 to 2318f93 Compare April 25, 2017 21:15
@MMatten
Copy link
Contributor Author

MMatten commented Apr 25, 2017

Pushed again.

javornikolov
javornikolov reviewed Apr 25, 2017
View reviewed changes
test_vm/Berksfile.lock Outdated
@@ -19,53 +17,57 @@ GRAPH
iptables (>= 0.0.0)
java (>= 0.0.0)
lvm (>= 0.0.0)
mysql (~> 8.2)
mysql (>= 0.0.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rebuild Berksfile.lock again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@MMatten MMatten force-pushed the fix-mysql-vagrant-test-vm-build branch from 2318f93 to 0128253 Compare April 25, 2017 21:39
@MMatten
Copy link
Contributor Author

MMatten commented Apr 25, 2017 •
edited

The VM is building well again 👍

Derby, HSQLDB, MySQL tests passing. Just PostgreSQL failing on CoreTests.CommonSuite.Symbols.

@javornikolov javornikolov added this to the Next milestone Apr 25, 2017
@javornikolov
Copy link
Contributor

Thank you @MMatten! Looks good to me now, I'm merging it...

@javornikolov javornikolov merged commit 9376a00 into master Apr 25, 2017
@javornikolov javornikolov deleted the fix-mysql-vagrant-test-vm-build branch April 25, 2017 22:05
@MMatten MMatten mentioned this pull request Apr 25, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javornikolov javornikolov javornikolov left review comments

Assignees
No one assigned
Labels
techdebt
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@MMatten @javornikolov