-
Notifications
You must be signed in to change notification settings - Fork 39
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
scylla role: support for 2022.2 (5.1) #232
base: master
Are you sure you want to change the base?
Conversation
I have only local tests on a sample VM done We might in next PR upgrade molecule to cover the matrix (and do at least supported versions tests on Ubuntu) |
tested internally on training: http://localhost:8081/job/Test/job/Lubos2/job/Ops/job/Create_cluster/4/console |
# Let's filter by the arch first and then cut the version column. | ||
# Then we will filter out all rows that start with a requested version string followed by a digit to filter out version like 2021.1.11 when 2021.1.1 was requested. | ||
# And finally, we are going to get rid of duplications. | ||
shell: apt list -a {{ scylla_package_prefix }}-machine-image 2>/dev/null | awk '{split($0,a," "); print a[2]}' | egrep "^{{ stripped_scylla_machine_image_version_escaped }}" | sort | uniq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use 'apt info' or even 'apt-show-versions' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apt-show-versions is not installed by default, so it would create a dependency, so we should rather stick to the clumsy way above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apt info won't list all package versions, resp. I don't see how to force it to list all versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay,
apt info -a scylla-enterprise | grep Version
could likely work too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use 'apt info' or even 'apt-show-versions' ?
Why? What advantage would it give us exactly?
apt info -a scylla-enterprise | grep Version
could likely work too
apt info -a scylla-enterprise
doesn't have a platform information and nothing guarantees that you will always have same versions for all platforms.
@vladzcloudius @igorribeiroduarte can you also have a look? |
rebased to last master (with Igors io properties fixes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lubos, please, write a much more detailed PR description and the description of every patch I asked to split this patch into.
file: | ||
state: absent | ||
path: /etc/apt/preferences.d/99-scylla | ||
path: "/etc/apt/preferences.d/99-{{ scylla_package_prefix }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a 2022.2-spefic code.
Please, add a more detailed explanation what this patch is about.
But even before that it seems like this patch can and should be split into a few patches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to mix this unfortunately, since I am using those blocks that used to install other versions to install scylla-image package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a better description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this patch you're making the install tasks more generic (which is not necessarily related to 2022.2) and also applying the changes related to `scyla-image' package (I/O settings and the installation itself), so I think it makes sense to divide this patch into at least two.
# scylla machine image: | ||
# 2022.2.0-20230112.1239c34-1 | ||
|
||
- name: Get versions of {{ scylla_edition }} machine image package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole hunk is a copy-paste of a similar block between lines 31-38 in the same file. This asks for making an external 'task' of it and calling it for different package names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, partially, this code has different version parsing
scylla machine image doesn't follow same versioning as Scylla :-(
I will try to generalize as much of this as possible into single task
2022.x switched to use a -o discard mount option instead of periodic fstrim note that upon upgrade fstrim was disabled (but discard wasn't enabled!), we should still be able to keep old setup and add support for discard at the same time |
5.2 also deperecates bootparam script : scylladb/scylladb@9acdd3a note that clock source needs to come from machine image pkg too: |
bootparam fixed in #276 |
addressed the comments, reformatted to new patches I still need to retest RHEL (so don't merge until I clear my RHEL test) |
56b3033
to
2f7a249
Compare
retested on Centos 7, worked with workaround for
(I basically commented out that module in /opt/scylladb/scripts/scylla_util.py , not sure if this is a bug, 2022 shouldn't really support Centos 7, shall it? ... it shall I checked https://www.scylladb.com/customer-portal/?product=ent&platform=centos&version=stable-release-2022.2 and we should support Centos 7.2 and above, I tested above on Centos 7.7) |
ok, after adding molecule for 5.2 it seems I can reproduce the signing key for repo gpg error (likely the same as in #269 ) |
a6d955b
to
0eb7f04
Compare
become: true | ||
# if above is not found leverage /opt/scylladb/python3/bin/python3 /opt/scylladb/scylla-machine-image/lib/scylla_cloud_io_setup.py ? | ||
# or provide our own role runner for pre 2022.2.1 ? | ||
when: (io_cloud_setup.stat.exists) and (io_prop_cloud_stat.stat.exists|bool == False) and (present_scylla_disks.stdout|int > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have you added this present_scylla_disks.stdout|int > 0
check only for cloud?
If you want to add this check, I think it should probably come in the beginning of this block and the role should fail in case the number of disks is 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the role should work also for situations where you have EBS disk or custom disk or just have everything on root disk
the tuning for various cloud providers should kick in ONLY when there is a raid, resp. /var/lib/scylla mount point (they check for that and won't proceed, so we cannot run that tuning for unsupported setups and have to continue with fallback to iotune - which is DEFAULT way of handling things in 2022 / 5.1)
hence this check is added, I should retest with scenario where you have EBS mounted - I have a bad feeling recent fixes by @syuu1228 will be confused by the fact that EBS volumes can now look like NVME disks ...
( https://github.com/scylladb/scylla-machine-image/blob/master/common/scylla_cloud_io_setup#L238 )
in such case you would get out of box a tuning for local nvme for networked EBS ...
thanks for pointing this out @igorribeiroduarte , this might be a regression in comparison to new GP3 EBS mounts in AWS which in devtree of kernel look like nvmes !
/opt/scylladb/scylla-machine-image/scylla_cloud_io_setup | ||
become: true | ||
# if above is not found leverage /opt/scylladb/python3/bin/python3 /opt/scylladb/scylla-machine-image/lib/scylla_cloud_io_setup.py ? | ||
# or provide our own role runner for pre 2022.2.1 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this comment about as well?
Let's not have personal notes in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still a lot of comments.
2022 node exporter now works for clean install and upgrades fix detection of node-exporter services make sure the name of node exporter service matches the check
while testing I realized we would need to pin this machine image file too if we won't go for latest |
2 tasks:
|
version of machine image should be a variable - it should be defaulting to scylla version until first "-" |
2022.1 and 5.0 should be supported too