build!: Switch to ubuntu-latest for builds #35450
Conversation
feanil
force-pushed
the
feanil/ubuntu_upgrade
branch
7 times, most recently
from
00854fb
to
c87161a
Compare
feanil
force-pushed
the
feanil/ubuntu_upgrade
branch
4 times, most recently
from
5988f69
to
3346d99
Compare
There was a problem hiding this comment.
Code looks great, just some requests around the messaging:
- link to the depr ticket [DEPR]: Testing explicitly on Ubuntu 20.04 public-engineering#278 in your commit message
- this will effectively switch CI from 20.04 to 22.04, please note that too
- your operator note is really helpful and I don't want it to get lost come Sumac. Maybe mark it
build!:instead ofbuild:, and/or add it to the Sumac ops notes page?
.github/workflows/unit-tests.yml
Outdated
| run: | | ||
| cat <<EOF | sudo tee /etc/ssl/openssl.cnf | ||
| [openssl_init] | ||
| providers = provider_sect |
There was a problem hiding this comment.
Weirdly, converting these to spaces breaks the workflow, and I couldn't easily figure out why. I meant to add a comment to that effect, I'll do so before merging this.
There was a problem hiding this comment.
Oh you know, it's because you're using a heredoc to write the literal contents of that block to openssl.cnf. I bet openssl.cnf syntax requires tabs instead of spaces, or something like that. There's a probably a prettier way to do it but I'm not really concerned. Thanks for the comment.
feanil
force-pushed
the
feanil/ubuntu_upgrade
branch
2 times, most recently
from
36f938d
to
1a8e2dd
Compare
feanil
changed the title
This code does not have any dependencies that are specific to any specific version of ubuntu. So instead of testing on a specific version and then needing to do work to keep the versions up-to-date, we switch to the ubuntu-latest target which should be sufficient for testing purposes. This work is being done as a part of openedx/platform-roadmap#377 closes #35314
This is no longer installed by default on ubuntu and so we have to either manually install it or just run the relevant commands in the container here it's already available. This lets us do some of the test setup in a more robust way.
We stopped using mongo on the runner directly a while ago so this is just an errant start that should have been removed.
feanil
force-pushed
the
feanil/ubuntu_upgrade
branch
3 times, most recently
from
d3bca41
to
93ba03a
Compare
Operators Note: In newer versions of ubuntu the MD4 hashing algorithm is disabled by default. To enable it the openssl config needs to be updated in a manner similar to what's being done here. Alternatively, you can set the `FEATURES['ENABLE_BLAKE2B_HASHING']` setting to `True` which will switch to a newer hashing algorithm where MD4 was previously used. Because this hashing is being used as a part of the edx-platform caching mechanism, this will effectively clear the cache for the items that use this hash. The will impact any items where the cache key might have been too big to store in memcache so it's hard to predict exactly which items will be impacted. BREAKING CHANGE: See the operator note above for more details as this may break for users transitioning from Ubuntu 20.04 to newer versions.
feanil
force-pushed
the
feanil/ubuntu_upgrade
branch
from
93ba03a
to
1804fbb
Compare
There was a problem hiding this comment.
@feanil: I want to add a "Request changes" review because I'd like some discussion before merge, but I don't like how github handles that so I won't.
Please see your Slack thread in the #cc-edx-platform channel. Here is a copy of my Slack comment:
Can you hold off on this before we get a chance to discuss? You never addressed my concern here: https://discuss.openedx.org/t/depr-testing-on-ubuntu-20-04/13887/7?u=robrap, and I'm not confident this is the right move for edx-platform. I am happy you have a branch and have shown all is well on 22.04. I also know a long running matrix with two versions has its downsides. But, a regression in the area of codejail is a large security risk, and I'm not yet ok with this change.
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
This may be the reason why we're seeing failures on CI, though I am not entirely sure why it took until now for this to manifest (as always 😭 ). I suspect some of the issue we're seeing is that we're installing mongo from the focal repository, and the actual ubuntu version we're running is noble (maybe?) |
This code does not have any dependencies that are specific to any specific
version of ubuntu. So instead of testing on a specific version and then needing
to do work to keep the versions up-to-date, we switch to the ubuntu-latest
target which should be sufficient for testing purposes.
This work is being done as a part of openedx/platform-roadmap#377
closes #35314
Part of openedx/public-engineering#278
is disabled by default. To enable it the openssl config needs to be
updated in a manner similar to what's being done here. Alternatively,
you can set the FEATURES['ENABLE_BLAKE2B_HASHING'] setting to True
which will switch to a newer hashing algorithm where MD4 was previously
used.
Because this hashing is being used as a part of the edx-platform caching
mechanism, this will effectively clear the cache for the items that use
this hash. The will impact any items where the cache key might have been
too big to store in memcache so it's hard to predict exactly which items
will be impacted.
This build switches CI from Ubuntu 20.04 to 22.04.
As a part of the review of this PR, we clarified that the edx-platform tests have never been testing with Codejail/Apparmor setup. The testing of that subsystem has always happened in the
codejailrepository. Making a note of that here for future readers in-case there is concern that this change would somehow break or not sufficiently test the codejail <-> apparmor integration.This came from the discussion here.