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

ENT-10739: Force default 'C' locale for package scriptlets #1312

Merged
merged 1 commit into from
Oct 13, 2023
Merged

ENT-10739: Force default 'C' locale for package scriptlets #1312

merged 1 commit into from
Oct 13, 2023

Conversation

craigcomstock
Copy link
Contributor

@craigcomstock craigcomstock commented Oct 5, 2023 •
edited
Loading

It was found that an LC_CTYPE=UTF-8 from iTerm2 on OSX was breaking the hub package install.

Also use su - (--login) option to preserve this LC_ALL setting

PostgreSQL docs suggest using 'C' locale unless otherwise needed
https://www.postgresql.org/docs/current/locale.html

The drawback of using locales other than C or POSIX in PostgreSQL is its performance impact. It slows character handling and prevents ordinary indexes from being used by LIKE. For this reason use locales only if you actually need them.

Ticket: ENT-10739
Changelog: title

together
https://github.com/cfengine/system-testing/pull/508 (rhel-8 hub deployment test use 2G instead of 1G RAM)

@craigcomstock craigcomstock marked this pull request as draft October 5, 2023 15:52
@craigcomstock craigcomstock marked this pull request as ready for review October 5, 2023 16:25
@craigcomstock
Copy link
Contributor Author

Tested this manually/locally with iTerm2 (but any shell will do really) when I set LC_CTYPE=UTF-8 and install with cf-remote install --hub vagrant@ubuntu-20 (aka versinon 3.21.2) I get the error in the ticket.

When I keep that LC_CTYPE env var and install the package from this ticket available in the github action artifacts: https://github.com/cfengine/buildscripts/suites/16918978368/artifacts/966586445 the installation succeeds! :)

@craigcomstock
Copy link
Contributor Author

@cf-bottom jenkins please

@cf-bottom
Copy link

Alright, I triggered a build:

Build Status

Jenkins: https://ci.cfengine.com/job/pr-pipeline/9931/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-9931/

vpodzime
vpodzime reviewed Oct 5, 2023
View reviewed changes
Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

Good job! I'm just not sure about the su - change.

packaging/common/cfengine-hub/postinstall.sh Outdated Show resolved Hide resolved
@craigcomstock craigcomstock changed the title ENT-10739: Force LC_ALL=C during package scriptlet postgres operations ENT-10739: Force default 'C' locale for package scriptlets Oct 5, 2023
@craigcomstock
Copy link
Contributor Author

looks like oslo-dc shutdown during the last jenkins run.

12:00:20 ./27_cf-secret/encrypt-decrypt.cf Pass
12:00:20 ./27_cf-secret/encrypt-decrypt-args.cf Pass
12:00:20 FATAL: command execution failed
12:07:47 java.io.EOFException
12:07:47 	at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2799)
12:07:47 	at java.io.ObjectInputStream$BlockDataInputStream.readShort(ObjectInputStream.java:3274)

@craigcomstock
Copy link
Contributor Author

@cf-bottom jenkins please, thanks. just run a smidge ahead of nightlites eh? ;)

@cf-bottom
Copy link

Sure, I triggered a build:

Build Status

Jenkins: https://ci.cfengine.com/job/pr-pipeline/9932/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-9932/

@craigcomstock
Improved locale env vars safety during installation for postgresql
2db105c 
It was found that an LC_CTYPE=UTF-8 from iTerm2 on OSX was breaking the hub package install.

PostgreSQL docs suggest using 'C' locale unless otherwise needed
https://www.postgresql.org/docs/current/locale.html

> The drawback of using locales other than C or POSIX in PostgreSQL is its performance impact. It slows character handling and prevents ordinary indexes from being used by LIKE. For this reason use locales only if you actually need them.

Ticket: ENT-10739
Changelog: title
@craigcomstock
Copy link
Contributor Author

@vpodzime this should be good to go now, @cf-bottom jenkins please.

@cf-bottom
Copy link

Alright, I triggered a build:

Build Status

Jenkins: https://ci.cfengine.com/job/pr-pipeline/9939/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-9939/

@craigcomstock
Copy link
Contributor Author

craigcomstock commented Oct 10, 2023 •
edited
Loading

sequential-tests broke in upgrade-triple test on masterfiles update from 3.18.5 to master, apache wasn't stopped when the policy expected it to be. Known issue? Flake? I doubt it has anything to do with this change as I see no errors in scriptlets/package installs.

Let's do a rebuild: Build Status

vpodzime
vpodzime approved these changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

Nice!

@craigcomstock
Copy link
Contributor Author

investigating odd new errors in hub upgrades.

error: Couldn"t fork %transfiletriggerun(systemd-239-74.el8_8.5.x86_64): Cannot allocate memory

@craigcomstock
Copy link
Contributor Author

investigating odd new errors in hub upgrades.

error: Couldn"t fork %transfiletriggerun(systemd-239-74.el8_8.5.x86_64): Cannot allocate memory

Could not reproduce with a local vagrant/virtual box VM. Will try the same ami/config as jenkins now.

@craigcomstock
Copy link
Contributor Author

retrying with 2GB rhel-8 instead of 1GB. In manual testing I had no problems but have had problems here several times. I can't find any reference to LC_ALL=C causing larger memory usage but... maybe?

Build Status

@craigcomstock
Copy link
Contributor Author

@cf-bottom jenkins

@cf-bottom
Copy link

Sure, I triggered a build:

Build Status

Jenkins: https://ci.cfengine.com/job/pr-pipeline/9948/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-9948/

@vpodzime
Copy link
Contributor

LC_ALL=C causing larger memory usage but... maybe?

hardly

@craigcomstock
Copy link
Contributor Author

bootstrap-tests failure was a flake I think: https://northerntech.atlassian.net/browse/ENT-10773
sequential-tests failure is because I can' "together" two buildscripts PRs 🤦 so I think this PR is "good to go".

vpodzime
vpodzime approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

ACK

@craigcomstock craigcomstock merged commit 0f8aa8e into master Oct 13, 2023
32 of 34 checks passed
@craigcomstock craigcomstock deleted the ENT-10739/master branch October 13, 2023 13:09
@craigcomstock
Copy link
Contributor Author

cherry picks
#1321
#1320

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olehermanse olehermanse olehermanse approved these changes

@vpodzime vpodzime vpodzime approved these changes

@nickanderson nickanderson Awaiting requested review from nickanderson

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@craigcomstock @cf-bottom @vpodzime @olehermanse