PR#2947: Prevent kojira from attempting to remove repos on other volumes. - koji

koji

#2947 Prevent kojira from attempting to remove repos on other volumes.

Closed 2 years ago by mikem. Opened 2 years ago by josepht.

josepht/koji rhelbld-5512 into master

Fix for case where 'info' is None.

Joe Simmons-Talbott • 2 years ago

4c4fb31

kojira: make 'ignore_other_volumes' a config only option.

Joe Simmons-Talbott • 2 years ago

23c1c97

Prevent kojira from attempting to remove repos on other volumes.

Joe Talbott • 2 years ago

391cb7b

util/kojira

file modified

+20 -4

		`@@ -445,6 +445,17 @@`
		`self.logger.info('Found repo %s, state=%s'`
		`% (repo_id, koji.REPO_STATES[data['state']]))`
		`repo = ManagedRepo(self, data, repodata)`
		`+ if self.options.ignore_other_volumes:`
		`+ info = repo.get_info()`
		`+ if info is None:`
		`+ continue`
		`+ volume = info.get('volume')`
		`+ if volume is not None and volume != 'DEFAULT':`
		`+ # Other volume`
		`+ self.logger.info("Skipping repo ({}) on other volume {}".format(`
		`+ repo_id, volume,`
		`+ ))`
		`+ continue`
		`self.repos[repo_id] = repo`
		`if not getTag(self.session, repo.tag_id) and not repo.expired():`
		`self.logger.info('Tag %d for repo %d disappeared, expiring.', repo.tag_id, repo_id)`
		`@@ -634,10 +645,14 @@`
		`session.logout()`

		`def pruneLocalRepos(self):`
		`+ volname = 'DEFAULT'`
		`+ volumedir = pathinfo.volumedir(volname)`
		`+ repodir = "%s/repos" % volumedir`
		`+ self._pruneLocalRepos(repodir, self.options.deleted_repo_lifetime)`
		`+`
		`for volinfo in self.session.listVolumes():`
		`- volumedir = pathinfo.volumedir(volinfo['name'])`
		`- repodir = "%s/repos" % volumedir`
		`- self._pruneLocalRepos(repodir, self.options.deleted_repo_lifetime)`
		`+ volname = volinfo['name']`
		`+ volumedir = pathinfo.volumedir(volname)`
		`distrepodir = "%s/repos-dist" % volumedir`
		`self._pruneLocalRepos(distrepodir, self.options.dist_repo_lifetime)`

		`@@ -1235,6 +1250,7 @@`
		`'cert': None,`
		`'serverca': None,`
		`'queue_file': None,`
		`+ 'ignore_other_volumes': False,`
		`}`
		`if config.has_section(section):`
		`int_opts = ('deleted_repo_lifetime', 'max_repo_tasks', 'repo_tasks_limit',`
		`@@ -1245,7 +1261,7 @@`
		`'cert', 'serverca', 'debuginfo_tags', 'queue_file',`
		`'source_tags', 'separate_source_tags', 'ignore_tags')`
		`bool_opts = ('verbose', 'debug', 'ignore_stray_repos', 'offline_retry',`
		`- 'no_ssl_verify', 'check_external_repos')`
		`+ 'no_ssl_verify', 'check_external_repos', 'ignore_other_volumes')`
		`legacy_opts = ('with_src', 'delete_batch_size', 'recent_tasks_lifetime')`
		`for name in config.options(section):`
		`if name in int_opts:`

util/kojira.conf

file modified

		`@@ -46,3 +46,6 @@`
		`; as otherwise you can end with weird behaviour. For details see`
		`; https://pagure.io/koji/issue/2159`
		`; check_external_repos = false`
		`+`
		`+ ; don't attempt to remove repos on non-default volumes`
		`+ ; ignore_other_volumes = false`

josepht commented 2 years ago

Adds '--ignore-other-volumes' flag and configuration option.

pretty please pagure-ci rebuild

2 years ago

pretty please pagure-ci rebuild

2 years ago

tkopecek commented 2 years ago

Issue #3011

mikem commented 2 years ago

Sorry for the delay. Looks good overall, just a few notes.

I don't think that we need the cli option. Most other kojira config options are only specified in the config file, so this seems like a little bit of a mismatch.

In pruneLocalRepos, since we're only considering the DEFAULT volume for non-dist repos, it might be simpler and clearer to pull that part out of the loop.

mikem commented 2 years ago

Also, I'm a little concerned about the possible performance impact of calling get_info() for each repo each time we call updateRepos. It might not be a problem, but we should check when testing this. If there is a slowdown, we can add some caching

ATTN: QE

Edited 2 years ago by mikem

64 new commits added

kojira: make 'ignore_other_volumes' a config only option.
fix flake8 line length
fix invalid date in changelog
doc: remove old mod_ssl instructions from server howto
kojira: don't fail on deleted needed tag
Add btype to protonmsg
Enable/disable channel
Increase coverage of CLI unit tests
cli: pass nosslverify opt to image task
Remove unnecessary line from unlock-tag CLI
Update webUI number of tasks
Fix py2 packaging
Release notes 1.25.1
add_rpm_sign catches IntegrityError
dist-repo takes inherited arch when arch is not set
Add CLI related to channels + add comments to channels
[kojixmlrpc] clean noisy error log
Hosts page with more filters and added channel column
fix test by using datetime.timezone instead of psycopg2.tz
Fix tests on centos 8
[policy] use "name" in result of lookup_name for CGs
listBuilds returns empty list, not error when not existing
Livecd handler set up release if release not given
kojiweb - Fix getting tag ID for buildMaven taskinfo page.
Drop download link from deleted build
kojihub - Use parse_task_params rather than manual task parsing.
additional import for test for some OS versions
Add noverifyssl option to oz image builds
Fix flake8 warnings.
koji-gc: Allow admins to force untagging builds.
Fix download-build unit test
cli: fix help text for download-build --type=remote-sources
lib: return taskLabel for unknown tasks
dev: use CentOS/Fedora registries for containers
hub: replace with py3 exception
web: better docs for KojiHubCA
drop old ClientCA reference
listBuilds accept also package name and user name
CLI Download-build check non exist sigkey
Remove jump/stops options from readFullInheritance
New CLI command userinfo
Read config options from main hub config and hub config dir
hub: fix SQL condition
basic test
remove debug
use format for name_template
update docs
sidetags: configurable naming template
Add the ability to specify custom metadata on an RPM build
Remove deprecated readGlobalInheritance
importlib instead of imp
tests - Add support for running tox with specific test(s)
update irc info
update .coveragerc to ignore p3 code
hub: policy test buildtag_inheritance
add pr
fix code block
misc fixes
update release notes
Release notes 1.25
Unify error messages related to hosts and users
kojira: use thread-local session object
kojira: don't fail on already deleted repo
kojira: skip unlocking of child locks

2 years ago

71 new commits added

fix tests
fix query logic
getTag returns query_event
query inheritance from last known tag appearance
fix getting tag from buildroot
honour taginfo option in policy_get_build_tags
Add unit tests notifications, list-channels
Increase API unit tests
Drop SELECT for check existing signatures for package from add-rpm-sig
doc: update profiles documentation
fix typo
Support packages that are head-signed
fix option propagation
expose force_auth in config
Honour --force-auth for anonymous commands
move btypes from headers to body of proton message
add warnings for remove-sig
Fix scripts for koji pkg and drop utils from py2
hub: fix docs for listBuilds "state" parameter name
set-task-priority fix permission error
Add all types to docs latest-build and readTaggedBuilds
import guestfs before dnf
hub: fix getBuild documented parameter name
Fix list-hosts with show-channels shows * with disabled channels
Fix disabled and enabled option when empty result
hub: document listHosts method
create symlink before import
CLI channels, hosts methods works with older hub
README: add koji-hs project
doc: add instructions for SSL DB connections
doc: fix "koji" CLI command name in signing instructions
fix "an user" -> "a user" grammar in help text and errors
Release notes 1.26
fix flake8
[doc][defining_hub_policies] update the doc
fix config typo
[hub] add non-host evalPolicy API
use scm_ as the prefix instead of scm for scminfo
update doc
more reasonable parameter name, and more doc strs
kojid: extend SCM.assert_allowed with hub policy
fix flake8
cli: improve help text for download-logs --match
cli: improve help text for download-logs --nvr
cli: clarify that download-logs operates on tasks
cli: remove duplicate download-logs --help text
cli: improve --config and --profile help text
doc: add signing documentation
Revert "PR#2978: Don't take new tasks if we've 0.0 capacity available"
fix flake8
updates
get_tag and accepts "auto" event
fix test_delay_times
Don't take new tasks if we've 0.0 capacity available
use decode for py3 bytes/strings
py3 Popen text mode fix
Prune user repos for image tasks
use getNextRelease for scratch image builds
setuptools BuildRequire for py3
Add koji.egg-info/ to .gitignore
fix python version parsing for py 3.10
DBConnectionString/dsn option for db connection
add debug message
protonmsg: cast body to text
hub: document readTaggedRPMS method
kojira: Do not ever clean up repositories where 'latest' points to
avoid masking hub errors, avoid duplicate check
fix unit tests, use error()
exit code, --all handling, typo
recreating absolute path bug
Add delete-rpm-sig CLI and deleteRPMSig hub call

2 years ago

rebased onto 391cb7b

2 years ago

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

julian8628 commented on line 7 of util/kojira 2 years ago

I'm a little confused here. It seems that repo.json is impossible to be on other volumes, because in repo.get_info(), volume is always None for self.get_path(). Thus, no matter if ignore_other_volumes is True or not, only repos in DEFAULT volume count.

Edited 2 years ago by julian8628

josepht commented 2 years ago

I'm a little confused here. It seems that repo.json is impossible to be on other volumes, because in repo.get_info(), volume is always None for self.get_path(). Thus, no matter if ignore_other_volumes is True or not, only repos in DEFAULT volume count.

I think that means I can just remove the ignore_other_volumes business altogether then.

1 new commit added

Fix for case where 'info' is None.

2 years ago

julian8628 commented 2 years ago

I think that means I can just remove the ignore_other_volumes business altogether then.

I believe so, @mikem what do you think?

Edited 2 years ago by julian8628

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

2 years ago

mikem commented 2 years ago

I'm a little confused here. It seems that repo.json is impossible to be on other volumes, because in repo.get_info(), volume is always None for self.get_path().

I think this is a deficiency in get_info(). Dist repos have repo.json files regardless of which volume they are on.

ETA: when Koji creates a dist repo on another volume, it always places a symlink on the default volume (similar to builds), so the distrepo path with no volume arg should be sufficient to get you to the repo.json file.

Edited 2 years ago by mikem

mikem commented 2 years ago

Ok, so this is a questionable feature designed to deal with a questionable setup. That said, I think that we shouldn't filter out repos in readCurrentRepos. If our hub is telling us about them, then we should be looking at them, but can just avoid deleting them.

mikem commented 2 years ago

Updates in PR #3126

julian8628 commented 2 years ago

I think this is a deficiency in get_info(). Dist repos have repo.json files regardless of which volume they are on.

I think there is an exception that if the kojira instance's DEFAULT volume is not the DEFAULT volume whose kojihub the dist repo dir was created on. The delete_thread of kojira won't recognize that dist repo as there's no it's repo.json symlink on that DEFAULT volume. So the "working" part is the pruneLocalRepos loop in this case.

Anyway, it has satisfied the purpose.

Edited 2 years ago by julian8628

mikem commented 2 years ago

I think there is an exception that if the kojira instance's DEFAULT volume is not the DEFAULT volume whose kojihub the dist repo dir was created on.

Ok, but if we're getting repo data from the hub, then it's reasonable to expect that the described repo is actually one that belongs to the hub.

The only place we really expect to encounter a repo that was generated by something other than the hub is in pruneLocalRepos. We don't call get_info there (we can't, it's not a ManagedRepo). We do read repo.json there, but only to detect tag mismatches.