Skip to content

Sync helm metadata worker when package file has metadatum and sync metadata with all channels

Sylvia Shenrequested to merge
564947-missing-helm-package-files into master

What does this MR do and why?

After we enabled packages_helm_metadata_cache feature flag, we noticed an NoMethodError occurred on Sentry.

The error happens because the given Helm package has package file without helm_file_metadatum.

There are only 3 events of this error and they are caused by the same user.

From logs on Kibana, we can see that one of the user deleted around ~16000 packages during the time period, and only 2 packages had this error. Another user also deleted around ~15000 packages and only 1 package had this error.

I tried to check the package on production Rails console, but since the package had been deleted, so we can't understand if the package had any files or not.

In theory, Helm package should always has 1 package file with the helm metadatum, and in Packages::MarkPackageForDestructionService, from the code we can see it's in the same transaction with renaming the package and package file, etc. So if the package is uploaded successfully, then there must be a package_file with helm_file_metadatum.

The reason of package_file has no helm_file_metadatum might be that something went wrong during ProcessFileService and in the end the worker didn't get retried. This issue happens even before we introduced Helm metadata cache feature, when user has Helm packages that have no helm_file_metadatum, the package in the end won't appear in the index.yaml when we generate index.yaml on the fly. because the index.yaml is generated by different channel, if there's no helm_file_metadatum, then the package won't be shown in the index.yaml.

Because of the reason above, this MR adds a guard to only sync Helm metadata cache when package has package_file and helm_file_metadatum.

We can consider creating another issue to investigate the reason of missing helm_file_metadatum if needed.

Secondly, during the MR review, we found another bug of the current implementation.

In the original implementation, we will only get 1 package file of the given package, it was under the impression that a Helm package will only have 1 package file, however, it's wrong.

When processing the package file, system will try to find the existing package under the same version and same name, then link the new package file to this package and remove the temporary package, that means, it's possible to have multiple package files under a Helm package which also means, the implementation was wrong.

Hence, we turned off the feature flag, and will prepare another MR to truncate the table and rollout the feature again.

This MR fix the bug by queueing sync workers for all the possible channels for the given package.

Database

Raw query

We need to query packages_helm_file_metadata by package.package_files to get the channel.

SELECT DISTINCT
    "packages_helm_file_metadata"."channel"
FROM
    "packages_helm_file_metadata"
WHERE
    "packages_helm_file_metadata"."package_file_id" IN (
        SELECT
            "packages_package_files"."id"
        FROM
            "packages_package_files"
        WHERE
            "packages_package_files"."package_id" = 2)

Query plan: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/42972/commands/131451

References

N/A

Screenshots or screen recordings

N/A

How to set up and validate locally

First issue:

  1. Stay in master branch and enter Rails console
  2. Find a Helm package, delete all package files 
    package = Packages::Helm::Package.last
package.package_files.first.helm_file_metadatum.delete
  3. Try to mark package as pending_destruction with the service 
    user = User.find_by(username: 'your-username')
::Packages::MarkPackageForDestructionService.new(container: package, current_user: user).execute
    It should return an error 
    => #<ServiceResponse:0x000000031a71e3d8 @http_status=400, @message="Failed to mark the package as pending destruction", @payload={}, @reason=nil, @status=:error>`
  4. Switch to this branch 564947-missing-helm-package-files
  5. Try to mark package as pending_destruction again 
    ::Packages::MarkPackageForDestructionService.new(container: package, current_user: user).execute
    It should return successful response 
    => #<ServiceResponse:0x000000033173c588 @http_status=:ok, @message="Package was successfully marked as pending destruction", @payload={}, @reason=nil, @status=:success>

Second issue:

  1. Switch to this branch 564947-missing-helm-package-files
  2. Open another tab, upload chart by the script: 
    curl --request POST \
    --user root:$TOKEN \
    --form 'chart=@test-chart-0.2.0.tgz' \
    "${GITLAB_URL}/api/v4/projects/${PROJECT_ID}/packages/helm/api/alpha/charts"

curl --request POST \
    --user root:$TOKEN \
    --form 'chart=@test-chart-0.2.0.tgz' \
    "${GITLAB_URL}/api/v4/projects/${PROJECT_ID}/packages/helm/api/beta/charts"
  3. Enter Sidekiq page (http://gdk.test:3000/admin/sidekiq) and stop the queue
  4. Enter Rails console and find the package, and call sync_helm_metadata_cache method 
    package = Packages::Package.find_by(name: 'test-chart', version: '0.2.0', package_type: 'helm')
package.sync_helm_metadata_cache
  5. There should be 2 jobs are queued in default queue (http://gdk.test:3000/admin/sidekiq/queues/default)

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #564947 (closed)

Edited by Sylvia Shen

Merge request reports